Improves validation and sanitization for CPA fields and values (#30694)

This change automatically removes options and sync attributes when sanitizing fields that don't support them. As per values, it returns an error when the value for a text type field is longer than the 64 characters limit we're currently applying. The PR fixes a bug on the create CPA field endpoint that was causing the attrs of the CPAField not to be decoded correctly. Co-authored-by: Miguel de la Cruz <miguel@ctrlz.es>
2025-04-16 16:04:30 +02:00
--- a/server/channels/api4/custom_profile_attributes_test.go
+++ b/server/channels/api4/custom_profile_attributes_test.go
@@ -8,6 +8,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"strings"
 	"testing"
 	"time"

@@ -215,6 +216,69 @@ func TestPatchCPAField(t *testing.T) {
 			require.NotEmpty(t, wsField.ID)
 			require.Equal(t, patchedField, &wsField)
 		})
+
+		t.Run("sanitization should remove options and sync details when necessary", func(t *testing.T) {
+			// Create a select field with options
+			optionID1 := model.NewId()
+			optionID2 := model.NewId()
+			selectField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
+				Name: model.NewId(),
+				Type: model.PropertyFieldTypeSelect,
+				Attrs: model.StringInterface{
+					"options": []map[string]any{
+						{"id": optionID1, "name": "Option 1", "color": "#FF0000"},
+						{"id": optionID2, "name": "Option 2", "color": "#00FF00"},
+					},
+				},
+			})
+			require.NoError(t, err)
+
+			createdField, _, err := client.CreateCPAField(context.Background(), selectField.ToPropertyField())
+			require.NoError(t, err)
+			require.NotNil(t, createdField)
+
+			// Verify options were created
+			options, ok := createdField.Attrs["options"]
+			require.True(t, ok)
+			require.NotNil(t, options)
+
+			// Patch to change type to text with LDAP attribute
+			// Options should be automatically removed even though we don't explicitly remove them
+			ldapAttr := "user_attribute"
+			textPatch := &model.PropertyFieldPatch{
+				Type:  model.NewPointer(model.PropertyFieldTypeText),
+				Attrs: &model.StringInterface{"ldap": ldapAttr},
+			}
+
+			patchedTextField, resp, err := client.PatchCPAField(context.Background(), createdField.ID, textPatch)
+			CheckOKStatus(t, resp)
+			require.NoError(t, err)
+			require.Equal(t, model.PropertyFieldTypeText, patchedTextField.Type)
+
+			// Verify options were removed
+			options = patchedTextField.Attrs["options"]
+			require.Empty(t, options)
+
+			// Verify LDAP attribute was set
+			ldap, ok := patchedTextField.Attrs["ldap"]
+			require.True(t, ok)
+			require.Equal(t, ldapAttr, ldap)
+
+			// Now patch to change type to date
+			// LDAP attribute should be automatically removed even though we don't explicitly remove it
+			datePatch := &model.PropertyFieldPatch{
+				Type: model.NewPointer(model.PropertyFieldTypeDate),
+			}
+
+			patchedDateField, resp, err := client.PatchCPAField(context.Background(), patchedTextField.ID, datePatch)
+			CheckOKStatus(t, resp)
+			require.NoError(t, err)
+			require.Equal(t, model.PropertyFieldTypeDate, patchedDateField.Type)
+
+			// Verify LDAP attribute was removed
+			ldap = patchedDateField.Attrs["ldap"]
+			require.Empty(t, ldap)
+		})
 	}, "a user with admin permissions should be able to patch the field")
 }

@@ -518,4 +582,27 @@ func TestPatchCPAValues(t *testing.T) {
 		require.NoError(t, json.Unmarshal(patchedValues[createdArrayField.ID], &actualValues))
 		require.Equal(t, optionsID[2:4], actualValues)
 	})
+
+	t.Run("an invalid patch should be rejected", func(t *testing.T) {
+		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)
+
+		// Create a value that's too long (over 64 characters)
+		tooLongValue := strings.Repeat("a", model.CPAValueTypeTextMaxLength+1)
+		values := map[string]json.RawMessage{
+			createdField.ID: json.RawMessage(fmt.Sprintf(`"%s"`, tooLongValue)),
+		}
+
+		_, resp, err := th.Client.PatchCPAValues(context.Background(), values)
+		CheckBadRequestStatus(t, resp)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "Failed to validate property value")
+	})
 }
