Adds a groupID filter to the property service methods (#30420)

* Adds a groupID filter to the property service methods

This allows the property service caller to directly ensure that a
given call is only going to affect a field or value that belongs to a
given group, instead of (for example) retrieving a property value
before deleting it by id to ensure that the value belongs to a
specific property group. The groupID filter is optional and has no
effect if called with the empty string value.

The changes also remove references to input sanitization on trimming
the whitespace for the CPA field names and validate at the API level
the input for the field patch endpoint.

* Fix linter

---------

Co-authored-by: Miguel de la Cruz <miguel@ctrlz.es>
Co-authored-by: Mattermost Build <build@mattermost.com>

server/channels/app/custom_profile_attributes.go

@@ -137,7 +137,9 @@ func (a *App) PatchCPAField(fieldID string, patch *model.PropertyFieldPatch) (*m
 		return nil, appErr
 	}
 
-	patchedField, err := a.Srv().propertyService.UpdatePropertyField(cpaField.ToPropertyField())
+	// we've already ensured that the field exists for the CPA group,
+	// we don't need to specify the groupID for the update
+	patchedField, err := a.Srv().propertyService.UpdatePropertyField("", cpaField.ToPropertyField())
 	if err != nil {
 		var nfErr *store.ErrNotFound
 		switch {
@@ -161,16 +163,7 @@ func (a *App) DeleteCPAField(id string) *model.AppError {
 		return model.NewAppError("DeleteCPAField", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
-	if _, err := a.Srv().propertyService.GetPropertyField(groupID, id); err != nil {
-		switch {
-		case errors.Is(err, sql.ErrNoRows):
-			return model.NewAppError("DeleteCPAField", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound)
-		default:
-			return model.NewAppError("DeleteCPAField", "app.custom_profile_attributes.get_property_field.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
-		}
-	}
-
-	if err := a.Srv().propertyService.DeletePropertyField(id); err != nil {
+	if err := a.Srv().propertyService.DeletePropertyField(groupID, id); err != nil {
 		var nfErr *store.ErrNotFound
 		switch {
 		case errors.As(err, &nfErr):

server/channels/app/custom_profile_attributes_test.go

@@ -390,9 +390,9 @@ func TestDeleteCPAField(t *testing.T) {
 	}
 
 	t.Run("should fail if the field doesn't exist", func(t *testing.T) {
-		appErr := th.App.DeleteCPAField(model.NewId())
-		require.NotNil(t, appErr)
-		require.Equal(t, "app.custom_profile_attributes.property_field_not_found.app_error", appErr.Id)
+		err := th.App.DeleteCPAField(model.NewId())
+		require.NotNil(t, err)
+		require.Equal(t, "app.custom_profile_attributes.property_field_delete.app_error", err.Id)
 	})
 
 	t.Run("should not allow to delete a field outside of CPA", func(t *testing.T) {
@@ -406,7 +406,7 @@ func TestDeleteCPAField(t *testing.T) {
 
 		dErr := th.App.DeleteCPAField(field.ID)
 		require.NotNil(t, dErr)
-		require.Equal(t, "app.custom_profile_attributes.property_field_not_found.app_error", dErr.Id)
+		require.Equal(t, "app.custom_profile_attributes.property_field_delete.app_error", dErr.Id)
 	})
 
 	t.Run("should correctly delete the field", func(t *testing.T) {
@@ -622,7 +622,7 @@ func TestPatchCPAValue(t *testing.T) {
 		}
 		createdField, err := th.App.Srv().propertyService.CreatePropertyField(newField)
 		require.NoError(t, err)
-		err = th.App.Srv().propertyService.DeletePropertyField(createdField.ID)
+		err = th.App.Srv().propertyService.DeletePropertyField(cpaGroupID, createdField.ID)
 		require.NoError(t, err)
 
 		userID := model.NewId()

server/channels/app/properties/property_field.go

@@ -4,6 +4,8 @@
 package properties
 
 import (
+	"fmt"
+
 	"github.com/mattermost/mattermost/server/public/model"
 )
 
@@ -32,8 +34,8 @@ func (ps *PropertyService) SearchPropertyFields(groupID, targetID string, opts m
 	return ps.fieldStore.SearchPropertyFields(opts)
 }
 
-func (ps *PropertyService) UpdatePropertyField(field *model.PropertyField) (*model.PropertyField, error) {
-	fields, err := ps.UpdatePropertyFields([]*model.PropertyField{field})
+func (ps *PropertyService) UpdatePropertyField(groupID string, field *model.PropertyField) (*model.PropertyField, error) {
+	fields, err := ps.UpdatePropertyFields(groupID, []*model.PropertyField{field})
 	if err != nil {
 		return nil, err
 	}
@@ -41,13 +43,21 @@ func (ps *PropertyService) UpdatePropertyField(field *model.PropertyField) (*mod
 	return fields[0], nil
 }
 
-func (ps *PropertyService) UpdatePropertyFields(fields []*model.PropertyField) ([]*model.PropertyField, error) {
-	return ps.fieldStore.Update(fields)
+func (ps *PropertyService) UpdatePropertyFields(groupID string, fields []*model.PropertyField) ([]*model.PropertyField, error) {
+	return ps.fieldStore.Update(groupID, fields)
 }
 
-func (ps *PropertyService) DeletePropertyField(id string) error {
+func (ps *PropertyService) DeletePropertyField(groupID, id string) error {
+	// if groupID is not empty, we need to check first that the field belongs to the group
+	if groupID != "" {
+		if _, err := ps.GetPropertyField(groupID, id); err != nil {
+			return fmt.Errorf("error getting property field %q for group %q: %w", id, groupID, err)
+		}
+	}
+
 	if err := ps.valueStore.DeleteForField(id); err != nil {
 		return err
 	}
-	return ps.fieldStore.Delete(id)
+
+	return ps.fieldStore.Delete(groupID, id)
 }

server/channels/app/properties/property_value.go

@@ -27,8 +27,8 @@ func (ps *PropertyService) SearchPropertyValues(groupID, targetID string, opts m
 	return ps.valueStore.SearchPropertyValues(opts)
 }
 
-func (ps *PropertyService) UpdatePropertyValue(value *model.PropertyValue) (*model.PropertyValue, error) {
-	values, err := ps.UpdatePropertyValues([]*model.PropertyValue{value})
+func (ps *PropertyService) UpdatePropertyValue(groupID string, value *model.PropertyValue) (*model.PropertyValue, error) {
+	values, err := ps.UpdatePropertyValues(groupID, []*model.PropertyValue{value})
 	if err != nil {
 		return nil, err
 	}
@@ -36,8 +36,8 @@ func (ps *PropertyService) UpdatePropertyValue(value *model.PropertyValue) (*mod
 	return values[0], nil
 }
 
-func (ps *PropertyService) UpdatePropertyValues(values []*model.PropertyValue) ([]*model.PropertyValue, error) {
-	return ps.valueStore.Update(values)
+func (ps *PropertyService) UpdatePropertyValues(groupID string, values []*model.PropertyValue) ([]*model.PropertyValue, error) {
+	return ps.valueStore.Update(groupID, values)
 }
 
 func (ps *PropertyService) UpsertPropertyValue(value *model.PropertyValue) (*model.PropertyValue, error) {
@@ -53,6 +53,6 @@ func (ps *PropertyService) UpsertPropertyValues(values []*model.PropertyValue) (
 	return ps.valueStore.Upsert(values)
 }
 
-func (ps *PropertyService) DeletePropertyValue(id string) error {
-	return ps.valueStore.Delete(id)
+func (ps *PropertyService) DeletePropertyValue(groupID, id string) error {
+	return ps.valueStore.Delete(groupID, id)
 }