diff --git a/api/v4/source/custom_profile_attributes.yaml b/api/v4/source/custom_profile_attributes.yaml index b1dcd45148..893162c041 100644 --- a/api/v4/source/custom_profile_attributes.yaml +++ b/api/v4/source/custom_profile_attributes.yaml @@ -189,7 +189,11 @@ id: type: string value: - type: string + oneOf: + - type: string + - type: array + items: + type: string responses: "200": description: Custom Profile Attribute values patch successful @@ -203,7 +207,11 @@ id: type: string value: - type: string + oneOf: + - type: string + - type: array + items: + type: string "400": $ref: "#/components/responses/BadRequest" "401": diff --git a/server/channels/api4/custom_profile_attributes.go b/server/channels/api4/custom_profile_attributes.go index 870e935348..bfc4ce24cd 100644 --- a/server/channels/api4/custom_profile_attributes.go +++ b/server/channels/api4/custom_profile_attributes.go @@ -5,6 +5,7 @@ package api4 import ( "encoding/json" + "fmt" "net/http" "strings" @@ -172,18 +173,48 @@ func deleteCPAField(c *Context, w http.ResponseWriter, r *http.Request) { ReturnStatusOK(w) } +func sanitizePropertyValue(fieldType model.PropertyFieldType, rawValue json.RawMessage) (json.RawMessage, error) { + switch fieldType { + case model.PropertyFieldTypeText, model.PropertyFieldTypeDate, model.PropertyFieldTypeSelect, model.PropertyFieldTypeUser: + var value string + if err := json.Unmarshal(rawValue, &value); err != nil { + return nil, err + } + value = strings.TrimSpace(value) + if fieldType == model.PropertyFieldTypeUser && value != "" && !model.IsValidId(value) { + return nil, fmt.Errorf("invalid user id") + } + return json.Marshal(value) + + case model.PropertyFieldTypeMultiselect, model.PropertyFieldTypeMultiuser: + var values []string + if err := json.Unmarshal(rawValue, &values); err != nil { + return nil, err + } + filteredValues := make([]string, 0, len(values)) + for _, v := range values { + trimmed := strings.TrimSpace(v) + if trimmed == "" { + continue + } + if fieldType == model.PropertyFieldTypeMultiuser && !model.IsValidId(trimmed) { + return nil, fmt.Errorf("invalid user id: %s", trimmed) + } + filteredValues = append(filteredValues, trimmed) + } + return json.Marshal(filteredValues) + + default: + return nil, fmt.Errorf("unknown field type: %s", fieldType) + } +} + func patchCPAValues(c *Context, w http.ResponseWriter, r *http.Request) { if c.App.Channels().License() == nil || !c.App.Channels().License().IsE20OrEnterprise() { c.Err = model.NewAppError("Api4.patchCPAValues", "api.custom_profile_attributes.license_error", nil, "", http.StatusForbidden) return } - var attributeValues map[string]string - if jsonErr := json.NewDecoder(r.Body).Decode(&attributeValues); jsonErr != nil { - c.SetInvalidParamWithErr("attrs", jsonErr) - return - } - // This check is unnecessary for now // Will be required when/if admins can patch other's values userID := c.AppContext.Session().UserId @@ -192,13 +223,43 @@ func patchCPAValues(c *Context, w http.ResponseWriter, r *http.Request) { return } + var updates map[string]json.RawMessage + if err := json.NewDecoder(r.Body).Decode(&updates); err != nil { + c.SetInvalidParamWithErr("value", err) + return + } + auditRec := c.MakeAuditRecord("patchCPAValues", audit.Fail) defer c.LogAuditRec(auditRec) audit.AddEventParameter(auditRec, "user_id", userID) - results := make(map[string]string) - for fieldID, value := range attributeValues { - patchedValue, appErr := c.App.PatchCPAValue(userID, fieldID, strings.TrimSpace(value)) + // Get all fields at once and build a map for quick lookup + allFields, appErr := c.App.ListCPAFields() + if appErr != nil { + c.Err = appErr + return + } + + fieldMap := make(map[string]*model.PropertyField) + for _, field := range allFields { + fieldMap[field.ID] = field + } + + results := make(map[string]json.RawMessage, len(updates)) + for fieldID, rawValue := range updates { + field, ok := fieldMap[fieldID] + if !ok { + c.Err = model.NewAppError("Api4.patchCPAValues", "api.custom_profile_attributes.field_not_found", nil, "", http.StatusBadRequest) + return + } + + sanitizedValue, err := sanitizePropertyValue(field.Type, rawValue) + if err != nil { + c.SetInvalidParam(fmt.Sprintf("value for field %s: %v", fieldID, err)) + return + } + + patchedValue, appErr := c.App.PatchCPAValue(userID, fieldID, sanitizedValue) if appErr != nil { c.Err = appErr return @@ -238,7 +299,7 @@ func listCPAValues(c *Context, w http.ResponseWriter, r *http.Request) { return } - returnValue := make(map[string]string) + returnValue := make(map[string]json.RawMessage) for _, value := range values { returnValue[value.FieldID] = value.Value } diff --git a/server/channels/api4/custom_profile_attributes_test.go b/server/channels/api4/custom_profile_attributes_test.go index ba63fa879a..37e278bdfb 100644 --- a/server/channels/api4/custom_profile_attributes_test.go +++ b/server/channels/api4/custom_profile_attributes_test.go @@ -5,6 +5,7 @@ package api4 import ( "context" + "encoding/json" "fmt" "os" "testing" @@ -232,7 +233,7 @@ func TestListCPAValues(t *testing.T) { require.Nil(t, appErr) require.NotNil(t, createdField) - _, appErr = th.App.PatchCPAValue(th.BasicUser.Id, createdField.ID, "Field Value") + _, appErr = th.App.PatchCPAValue(th.BasicUser.Id, createdField.ID, json.RawMessage(`"Field Value"`)) require.Nil(t, appErr) t.Run("endpoint should not work if no valid license is present", func(t *testing.T) { @@ -257,6 +258,28 @@ func TestListCPAValues(t *testing.T) { require.Len(t, values, 1) }) + t.Run("should handle array values correctly", func(t *testing.T) { + arrayField := &model.PropertyField{ + Name: model.NewId(), + Type: model.PropertyFieldTypeMultiselect, + } + createdArrayField, appErr := th.App.CreateCPAField(arrayField) + require.Nil(t, appErr) + require.NotNil(t, createdArrayField) + + _, appErr = th.App.PatchCPAValue(th.BasicUser.Id, createdArrayField.ID, json.RawMessage(`["option1", "option2", "option3"]`)) + require.Nil(t, appErr) + + values, resp, err := th.Client.ListCPAValues(context.Background(), th.BasicUser.Id) + CheckOKStatus(t, resp) + require.NoError(t, err) + require.NotEmpty(t, values) + + var arrayValues []string + require.NoError(t, json.Unmarshal(values[createdArrayField.ID], &arrayValues)) + require.Equal(t, []string{"option1", "option2", "option3"}, arrayValues) + }) + t.Run("non team member should NOT be able to list values", func(t *testing.T) { resp, err := th.SystemAdminClient.RemoveTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id) CheckOKStatus(t, resp) @@ -268,6 +291,158 @@ func TestListCPAValues(t *testing.T) { }) } +func TestSanitizePropertyValue(t *testing.T) { + t.Run("text field type", func(t *testing.T) { + t.Run("valid text", func(t *testing.T) { + result, err := sanitizePropertyValue(model.PropertyFieldTypeText, json.RawMessage(`"hello world"`)) + require.NoError(t, err) + var value string + require.NoError(t, json.Unmarshal(result, &value)) + require.Equal(t, "hello world", value) + }) + + t.Run("empty text should be allowed", func(t *testing.T) { + result, err := sanitizePropertyValue(model.PropertyFieldTypeText, json.RawMessage(`""`)) + require.NoError(t, err) + var value string + require.NoError(t, json.Unmarshal(result, &value)) + require.Empty(t, value) + }) + + t.Run("invalid JSON", func(t *testing.T) { + _, err := sanitizePropertyValue(model.PropertyFieldTypeText, json.RawMessage(`invalid`)) + require.Error(t, err) + }) + + t.Run("wrong type", func(t *testing.T) { + _, err := sanitizePropertyValue(model.PropertyFieldTypeText, json.RawMessage(`123`)) + require.Error(t, err) + require.Contains(t, err.Error(), "json: cannot unmarshal number into Go value of type string") + }) + }) + + t.Run("date field type", func(t *testing.T) { + t.Run("valid date", func(t *testing.T) { + result, err := sanitizePropertyValue(model.PropertyFieldTypeDate, json.RawMessage(`"2023-01-01"`)) + require.NoError(t, err) + var value string + require.NoError(t, json.Unmarshal(result, &value)) + require.Equal(t, "2023-01-01", value) + }) + + t.Run("empty date should be allowed", func(t *testing.T) { + result, err := sanitizePropertyValue(model.PropertyFieldTypeDate, json.RawMessage(`""`)) + require.NoError(t, err) + var value string + require.NoError(t, json.Unmarshal(result, &value)) + require.Empty(t, value) + }) + }) + + t.Run("select field type", func(t *testing.T) { + t.Run("valid option", func(t *testing.T) { + result, err := sanitizePropertyValue(model.PropertyFieldTypeSelect, json.RawMessage(`"option1"`)) + require.NoError(t, err) + var value string + require.NoError(t, json.Unmarshal(result, &value)) + require.Equal(t, "option1", value) + }) + + t.Run("empty option should be allowed", func(t *testing.T) { + result, err := sanitizePropertyValue(model.PropertyFieldTypeSelect, json.RawMessage(`""`)) + require.NoError(t, err) + var value string + require.NoError(t, json.Unmarshal(result, &value)) + require.Empty(t, value) + }) + }) + + t.Run("user field type", func(t *testing.T) { + t.Run("valid user ID", func(t *testing.T) { + validID := model.NewId() + result, err := sanitizePropertyValue(model.PropertyFieldTypeUser, json.RawMessage(fmt.Sprintf(`"%s"`, validID))) + require.NoError(t, err) + var value string + require.NoError(t, json.Unmarshal(result, &value)) + require.Equal(t, validID, value) + }) + + t.Run("empty user ID should be allowed", func(t *testing.T) { + _, err := sanitizePropertyValue(model.PropertyFieldTypeUser, json.RawMessage(`""`)) + require.NoError(t, err) + }) + + t.Run("invalid user ID format", func(t *testing.T) { + _, err := sanitizePropertyValue(model.PropertyFieldTypeUser, json.RawMessage(`"invalid-id"`)) + require.Error(t, err) + require.Equal(t, "invalid user id", err.Error()) + }) + }) + + t.Run("multiselect field type", func(t *testing.T) { + t.Run("valid options", func(t *testing.T) { + result, err := sanitizePropertyValue(model.PropertyFieldTypeMultiselect, json.RawMessage(`["option1", "option2"]`)) + require.NoError(t, err) + var values []string + require.NoError(t, json.Unmarshal(result, &values)) + require.Equal(t, []string{"option1", "option2"}, values) + }) + + t.Run("empty array", func(t *testing.T) { + _, err := sanitizePropertyValue(model.PropertyFieldTypeMultiselect, json.RawMessage(`[]`)) + require.NoError(t, err) + }) + + t.Run("array with empty values should filter them out", func(t *testing.T) { + result, err := sanitizePropertyValue(model.PropertyFieldTypeMultiselect, json.RawMessage(`["option1", "", "option2", " ", "option3"]`)) + require.NoError(t, err) + var values []string + require.NoError(t, json.Unmarshal(result, &values)) + require.Equal(t, []string{"option1", "option2", "option3"}, values) + }) + }) + + t.Run("multiuser field type", func(t *testing.T) { + t.Run("valid user IDs", func(t *testing.T) { + validID1 := model.NewId() + validID2 := model.NewId() + result, err := sanitizePropertyValue(model.PropertyFieldTypeMultiuser, json.RawMessage(fmt.Sprintf(`["%s", "%s"]`, validID1, validID2))) + require.NoError(t, err) + var values []string + require.NoError(t, json.Unmarshal(result, &values)) + require.Equal(t, []string{validID1, validID2}, values) + }) + + t.Run("empty array", func(t *testing.T) { + _, err := sanitizePropertyValue(model.PropertyFieldTypeMultiuser, json.RawMessage(`[]`)) + require.NoError(t, err) + }) + + t.Run("array with empty strings should be filtered out", func(t *testing.T) { + validID1 := model.NewId() + validID2 := model.NewId() + result, err := sanitizePropertyValue(model.PropertyFieldTypeMultiuser, json.RawMessage(fmt.Sprintf(`["%s", "", " ", "%s"]`, validID1, validID2))) + require.NoError(t, err) + var values []string + require.NoError(t, json.Unmarshal(result, &values)) + require.Equal(t, []string{validID1, validID2}, values) + }) + + t.Run("array with invalid ID should return error", func(t *testing.T) { + validID1 := model.NewId() + _, err := sanitizePropertyValue(model.PropertyFieldTypeMultiuser, json.RawMessage(fmt.Sprintf(`["%s", "invalid-id"]`, validID1))) + require.Error(t, err) + require.Equal(t, "invalid user id: invalid-id", err.Error()) + }) + }) + + t.Run("unknown field type", func(t *testing.T) { + _, err := sanitizePropertyValue("unknown", json.RawMessage(`"value"`)) + require.Error(t, err) + require.Equal(t, "unknown field type: unknown", err.Error()) + }) +} + func TestPatchCPAValues(t *testing.T) { os.Setenv("MM_FEATUREFLAGS_CUSTOMPROFILEATTRIBUTES", "true") defer os.Unsetenv("MM_FEATUREFLAGS_CUSTOMPROFILEATTRIBUTES") @@ -283,7 +458,7 @@ func TestPatchCPAValues(t *testing.T) { require.NotNil(t, createdField) t.Run("endpoint should not work if no valid license is present", func(t *testing.T) { - values := map[string]string{createdField.ID: "Field Value"} + values := map[string]json.RawMessage{createdField.ID: json.RawMessage(`"Field Value"`)} patchedValues, resp, err := th.Client.PatchCPAValues(context.Background(), values) CheckForbiddenStatus(t, resp) require.Error(t, err) @@ -295,22 +470,26 @@ func TestPatchCPAValues(t *testing.T) { th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) t.Run("any team member should be able to create their own values", func(t *testing.T) { - values := map[string]string{} + values := map[string]json.RawMessage{} value := "Field Value" - values[createdField.ID] = fmt.Sprintf(" %s ", value) // value should be sanitized + values[createdField.ID] = json.RawMessage(fmt.Sprintf(`" %s "`, value)) // value should be sanitized patchedValues, resp, err := th.Client.PatchCPAValues(context.Background(), values) CheckOKStatus(t, resp) require.NoError(t, err) require.NotEmpty(t, patchedValues) require.Len(t, patchedValues, 1) - require.Equal(t, value, patchedValues[createdField.ID]) + var actualValue string + require.NoError(t, json.Unmarshal(patchedValues[createdField.ID], &actualValue)) + require.Equal(t, value, actualValue) values, resp, err = th.Client.ListCPAValues(context.Background(), th.BasicUser.Id) CheckOKStatus(t, resp) require.NoError(t, err) require.NotEmpty(t, values) require.Len(t, values, 1) - require.Equal(t, "Field Value", values[createdField.ID]) + actualValue = "" + require.NoError(t, json.Unmarshal(values[createdField.ID], &actualValue)) + require.Equal(t, value, actualValue) }) t.Run("any team member should be able to patch their own values", func(t *testing.T) { @@ -321,15 +500,51 @@ func TestPatchCPAValues(t *testing.T) { require.Len(t, values, 1) value := "Updated Field Value" - values[createdField.ID] = fmt.Sprintf(" %s \t", value) // value should be sanitized + values[createdField.ID] = json.RawMessage(fmt.Sprintf(`" %s \t"`, value)) // value should be sanitized patchedValues, resp, err := th.Client.PatchCPAValues(context.Background(), values) CheckOKStatus(t, resp) require.NoError(t, err) - require.Equal(t, value, patchedValues[createdField.ID]) + var actualValue string + require.NoError(t, json.Unmarshal(patchedValues[createdField.ID], &actualValue)) + require.Equal(t, value, actualValue) values, resp, err = th.Client.ListCPAValues(context.Background(), th.BasicUser.Id) CheckOKStatus(t, resp) require.NoError(t, err) - require.Equal(t, value, values[createdField.ID]) + actualValue = "" + require.NoError(t, json.Unmarshal(values[createdField.ID], &actualValue)) + require.Equal(t, value, actualValue) + }) + + t.Run("should handle array values correctly", func(t *testing.T) { + arrayField := &model.PropertyField{ + Name: model.NewId(), + Type: model.PropertyFieldTypeMultiselect, + } + createdArrayField, appErr := th.App.CreateCPAField(arrayField) + require.Nil(t, appErr) + require.NotNil(t, createdArrayField) + + values := map[string]json.RawMessage{ + createdArrayField.ID: json.RawMessage(`["option1", "option2", "option3"]`), + } + patchedValues, resp, err := th.Client.PatchCPAValues(context.Background(), values) + CheckOKStatus(t, resp) + require.NoError(t, err) + require.NotEmpty(t, patchedValues) + + var actualValues []string + require.NoError(t, json.Unmarshal(patchedValues[createdArrayField.ID], &actualValues)) + require.Equal(t, []string{"option1", "option2", "option3"}, actualValues) + + // Test updating array values + values[createdArrayField.ID] = json.RawMessage(`["newOption1", "newOption2"]`) + patchedValues, resp, err = th.Client.PatchCPAValues(context.Background(), values) + CheckOKStatus(t, resp) + require.NoError(t, err) + + actualValues = nil + require.NoError(t, json.Unmarshal(patchedValues[createdArrayField.ID], &actualValues)) + require.Equal(t, []string{"newOption1", "newOption2"}, actualValues) }) } diff --git a/server/channels/app/custom_profile_attributes.go b/server/channels/app/custom_profile_attributes.go index d99ca72ae9..a3cd1fafed 100644 --- a/server/channels/app/custom_profile_attributes.go +++ b/server/channels/app/custom_profile_attributes.go @@ -4,6 +4,7 @@ package app import ( + "encoding/json" "net/http" "github.com/mattermost/mattermost/server/public/model" @@ -191,7 +192,7 @@ func (a *App) GetCPAValue(valueID string) (*model.PropertyValue, *model.AppError return value, nil } -func (a *App) PatchCPAValue(userID string, fieldID string, value string) (*model.PropertyValue, *model.AppError) { +func (a *App) PatchCPAValue(userID string, fieldID string, value json.RawMessage) (*model.PropertyValue, *model.AppError) { groupID, err := a.cpaGroupID() if err != nil { return nil, model.NewAppError("PatchCPAValues", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) diff --git a/server/channels/app/custom_profile_attributes_test.go b/server/channels/app/custom_profile_attributes_test.go index bf0ba4e9f2..e972f270fb 100644 --- a/server/channels/app/custom_profile_attributes_test.go +++ b/server/channels/app/custom_profile_attributes_test.go @@ -4,6 +4,7 @@ package app import ( + "encoding/json" "fmt" "net/http" "os" @@ -295,7 +296,7 @@ func TestDeleteCPAField(t *testing.T) { TargetType: "user", GroupID: cpaGroupID, FieldID: createdField.ID, - Value: fmt.Sprintf("Value %d", i), + Value: json.RawMessage(fmt.Sprintf(`"Value %d"`, i)), } value, err := th.App.Srv().propertyService.CreatePropertyValue(newValue) require.NoError(t, err) @@ -375,7 +376,7 @@ func TestGetCPAValue(t *testing.T) { TargetType: "user", GroupID: model.NewId(), FieldID: fieldID, - Value: "Value", + Value: json.RawMessage(`"Value"`), } propertyValue, err := th.App.Srv().propertyService.CreatePropertyValue(propertyValue) require.NoError(t, err) @@ -391,7 +392,7 @@ func TestGetCPAValue(t *testing.T) { TargetType: "user", GroupID: cpaGroupID, FieldID: fieldID, - Value: "Value", + Value: json.RawMessage(`"Value"`), } propertyValue, err := th.App.Srv().propertyService.CreatePropertyValue(propertyValue) require.NoError(t, err) @@ -400,6 +401,33 @@ func TestGetCPAValue(t *testing.T) { require.Nil(t, appErr) require.NotNil(t, pv) }) + + t.Run("should handle array values correctly", func(t *testing.T) { + arrayField := &model.PropertyField{ + GroupID: cpaGroupID, + Name: model.NewId(), + Type: model.PropertyFieldTypeMultiselect, + } + createdField, err := th.App.Srv().propertyService.CreatePropertyField(arrayField) + require.NoError(t, err) + + propertyValue := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "user", + GroupID: cpaGroupID, + FieldID: createdField.ID, + Value: json.RawMessage(`["option1", "option2", "option3"]`), + } + propertyValue, err = th.App.Srv().propertyService.CreatePropertyValue(propertyValue) + require.NoError(t, err) + + pv, appErr := th.App.GetCPAValue(propertyValue.ID) + require.Nil(t, appErr) + require.NotNil(t, pv) + var arrayValues []string + require.NoError(t, json.Unmarshal(pv.Value, &arrayValues)) + require.Equal(t, []string{"option1", "option2", "option3"}, arrayValues) + }) } func TestPatchCPAValue(t *testing.T) { @@ -413,7 +441,7 @@ func TestPatchCPAValue(t *testing.T) { t.Run("should fail if the field doesn't exist", func(t *testing.T) { invalidFieldID := model.NewId() - _, appErr := th.App.PatchCPAValue(model.NewId(), invalidFieldID, "fieldValue") + _, appErr := th.App.PatchCPAValue(model.NewId(), invalidFieldID, json.RawMessage(`"fieldValue"`)) require.NotNil(t, appErr) }) @@ -427,18 +455,18 @@ func TestPatchCPAValue(t *testing.T) { require.NoError(t, err) userID := model.NewId() - patchedValue, appErr := th.App.PatchCPAValue(userID, createdField.ID, "test value") + patchedValue, appErr := th.App.PatchCPAValue(userID, createdField.ID, json.RawMessage(`"test value"`)) require.Nil(t, appErr) require.NotNil(t, patchedValue) - require.Equal(t, "test value", patchedValue.Value) + require.Equal(t, json.RawMessage(`"test value"`), patchedValue.Value) require.Equal(t, userID, patchedValue.TargetID) t.Run("should correctly patch the CPA property value", func(t *testing.T) { - patch2, appErr := th.App.PatchCPAValue(userID, createdField.ID, "new patched value") + patch2, appErr := th.App.PatchCPAValue(userID, createdField.ID, json.RawMessage(`"new patched value"`)) require.Nil(t, appErr) require.NotNil(t, patch2) require.Equal(t, patchedValue.ID, patch2.ID) - require.Equal(t, "new patched value", patch2.Value) + require.Equal(t, json.RawMessage(`"new patched value"`), patch2.Value) require.Equal(t, userID, patch2.TargetID) }) }) @@ -455,8 +483,37 @@ func TestPatchCPAValue(t *testing.T) { require.NoError(t, err) userID := model.NewId() - patchedValue, appErr := th.App.PatchCPAValue(userID, createdField.ID, "test value") + patchedValue, appErr := th.App.PatchCPAValue(userID, createdField.ID, json.RawMessage(`"test value"`)) require.NotNil(t, appErr) require.Nil(t, patchedValue) }) + + t.Run("should handle array values correctly", func(t *testing.T) { + arrayField := &model.PropertyField{ + GroupID: cpaGroupID, + Name: model.NewId(), + Type: model.PropertyFieldTypeMultiselect, + } + createdField, err := th.App.Srv().propertyService.CreatePropertyField(arrayField) + require.NoError(t, err) + + userID := model.NewId() + patchedValue, appErr := th.App.PatchCPAValue(userID, createdField.ID, json.RawMessage(`["option1", "option2", "option3"]`)) + require.Nil(t, appErr) + require.NotNil(t, patchedValue) + var arrayValues []string + require.NoError(t, json.Unmarshal(patchedValue.Value, &arrayValues)) + require.Equal(t, []string{"option1", "option2", "option3"}, arrayValues) + require.Equal(t, userID, patchedValue.TargetID) + + // Update array values + updatedValue, appErr := th.App.PatchCPAValue(userID, createdField.ID, json.RawMessage(`["newOption1", "newOption2"]`)) + require.Nil(t, appErr) + require.NotNil(t, updatedValue) + require.Equal(t, patchedValue.ID, updatedValue.ID) + arrayValues = nil + require.NoError(t, json.Unmarshal(updatedValue.Value, &arrayValues)) + require.Equal(t, []string{"newOption1", "newOption2"}, arrayValues) + require.Equal(t, userID, updatedValue.TargetID) + }) } diff --git a/server/channels/store/sqlstore/property_value_store.go b/server/channels/store/sqlstore/property_value_store.go index a57604203f..80c686aa0d 100644 --- a/server/channels/store/sqlstore/property_value_store.go +++ b/server/channels/store/sqlstore/property_value_store.go @@ -4,8 +4,6 @@ package sqlstore import ( - "database/sql" - "encoding/json" "fmt" sq "github.com/mattermost/squirrel" @@ -15,89 +13,6 @@ import ( "github.com/mattermost/mattermost/server/v8/channels/store" ) -func (s *SqlPropertyValueStore) propertyValueToInsertMap(value *model.PropertyValue) (map[string]any, error) { - valueJSON, err := json.Marshal(value.Value) - if err != nil { - return nil, errors.Wrap(err, "property_value_to_insert_map_marshal_value") - } - if s.IsBinaryParamEnabled() { - valueJSON = AppendBinaryFlag(valueJSON) - } - - return map[string]any{ - "ID": value.ID, - "TargetID": value.TargetID, - "TargetType": value.TargetType, - "GroupID": value.GroupID, - "FieldID": value.FieldID, - "Value": valueJSON, - "CreateAt": value.CreateAt, - "UpdateAt": value.UpdateAt, - "DeleteAt": value.DeleteAt, - }, nil -} - -func (s *SqlPropertyValueStore) propertyValueToUpdateMap(value *model.PropertyValue) (map[string]any, error) { - valueJSON, err := json.Marshal(value.Value) - if err != nil { - return nil, errors.Wrap(err, "property_value_to_udpate_map_marshal_value") - } - if s.IsBinaryParamEnabled() { - valueJSON = AppendBinaryFlag(valueJSON) - } - - return map[string]any{ - "Value": valueJSON, - "UpdateAt": value.UpdateAt, - "DeleteAt": value.DeleteAt, - }, nil -} - -func propertyValuesFromRows(rows *sql.Rows) ([]*model.PropertyValue, error) { - results := []*model.PropertyValue{} - - for rows.Next() { - var value model.PropertyValue - var valueJSON string - - err := rows.Scan( - &value.ID, - &value.TargetID, - &value.TargetType, - &value.GroupID, - &value.FieldID, - &valueJSON, - &value.CreateAt, - &value.UpdateAt, - &value.DeleteAt, - ) - if err != nil { - return nil, err - } - - if err := json.Unmarshal([]byte(valueJSON), &value.Value); err != nil { - return nil, errors.Wrap(err, "property_values_from_rows_unmarshal_value") - } - - results = append(results, &value) - } - - return results, nil -} - -func propertyValueFromRows(rows *sql.Rows) (*model.PropertyValue, error) { - values, err := propertyValuesFromRows(rows) - if err != nil { - return nil, err - } - - if len(values) > 0 { - return values[0], nil - } - - return nil, sql.ErrNoRows -} - type SqlPropertyValueStore struct { *SqlStore @@ -125,15 +40,15 @@ func (s *SqlPropertyValueStore) Create(value *model.PropertyValue) (*model.Prope return nil, errors.Wrap(err, "property_value_create_isvalid") } - insertMap, err := s.propertyValueToInsertMap(value) - if err != nil { - return nil, err + valueJSON := value.Value + if s.IsBinaryParamEnabled() { + valueJSON = AppendBinaryFlag(valueJSON) } builder := s.getQueryBuilder(). Insert("PropertyValues"). - SetMap(insertMap) - + Columns("ID", "TargetID", "TargetType", "GroupID", "FieldID", "Value", "CreateAt", "UpdateAt", "DeleteAt"). + Values(value.ID, value.TargetID, value.TargetType, value.GroupID, value.FieldID, valueJSON, value.CreateAt, value.UpdateAt, value.DeleteAt) if _, err := s.GetMaster().ExecBuilder(builder); err != nil { return nil, errors.Wrap(err, "property_value_create_insert") } @@ -142,45 +57,23 @@ func (s *SqlPropertyValueStore) Create(value *model.PropertyValue) (*model.Prope } func (s *SqlPropertyValueStore) Get(id string) (*model.PropertyValue, error) { - queryString, args, err := s.tableSelectQuery. - Where(sq.Eq{"id": id}). - ToSql() - if err != nil { - return nil, errors.Wrap(err, "property_value_get_tosql") - } + builder := s.tableSelectQuery.Where(sq.Eq{"id": id}) - rows, err := s.GetReplica().Query(queryString, args...) - if err != nil { + var value model.PropertyValue + if err := s.GetReplica().GetBuilder(&value, builder); err != nil { return nil, errors.Wrap(err, "property_value_get_select") } - defer rows.Close() - value, err := propertyValueFromRows(rows) - if err != nil { - return nil, errors.Wrap(err, "property_value_get_propertyvaluefromrows") - } - - return value, nil + return &value, nil } func (s *SqlPropertyValueStore) GetMany(ids []string) ([]*model.PropertyValue, error) { - queryString, args, err := s.tableSelectQuery. - Where(sq.Eq{"id": ids}). - ToSql() - if err != nil { - return nil, errors.Wrap(err, "property_value_get_many_tosql") - } + builder := s.tableSelectQuery.Where(sq.Eq{"id": ids}) - rows, err := s.GetReplica().Query(queryString, args...) - if err != nil { + var values []*model.PropertyValue + if err := s.GetReplica().SelectBuilder(&values, builder); err != nil { return nil, errors.Wrap(err, "property_value_get_many_query") } - defer rows.Close() - - values, err := propertyValuesFromRows(rows) - if err != nil { - return nil, errors.Wrap(err, "property_value_get_many_propertyvaluesfromrows") - } if len(values) < len(ids) { return nil, fmt.Errorf("missmatch results: got %d results of the %d ids passed", len(values), len(ids)) @@ -198,46 +91,35 @@ func (s *SqlPropertyValueStore) SearchPropertyValues(opts model.PropertyValueSea return nil, errors.New("per page must be positive integer greater than zero") } - query := s.tableSelectQuery. + builder := s.tableSelectQuery. OrderBy("CreateAt ASC"). Offset(uint64(opts.Page * opts.PerPage)). Limit(uint64(opts.PerPage)) if !opts.IncludeDeleted { - query = query.Where(sq.Eq{"DeleteAt": 0}) + builder = builder.Where(sq.Eq{"DeleteAt": 0}) } if opts.GroupID != "" { - query = query.Where(sq.Eq{"GroupID": opts.GroupID}) + builder = builder.Where(sq.Eq{"GroupID": opts.GroupID}) } if opts.TargetType != "" { - query = query.Where(sq.Eq{"TargetType": opts.TargetType}) + builder = builder.Where(sq.Eq{"TargetType": opts.TargetType}) } if opts.TargetID != "" { - query = query.Where(sq.Eq{"TargetID": opts.TargetID}) + builder = builder.Where(sq.Eq{"TargetID": opts.TargetID}) } if opts.FieldID != "" { - query = query.Where(sq.Eq{"FieldID": opts.FieldID}) + builder = builder.Where(sq.Eq{"FieldID": opts.FieldID}) } - queryString, args, err := query.ToSql() - if err != nil { - return nil, errors.Wrap(err, "property_value_search_tosql") - } - - rows, err := s.GetReplica().Query(queryString, args...) - if err != nil { + var values []*model.PropertyValue + if err := s.GetReplica().SelectBuilder(&values, builder); err != nil { return nil, errors.Wrap(err, "property_value_search_query") } - defer rows.Close() - - values, err := propertyValuesFromRows(rows) - if err != nil { - return nil, errors.Wrap(err, "property_value_search_propertyvaluesfromrows") - } return values, nil } @@ -261,14 +143,16 @@ func (s *SqlPropertyValueStore) Update(values []*model.PropertyValue) (_ []*mode return nil, errors.Wrap(err, "property_value_update_isvalid") } - updateMap, err := s.propertyValueToUpdateMap(value) - if err != nil { - return nil, err + valueJSON := value.Value + if s.IsBinaryParamEnabled() { + valueJSON = AppendBinaryFlag(valueJSON) } queryString, args, err := s.getQueryBuilder(). Update("PropertyValues"). - SetMap(updateMap). + Set("Value", valueJSON). + Set("UpdateAt", value.UpdateAt). + Set("DeleteAt", value.DeleteAt). Where(sq.Eq{"id": value.ID}). ToSql() if err != nil { diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index c43fd6810e..271f1500d0 100644 --- a/server/channels/store/storetest/property_value_store.go +++ b/server/channels/store/storetest/property_value_store.go @@ -5,6 +5,7 @@ package storetest import ( "database/sql" + "encoding/json" "fmt" "testing" "time" @@ -17,6 +18,7 @@ import ( func TestPropertyValueStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) { t.Run("CreatePropertyValue", func(t *testing.T) { testCreatePropertyValue(t, rctx, ss) }) + t.Run("CreatePropertyValueWithArray", func(t *testing.T) { testCreatePropertyValueWithArray(t, rctx, ss) }) t.Run("GetPropertyValue", func(t *testing.T) { testGetPropertyValue(t, rctx, ss) }) t.Run("GetManyPropertyValues", func(t *testing.T) { testGetManyPropertyValues(t, rctx, ss) }) t.Run("UpdatePropertyValue", func(t *testing.T) { testUpdatePropertyValue(t, rctx, ss) }) @@ -51,7 +53,7 @@ func testCreatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: model.NewId(), - Value: "test value", + Value: json.RawMessage(`"test value"`), } t.Run("should be able to create a property value", func(t *testing.T) { @@ -84,7 +86,7 @@ func testGetPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: model.NewId(), - Value: "test value", + Value: json.RawMessage(`"test value"`), } _, err := ss.PropertyValue().Create(newValue) require.NoError(t, err) @@ -111,7 +113,7 @@ func testGetManyPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: model.NewId(), - Value: fmt.Sprintf("test value %d", i), + Value: json.RawMessage(fmt.Sprintf(`"test value %d"`, i)), } _, err := ss.PropertyValue().Create(newValue) require.NoError(t, err) @@ -142,7 +144,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: model.NewId(), - Value: "test value", + Value: json.RawMessage(`"test value"`), CreateAt: model.GetMillis(), } updatedValue, err := ss.PropertyValue().Update([]*model.PropertyValue{value}) @@ -157,7 +159,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: model.NewId(), - Value: "test value", + Value: json.RawMessage(`"test value"`), } _, err := ss.PropertyValue().Create(value) require.NoError(t, err) @@ -181,7 +183,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: model.NewId(), - Value: "value 1", + Value: json.RawMessage(`"value 1"`), } value2 := &model.PropertyValue{ @@ -189,7 +191,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: model.NewId(), - Value: "value 2", + Value: json.RawMessage(`"value 2"`), } for _, value := range []*model.PropertyValue{value1, value2} { @@ -199,8 +201,8 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { } time.Sleep(10 * time.Millisecond) - value1.Value = "updated value 1" - value2.Value = "updated value 2" + value1.Value = json.RawMessage(`"updated value 1"`) + value2.Value = json.RawMessage(`"updated value 2"`) _, err := ss.PropertyValue().Update([]*model.PropertyValue{value1, value2}) require.NoError(t, err) @@ -208,13 +210,13 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { // Verify first value updated1, err := ss.PropertyValue().Get(value1.ID) require.NoError(t, err) - require.Equal(t, "updated value 1", updated1.Value) + require.Equal(t, json.RawMessage(`"updated value 1"`), updated1.Value) require.Greater(t, updated1.UpdateAt, updated1.CreateAt) // Verify second value updated2, err := ss.PropertyValue().Get(value2.ID) require.NoError(t, err) - require.Equal(t, "updated value 2", updated2.Value) + require.Equal(t, json.RawMessage(`"updated value 2"`), updated2.Value) require.Greater(t, updated2.UpdateAt, updated2.CreateAt) }) @@ -226,7 +228,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: groupID, FieldID: model.NewId(), - Value: "Value 1", + Value: json.RawMessage(`"Value 1"`), } value2 := &model.PropertyValue{ @@ -234,7 +236,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: groupID, FieldID: model.NewId(), - Value: "Value 2", + Value: json.RawMessage(`"Value 2"`), } for _, value := range []*model.PropertyValue{value1, value2} { @@ -246,7 +248,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { originalUpdateAt2 := value2.UpdateAt // Try to update both value, but make one invalid - value1.Value = "Valid update" + value1.Value = json.RawMessage(`"Valid update"`) value2.GroupID = "Invalid ID" _, err := ss.PropertyValue().Update([]*model.PropertyValue{value1, value2}) @@ -256,7 +258,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { // Check that values were not updated updated1, err := ss.PropertyValue().Get(value1.ID) require.NoError(t, err) - require.Equal(t, "Value 1", updated1.Value) + require.Equal(t, json.RawMessage(`"Value 1"`), updated1.Value) require.Equal(t, originalUpdateAt1, updated1.UpdateAt) updated2, err := ss.PropertyValue().Get(value2.ID) @@ -279,7 +281,7 @@ func testDeletePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: model.NewId(), - Value: "test value", + Value: json.RawMessage(`"test value"`), } value, err := ss.PropertyValue().Create(newValue) require.NoError(t, err) @@ -300,7 +302,7 @@ func testDeletePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: model.NewId(), - Value: "test value", + Value: json.RawMessage(`"test value"`), } value, err := ss.PropertyValue().Create(sameDetailsValue) require.NoError(t, err) @@ -320,7 +322,7 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { TargetID: targetID, TargetType: "test_type", FieldID: fieldID, - Value: "value 1", + Value: json.RawMessage(`"value 1"`), } value2 := &model.PropertyValue{ @@ -328,7 +330,7 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { TargetID: targetID, TargetType: "other_type", FieldID: model.NewId(), - Value: "value 2", + Value: json.RawMessage(`"value 2"`), } value3 := &model.PropertyValue{ @@ -336,7 +338,7 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { TargetID: model.NewId(), TargetType: "test_type", FieldID: model.NewId(), - Value: "value 3", + Value: json.RawMessage(`"value 3"`), } value4 := &model.PropertyValue{ @@ -344,7 +346,7 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { TargetID: model.NewId(), TargetType: "test_type", FieldID: fieldID, - Value: "value 4", + Value: json.RawMessage(`"value 4"`), } for _, value := range []*model.PropertyValue{value1, value2, value3, value4} { @@ -484,6 +486,56 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { } } +func testCreatePropertyValueWithArray(t *testing.T, _ request.CTX, ss store.Store) { + t.Run("should create a property value with array", func(t *testing.T) { + newValue := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`["option1", "option2", "option3"]`), + } + + value, err := ss.PropertyValue().Create(newValue) + require.NoError(t, err) + require.NotZero(t, value.ID) + require.NotZero(t, value.CreateAt) + require.NotZero(t, value.UpdateAt) + require.Zero(t, value.DeleteAt) + + // Verify array values + var arrayValues []string + require.NoError(t, json.Unmarshal(value.Value, &arrayValues)) + require.Equal(t, []string{"option1", "option2", "option3"}, arrayValues) + }) + + t.Run("should update array values", func(t *testing.T) { + value := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`["initial1", "initial2"]`), + } + + created, err := ss.PropertyValue().Create(value) + require.NoError(t, err) + require.NotZero(t, created.ID) + + created.Value = json.RawMessage(`["updated1", "updated2", "updated3"]`) + updated, err := ss.PropertyValue().Update([]*model.PropertyValue{created}) + require.NoError(t, err) + require.NotZero(t, updated) + + // Verify updated array values + retrieved, err := ss.PropertyValue().Get(created.ID) + require.NoError(t, err) + var arrayValues []string + require.NoError(t, json.Unmarshal(retrieved.Value, &arrayValues)) + require.Equal(t, []string{"updated1", "updated2", "updated3"}, arrayValues) + }) +} + func testDeleteForField(t *testing.T, _ request.CTX, ss store.Store) { fieldID := model.NewId() @@ -493,7 +545,7 @@ func testDeleteForField(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: fieldID, - Value: "value 1", + Value: json.RawMessage(`"value 1"`), } value2 := &model.PropertyValue{ @@ -501,7 +553,7 @@ func testDeleteForField(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: fieldID, - Value: "value 2", + Value: json.RawMessage(`"value 2"`), } value3 := &model.PropertyValue{ @@ -509,7 +561,7 @@ func testDeleteForField(t *testing.T, _ request.CTX, ss store.Store) { TargetType: "test_type", GroupID: model.NewId(), FieldID: model.NewId(), // Different field ID - Value: "value 3", + Value: json.RawMessage(`"value 3"`), } for _, value := range []*model.PropertyValue{value1, value2, value3} { diff --git a/server/i18n/en.json b/server/i18n/en.json index cffe871b7c..64a46d6a29 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -1857,6 +1857,10 @@ "id": "api.custom_groups.no_remote_id", "translation": "remote_id must be blank for custom group" }, + { + "id": "api.custom_profile_attributes.field_not_found", + "translation": "trying to patch a field that does not exist" + }, { "id": "api.custom_profile_attributes.license_error", "translation": "Your license does not support Custom Profile Attributes." diff --git a/server/public/model/client4.go b/server/public/model/client4.go index 6c8910516c..2a6bb06d7b 100644 --- a/server/public/model/client4.go +++ b/server/public/model/client4.go @@ -9465,21 +9465,21 @@ func (c *Client4) DeleteCPAField(ctx context.Context, fieldID string) (*Response return BuildResponse(r), nil } -func (c *Client4) ListCPAValues(ctx context.Context, userID string) (map[string]string, *Response, error) { +func (c *Client4) ListCPAValues(ctx context.Context, userID string) (map[string]json.RawMessage, *Response, error) { r, err := c.DoAPIGet(ctx, c.userCustomProfileAttributesRoute(userID), "") if err != nil { return nil, BuildResponse(r), err } defer closeBody(r) - fields := make(map[string]string) + fields := make(map[string]json.RawMessage) if err := json.NewDecoder(r.Body).Decode(&fields); err != nil { return nil, nil, NewAppError("ListCPAValues", "api.unmarshal_error", nil, "", http.StatusInternalServerError).Wrap(err) } return fields, BuildResponse(r), nil } -func (c *Client4) PatchCPAValues(ctx context.Context, values map[string]string) (map[string]string, *Response, error) { +func (c *Client4) PatchCPAValues(ctx context.Context, values map[string]json.RawMessage) (map[string]json.RawMessage, *Response, error) { buf, err := json.Marshal(values) if err != nil { return nil, nil, NewAppError("PatchCPAValues", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -9491,7 +9491,7 @@ func (c *Client4) PatchCPAValues(ctx context.Context, values map[string]string) } defer closeBody(r) - var patchedValues map[string]string + var patchedValues map[string]json.RawMessage if err := json.NewDecoder(r.Body).Decode(&patchedValues); err != nil { return nil, nil, NewAppError("PatchCPAValues", "api.unmarshal_error", nil, "", http.StatusInternalServerError).Wrap(err) } diff --git a/server/public/model/property_field.go b/server/public/model/property_field.go index 2317a06b5d..4a07d65712 100644 --- a/server/public/model/property_field.go +++ b/server/public/model/property_field.go @@ -71,12 +71,12 @@ func (pf *PropertyField) IsValid() error { return NewAppError("PropertyField.IsValid", "model.property_field.is_valid.app_error", map[string]any{"FieldName": "name", "Reason": "value cannot be empty"}, "id="+pf.ID, http.StatusBadRequest) } - if !(pf.Type == PropertyFieldTypeText || - pf.Type == PropertyFieldTypeSelect || - pf.Type == PropertyFieldTypeMultiselect || - pf.Type == PropertyFieldTypeDate || - pf.Type == PropertyFieldTypeUser || - pf.Type == PropertyFieldTypeMultiuser) { + if pf.Type != PropertyFieldTypeText && + pf.Type != PropertyFieldTypeSelect && + pf.Type != PropertyFieldTypeMultiselect && + pf.Type != PropertyFieldTypeDate && + pf.Type != PropertyFieldTypeUser && + pf.Type != PropertyFieldTypeMultiuser { return NewAppError("PropertyField.IsValid", "model.property_field.is_valid.app_error", map[string]any{"FieldName": "type", "Reason": "unknown value"}, "id="+pf.ID, http.StatusBadRequest) } diff --git a/server/public/model/property_value.go b/server/public/model/property_value.go index 244fed6847..17ef1709be 100644 --- a/server/public/model/property_value.go +++ b/server/public/model/property_value.go @@ -3,18 +3,21 @@ package model -import "net/http" +import ( + "encoding/json" + "net/http" +) type PropertyValue struct { - ID string `json:"id"` - TargetID string `json:"target_id"` - TargetType string `json:"target_type"` - GroupID string `json:"group_id"` - FieldID string `json:"field_id"` - Value string `json:"value"` - CreateAt int64 `json:"create_at"` - UpdateAt int64 `json:"update_at"` - DeleteAt int64 `json:"delete_at"` + ID string `json:"id"` + TargetID string `json:"target_id"` + TargetType string `json:"target_type"` + GroupID string `json:"group_id"` + FieldID string `json:"field_id"` + Value json.RawMessage `json:"value"` + CreateAt int64 `json:"create_at"` + UpdateAt int64 `json:"update_at"` + DeleteAt int64 `json:"delete_at"` } func (pv *PropertyValue) PreSave() {