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() })