diff --git a/server/channels/api4/custom_profile_attributes.go b/server/channels/api4/custom_profile_attributes.go index bfc4ce24cda..543f669dd9a 100644 --- a/server/channels/api4/custom_profile_attributes.go +++ b/server/channels/api4/custom_profile_attributes.go @@ -53,7 +53,7 @@ func createCPAField(c *Context, w http.ResponseWriter, r *http.Request) { return } - var pf *model.PropertyField + var pf *model.CPAField err := json.NewDecoder(r.Body).Decode(&pf) if err != nil || pf == nil { c.SetInvalidParamWithErr("property_field", err) diff --git a/server/channels/api4/custom_profile_attributes_test.go b/server/channels/api4/custom_profile_attributes_test.go index f75b0160bc2..bea5cd8dd1f 100644 --- a/server/channels/api4/custom_profile_attributes_test.go +++ b/server/channels/api4/custom_profile_attributes_test.go @@ -100,14 +100,15 @@ func TestListCPAFields(t *testing.T) { th := Setup(t) defer th.TearDown() - field := &model.PropertyField{ + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ Name: model.NewId(), Type: model.PropertyFieldTypeText, Attrs: map[string]any{"visibility": "when_set"}, - } + }) + require.NoError(t, err) createdField, err := th.App.CreateCPAField(field) - require.Nil(t, err) + require.NoError(t, err) require.NotNil(t, createdField) t.Run("endpoint should not work if no valid license is present", func(t *testing.T) { @@ -158,10 +159,12 @@ func TestPatchCPAField(t *testing.T) { th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) t.Run("a user without admin permissions should not be able to patch a field", func(t *testing.T) { - field := &model.PropertyField{ + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ Name: model.NewId(), Type: model.PropertyFieldTypeText, - } + }) + require.NoError(t, err) + createdField, appErr := th.App.CreateCPAField(field) require.Nil(t, appErr) require.NotNil(t, createdField) @@ -175,10 +178,12 @@ func TestPatchCPAField(t *testing.T) { th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) { webSocketClient := th.CreateConnectedWebSocketClient(t) - field := &model.PropertyField{ + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ Name: model.NewId(), Type: model.PropertyFieldTypeText, - } + }) + require.NoError(t, err) + createdField, appErr := th.App.CreateCPAField(field) require.Nil(t, appErr) require.NotNil(t, createdField) @@ -294,10 +299,12 @@ func TestListCPAValues(t *testing.T) { th.RemovePermissionFromRole(model.PermissionViewMembers.Id, model.SystemUserRoleId) defer th.AddPermissionToRole(model.PermissionViewMembers.Id, model.SystemUserRoleId) - field := &model.PropertyField{ + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ Name: model.NewId(), Type: model.PropertyFieldTypeText, - } + }) + require.NoError(t, err) + createdField, appErr := th.App.CreateCPAField(field) require.Nil(t, appErr) require.NotNil(t, createdField) @@ -328,10 +335,12 @@ func TestListCPAValues(t *testing.T) { }) t.Run("should handle array values correctly", func(t *testing.T) { - arrayField := &model.PropertyField{ + arrayField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ Name: model.NewId(), Type: model.PropertyFieldTypeMultiselect, - } + }) + require.NoError(t, err) + createdArrayField, appErr := th.App.CreateCPAField(arrayField) require.Nil(t, appErr) require.NotNil(t, createdArrayField) @@ -518,10 +527,12 @@ func TestPatchCPAValues(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() - field := &model.PropertyField{ + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ Name: model.NewId(), Type: model.PropertyFieldTypeText, - } + }) + require.NoError(t, err) + createdField, appErr := th.App.CreateCPAField(field) require.Nil(t, appErr) require.NotNil(t, createdField) @@ -609,10 +620,12 @@ func TestPatchCPAValues(t *testing.T) { }) t.Run("should handle array values correctly", func(t *testing.T) { - arrayField := &model.PropertyField{ + arrayField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ Name: model.NewId(), Type: model.PropertyFieldTypeMultiselect, - } + }) + require.NoError(t, err) + createdArrayField, appErr := th.App.CreateCPAField(arrayField) require.Nil(t, appErr) require.NotNil(t, createdArrayField) diff --git a/server/channels/app/custom_profile_attributes.go b/server/channels/app/custom_profile_attributes.go index 01bf923cdf2..6649148362b 100644 --- a/server/channels/app/custom_profile_attributes.go +++ b/server/channels/app/custom_profile_attributes.go @@ -7,7 +7,6 @@ import ( "encoding/json" "net/http" "sort" - "strings" "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/v8/channels/store" @@ -92,16 +91,12 @@ func (a *App) CreateCPAField(field *model.CPAField) (*model.PropertyField, *mode return nil, model.NewAppError("CreateCPAField", "app.custom_profile_attributes.limit_reached.app_error", nil, "", http.StatusUnprocessableEntity).Wrap(err) } - if appErr := field.Validate(); appErr != nil { + field.GroupID = groupID + + if appErr := field.Sanitize(); appErr != nil { return nil, appErr } - field.GroupID = groupID - - if appErr := field.Validate(); appErr != nil { - return nil, appErr - } - newField, err := a.Srv().propertyService.CreatePropertyField(field.ToPropertyField()) if err != nil { var appErr *model.AppError @@ -136,7 +131,7 @@ func (a *App) PatchCPAField(fieldID string, patch *model.PropertyFieldPatch) (*m return nil, model.NewAppError("UpdateCPAField", "app.custom_profile_attributes.property_field_conversion.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - if appErr := cpaField.Validate(); appErr != nil { + if appErr := cpaField.Sanitize(); appErr != nil { return nil, appErr } @@ -278,4 +273,3 @@ func (a *App) PatchCPAValues(userID string, fieldValueMap map[string]json.RawMes return updatedValues, nil } - diff --git a/server/channels/app/custom_profile_attributes_test.go b/server/channels/app/custom_profile_attributes_test.go index 5791e00fd1d..038389434a4 100644 --- a/server/channels/app/custom_profile_attributes_test.go +++ b/server/channels/app/custom_profile_attributes_test.go @@ -47,12 +47,13 @@ func TestGetCPAField(t *testing.T) { }) t.Run("should get an existing CPA field", func(t *testing.T) { - field := &model.PropertyField{ + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ GroupID: cpaGroupID, Name: "Test Field", Type: model.PropertyFieldTypeText, Attrs: model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityHidden}, - } + }) + require.Nil(t, err) createdField, err := th.App.CreateCPAField(field) require.Nil(t, err) @@ -120,7 +121,8 @@ func TestCreateCPAField(t *testing.T) { require.NoError(t, cErr) t.Run("should fail if the field is not valid", func(t *testing.T) { - field := &model.PropertyField{Name: model.NewId()} + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{Name: model.NewId()}) + require.Nil(t, err) createdField, err := th.App.CreateCPAField(field) require.NotNil(t, err) @@ -128,11 +130,12 @@ func TestCreateCPAField(t *testing.T) { }) t.Run("should not be able to create a property field for a different feature", func(t *testing.T) { - field := &model.PropertyField{ + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ GroupID: model.NewId(), Name: model.NewId(), Type: model.PropertyFieldTypeText, - } + }) + require.NoError(t, err) createdField, appErr := th.App.CreateCPAField(field) require.Nil(t, appErr) @@ -140,12 +143,13 @@ func TestCreateCPAField(t *testing.T) { }) t.Run("should correctly create a CPA field", func(t *testing.T) { - field := &model.PropertyField{ + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ GroupID: cpaGroupID, Name: model.NewId(), Type: model.PropertyFieldTypeText, Attrs: model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityHidden}, - } + }) + require.NoError(t, err) createdField, err := th.App.CreateCPAField(field) require.Nil(t, err) @@ -170,19 +174,23 @@ func TestCreateCPAField(t *testing.T) { t.Run("should not be able to create CPA fields above the limit", func(t *testing.T) { // we create the rest of the fields required to reach the limit for i := 1; i <= CustomProfileAttributesFieldLimit; i++ { - field := &model.PropertyField{ + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ Name: model.NewId(), Type: model.PropertyFieldTypeText, - } + }) + require.NoError(t, err) + createdField, err := th.App.CreateCPAField(field) require.Nil(t, err) require.NotZero(t, createdField.ID) } // then, we create a last one that would exceed the limit - field := &model.PropertyField{ - Name: model.NewId(), - Type: model.PropertyFieldTypeText, + field := &model.CPAField{ + PropertyField: model.PropertyField{ + Name: model.NewId(), + Type: model.PropertyFieldTypeText, + }, } createdField, err := th.App.CreateCPAField(field) require.NotNil(t, err) @@ -200,9 +208,11 @@ func TestCreateCPAField(t *testing.T) { require.Nil(t, th.App.DeleteCPAField(fields[0].ID)) // creating a new one should work now - field := &model.PropertyField{ - Name: model.NewId(), - Type: model.PropertyFieldTypeText, + field := &model.CPAField{ + PropertyField: model.PropertyField{ + Name: model.NewId(), + Type: model.PropertyFieldTypeText, + }, } createdField, err := th.App.CreateCPAField(field) require.Nil(t, err) @@ -220,12 +230,14 @@ func TestPatchCPAField(t *testing.T) { cpaGroupID, cErr := th.App.cpaGroupID() require.NoError(t, cErr) - newField := &model.PropertyField{ + newField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ GroupID: cpaGroupID, Name: model.NewId(), Type: model.PropertyFieldTypeText, Attrs: model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityHidden}, - } + }) + require.NoError(t, err) + createdField, err := th.App.CreateCPAField(newField) require.Nil(t, err) @@ -248,6 +260,7 @@ func TestPatchCPAField(t *testing.T) { Name: model.NewId(), Type: model.PropertyFieldTypeText, } + field, err := th.App.Srv().propertyService.CreatePropertyField(newField) require.NoError(t, err) @@ -272,7 +285,7 @@ func TestPatchCPAField(t *testing.T) { t.Run("should preserve option IDs when patching select field options", func(t *testing.T) { // Create a select field with options - selectField := &model.PropertyField{ + selectField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ GroupID: cpaGroupID, Name: "Select Field", Type: model.PropertyFieldTypeSelect, @@ -288,7 +301,9 @@ func TestPatchCPAField(t *testing.T) { }, }, }, - } + }) + require.NoError(t, err) + createdSelectField, err := th.App.CreateCPAField(selectField) require.Nil(t, err) @@ -351,11 +366,13 @@ func TestDeleteCPAField(t *testing.T) { cpaGroupID, cErr := th.App.cpaGroupID() require.NoError(t, cErr) - newField := &model.PropertyField{ + newField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ GroupID: cpaGroupID, Name: model.NewId(), Type: model.PropertyFieldTypeText, - } + }) + require.NoError(t, err) + createdField, err := th.App.CreateCPAField(newField) require.Nil(t, err) @@ -643,176 +660,3 @@ func TestPatchCPAValue(t *testing.T) { require.Equal(t, userID, updatedValue.TargetID) }) } - -func TestValidateCustomProfileAttributesField(t *testing.T) { - tests := []struct { - name string - field *model.PropertyField - expectError bool - errorId string - expectedAttrs model.StringInterface - }{ - { - name: "valid text field with no value type", - field: &model.PropertyField{ - Type: model.PropertyFieldTypeText, - Attrs: model.StringInterface{}, - }, - expectError: false, - expectedAttrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsVisibility: "when_set", - }, - }, - { - name: "valid text field with valid value type and whitespace", - field: &model.PropertyField{ - Type: model.PropertyFieldTypeText, - Attrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsValueType: " email ", - }, - }, - expectError: false, - expectedAttrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsVisibility: "when_set", - model.CustomProfileAttributesPropertyAttrsValueType: model.CustomProfileAttributesValueTypeEmail, - }, - }, - { - name: "valid text field with visibility and whitespace", - field: &model.PropertyField{ - Type: model.PropertyFieldTypeText, - Attrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsVisibility: " hidden ", - }, - }, - expectError: false, - expectedAttrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityHidden, - }, - }, - { - name: "invalid text field with invalid value type", - field: &model.PropertyField{ - Type: model.PropertyFieldTypeText, - Attrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsValueType: "invalid_type", - }, - }, - expectError: true, - errorId: "app.custom_profile_attributes.unknown_value_type.app_error", - }, - { - name: "valid select field with valid options", - field: &model.PropertyField{ - Type: model.PropertyFieldTypeSelect, - Attrs: model.StringInterface{ - model.PropertyFieldAttributeOptions: []any{ - map[string]interface{}{ - "name": "Option 1", - "color": "#123456", - }, - map[string]interface{}{ - "name": "Option 2", - "color": "#654321", - }, - }, - }, - }, - expectError: false, - expectedAttrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityDefault, - model.PropertyFieldAttributeOptions: model.PropertyOptions[*model.CustomProfileAttributesSelectOption]{ - {Name: "Option 1", Color: "#123456"}, - {Name: "Option 2", Color: "#654321"}, - }, - }, - }, - { - name: "invalid select field with duplicate option names", - field: &model.PropertyField{ - Type: model.PropertyFieldTypeSelect, - Attrs: model.StringInterface{ - model.PropertyFieldAttributeOptions: []any{ - map[string]interface{}{ - "name": "Option 1", - "color": "opt1", - }, - map[string]interface{}{ - "name": "Option 1", - "color": "opt2", - }, - }, - }, - }, - expectError: true, - errorId: "app.custom_profile_attributes.invalid_options.app_error", - }, - { - name: "invalid select field with non-array options", - field: &model.PropertyField{ - Type: model.PropertyFieldTypeSelect, - Attrs: model.StringInterface{ - model.PropertyFieldAttributeOptions: "not an array", - }, - }, - expectError: true, - errorId: "app.custom_profile_attributes.invalid_options.app_error", - }, - { - name: "invalid select field with invalid option format", - field: &model.PropertyField{ - Type: model.PropertyFieldTypeSelect, - Attrs: model.StringInterface{ - model.PropertyFieldAttributeOptions: []interface{}{ - "some string", - }, - }, - }, - expectError: true, - errorId: "app.custom_profile_attributes.invalid_options.app_error", - }, - { - name: "invalid field with unknown visibility", - field: &model.PropertyField{ - Type: model.PropertyFieldTypeText, - Attrs: model.StringInterface{ - model.CustomProfileAttributesPropertyAttrsVisibility: "unknown", - }, - }, - expectError: true, - errorId: "app.custom_profile_attributes.unknown_visibility.app_error", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := validateCustomProfileAttributesField(tt.field) - if tt.expectError { - require.NotNil(t, err) - require.Equal(t, tt.errorId, err.Id) - } else { - var ogErr error - if err != nil { - ogErr = err.Unwrap() - } - require.Nilf(t, err, "unexpected error: %v, with original error: %v", err, ogErr) - if tt.expectedAttrs != nil { - for key, value := range tt.expectedAttrs { - if key == model.PropertyFieldAttributeOptions { - expectedOptions := value.(model.PropertyOptions[*model.CustomProfileAttributesSelectOption]) - actualOptions := tt.field.Attrs[model.PropertyFieldAttributeOptions].(model.PropertyOptions[*model.CustomProfileAttributesSelectOption]) - // remove IDs from actualOptions to compare - for i := range actualOptions { - actualOptions[i].ID = "" - } - - require.ElementsMatch(t, expectedOptions, actualOptions) - } else { - require.Equal(t, value, tt.field.Attrs[key]) - } - } - } - } - }) - } -} diff --git a/server/public/model/custom_profile_attributes.go b/server/public/model/custom_profile_attributes.go index ae8bc254ef8..7ba291eb7ab 100644 --- a/server/public/model/custom_profile_attributes.go +++ b/server/public/model/custom_profile_attributes.go @@ -128,7 +128,7 @@ func (c *CPAField) ToPropertyField() *PropertyField { return &pf } -func (c *CPAField) Validate() *AppError { +func (c *CPAField) Sanitize() *AppError { switch c.Type { case PropertyFieldTypeText: if valueType := strings.TrimSpace(c.Attrs.ValueType); valueType != "" { @@ -140,6 +140,14 @@ func (c *CPAField) Validate() *AppError { case PropertyFieldTypeSelect, PropertyFieldTypeMultiselect: options := c.Attrs.Options + + // add an ID to options with no ID + for i := range options { + if options[i].ID == "" { + options[i].ID = NewId() + } + } + if err := options.IsValid(); err != nil { return NewAppError("ValidateCPAField", "app.custom_profile_attributes.invalid_options.app_error", nil, "", http.StatusUnprocessableEntity).Wrap(err) } diff --git a/server/public/model/custom_profile_attributes_test.go b/server/public/model/custom_profile_attributes_test.go index e0b65ad788e..db06b972f18 100644 --- a/server/public/model/custom_profile_attributes_test.go +++ b/server/public/model/custom_profile_attributes_test.go @@ -289,3 +289,186 @@ func TestCustomProfileAttributeSelectOptionIsValid(t *testing.T) { }) } } + +func TestCPAField_Sanitize(t *testing.T) { + tests := []struct { + name string + field *CPAField + expectError bool + errorId string + expectedAttrs CPAAttrs + checkOptionsID bool + }{ + { + name: "valid text field with no value type", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeText, + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: "when_set", + }, + }, + { + name: "valid text field with valid value type and whitespace", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeText, + }, + Attrs: CPAAttrs{ + ValueType: " email ", + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: "when_set", + ValueType: CustomProfileAttributesValueTypeEmail, + }, + }, + { + name: "valid text field with visibility and whitespace", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeText, + }, + Attrs: CPAAttrs{ + Visibility: " hidden ", + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: CustomProfileAttributesVisibilityHidden, + }, + }, + { + name: "invalid text field with invalid value type", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeText, + }, + Attrs: CPAAttrs{ + ValueType: "invalid_type", + }, + }, + expectError: true, + errorId: "app.custom_profile_attributes.unknown_value_type.app_error", + }, + { + name: "valid select field with valid options", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeSelect, + }, + Attrs: CPAAttrs{ + Options: []*CustomProfileAttributesSelectOption{ + { + Name: "Option 1", + Color: "#123456", + }, + { + Name: "Option 2", + Color: "#654321", + }, + }, + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: CustomProfileAttributesVisibilityDefault, + Options: PropertyOptions[*CustomProfileAttributesSelectOption]{ + {Name: "Option 1", Color: "#123456"}, + {Name: "Option 2", Color: "#654321"}, + }, + }, + }, + { + name: "valid select field with valid options with ids", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeSelect, + }, + Attrs: CPAAttrs{ + Options: []*CustomProfileAttributesSelectOption{ + { + ID: "t9ceh651eir4zkhyh4m54s5r7w", + Name: "Option 1", + Color: "#123456", + }, + }, + }, + }, + expectError: false, + expectedAttrs: CPAAttrs{ + Visibility: CustomProfileAttributesVisibilityDefault, + Options: PropertyOptions[*CustomProfileAttributesSelectOption]{ + {ID: "t9ceh651eir4zkhyh4m54s5r7w", Name: "Option 1", Color: "#123456"}, + }, + }, + checkOptionsID: true, + }, + { + name: "invalid select field with duplicate option names", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeSelect, + }, + Attrs: CPAAttrs{ + Options: []*CustomProfileAttributesSelectOption{ + { + Name: "Option 1", + Color: "opt1", + }, + { + Name: "Option 1", + Color: "opt2", + }, + }, + }, + }, + expectError: true, + errorId: "app.custom_profile_attributes.invalid_options.app_error", + }, + { + name: "invalid field with unknown visibility", + field: &CPAField{ + PropertyField: PropertyField{ + Type: PropertyFieldTypeText, + }, + Attrs: CPAAttrs{ + Visibility: "unknown", + }, + }, + expectError: true, + errorId: "app.custom_profile_attributes.unknown_visibility.app_error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.field.Sanitize() + if tt.expectError { + require.NotNil(t, err) + require.Equal(t, tt.errorId, err.Id) + } else { + var ogErr error + if err != nil { + ogErr = err.Unwrap() + } + require.Nilf(t, err, "unexpected error: %v, with original error: %v", err, ogErr) + + assert.Equal(t, tt.expectedAttrs.Visibility, tt.field.Attrs.Visibility) + assert.Equal(t, tt.expectedAttrs.ValueType, tt.field.Attrs.ValueType) + + for i := range tt.expectedAttrs.Options { + if tt.checkOptionsID { + assert.Equal(t, tt.expectedAttrs.Options[i].ID, tt.field.Attrs.Options[i].ID) + } + assert.Equal(t, tt.expectedAttrs.Options[i].Name, tt.field.Attrs.Options[i].Name) + assert.Equal(t, tt.expectedAttrs.Options[i].Color, tt.field.Attrs.Options[i].Color) + } + } + }) + } +}