--- a/server/public/model/custom_profile_attributes.go
+++ b/server/public/model/custom_profile_attributes.go
@@ -46,11 +46,13 @@ const (
 	CustomProfileAttributesVisibilityWhenSet = "when_set"
 	CustomProfileAttributesVisibilityAlways  = "always"
 	CustomProfileAttributesVisibilityDefault = CustomProfileAttributesVisibilityWhenSet
-)

-const (
+	// CPA options
 	CPAOptionNameMaxLength  = 128
 	CPAOptionColorMaxLength = 128
+
+	// CPA value constraints
+	CPAValueTypeTextMaxLength = 64
 )

 func IsKnownCPAValueType(valueType string) bool {
@@ -146,7 +148,28 @@ func (c *CPAField) ToPropertyField() *PropertyField {
 	return &pf
 }

+// SupportsOptions checks the CPAField type and determines if the type
+// supports the use of options
+func (c *CPAField) SupportsOptions() bool {
+	return c.Type == PropertyFieldTypeSelect || c.Type == PropertyFieldTypeMultiselect
+}
+
+// SupportsSyncing checks the CPAField type and determines if it
+// supports syncing with external sources of truth
+func (c *CPAField) SupportsSyncing() bool {
+	return c.Type == PropertyFieldTypeText
+}
+
 func (c *CPAField) SanitizeAndValidate() *AppError {
+	// first we clean unused attributes depending on the field type
+	if !c.SupportsOptions() {
+		c.Attrs.Options = nil
+	}
+	if !c.SupportsSyncing() {
+		c.Attrs.LDAP = ""
+		c.Attrs.SAML = ""
+	}
+
 	switch c.Type {
 	case PropertyFieldTypeText:
 		if valueType := strings.TrimSpace(c.Attrs.ValueType); valueType != "" {
@@ -230,6 +253,10 @@ func SanitizeAndValidatePropertyValue(cpaField *CPAField, rawValue json.RawMessa
 		value = strings.TrimSpace(value)

 		if fieldType == PropertyFieldTypeText {
+			if len(value) > CPAValueTypeTextMaxLength {
+				return nil, fmt.Errorf("value too long")
+			}
+
 			if cpaField.Attrs.ValueType == CustomProfileAttributesValueTypeEmail && !IsValidEmail(value) {
 				return nil, fmt.Errorf("invalid email")
 			}
--- a/server/public/model/custom_profile_attributes_test.go
+++ b/server/public/model/custom_profile_attributes_test.go
@@ -481,6 +481,192 @@ func TestCPAField_SanitizeAndValidate(t *testing.T) {
 			expectError: true,
 			errorId:     "app.custom_profile_attributes.sanitize_and_validate.app_error",
 		},
+
+		// Test options cleaning for types that don't support options
+		{
+			name: "text field with options should clean options",
+			field: &CPAField{
+				PropertyField: PropertyField{
+					Type: PropertyFieldTypeText,
+				},
+				Attrs: CPAAttrs{
+					Options: []*CustomProfileAttributesSelectOption{
+						{
+							ID:    NewId(),
+							Name:  "Option 1",
+							Color: "#123456",
+						},
+					},
+				},
+			},
+			expectError: false,
+			expectedAttrs: CPAAttrs{
+				Visibility: CustomProfileAttributesVisibilityDefault,
+				Options:    nil, // Options should be cleaned
+			},
+		},
+		{
+			name: "date field with options should clean options",
+			field: &CPAField{
+				PropertyField: PropertyField{
+					Type: PropertyFieldTypeDate,
+				},
+				Attrs: CPAAttrs{
+					Options: []*CustomProfileAttributesSelectOption{
+						{
+							ID:    NewId(),
+							Name:  "Option 1",
+							Color: "#123456",
+						},
+					},
+				},
+			},
+			expectError: false,
+			expectedAttrs: CPAAttrs{
+				Visibility: CustomProfileAttributesVisibilityDefault,
+				Options:    nil, // Options should be cleaned
+			},
+		},
+		{
+			name: "user field with options should clean options",
+			field: &CPAField{
+				PropertyField: PropertyField{
+					Type: PropertyFieldTypeUser,
+				},
+				Attrs: CPAAttrs{
+					Options: []*CustomProfileAttributesSelectOption{
+						{
+							ID:    NewId(),
+							Name:  "Option 1",
+							Color: "#123456",
+						},
+					},
+				},
+			},
+			expectError: false,
+			expectedAttrs: CPAAttrs{
+				Visibility: CustomProfileAttributesVisibilityDefault,
+				Options:    nil, // Options should be cleaned
+			},
+		},
+
+		// Test options preservation for types that support options
+		{
+			name: "select field with options should preserve options",
+			field: &CPAField{
+				PropertyField: PropertyField{
+					Type: PropertyFieldTypeSelect,
+				},
+				Attrs: CPAAttrs{
+					Options: []*CustomProfileAttributesSelectOption{
+						{
+							ID:    NewId(),
+							Name:  "Option 1",
+							Color: "#123456",
+						},
+					},
+				},
+			},
+			expectError: false,
+			expectedAttrs: CPAAttrs{
+				Visibility: CustomProfileAttributesVisibilityDefault,
+				Options: PropertyOptions[*CustomProfileAttributesSelectOption]{
+					{Name: "Option 1", Color: "#123456"},
+				},
+			},
+		},
+		{
+			name: "multiselect field with options should preserve options",
+			field: &CPAField{
+				PropertyField: PropertyField{
+					Type: PropertyFieldTypeMultiselect,
+				},
+				Attrs: CPAAttrs{
+					Options: []*CustomProfileAttributesSelectOption{
+						{
+							ID:    NewId(),
+							Name:  "Option 1",
+							Color: "#123456",
+						},
+					},
+				},
+			},
+			expectError: false,
+			expectedAttrs: CPAAttrs{
+				Visibility: CustomProfileAttributesVisibilityDefault,
+				Options: PropertyOptions[*CustomProfileAttributesSelectOption]{
+					{Name: "Option 1", Color: "#123456"},
+				},
+			},
+		},
+
+		// Test syncing attributes cleaning for types that don't support syncing
+		{
+			name: "select field with LDAP and SAML should clean syncing attributes",
+			field: &CPAField{
+				PropertyField: PropertyField{
+					Type: PropertyFieldTypeSelect,
+				},
+				Attrs: CPAAttrs{
+					LDAP: "ldap_attribute",
+					SAML: "saml_attribute",
+					Options: []*CustomProfileAttributesSelectOption{
+						{
+							ID:    NewId(),
+							Name:  "Option 1",
+							Color: "#123456",
+						},
+					},
+				},
+			},
+			expectError: false,
+			expectedAttrs: CPAAttrs{
+				Visibility: CustomProfileAttributesVisibilityDefault,
+				LDAP:       "", // Should be cleaned
+				SAML:       "", // Should be cleaned
+				Options: PropertyOptions[*CustomProfileAttributesSelectOption]{
+					{Name: "Option 1", Color: "#123456"},
+				},
+			},
+		},
+		{
+			name: "date field with LDAP and SAML should clean syncing attributes",
+			field: &CPAField{
+				PropertyField: PropertyField{
+					Type: PropertyFieldTypeDate,
+				},
+				Attrs: CPAAttrs{
+					LDAP: "ldap_attribute",
+					SAML: "saml_attribute",
+				},
+			},
+			expectError: false,
+			expectedAttrs: CPAAttrs{
+				Visibility: CustomProfileAttributesVisibilityDefault,
+				LDAP:       "", // Should be cleaned
+				SAML:       "", // Should be cleaned
+			},
+		},
+
+		// Test syncing attributes preservation for types that support syncing
+		{
+			name: "text field with LDAP and SAML should preserve syncing attributes",
+			field: &CPAField{
+				PropertyField: PropertyField{
+					Type: PropertyFieldTypeText,
+				},
+				Attrs: CPAAttrs{
+					LDAP: "ldap_attribute",
+					SAML: "saml_attribute",
+				},
+			},
+			expectError: false,
+			expectedAttrs: CPAAttrs{
+				Visibility: CustomProfileAttributesVisibilityDefault,
+				LDAP:       "ldap_attribute", // Should be preserved
+				SAML:       "saml_attribute", // Should be preserved
+			},
+		},
 	}

 	for _, tt := range tests {
@@ -539,6 +725,13 @@ func TestSanitizeAndValidatePropertyValue(t *testing.T) {
 			require.Error(t, err)
 			require.Contains(t, err.Error(), "json: cannot unmarshal number into Go value of type string")
 		})
+
+		t.Run("value too long", func(t *testing.T) {
+			longValue := strings.Repeat("a", CPAValueTypeTextMaxLength+1)
+			_, err := SanitizeAndValidatePropertyValue(&CPAField{PropertyField: PropertyField{Type: PropertyFieldTypeText}}, json.RawMessage(fmt.Sprintf(`"%s"`, longValue)))
+			require.Error(t, err)
+			require.Equal(t, "value too long", err.Error())
+		})
 	})

 	t.Run("date field type", func(t *testing.T) {