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

@@ -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

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
 			}

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