CPA Display Name Support (#36247)

* Phase 1: CPA display_name + CEL-safe name validation (server)

- Add typed DisplayName field to CPAAttrs + display_name attr key constant.
- Add ValidateCPAFieldName helper enforcing CEL IDENTIFIER + reserved-word blacklist.
- Wire validation into App.CreateCPAField (always) and App.PatchCPAField (lenient grandfather: skip when Name unchanged).
- Trim + 255-rune cap DisplayName in CPAField.SanitizeAndValidate.
- Developer-facing godoc note documenting rule, sources of truth, and Option C scoping.
- Asserting test for documented Option C plugin-API bypass (closed by PR #36173).

Spec: planner/projects/property-display-name/ideas/001-cpa-display-name/spec.md
Plan: .planning/phase-1/PLAN.md
Made-with: Cursor

* Phase 1 (review): address Reza's Major + Minor findings

- Rename misleading subtest "empty DisplayName is omitted from attrs"
  to "empty DisplayName round-trips as empty string" (Major #1).
- Add TestCPAAttrs_JSONOmitEmpty pinning the omitempty wire-format
  contract that PR #36173's typed-attrs strategy relies on (Major #1).
- Extend TestValidateCPAFieldName: case-sensitivity (IN/In ok),
  single-character names (a/_/A ok), missing "as" reserved word
  (Minor #2). Add whitespace-only DisplayName case (Minor #2).
- Document PropertyFieldNameMaxRunes reuse in SanitizeAndValidate
  to prevent drift (Minor #3).
- Replace broken PLAN-server.md reference in bypass-test docstring
  with in-tree CPAAttrs godoc reference (Minor #4).
- Document omitempty semantics on CPAAttrs.DisplayName field to
  prevent the same misreading caught in review (Minor #5).
- Document grouping intent above CPAFieldNameReservedWords (Minor #8).

Review: .planning/phase-1/REVIEW.md
Made-with: Cursor

* Phase 2: in-app backfill migration for CPA display_name

- Add cpaDisplayNameBackfillKey + cpaDisplayNameBackfillVersion constants.
- Implement (*Server).doSetupCPADisplayNameBackfill: idempotent, cursor-paged
  scan over CPA group fields; backfill attrs.display_name = name when empty.
- Register in m1 migration slice in doAppMigrations (mlog.Fatal on error,
  matching existing convention).
- Three migration tests: NoExistingFields, BackfillsMissing, Idempotent.

System-key idempotency + per-field DisplayName-empty check together provide
HA-safe behavior on rolling deploys (last-write-wins on the System key;
data-level idempotency from the per-field check).

Spec: planner/projects/property-display-name/ideas/001-cpa-display-name/spec.md
Plan: .planning/phase-2/PLAN.md
Made-with: Cursor

* Phase 2 (review): document race + harden idempotency test

- Document SearchPropertyFields→UpdatePropertyFields rolling-deploy
  race: stale snapshot can revert concurrent admin CPA rename. Pre-
  existing systemic shape (no UpdateAt optimistic-lock); narrow
  window; bounded blast radius (admin re-rename, ABAC ID-keyed).
  Accepted limitation per spec Out of Scope (Major #1, Option C).
- Tighten TestCPADisplayNameBackfill_Idempotent: snapshot UpdateAt
  before second run; assert no DB write on the System key or the
  field row (Major #2).
- Extract clearCPABackfillMarker helper with explanatory godoc to
  centralize the 3x-repeated test precondition (Minor #1).
- Comment fieldA seed as the "key-present-as-empty-string" idempotency
  boundary case (Minor #6).
- Add godoc to doSetupCPADisplayNameBackfill (Minor #10).

Review: .planning/phase-2/REVIEW.md
Made-with: Cursor

* Linting

* Removing unnecessary comments

* Clean up tests

* Linting

* Fix tests

* Updated API doc

* Fix tests

* PR Feedback

* Move migration to PropertyService

* Linting

* Linting

* Removed pagination

---------

Co-authored-by: Mattermost Build <build@mattermost.com>
This commit is contained in:
Maria A Nunez
2026-05-04 10:33:05 -04:00
committed by GitHub
co-authored by Mattermost Build
parent 846791aa65
commit 724c5b7191
11 changed files with 842 additions and 70 deletions
@@ -46,6 +46,13 @@
properties:
name:
type: string
description: >
The internal identifier for this attribute. Must match
`^[A-Za-z_][A-Za-z0-9_]*$` and must not be a CEL reserved
word (true, false, null, in, as, break, const, continue, else,
for, function, if, import, let, loop, package, namespace,
return, var, void, while). This name is used in ABAC policy
expressions as `user.attributes.<name>`.
type:
type: string
attrs:
@@ -90,6 +97,11 @@
description: "Access mode of the field"
enum: ["", "source_only", "shared_only"]
default: ""
display_name:
type: string
description: >
Human-readable label shown in the UI. Defaults to the field
`name` when omitted or empty. Maximum 255 characters.
responses:
"201":
description: Custom Profile Attribute field creation successful
@@ -103,6 +115,15 @@
$ref: "#/components/responses/Unauthorized"
"403":
$ref: "#/components/responses/Forbidden"
"422":
description: >
Validation error. Returned when `name` does not match the
required identifier pattern, is a CEL reserved word, or when
`attrs.display_name` exceeds 255 characters.
content:
application/json:
schema:
$ref: "#/components/schemas/AppError"
"/api/v4/custom_profile_attributes/fields/{field_id}":
patch:
@@ -140,6 +161,12 @@
properties:
name:
type: string
description: >
New name for the attribute. When changed, must match
`^[A-Za-z_][A-Za-z0-9_]*$` and must not be a CEL reserved
word. Pre-existing fields with non-conforming names remain
patchable on all other attributes; the validation only fires
when `name` actually changes.
type:
type: string
attrs:
@@ -186,6 +213,10 @@
description: "Access mode of the field"
enum: ["", "source_only", "shared_only"]
default: ""
display_name:
type: string
description: >
Human-readable label shown in the UI. Maximum 255 characters.
responses:
"200":
description: Custom Profile Attribute field patch successful
@@ -199,6 +230,15 @@
$ref: "#/components/responses/Unauthorized"
"403":
$ref: "#/components/responses/Forbidden"
"422":
description: >
Validation error. Returned when a `name` change does not match
the required identifier pattern, is a CEL reserved word, or
when `attrs.display_name` exceeds 255 characters.
content:
application/json:
schema:
$ref: "#/components/schemas/AppError"
delete:
tags:
@@ -16,6 +16,10 @@ import (
"github.com/stretchr/testify/require"
)
func celSafeName() string {
return "f_" + model.NewId()
}
func TestCreateCPAField(t *testing.T) {
mainHelper.Parallel(t)
th := SetupConfig(t, func(cfg *model.Config) {
@@ -23,7 +27,7 @@ func TestCreateCPAField(t *testing.T) {
})
th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) {
field := &model.PropertyField{Name: model.NewId(), Type: model.PropertyFieldTypeText}
field := &model.PropertyField{Name: celSafeName(), Type: model.PropertyFieldTypeText}
createdField, resp, err := client.CreateCPAField(context.Background(), field)
CheckForbiddenStatus(t, resp)
@@ -37,7 +41,7 @@ func TestCreateCPAField(t *testing.T) {
t.Run("a user without admin permissions should not be able to create a field", func(t *testing.T) {
field := &model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
}
@@ -47,7 +51,7 @@ func TestCreateCPAField(t *testing.T) {
})
th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) {
field := &model.PropertyField{Name: model.NewId()}
field := &model.PropertyField{Name: celSafeName()}
createdField, resp, err := client.CreateCPAField(context.Background(), field)
CheckBadRequestStatus(t, resp)
@@ -58,7 +62,7 @@ func TestCreateCPAField(t *testing.T) {
th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) {
webSocketClient := th.CreateConnectedWebSocketClient(t)
name := model.NewId()
name := celSafeName()
field := &model.PropertyField{
Name: fmt.Sprintf(" %s\t", name), // name should be sanitized
Type: model.PropertyFieldTypeText,
@@ -96,7 +100,7 @@ func TestCreateCPAField(t *testing.T) {
th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) {
managedField := &model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{
model.CustomProfileAttributesPropertyAttrsManaged: "admin",
@@ -121,7 +125,7 @@ func TestListCPAFields(t *testing.T) {
})
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
Attrs: map[string]any{"visibility": "when_set"},
})
@@ -167,7 +171,7 @@ func TestPatchCPAField(t *testing.T) {
})
th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) {
patch := &model.PropertyFieldPatch{Name: model.NewPointer(model.NewId())}
patch := &model.PropertyFieldPatch{Name: model.NewPointer(celSafeName())}
patchedField, resp, err := client.PatchCPAField(context.Background(), model.NewId(), patch)
CheckForbiddenStatus(t, resp)
require.Error(t, err)
@@ -180,7 +184,7 @@ func TestPatchCPAField(t *testing.T) {
t.Run("a user without admin permissions should not be able to patch a field", func(t *testing.T) {
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -189,7 +193,7 @@ func TestPatchCPAField(t *testing.T) {
require.Nil(t, appErr)
require.NotNil(t, createdField)
patch := &model.PropertyFieldPatch{Name: model.NewPointer(model.NewId())}
patch := &model.PropertyFieldPatch{Name: model.NewPointer(celSafeName())}
_, resp, err := th.Client.PatchCPAField(context.Background(), createdField.ID, patch)
CheckForbiddenStatus(t, resp)
require.Error(t, err)
@@ -199,7 +203,7 @@ func TestPatchCPAField(t *testing.T) {
webSocketClient := th.CreateConnectedWebSocketClient(t)
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -208,7 +212,7 @@ func TestPatchCPAField(t *testing.T) {
require.Nil(t, appErr)
require.NotNil(t, createdField)
newName := model.NewId()
newName := celSafeName()
patch := &model.PropertyFieldPatch{Name: model.NewPointer(fmt.Sprintf(" %s \t ", newName))} // name should be sanitized
patchedField, resp, err := client.PatchCPAField(context.Background(), createdField.ID, patch)
CheckOKStatus(t, resp)
@@ -241,7 +245,7 @@ func TestPatchCPAField(t *testing.T) {
optionID1 := model.NewId()
optionID2 := model.NewId()
selectField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeSelect,
Attrs: model.StringInterface{
"options": []map[string]any{
@@ -303,7 +307,7 @@ func TestPatchCPAField(t *testing.T) {
th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) {
// Create a regular field first
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -361,7 +365,7 @@ func TestDeleteCPAField(t *testing.T) {
t.Run("a user without admin permissions should not be able to delete a field", func(t *testing.T) {
field := &model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
}
createdField, _, err := th.SystemAdminClient.CreateCPAField(context.Background(), field)
@@ -377,7 +381,7 @@ func TestDeleteCPAField(t *testing.T) {
webSocketClient := th.CreateConnectedWebSocketClient(t)
field := &model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
}
createdField, _, err := th.SystemAdminClient.CreateCPAField(context.Background(), field)
@@ -426,7 +430,7 @@ func TestListCPAValues(t *testing.T) {
defer th.AddPermissionToRole(t, model.PermissionViewMembers.Id, model.SystemUserRoleId)
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -464,7 +468,7 @@ func TestListCPAValues(t *testing.T) {
optionID1 := model.NewId()
optionID2 := model.NewId()
arrayField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeMultiselect,
Attrs: model.StringInterface{
"options": []map[string]any{
@@ -511,7 +515,7 @@ func TestPatchCPAValues(t *testing.T) {
}).InitBasic(t)
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -606,7 +610,7 @@ func TestPatchCPAValues(t *testing.T) {
optionsID := []string{model.NewId(), model.NewId(), model.NewId(), model.NewId()}
arrayField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeMultiselect,
Attrs: model.StringInterface{
"options": []map[string]any{
@@ -649,7 +653,7 @@ func TestPatchCPAValues(t *testing.T) {
t.Run("should fail if any of the values belongs to a field that is LDAP/SAML synced", func(t *testing.T) {
// Create a field with LDAP attribute
ldapField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{
model.CustomProfileAttributesPropertyAttrsLDAP: "ldap_attr",
@@ -663,7 +667,7 @@ func TestPatchCPAValues(t *testing.T) {
// Create a field with SAML attribute
samlField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{
model.CustomProfileAttributesPropertyAttrsSAML: "saml_attr",
@@ -706,7 +710,7 @@ func TestPatchCPAValues(t *testing.T) {
t.Run("an invalid patch should be rejected", func(t *testing.T) {
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -730,7 +734,7 @@ func TestPatchCPAValues(t *testing.T) {
t.Run("admin-managed fields", func(t *testing.T) {
// Create a managed field (only admins can create fields)
managedField := &model.PropertyField{
Name: "Managed Field",
Name: "managed_field",
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{
model.CustomProfileAttributesPropertyAttrsManaged: "admin",
@@ -744,7 +748,7 @@ func TestPatchCPAValues(t *testing.T) {
// Create a non-managed field for comparison
regularField := &model.PropertyField{
Name: "Regular Field",
Name: "regular_field",
Type: model.PropertyFieldTypeText,
}
@@ -876,7 +880,7 @@ func TestPatchCPAValuesForUser(t *testing.T) {
}).InitBasic(t)
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -971,7 +975,7 @@ func TestPatchCPAValuesForUser(t *testing.T) {
optionsID := []string{model.NewId(), model.NewId(), model.NewId(), model.NewId()}
arrayField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeMultiselect,
Attrs: model.StringInterface{
"options": []map[string]any{
@@ -1014,7 +1018,7 @@ func TestPatchCPAValuesForUser(t *testing.T) {
t.Run("should fail if any of the values belongs to a field that is LDAP/SAML synced", func(t *testing.T) {
// Create a field with LDAP attribute
ldapField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{
model.CustomProfileAttributesPropertyAttrsLDAP: "ldap_attr",
@@ -1028,7 +1032,7 @@ func TestPatchCPAValuesForUser(t *testing.T) {
// Create a field with SAML attribute
samlField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{
model.CustomProfileAttributesPropertyAttrsSAML: "saml_attr",
@@ -1071,7 +1075,7 @@ func TestPatchCPAValuesForUser(t *testing.T) {
t.Run("an invalid patch should be rejected", func(t *testing.T) {
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -1095,7 +1099,7 @@ func TestPatchCPAValuesForUser(t *testing.T) {
t.Run("admin-managed fields", func(t *testing.T) {
// Create a managed field (only admins can create fields)
managedField := &model.PropertyField{
Name: "Managed Field",
Name: "managed_field_v2",
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{
model.CustomProfileAttributesPropertyAttrsManaged: "admin",
@@ -1109,7 +1113,7 @@ func TestPatchCPAValuesForUser(t *testing.T) {
// Create a non-managed field for comparison
regularField := &model.PropertyField{
Name: "Regular Field",
Name: "regular_field_v2",
Type: model.PropertyFieldTypeText,
}
@@ -108,6 +108,10 @@ func (a *App) CreateCPAField(rctx request.CTX, field *model.CPAField) (*model.CP
return nil, appErr
}
if appErr = model.ValidateCPAFieldName(field.Name); appErr != nil {
return nil, appErr
}
newField, appErr := a.CreatePropertyField(rctx, field.ToPropertyField(), false, "")
if appErr != nil {
return nil, appErr
@@ -130,6 +134,7 @@ func (a *App) PatchCPAField(rctx request.CTX, fieldID string, patch *model.Prope
if appErr != nil {
return nil, appErr
}
originalName := existingField.Name
shouldDeleteValues := false
if patch.Type != nil && *patch.Type != existingField.Type {
@@ -144,6 +149,14 @@ func (a *App) PatchCPAField(rctx request.CTX, fieldID string, patch *model.Prope
return nil, appErr
}
// Lenient grandfather: only validate Name against CEL rules when it actually changes.
// Pre-existing fields with invalid names remain editable on all other attrs.
if existingField.Name != originalName {
if appErr = model.ValidateCPAFieldName(existingField.Name); appErr != nil {
return nil, appErr
}
}
groupID, appErr := a.CpaGroupID()
if appErr != nil {
return nil, model.NewAppError("PatchCPAField", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(appErr)
@@ -14,6 +14,10 @@ import (
"github.com/stretchr/testify/require"
)
func celSafeName() string {
return "f_" + model.NewId()
}
func TestGetCPAField(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t).InitBasic(t)
@@ -54,7 +58,7 @@ func TestGetCPAField(t *testing.T) {
t.Run("should get an existing CPA field", func(t *testing.T) {
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: cpaID,
Name: "Test Field",
Name: "test_field",
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityHidden},
})
@@ -67,7 +71,7 @@ func TestGetCPAField(t *testing.T) {
fetchedField, appErr := th.App.GetCPAField(rctx, createdField.ID)
require.Nil(t, appErr)
require.Equal(t, createdField.ID, fetchedField.ID)
require.Equal(t, "Test Field", fetchedField.Name)
require.Equal(t, "test_field", fetchedField.Name)
require.Equal(t, model.CustomProfileAttributesVisibilityHidden, fetchedField.Attrs.Visibility)
})
@@ -111,7 +115,7 @@ func TestGetCPAField(t *testing.T) {
// Create LDAP synced field
ldapField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: cpaID,
Name: "LDAP Field",
Name: "ldap_field",
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{
model.CustomProfileAttributesPropertyAttrsLDAP: "ldap_attribute",
@@ -124,7 +128,7 @@ func TestGetCPAField(t *testing.T) {
// Create SAML synced field
samlField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: cpaID,
Name: "SAML Field",
Name: "saml_field",
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{
model.CustomProfileAttributesPropertyAttrsSAML: "saml_attribute",
@@ -250,7 +254,7 @@ func TestListCPAFields(t *testing.T) {
t.Run("list fields should return defaults for fields created without visibility and sort_order", func(t *testing.T) {
// Create a field with minimal attrs (no visibility or sort_order)
fieldMinimal, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: "Field Without Defaults",
Name: "field_without_defaults",
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{}, // Empty attrs - no visibility or sort_order
})
@@ -261,7 +265,7 @@ func TestListCPAFields(t *testing.T) {
// Create another field to ensure we test list results with explicit values
fieldNormal, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: "Normal Field",
Name: "normal_field",
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{
model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityAlways,
@@ -310,7 +314,7 @@ func TestCreateCPAField(t *testing.T) {
rctx := th.emptyContextWithCallerID(anonymousCallerId)
t.Run("should fail if the field is not valid", func(t *testing.T) {
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{Name: model.NewId()})
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{Name: celSafeName()})
require.NoError(t, err)
createdField, err := th.App.CreateCPAField(rctx, field)
@@ -321,7 +325,7 @@ 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, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: model.NewId(),
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -334,7 +338,7 @@ func TestCreateCPAField(t *testing.T) {
t.Run("should correctly create a CPA field", func(t *testing.T) {
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: cpaID,
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityHidden},
})
@@ -357,7 +361,7 @@ func TestCreateCPAField(t *testing.T) {
// Create a CPAField with DeleteAt != 0
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: cpaID,
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityHidden},
})
@@ -388,7 +392,7 @@ func TestCreateCPAField(t *testing.T) {
// we create the rest of the fields required to reach the limit
for i := 1; i <= CustomProfileAttributesFieldLimit; i++ {
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: model.NewId(),
Name: fmt.Sprintf("f_%d_%s", i, model.NewId()),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -401,7 +405,7 @@ func TestCreateCPAField(t *testing.T) {
// then, we create a last one that would exceed the limit
field := &model.CPAField{
PropertyField: model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
},
}
@@ -423,7 +427,7 @@ func TestCreateCPAField(t *testing.T) {
// creating a new one should work now
field := &model.CPAField{
PropertyField: model.PropertyField{
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
},
}
@@ -445,7 +449,7 @@ func TestPatchCPAField(t *testing.T) {
newField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: cpaID,
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityHidden},
})
@@ -455,7 +459,7 @@ func TestPatchCPAField(t *testing.T) {
require.Nil(t, appErr)
patch := &model.PropertyFieldPatch{
Name: model.NewPointer("Patched name"),
Name: model.NewPointer("patched_name"),
Attrs: model.NewPointer(model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityWhenSet}),
TargetID: model.NewPointer(model.NewId()),
TargetType: model.NewPointer(model.NewId()),
@@ -495,7 +499,7 @@ func TestPatchCPAField(t *testing.T) {
updatedField, appErr := th.App.PatchCPAField(rctx, createdField.ID, patch)
require.Nil(t, appErr)
require.Equal(t, createdField.ID, updatedField.ID)
require.Equal(t, "Patched name", updatedField.Name)
require.Equal(t, "patched_name", updatedField.Name)
require.Equal(t, model.CustomProfileAttributesVisibilityWhenSet, updatedField.Attrs.Visibility)
require.Empty(t, updatedField.TargetID, "CPA should not allow to patch the field's target ID")
require.Empty(t, updatedField.TargetType, "CPA should not allow to patch the field's target type")
@@ -506,7 +510,7 @@ func TestPatchCPAField(t *testing.T) {
// Create a select field with options
selectField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: cpaID,
Name: "Select Field",
Name: "select_field",
Type: model.PropertyFieldTypeSelect,
Attrs: map[string]any{
model.PropertyFieldAttributeOptions: []any{
@@ -579,7 +583,7 @@ func TestPatchCPAField(t *testing.T) {
// Create a select field with options
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: cpaID,
Name: "Select Field with values",
Name: "select_field_with_values",
Type: model.PropertyFieldTypeSelect,
Attrs: model.StringInterface{
model.PropertyFieldAttributeOptions: []any{
@@ -612,7 +616,7 @@ func TestPatchCPAField(t *testing.T) {
// Patch the field without changing type (just update name and add a new option)
patch := &model.PropertyFieldPatch{
Name: model.NewPointer("Updated select field name"),
Name: model.NewPointer("updated_select_field_name"),
Attrs: model.NewPointer(model.StringInterface{
model.PropertyFieldAttributeOptions: []any{
map[string]any{
@@ -633,7 +637,7 @@ func TestPatchCPAField(t *testing.T) {
}
updatedField, appErr := th.App.PatchCPAField(rctx, createdField.ID, patch)
require.Nil(t, appErr)
require.Equal(t, "Updated select field name", updatedField.Name)
require.Equal(t, "updated_select_field_name", updatedField.Name)
require.Equal(t, model.PropertyFieldTypeSelect, updatedField.Type)
// Verify values still exist
@@ -647,7 +651,7 @@ func TestPatchCPAField(t *testing.T) {
// Create a select field with options
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: cpaID,
Name: "Select Field with type change",
Name: "select_field_with_type_change",
Type: model.PropertyFieldTypeSelect,
Attrs: model.StringInterface{
model.PropertyFieldAttributeOptions: []any{
@@ -709,7 +713,7 @@ func TestDeleteCPAField(t *testing.T) {
newField, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: cpaID,
Name: model.NewId(),
Name: celSafeName(),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -1075,7 +1079,7 @@ func TestDeleteCPAValues(t *testing.T) {
for i := 1; i <= 3; i++ {
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
GroupID: cpaID,
Name: fmt.Sprintf("Field %d", i),
Name: fmt.Sprintf("field_%d", i),
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
@@ -1128,3 +1132,165 @@ func TestDeleteCPAValues(t *testing.T) {
require.Len(t, values, 3)
})
}
func TestCreateCPAField_RejectsInvalidName(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t).InitBasic(t)
rctx := th.emptyContextWithCallerID(anonymousCallerId)
tests := []struct {
name string
fieldName string
wantErrID string
}{
{
name: "space in name",
fieldName: "My Field",
wantErrID: "model.cpa_field.name.invalid_charset.app_error",
},
{
name: "leading digit",
fieldName: "7department",
wantErrID: "model.cpa_field.name.invalid_charset.app_error",
},
{
name: "reserved word in",
fieldName: "in",
wantErrID: "model.cpa_field.name.reserved_word.app_error",
},
{
name: "reserved word true",
fieldName: "true",
wantErrID: "model.cpa_field.name.reserved_word.app_error",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: tt.fieldName,
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
_, appErr := th.App.CreateCPAField(rctx, field)
require.NotNil(t, appErr, "expected error for name %q", tt.fieldName)
require.Equal(t, tt.wantErrID, appErr.Id)
})
}
}
func TestCreateCPAField_AcceptsValidName(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t).InitBasic(t)
rctx := th.emptyContextWithCallerID(anonymousCallerId)
validNames := []string{"department", "_private", "A1", "a_b_c", "Department", "DEPT"}
for _, n := range validNames {
t.Run(n, func(t *testing.T) {
field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: n,
Type: model.PropertyFieldTypeText,
})
require.NoError(t, err)
created, appErr := th.App.CreateCPAField(rctx, field)
require.Nil(t, appErr, "unexpected error for name %q: %v", n, appErr)
require.NotEmpty(t, created.ID)
_ = th.App.DeleteCPAField(rctx, created.ID)
})
}
}
func TestPatchCPAField_GrandfatherSkipsValidationOnUnchangedName(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t).InitBasic(t)
cpaID, cErr := th.App.CpaGroupID()
require.Nil(t, cErr)
rctx := th.emptyContextWithCallerID(anonymousCallerId)
// Seed a field with an invalid CPA name directly via CreatePropertyField (bypassing CPA validation).
// This simulates a pre-existing legacy field whose name violates the new CEL rule.
legacyField, err := th.App.CreatePropertyField(rctx, &model.PropertyField{
GroupID: cpaID,
Name: "My Legacy Field",
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{model.CustomProfileAttributesPropertyAttrsVisibility: model.CustomProfileAttributesVisibilityWhenSet},
}, false, "")
require.Nil(t, err)
defer func() { _ = th.App.DeleteCPAField(rctx, legacyField.ID) }()
t.Run("patching only visibility leaves invalid name unchanged (grandfather passes)", func(t *testing.T) {
newVisibility := model.CustomProfileAttributesVisibilityAlways
patch := &model.PropertyFieldPatch{
Attrs: &model.StringInterface{
model.CustomProfileAttributesPropertyAttrsVisibility: newVisibility,
},
}
patched, appErr := th.App.PatchCPAField(rctx, legacyField.ID, patch)
require.Nil(t, appErr, "grandfather: patching non-name attrs on a legacy field must not trigger validation")
require.Equal(t, "My Legacy Field", patched.Name, "name must remain unchanged")
require.Equal(t, newVisibility, patched.Attrs.Visibility)
})
t.Run("patching name to another invalid value returns validation error", func(t *testing.T) {
stillInvalidName := "still invalid name"
patch := &model.PropertyFieldPatch{
Name: model.NewPointer(stillInvalidName),
}
_, appErr := th.App.PatchCPAField(rctx, legacyField.ID, patch)
require.NotNil(t, appErr, "renaming to an invalid name must be rejected")
require.Equal(t, "model.cpa_field.name.invalid_charset.app_error", appErr.Id)
})
t.Run("patching name to a valid value succeeds", func(t *testing.T) {
validName := "my_legacy_field"
patch := &model.PropertyFieldPatch{
Name: model.NewPointer(validName),
}
patched, appErr := th.App.PatchCPAField(rctx, legacyField.ID, patch)
require.Nil(t, appErr, "renaming to a valid CEL identifier must succeed")
require.Equal(t, validName, patched.Name)
})
}
// TestCreatePropertyField_BypassesCPANameValidation_ExpectedBehavior asserts the documented
// Option C bypass: the generic property-field App API does NOT enforce the CPA name regex
// on master. This is intentional and time-bounded.
//
// PR #36173's AttributeValidationHook will close the bypass at the property-service layer.
// Do NOT "fix" this test by adding CPA name validation in App.CreatePropertyField ahead of
// #36173 landing — doing so would conflict with @davidkrauser's diff.
//
// See spec.md §Out of Scope and the CPAAttrs godoc block in
// server/public/model/custom_profile_attributes.go (§Non-enforcement) for full context.
func TestCreatePropertyField_BypassesCPANameValidation_ExpectedBehavior(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t).InitBasic(t)
cpaID, cErr := th.App.CpaGroupID()
require.Nil(t, cErr)
rctx := th.emptyContextWithCallerID(anonymousCallerId)
// "My Field" violates CPAFieldNamePattern — would be rejected by CreateCPAField.
// Via CreatePropertyField (the generic property API), it must succeed.
field := &model.PropertyField{
GroupID: cpaID,
Name: "My Field",
Type: model.PropertyFieldTypeText,
}
created, appErr := th.App.CreatePropertyField(rctx, field, false, "")
require.Nil(t, appErr,
"CreatePropertyField must NOT enforce the CPA name regex on master — "+
"that enforcement belongs to PR #36173's AttributeValidationHook")
require.NotEmpty(t, created.ID)
_ = th.App.DeleteCPAField(rctx, created.ID)
}
+39
View File
@@ -32,6 +32,7 @@ const (
contentFlaggingSetupDoneKey = "content_flagging_setup_done"
contentFlaggingMigrationVersion = "v5"
managedCategorySetupDoneKey = "managed_category_setup_done"
cpaDisplayNameBackfillKey = "cpa_display_name_backfill_done"
contentFlaggingPropertyNameFlaggedPostId = "flagged_post_id"
ContentFlaggingPropertyNameStatus = "status"
@@ -805,6 +806,43 @@ func (s *Server) doSetupManagedCategoryProperties() error {
return s.cacheManagedCategoryIDs()
}
func (s *Server) doSetupCPADisplayNameBackfill(rctx request.CTX) error {
var nfErr *store.ErrNotFound
data, err := s.Store().System().GetByName(cpaDisplayNameBackfillKey)
if err != nil && !errors.As(err, &nfErr) {
return fmt.Errorf("could not query CPA display_name backfill migration: %w", err)
}
if data != nil {
return nil
}
// The properties package owns the actual field iteration and update logic.
// It deliberately bypasses the access-control layer for this single,
// well-defined backfill so it can update protected (e.g. UAS-managed) CPA
// fields whose source plugin is not the system. Keeping the bypass behind
// an explicitly named method on PropertyService avoids exposing a general
// "skip access control" surface from this package.
backfilled, skipped, err := s.propertyService.MigrateBackfillCPADisplayName(rctx)
if err != nil {
return fmt.Errorf("failed to backfill CPA display_name: %w", err)
}
mlog.Info("CPA display_name backfill migration completed",
mlog.Int("backfilled", backfilled),
mlog.Int("skipped", skipped),
)
if err := s.Store().System().SaveOrUpdate(&model.System{
Name: cpaDisplayNameBackfillKey,
Value: "true",
}); err != nil {
return fmt.Errorf("failed to mark CPA display_name backfill as complete: %w", err)
}
return nil
}
func (s *Server) cacheManagedCategoryIDs() error {
group, err := s.propertyService.GetPropertyGroup(model.ManagedCategoryPropertyGroupName)
if err != nil {
@@ -1031,6 +1069,7 @@ func (s *Server) doAppMigrations() {
{"Delete Orphan Drafts Migration", s.doDeleteOrphanDraftsMigration},
{"Delete Invalid Dms Preferences Migration", s.doDeleteDmsPreferencesMigration},
{"Access Control Policy V0.3 Migration", s.doAccessControlPolicyV0_3Migration},
{"CPA DisplayName Backfill", s.doSetupCPADisplayNameBackfill},
}
rctx := request.EmptyContext(s.Log())
+168
View File
@@ -4,6 +4,7 @@
package app
import (
"context"
"testing"
"github.com/mattermost/mattermost/server/public/model"
@@ -56,3 +57,170 @@ func TestDoSetupContentFlaggingProperties(t *testing.T) {
require.Equal(t, "v5", data.Value)
})
}
// clearCPABackfillMarker removes the System-key marker for the CPA display_name backfill
// so the migration body actually executes when called from a test. Setup(t) runs
// doAppMigrations which now includes the backfill; without clearing, the System key is
// already present and doSetupCPADisplayNameBackfill short-circuits at the idempotency check
// — the test would then pass for the wrong reason.
func clearCPABackfillMarker(t *testing.T, th *TestHelper) {
t.Helper()
_, err := th.Store.System().PermanentDeleteByName(cpaDisplayNameBackfillKey)
require.NoError(t, err, "failed to clear CPA backfill marker for test isolation")
}
func TestCPADisplayNameBackfill_NoExistingFields(t *testing.T) {
th := Setup(t)
clearCPABackfillMarker(t, th)
err := th.Server.doSetupCPADisplayNameBackfill(th.Context)
require.NoError(t, err)
data, sysErr := th.Store.System().GetByName(cpaDisplayNameBackfillKey)
require.NoError(t, sysErr)
require.NotNil(t, data)
require.Equal(t, "true", data.Value)
}
func TestCPADisplayNameBackfill_BackfillsMissing(t *testing.T) {
th := Setup(t)
clearCPABackfillMarker(t, th)
// fieldA exercises the "display_name present as empty string in JSONB" case — the true
// idempotency boundary.
fieldABase, convErr := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: "department",
Type: model.PropertyFieldTypeText,
})
require.NoError(t, convErr)
fieldA, appErr := th.App.CreateCPAField(th.Context, fieldABase)
require.Nil(t, appErr)
require.Equal(t, "", fieldA.Attrs.DisplayName, "seed invariant: fieldA must have empty display_name")
fieldBBase, convErr := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: "job_title",
Type: model.PropertyFieldTypeText,
})
require.NoError(t, convErr)
fieldBBase.Attrs.DisplayName = "Job Title"
fieldB, appErr := th.App.CreateCPAField(th.Context, fieldBBase)
require.Nil(t, appErr)
require.Equal(t, "Job Title", fieldB.Attrs.DisplayName, "seed invariant: fieldB must have display_name set")
err := th.Server.doSetupCPADisplayNameBackfill(th.Context)
require.NoError(t, err)
updatedFieldA, appErr := th.App.GetCPAField(th.Context, fieldA.ID)
require.Nil(t, appErr)
require.Equal(t, "department", updatedFieldA.Attrs.DisplayName,
"fieldA: display_name must be backfilled to field name")
updatedFieldB, appErr := th.App.GetCPAField(th.Context, fieldB.ID)
require.Nil(t, appErr)
require.Equal(t, "Job Title", updatedFieldB.Attrs.DisplayName,
"fieldB: display_name must not be overwritten when already set")
data, sysErr := th.Store.System().GetByName(cpaDisplayNameBackfillKey)
require.NoError(t, sysErr)
require.NotNil(t, data)
require.Equal(t, "true", data.Value)
}
func TestCPADisplayNameBackfill_Idempotent(t *testing.T) {
th := Setup(t)
clearCPABackfillMarker(t, th)
fieldBase, convErr := model.NewCPAFieldFromPropertyField(&model.PropertyField{
Name: "location",
Type: model.PropertyFieldTypeText,
})
require.NoError(t, convErr)
seeded, appErr := th.App.CreateCPAField(th.Context, fieldBase)
require.Nil(t, appErr)
err := th.Server.doSetupCPADisplayNameBackfill(th.Context)
require.NoError(t, err)
data1, sysErr := th.Store.System().GetByName(cpaDisplayNameBackfillKey)
require.NoError(t, sysErr)
require.Equal(t, "true", data1.Value)
updatedAfterFirst, appErr := th.App.GetCPAField(th.Context, seeded.ID)
require.Nil(t, appErr)
require.Equal(t, "location", updatedAfterFirst.Attrs.DisplayName)
// Snapshot UpdateAt before the second run so we can prove the second run is a no-op
// at the DB-write level. PropertyField.UpdateAt is set to model.GetMillis() on every
// write, so a re-run would change it. (model.System exposes only Name + Value, so the
// System key cannot be probed the same way; the System-key SaveOrUpdate is gated by
// the same short-circuit that gates the field write, so the field check is sufficient.)
firstFieldUpdate := updatedAfterFirst.UpdateAt
// Second run: idempotency check fires immediately, returns nil without any DB writes.
err = th.Server.doSetupCPADisplayNameBackfill(th.Context)
require.NoError(t, err)
data2, sysErr := th.Store.System().GetByName(cpaDisplayNameBackfillKey)
require.NoError(t, sysErr)
require.Equal(t, "true", data2.Value)
updatedAfterSecond, appErr := th.App.GetCPAField(th.Context, seeded.ID)
require.Nil(t, appErr)
require.Equal(t, "location", updatedAfterSecond.Attrs.DisplayName,
"second run must not change display_name")
require.Equal(t, firstFieldUpdate, updatedAfterSecond.UpdateAt,
"second run must not re-write the field row")
}
func TestCPADisplayNameBackfill_BackfillsProtectedSourceOnlyField(t *testing.T) {
th := Setup(t)
clearCPABackfillMarker(t, th)
groupID, appErr := th.App.CpaGroupID()
require.Nil(t, appErr)
// Insert directly via the store so we bypass the property service's
// access-control routing (which would reject creating a protected
// source_only field from a non-plugin caller). Type=text avoids the
// options-stripping branch in read access control, but the migration's
// correctness here doesn't depend on the field type.
field := &model.PropertyField{
GroupID: groupID,
Name: "uas_employee_id",
Type: model.PropertyFieldTypeText,
Attrs: model.StringInterface{
model.PropertyAttrsProtected: true,
model.PropertyAttrsAccessMode: model.PropertyAccessModeSourceOnly,
model.PropertyAttrsSourcePluginID: "com.mattermost.uas-plugin",
// display_name intentionally omitted - this is the state the migration
// is designed to fix.
},
}
created, err := th.Store.PropertyField().Create(field)
require.NoError(t, err, "seed: protected source_only field must be insertable directly via the store")
err = th.Server.doSetupCPADisplayNameBackfill(th.Context)
require.NoError(t, err, "migration must succeed even when CPA fields are protected and owned by a plugin")
// Read back via the store directly to avoid any read-access filtering
// the AC layer might apply for a non-source-plugin caller.
got, err := th.Store.PropertyField().Get(context.Background(), groupID, created.ID)
require.NoError(t, err)
require.Equal(t, "uas_employee_id", got.Attrs[model.CustomProfileAttributesPropertyAttrsDisplayName],
"display_name must be backfilled to the field name even on protected/source_only fields")
// Confirm the protection metadata was preserved untouched.
require.Equal(t, true, got.Attrs[model.PropertyAttrsProtected], "protected flag must be preserved")
require.Equal(t, model.PropertyAccessModeSourceOnly, got.Attrs[model.PropertyAttrsAccessMode], "access_mode must be preserved")
require.Equal(t, "com.mattermost.uas-plugin", got.Attrs[model.PropertyAttrsSourcePluginID], "source_plugin_id must be preserved")
data, sysErr := th.Store.System().GetByName(cpaDisplayNameBackfillKey)
require.NoError(t, sysErr)
require.NotNil(t, data)
require.Equal(t, "true", data.Value)
}
@@ -0,0 +1,83 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.
package properties
import (
"fmt"
"github.com/mattermost/mattermost/server/public/model"
"github.com/mattermost/mattermost/server/public/shared/request"
)
// MigrateBackfillCPADisplayName backfills the CPA display_name attribute on
// every CPA PropertyField that is missing one (absent key or empty string).
//
// This is the only public entry point that performs writes to CPA fields
// without going through the access-control layer. It does so deliberately:
// the backfill is a one-shot system migration and the access-control layer
// would otherwise reject writes against fields whose source plugin is not the
// caller (e.g. UAS-managed CPA fields with attrs["protected"]=true). Confining
// the bypass to this single, named, side-effect-bounded method avoids
// introducing a general "skip access control" surface that other code could
// reach for.
//
// The method is idempotent at the field level: fields that already have a
// non-empty display_name are skipped. The caller (app/migrations.go) is
// responsible for the System-key idempotency wrapper that prevents the whole
// migration from running twice.
//
// Returns the number of fields that were backfilled and the number that were
// skipped, so the caller can log a summary.
func (ps *PropertyService) MigrateBackfillCPADisplayName(rctx request.CTX) (backfilled int, skipped int, err error) {
group, err := ps.Group(model.CustomProfileAttributesPropertyGroupName)
if err != nil {
return 0, 0, fmt.Errorf("MigrateBackfillCPADisplayName: failed to get CPA property group: %w", err)
}
groupID := group.ID
const cpaFieldLimit = 20
var fieldsToUpdate []*model.PropertyField
// Use the unexported searchPropertyFields to bypass access control.
// AC would filter out (or strip options from) protected fields when
// the caller is not the source plugin, which would corrupt the
// fields we then try to write back. CPA creation is capped at 20
// active fields, so a single page covers the full migration scope.
fields, searchErr := ps.searchPropertyFields(groupID, model.PropertyFieldSearchOpts{
PerPage: cpaFieldLimit,
})
if searchErr != nil {
return 0, 0, fmt.Errorf("MigrateBackfillCPADisplayName: failed to search CPA fields: %w", searchErr)
}
for _, pf := range fields {
cpaField, convErr := model.NewCPAFieldFromPropertyField(pf)
if convErr != nil {
return 0, 0, fmt.Errorf("MigrateBackfillCPADisplayName: failed to convert property field %q: %w", pf.ID, convErr)
}
// Backfill if display_name is absent OR empty-string. This covers
// fields created before display_name existed, fields created after
// without an explicit display_name (stored as ""), and fields
// patched with display_name="".
if cpaField.Attrs.DisplayName != "" {
skipped++
continue
}
cpaField.Attrs.DisplayName = cpaField.Name
fieldsToUpdate = append(fieldsToUpdate, cpaField.ToPropertyField())
}
if len(fieldsToUpdate) > 0 {
// Use the unexported updatePropertyFields for the same reason as
// searchPropertyFields above: the AC layer rejects writes from the
// system to fields owned by a source plugin.
if _, _, updateErr := ps.updatePropertyFields(groupID, fieldsToUpdate); updateErr != nil {
return 0, 0, fmt.Errorf("MigrateBackfillCPADisplayName: failed to update CPA fields: %w", updateErr)
}
}
return len(fieldsToUpdate), skipped, nil
}
+1
View File
@@ -41,6 +41,7 @@ func GetMockStoreForSetupFunctions() *mocks.Store {
systemStore.On("GetByName", "PostPriorityConfigDefaultTrueMigrationComplete").Return(&model.System{Name: "PostPriorityConfigDefaultTrueMigrationComplete", Value: "true"}, nil)
systemStore.On("GetByName", "content_flagging_setup_done").Return(&model.System{Name: "content_flagging_setup_done", Value: "true"}, nil)
systemStore.On("GetByName", "managed_category_setup_done").Return(&model.System{Name: "managed_category_setup_done", Value: "true"}, nil)
systemStore.On("GetByName", "cpa_display_name_backfill_done").Return(&model.System{Name: "cpa_display_name_backfill_done", Value: "true"}, nil)
systemStore.On("GetByName", model.MigrationKeyEmojiPermissionsSplit).Return(&model.System{Name: model.MigrationKeyEmojiPermissionsSplit, Value: "true"}, nil)
systemStore.On("GetByName", model.MigrationKeyWebhookPermissionsSplit).Return(&model.System{Name: model.MigrationKeyWebhookPermissionsSplit, Value: "true"}, nil)
systemStore.On("GetByName", model.MigrationKeyIntegrationsOwnPermissions).Return(&model.System{Name: model.MigrationKeyIntegrationsOwnPermissions, Value: "true"}, nil)
+12
View File
@@ -5934,6 +5934,10 @@
"id": "app.custom_profile_attributes.sanitize_and_validate.app_error",
"translation": "Invalid property value attributes : {{.AttributeName}} ({{.Reason}})."
},
{
"id": "app.custom_profile_attributes.sanitize_and_validate.display_name_too_long.app_error",
"translation": "CPA field display_name exceeds the maximum length of {{.MaxRunes}} characters."
},
{
"id": "app.custom_profile_attributes.search_property_fields.app_error",
"translation": "Unable to search User Attribute fields"
@@ -11590,6 +11594,14 @@
"id": "model.config.is_valid.write_timeout.app_error",
"translation": "Invalid value for write timeout."
},
{
"id": "model.cpa_field.name.invalid_charset.app_error",
"translation": "Invalid CPA field name '{{.Name}}': must match ^[A-Za-z_][A-Za-z0-9_]*$ (CEL identifier rule)."
},
{
"id": "model.cpa_field.name.reserved_word.app_error",
"translation": "Invalid CPA field name '{{.Name}}': this is a CEL reserved word and cannot be used as a field identifier."
},
{
"id": "model.dcr.is_valid.client_name.app_error",
"translation": "Client name must be 64 characters or less."
@@ -15,19 +15,22 @@ import (
"fmt"
"net/http"
"net/url"
"regexp"
"strings"
"unicode/utf8"
)
const CustomProfileAttributesPropertyGroupName = "custom_profile_attributes"
const (
// Attributes keys
CustomProfileAttributesPropertyAttrsSortOrder = "sort_order"
CustomProfileAttributesPropertyAttrsValueType = "value_type"
CustomProfileAttributesPropertyAttrsVisibility = "visibility"
CustomProfileAttributesPropertyAttrsLDAP = "ldap"
CustomProfileAttributesPropertyAttrsSAML = "saml"
CustomProfileAttributesPropertyAttrsManaged = "managed"
CustomProfileAttributesPropertyAttrsSortOrder = "sort_order"
CustomProfileAttributesPropertyAttrsValueType = "value_type"
CustomProfileAttributesPropertyAttrsVisibility = "visibility"
CustomProfileAttributesPropertyAttrsLDAP = "ldap"
CustomProfileAttributesPropertyAttrsSAML = "saml"
CustomProfileAttributesPropertyAttrsManaged = "managed"
CustomProfileAttributesPropertyAttrsDisplayName = "display_name"
// Value Types
CustomProfileAttributesValueTypeEmail = "email"
@@ -70,6 +73,51 @@ func IsKnownCPAVisibility(visibility string) bool {
return false
}
// CPAFieldNamePattern defines the character set allowed for CPA field names.
// Matches the CEL IDENTIFIER grammar (^[A-Za-z_][A-Za-z0-9_]*$) used by the
// ABAC engine (cel-go v0.27.0). Leading underscore is permitted — this is consistent
// with both the CEL grammar and the enterprise unparser (identifierPartPattern in
// access_control/cel_utils/normalizer.go).
var CPAFieldNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`)
// CPAFieldNameReservedWords is the set of CEL keywords that cannot be used as CPA
// field names. Bare use of these tokens in member-select position (e.g.
// user.attributes.in) either fails CEL parse or requires backtick quoting that
// the ABAC visual builder (ToCEL) does not currently emit.
//
// List sourced from cel-go v0.27.0 CEL.g4 lexer rules.
// Grouped: literals (true/false/null), operator-keywords (in/as), then alphabetical reserved keywords.
var CPAFieldNameReservedWords = map[string]struct{}{
"true": {}, "false": {}, "null": {},
"in": {}, "as": {},
"break": {}, "const": {}, "continue": {}, "else": {},
"for": {}, "function": {}, "if": {}, "import": {},
"let": {}, "loop": {}, "package": {}, "namespace": {},
"return": {}, "var": {}, "void": {}, "while": {},
}
func ValidateCPAFieldName(name string) *AppError {
if !CPAFieldNamePattern.MatchString(name) {
return NewAppError(
"ValidateCPAFieldName",
"model.cpa_field.name.invalid_charset.app_error",
map[string]any{"Name": name},
"",
http.StatusUnprocessableEntity,
)
}
if _, reserved := CPAFieldNameReservedWords[name]; reserved {
return NewAppError(
"ValidateCPAFieldName",
"model.cpa_field.name.reserved_word.app_error",
map[string]any{"Name": name},
"",
http.StatusUnprocessableEntity,
)
}
return nil
}
type CustomProfileAttributesSelectOption struct {
ID string `json:"id"`
Name string `json:"name"`
@@ -117,6 +165,19 @@ type CPAField struct {
Attrs CPAAttrs `json:"attrs"`
}
// CPAAttrs holds the typed attributes for a CPA (Custom Profile Attributes) field.
//
// # CEL-safe-identifier validation for Name
//
// CPA field names double as identifiers in ABAC CEL policy expressions of the form
// user.attributes.<name>. To be valid in that position without backtick quoting,
// Name must satisfy [CPAFieldNamePattern] (^[A-Za-z_][A-Za-z0-9_]*$) and must not
// appear in [CPAFieldNameReservedWords].
//
// # DisplayName
//
// DisplayName carries the user-facing label (e.g. "Department Head") separately
// from Name (the CEL identifier, e.g. "department_head").
type CPAAttrs struct {
Visibility string `json:"visibility"`
SortOrder float64 `json:"sort_order"`
@@ -128,6 +189,7 @@ type CPAAttrs struct {
Protected bool `json:"protected"`
SourcePluginID string `json:"source_plugin_id"`
AccessMode string `json:"access_mode"`
DisplayName string `json:"display_name,omitempty"` // omitempty applies only to direct JSON marshal of CPAAttrs; ToPropertyField always writes the key into the underlying StringInterface map.
}
func (c *CPAField) IsSynced() bool {
@@ -175,16 +237,17 @@ func (c *CPAField) ToPropertyField() *PropertyField {
pf := c.PropertyField
pf.Attrs = StringInterface{
CustomProfileAttributesPropertyAttrsVisibility: c.Attrs.Visibility,
CustomProfileAttributesPropertyAttrsSortOrder: c.Attrs.SortOrder,
CustomProfileAttributesPropertyAttrsValueType: c.Attrs.ValueType,
PropertyFieldAttributeOptions: c.Attrs.Options,
CustomProfileAttributesPropertyAttrsLDAP: c.Attrs.LDAP,
CustomProfileAttributesPropertyAttrsSAML: c.Attrs.SAML,
CustomProfileAttributesPropertyAttrsManaged: c.Attrs.Managed,
PropertyAttrsProtected: c.Attrs.Protected,
PropertyAttrsSourcePluginID: c.Attrs.SourcePluginID,
PropertyAttrsAccessMode: c.Attrs.AccessMode,
CustomProfileAttributesPropertyAttrsVisibility: c.Attrs.Visibility,
CustomProfileAttributesPropertyAttrsSortOrder: c.Attrs.SortOrder,
CustomProfileAttributesPropertyAttrsValueType: c.Attrs.ValueType,
PropertyFieldAttributeOptions: c.Attrs.Options,
CustomProfileAttributesPropertyAttrsLDAP: c.Attrs.LDAP,
CustomProfileAttributesPropertyAttrsSAML: c.Attrs.SAML,
CustomProfileAttributesPropertyAttrsManaged: c.Attrs.Managed,
PropertyAttrsProtected: c.Attrs.Protected,
PropertyAttrsSourcePluginID: c.Attrs.SourcePluginID,
PropertyAttrsAccessMode: c.Attrs.AccessMode,
CustomProfileAttributesPropertyAttrsDisplayName: c.Attrs.DisplayName,
}
return &pf
@@ -273,6 +336,15 @@ func (c *CPAField) SanitizeAndValidate() *AppError {
c.Attrs.Managed = managed
}
// Sanitize and validate display_name
// Reuses PropertyFieldNameMaxRunes to keep the DisplayName cap aligned with the Name cap; do NOT introduce a separate constant.
c.Attrs.DisplayName = strings.TrimSpace(c.Attrs.DisplayName)
if utf8.RuneCountInString(c.Attrs.DisplayName) > PropertyFieldNameMaxRunes {
return NewAppError("SanitizeAndValidate", "app.custom_profile_attributes.sanitize_and_validate.display_name_too_long.app_error", map[string]any{
"MaxRunes": PropertyFieldNameMaxRunes,
}, "", http.StatusUnprocessableEntity)
}
return nil
}
@@ -885,6 +885,180 @@ func TestCPAField_SanitizeAndValidate(t *testing.T) {
})
}
})
t.Run("display_name sanitization", func(t *testing.T) {
displayNameTests := []struct {
name string
displayName string
expectError bool
errorId string
expectedValue string
}{
{
name: "empty display_name is allowed",
displayName: "",
expectError: false,
expectedValue: "",
},
{
name: "display_name with surrounding whitespace is trimmed",
displayName: " Department Head ",
expectError: false,
expectedValue: "Department Head",
},
{
name: "all-whitespace display_name is trimmed to empty and allowed",
displayName: " ",
expectError: false,
expectedValue: "",
},
{
name: "display_name at exactly 255 runes is accepted",
displayName: strings.Repeat("a", PropertyFieldNameMaxRunes),
expectError: false,
expectedValue: strings.Repeat("a", PropertyFieldNameMaxRunes),
},
{
name: "display_name at 256 runes is rejected",
displayName: strings.Repeat("a", PropertyFieldNameMaxRunes+1),
expectError: true,
errorId: "app.custom_profile_attributes.sanitize_and_validate.display_name_too_long.app_error",
},
}
for _, tt := range displayNameTests {
t.Run(tt.name, func(t *testing.T) {
field := &CPAField{
PropertyField: PropertyField{
Type: PropertyFieldTypeText,
},
Attrs: CPAAttrs{
DisplayName: tt.displayName,
},
}
appErr := field.SanitizeAndValidate()
if tt.expectError {
require.NotNil(t, appErr)
require.Equal(t, tt.errorId, appErr.Id)
} else {
require.Nil(t, appErr)
assert.Equal(t, tt.expectedValue, field.Attrs.DisplayName,
"DisplayName must be trimmed after SanitizeAndValidate")
}
})
}
})
}
func TestValidateCPAFieldName(t *testing.T) {
tests := []struct {
name string
input string
wantErrID string // empty means expect nil (valid)
}{
// Accept
{name: "simple lowercase", input: "department", wantErrID: ""},
{name: "leading underscore", input: "_private", wantErrID: ""},
{name: "uppercase start", input: "Department", wantErrID: ""},
{name: "single uppercase", input: "A1", wantErrID: ""},
{name: "underscore separator", input: "a_b_c", wantErrID: ""},
{name: "all uppercase", input: "DEPT", wantErrID: ""},
// Case sensitivity of reserved-word lookup
{name: "case-sensitive: IN is not reserved", input: "IN", wantErrID: ""},
{name: "case-sensitive: In is not reserved", input: "In", wantErrID: ""},
// Single-character valid names
{name: "single lowercase letter", input: "a", wantErrID: ""},
{name: "single underscore", input: "_", wantErrID: ""},
{name: "single uppercase letter", input: "A", wantErrID: ""},
// Reject — charset
{name: "space in name", input: "My Field", wantErrID: "model.cpa_field.name.invalid_charset.app_error"},
{name: "leading digit", input: "7department", wantErrID: "model.cpa_field.name.invalid_charset.app_error"},
{name: "hyphen", input: "foo-bar", wantErrID: "model.cpa_field.name.invalid_charset.app_error"},
{name: "emoji", input: "🎯", wantErrID: "model.cpa_field.name.invalid_charset.app_error"},
{name: "empty string", input: "", wantErrID: "model.cpa_field.name.invalid_charset.app_error"},
{name: "trailing space", input: "name ", wantErrID: "model.cpa_field.name.invalid_charset.app_error"},
{name: "non-ASCII letter", input: "départment", wantErrID: "model.cpa_field.name.invalid_charset.app_error"},
// Reject — reserved words
{name: "reserved: in", input: "in", wantErrID: "model.cpa_field.name.reserved_word.app_error"},
{name: "reserved: as", input: "as", wantErrID: "model.cpa_field.name.reserved_word.app_error"},
{name: "reserved: true", input: "true", wantErrID: "model.cpa_field.name.reserved_word.app_error"},
{name: "reserved: false", input: "false", wantErrID: "model.cpa_field.name.reserved_word.app_error"},
{name: "reserved: null", input: "null", wantErrID: "model.cpa_field.name.reserved_word.app_error"},
{name: "reserved: function", input: "function", wantErrID: "model.cpa_field.name.reserved_word.app_error"},
{name: "reserved: var", input: "var", wantErrID: "model.cpa_field.name.reserved_word.app_error"},
{name: "reserved: return", input: "return", wantErrID: "model.cpa_field.name.reserved_word.app_error"},
{name: "reserved: if", input: "if", wantErrID: "model.cpa_field.name.reserved_word.app_error"},
{name: "reserved: for", input: "for", wantErrID: "model.cpa_field.name.reserved_word.app_error"},
{name: "reserved: import", input: "import", wantErrID: "model.cpa_field.name.reserved_word.app_error"},
// Boundary — reserved-word prefix/suffix not reserved (e.g. "trueish")
{name: "reserved word as prefix", input: "trueish", wantErrID: ""},
{name: "reserved word as suffix", input: "my_null", wantErrID: ""},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
appErr := ValidateCPAFieldName(tt.input)
if tt.wantErrID == "" {
require.Nil(t, appErr, "expected nil for input %q, got %v", tt.input, appErr)
} else {
require.NotNil(t, appErr, "expected error for input %q", tt.input)
require.Equal(t, tt.wantErrID, appErr.Id)
}
})
}
}
func TestCPAField_ToPropertyField_DisplayName(t *testing.T) {
t.Run("DisplayName round-trips through ToPropertyField and NewCPAFieldFromPropertyField", func(t *testing.T) {
original := &CPAField{
PropertyField: PropertyField{
ID: NewId(),
GroupID: CustomProfileAttributesPropertyGroupName,
Name: "department",
Type: PropertyFieldTypeText,
},
Attrs: CPAAttrs{
Visibility: CustomProfileAttributesVisibilityAlways,
SortOrder: 3.0,
DisplayName: "Department",
},
}
pf := original.ToPropertyField()
require.NotNil(t, pf)
require.Equal(t, "Department", pf.Attrs[CustomProfileAttributesPropertyAttrsDisplayName],
"DisplayName must be written into attrs StringInterface by ToPropertyField")
roundTripped, err := NewCPAFieldFromPropertyField(pf)
require.NoError(t, err)
require.Equal(t, "Department", roundTripped.Attrs.DisplayName,
"DisplayName must survive the ToPropertyField → NewCPAFieldFromPropertyField round-trip")
})
t.Run("empty DisplayName round-trips as empty string", func(t *testing.T) {
field := &CPAField{
PropertyField: PropertyField{
ID: NewId(),
GroupID: CustomProfileAttributesPropertyGroupName,
Name: "department",
Type: PropertyFieldTypeText,
},
Attrs: CPAAttrs{
Visibility: CustomProfileAttributesVisibilityWhenSet,
},
}
pf := field.ToPropertyField()
// With omitempty, an empty DisplayName should still be written (as empty string) to
// the StringInterface; NewCPAFieldFromPropertyField should unmarshal it as "".
roundTripped, err := NewCPAFieldFromPropertyField(pf)
require.NoError(t, err)
require.Equal(t, "", roundTripped.Attrs.DisplayName)
})
}
func TestSanitizeAndValidatePropertyValue(t *testing.T) {