From 1920331fba46efeffd8e85ef41052709831ba60e 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 4174f7d11..9eba84676 100644 --- a/compose/service/record.go +++ b/compose/service/record.go @@ -779,6 +779,19 @@ func (svc record) procCreate(invokerID uint64, m *types.Module, new *types.Recor 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(m, new) } @@ -824,11 +837,6 @@ func (svc record) procUpdate(invokerID uint64, m *types.Module, upd *types.Recor 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 @@ -842,6 +850,34 @@ func (svc record) procUpdate(invokerID uint64, m *types.Module, upd *types.Recor } } + // 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(m, upd) } @@ -1372,10 +1408,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 1208bab26..88e46af56 100644 --- a/compose/types/record_value.go +++ b/compose/types/record_value.go @@ -174,7 +174,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, @@ -192,9 +192,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 445e12694..849449156 100644 --- a/tests/compose/record_test.go +++ b/tests/compose/record_test.go @@ -450,7 +450,7 @@ func TestRecordImportImportProgress_sessionNotFound(t *testing.T) { func TestRecordFieldModulePermissionCheck(t *testing.T) { h := newHelper(t) - // make a standad 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").PermissionResource(), "record.value.read") @@ -495,7 +495,7 @@ func TestRecordFieldModulePermissionCheck(t *testing.T) { b().JSON(fmt.Sprintf(`{"values": [{"name": "email", "value": "changed-email"}]}`)). Expect(t). Status(http.StatusOK). - Assert(helpers.AssertError("not allowed to change value of field email")). + Assert(helpers.AssertError("1 issue(s) found")). End() })