From eca90b1fc184b10d4751cc70a8477d6b215b6421 Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Thu, 29 Oct 2020 07:38:50 +0100 Subject: [PATCH] Fix record update with update-denied fields Scenario: - values already exist on the record - record is updated with users that can not update all fields - only values for updatable fields are provided Access control was moved from generalValueSetValidation to procCreate/procUpdate to allow return of record value error set. Existing values from fields with update-deny set are copied to updating set to ensure proper handling by RVS's Merge fn(). --- compose/service/record.go | 50 ++++++++++++++++++++++++++++------- compose/types/record_value.go | 6 +++-- tests/compose/record_test.go | 4 +-- 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/compose/service/record.go b/compose/service/record.go index f0c6b17cc..23ac059cb 100644 --- a/compose/service/record.go +++ b/compose/service/record.go @@ -697,6 +697,19 @@ func (svc record) procCreate(ctx context.Context, s store.Storer, invokerID uint new.OwnedBy = invokerID } + rve := &types.RecordValueErrorSet{} + _ = new.Values.Walk(func(v *types.RecordValue) error { + if v.IsUpdated() && !svc.ac.CanUpdateRecordValue(svc.ctx, m.Fields.FindByName(v.Name)) { + rve.Push(types.RecordValueError{Kind: "updateDenied", Meta: map[string]interface{}{"field": v.Name, "value": v.Value}}) + } + + return nil + }) + + if !rve.IsValid() { + return rve + } + // Run validation of the updated records return svc.validator.Run(ctx, s, m, new) } @@ -742,11 +755,6 @@ func (svc record) procUpdate(ctx context.Context, s store.Storer, invokerID uint upd.DeletedAt = old.DeletedAt upd.DeletedBy = old.DeletedBy - // Merge new (updated) values with old ones - // This way we get list of updated, stale and deleted values - // that we can selectively update in the repository - upd.Values = old.Values.Merge(upd.Values) - if upd.OwnedBy == 0 { if old.OwnedBy > 0 { // Owner not set/send in the payload @@ -760,6 +768,34 @@ func (svc record) procUpdate(ctx context.Context, s store.Storer, invokerID uint } } + // Value merge process does not know anything about permissions so + // in case when new values are missing but do exist in the old set and their update/read is denied + // we need to copy them to ensure value merge process them correctly + for _, f := range m.Fields { + if len(upd.Values.FilterByName(f.Name)) == 0 && !svc.ac.CanUpdateRecordValue(svc.ctx, m.Fields.FindByName(f.Name)) { + // copy all fields from old to new + upd.Values = append(upd.Values, old.Values.FilterByName(f.Name).GetClean()...) + } + } + + // Merge new (updated) values with old ones + // This way we get list of updated, stale and deleted values + // that we can selectively update in the repository + upd.Values = old.Values.Merge(upd.Values) + + rve := &types.RecordValueErrorSet{} + _ = upd.Values.Walk(func(v *types.RecordValue) error { + if v.IsUpdated() && !svc.ac.CanUpdateRecordValue(svc.ctx, m.Fields.FindByName(v.Name)) { + rve.Push(types.RecordValueError{Kind: "updateDenied", Meta: map[string]interface{}{"field": v.Name, "value": v.Value}}) + } + + return nil + }) + + if !rve.IsValid() { + return rve + } + // Run validation of the updated records return svc.validator.Run(ctx, s, m, upd) } @@ -1260,10 +1296,6 @@ func (svc record) generalValueSetValidation(m *types.Module, vv types.RecordValu return RecordErrFieldNotFound(aProps.setField(v.Name)) } - if !svc.ac.CanUpdateRecordValue(svc.ctx, field) { - return RecordErrNotAllowedToChangeFieldValue(aProps.setField(v.Name)) - } - if field.IsRef() { if v.Value == "" { return nil diff --git a/compose/types/record_value.go b/compose/types/record_value.go index c2b388fc7..8c4c3d6e1 100644 --- a/compose/types/record_value.go +++ b/compose/types/record_value.go @@ -194,7 +194,7 @@ func (set RecordValueSet) Merge(new RecordValueSet) (out RecordValueSet) { out = make([]*RecordValue, 0) for s := range set { - // Mark all old as deleted + // Mark all old as updated out = append(out, &RecordValue{ Name: set[s].Name, Value: set[s].Value, @@ -212,9 +212,11 @@ func (set RecordValueSet) Merge(new RecordValueSet) (out RecordValueSet) { ex.DeletedAt = new[n].DeletedAt if ex.OldValue == new[n].Value { + // Value is the same ex.Updated = false } else if !ex.Updated { - // Did value change? + // Value changed and old one was not marked as updated before + // See if values really changed and update old value on existing value ex.Updated = ex.Value != new[n].Value ex.OldValue = ex.Value } diff --git a/tests/compose/record_test.go b/tests/compose/record_test.go index e33dadc74..32532f8a9 100644 --- a/tests/compose/record_test.go +++ b/tests/compose/record_test.go @@ -477,7 +477,7 @@ func TestRecordFieldModulePermissionCheck(t *testing.T) { h := newHelper(t) h.clearRecords() - // make a standard module, and prevent current user to + // make a standard module, and prevent (DENY) current user to // read from "name" and update "email" fields module := h.repoMakeRecordModuleWithFields("record testing module") h.deny(module.Fields.FindByName("name").RBACResource(), "record.value.read") @@ -535,7 +535,7 @@ func TestRecordFieldModulePermissionCheck(t *testing.T) { Header("Accept", "application/json"). Expect(t). Status(http.StatusOK). - Assert(helpers.AssertError("not allowed to change value of field email")). + Assert(helpers.AssertError("1 issue(s) found")). End() })