[MM-62552] Custom Profile Attributes: use json.RawMessage for the value. (#29989)
* refactor: Move property value sanitization to model layer * feat: Add value sanitization for custom profile attributes * refactor: Update custom profile attributes to use json.RawMessage * refactor: Update patchCustomProfileAttribute to handle json.RawMessage directly * refactor: Refactor custom profile attributes handler with improved validation * refactor: Rename `patchCustomProfileAttribute` to `patchCPAValues` * refactor: Replace ReturnJSON with json.NewEncoder and add error logging * feat: Add encoding/json import to property_value.go * refactor: Update property value tests to use json.RawMessage * fix: Convert string value to json.RawMessage in property value test * fix: Convert string literals to json.RawMessage in property value tests * fix: Add missing encoding/json import in custom_profile_attributes.go * fix: Preserve JSON RawMessage type in listCPAValues function * fix: Update custom profile attributes test to use json.RawMessage * feat: Add json import to custom_profile_attributes_test.go * refactor: Update ListCPAValues and PatchCPAValues to use json.RawMessage * refactor: Rename `actualValue` to `updatedValue` in custom profile attributes test * refactor: Improve user permission and audit logging for custom profile attributes patch * refactor: Optimize CPA field lookup by using ListCPAFields() and map * fix: Correct user ID reference in custom profile attributes patch endpoint * refactor: Change patchCPAValues to use map[string]json.RawMessage for results * refactor: format and fix tests * test: Add comprehensive unit tests for sanitizePropertyValue function * test: Add test case for invalid property value type * feat: Use `model.NewId()` to generate valid IDs in custom profile attributes tests * refactor: Replace hardcoded IDs with dynamic variables in custom profile attributes test * refactor: restore variable name * refactor: drop undesired changes * chore: refresh app layers * feat: Update API definition to support string or string array values for custom profile attributes * test: Add test cases for multiselect custom profile attribute values * test: Add tests for multiselect custom profile attribute values * test: Isolate array value test in separate t.Run * test: Add test case for multiselect array values in custom profile attributes * refactor: Move array value test from TestCreateCPAField to TestPatchCPAValue * test: Update custom profile attributes test assertions * test: add test case for handling array values in GetCPAValue * test: Add array value tests for property value store * refactor(store): no need to convert to json the rawmessage * chore: lint * i18n * use model to interface with sqlx * fix: Allow empty strings for text, date, and select profile attributes * refactor: Filter out empty strings in multiselect and multiuser fields * refactor: Update multiuser field sanitization to validate and error on invalid IDs * refactor: Simplify sanitizePropertyValue function with reduced code duplication * fix: Allow empty user ID in custom profile attribute sanitization * refactor: Convert comment-based subtests to nested t.Run in TestSanitizePropertyValue * refactor: Convert comment-based subtests to nested t.Run tests in TestSanitizePropertyValue --------- Co-authored-by: Mattermost Build <build@mattermost.com>
Этот коммит содержится в:
Julien Tant
@@ -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
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
|
||||
Ссылка в новой задаче
Block a user