MM-63848: Enforce unique names for parent access control policies (#35676)

* MM-63848: Enforce unique names for parent access control policies

* Revert accidental package-lock.json change

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix translation file

* Fix lint shadows, migration overflow, and unique constraint fallback

* Remove pre-check, combine migrations, fix overflow

* Combine migrations using regular CREATE INDEX

* add missing translation

* kip revision bump on policy name-only changes and add test to cover this scenario

* MM-63848: Fix tx commit on name-only update, unique test names

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Mattermost Build <build@mattermost.com>
This commit is contained in:
Pablo Vélez 2026-03-25 11:12:13 +01:00 committed by GitHub
parent cc66081f6b
commit 95e33dbc72
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 430 additions and 9 deletions

View file

@ -365,4 +365,56 @@ test.describe('ABAC Policies - Create Policies', () => {
const user3AfterSync = await verifyUserInChannel(adminClient, nonSatisfyingUserInChannel.id, privateChannel.id);
expect(user3AfterSync).toBe(false); // AUTO-REMOVED
});
/**
* MM-63848: Creating a policy with a name that already exists should show an error
*/
test('MM-63848 Should show error when creating policy with duplicate name', async ({pw}) => {
await pw.skipIfNoLicense();
const {adminUser, adminClient, team} = await pw.initSetup();
await enableUserManagedAttributes(adminClient);
const departmentAttribute: CustomProfileAttribute[] = [{name: 'Department', type: 'text', value: ''}];
await setupCustomProfileAttributeFields(adminClient, departmentAttribute);
const privateChannel = await createPrivateChannelForABAC(adminClient, team.id);
const {systemConsolePage} = await pw.testBrowser.login(adminUser);
const page = systemConsolePage.page;
await navigateToABACPage(page);
await enableABAC(page);
// Create the first policy
const policyName = `Duplicate Test ${await pw.random.id()}`;
await createBasicPolicy(page, {
name: policyName,
attribute: 'Department',
operator: '==',
value: 'Engineering',
autoSync: false,
channels: [privateChannel.display_name],
});
// Navigate back and try to create another policy with the same name
await navigateToABACPage(page);
await createBasicPolicy(page, {
name: policyName,
attribute: 'Department',
operator: '==',
value: 'Sales',
autoSync: false,
channels: [],
});
// Verify error message is shown
const errorMessage = page.locator('.EditPolicy__error');
await expect(errorMessage).toBeVisible({timeout: 5000});
const errorText = await errorMessage.textContent();
expect(errorText).toContain('A policy with this name already exists');
});
});

View file

@ -814,4 +814,92 @@ test.describe('ABAC Policy Management - Edit Policies', () => {
// Step 6: salesRemoteUser should NOT be in channel (never satisfied Dept requirement)
expect(salesRemoteAfterEdit).toBe(false);
});
/**
* MM-63848: Renaming a policy to a name that already exists should show an error
*/
test('MM-63848 Should show error when renaming policy to an existing name', async ({pw}) => {
await pw.skipIfNoLicense();
const {adminUser, adminClient, team} = await pw.initSetup();
await enableUserManagedAttributes(adminClient);
const departmentAttribute: CustomProfileAttribute[] = [{name: 'Department', type: 'text', value: ''}];
await setupCustomProfileAttributeFields(adminClient, departmentAttribute);
const privateChannel = await createPrivateChannelForABAC(adminClient, team.id);
const {systemConsolePage} = await pw.testBrowser.login(adminUser);
const page = systemConsolePage.page;
await navigateToABACPage(page);
await enableABAC(page);
// Create two policies with different names
const policyName1 = `Edit Dup Test A ${await pw.random.id()}`;
await createBasicPolicy(page, {
name: policyName1,
attribute: 'Department',
operator: '==',
value: 'Engineering',
autoSync: false,
channels: [privateChannel.display_name],
});
await navigateToABACPage(page);
const privateChannel2 = await createPrivateChannelForABAC(adminClient, team.id);
const policyName2 = `Edit Dup Test B ${await pw.random.id()}`;
await createBasicPolicy(page, {
name: policyName2,
attribute: 'Department',
operator: '==',
value: 'Sales',
autoSync: false,
channels: [privateChannel2.display_name],
});
// Navigate back and edit policy2's name to match policy1
await navigateToABACPage(page);
await page.waitForTimeout(1000);
// Search for the second policy
const policySearchInput = page.locator('input[placeholder*="Search" i]').first();
if (await policySearchInput.isVisible({timeout: 3000})) {
await policySearchInput.fill(policyName2);
await page.waitForTimeout(1000);
}
const policyRow = page.locator('tr.clickable, .DataGrid_row').filter({hasText: policyName2}).first();
await policyRow.waitFor({state: 'visible', timeout: 10000});
await policyRow.click();
await page.waitForLoadState('networkidle');
await page.waitForTimeout(1000);
// Change the name to match the first policy
const nameInput = page.locator('#admin\\.access_control\\.policy\\.edit_policy\\.policyName');
await nameInput.waitFor({state: 'visible', timeout: 10000});
await nameInput.fill('');
await nameInput.fill(policyName1);
// Save and expect failure
const saveButton = page.getByRole('button', {name: 'Save'});
await saveButton.click();
await page.waitForTimeout(2000);
// Handle confirmation modal if it appears
const applyPolicyButton = page.getByRole('button', {name: /apply policy/i});
if (await applyPolicyButton.isVisible({timeout: 3000}).catch(() => false)) {
await applyPolicyButton.click();
await page.waitForTimeout(2000);
}
// Verify error message is shown
const errorMessage = page.locator('.EditPolicy__error');
await expect(errorMessage).toBeVisible({timeout: 5000});
const errorText = await errorMessage.textContent();
expect(errorText).toContain('A policy with this name already exists');
});
});

View file

@ -313,3 +313,5 @@ channels/db/migrations/postgres/000157_backfill_roles_schemeid.down.sql
channels/db/migrations/postgres/000157_backfill_roles_schemeid.up.sql
channels/db/migrations/postgres/000158_add_roles_schemeid_index.down.sql
channels/db/migrations/postgres/000158_add_roles_schemeid_index.up.sql
channels/db/migrations/postgres/000159_deduplicate_policy_names.down.sql
channels/db/migrations/postgres/000159_deduplicate_policy_names.up.sql

View file

@ -0,0 +1,2 @@
DROP INDEX IF EXISTS idx_accesscontrolpolicies_name_type;
-- Name deduplication cannot be reliably reversed.

View file

@ -0,0 +1,13 @@
-- Deduplicate parent policy names before adding unique constraint.
-- The oldest policy (by CreateAt) keeps its original name; duplicates get ' (<id>)' appended.
UPDATE AccessControlPolicies AS p
SET Name = LEFT(p.Name, 128 - LENGTH(' (' || p.ID || ')')) || ' (' || p.ID || ')'
FROM (
SELECT ID, Name, ROW_NUMBER() OVER (PARTITION BY Name ORDER BY CreateAt ASC) AS rn
FROM AccessControlPolicies
WHERE Type = 'parent'
) AS dupes
WHERE p.ID = dupes.ID
AND dupes.rn > 1;
CREATE UNIQUE INDEX IF NOT EXISTS idx_accesscontrolpolicies_name_type ON AccessControlPolicies (Name, Type) WHERE Type = 'parent';

View file

@ -209,10 +209,27 @@ func (s *SqlAccessControlPolicyStore) Save(rctx request.CTX, policy *model.Acces
}
// Check if the policy has actually changed
// We compare data, name, and version fields, and ensure type hasn't changed
// We compare data and version fields. Name changes are cosmetic
// and should not create a new revision in history.
if bytes.Equal(storePolicy.Data, tmp.Data) &&
storePolicy.Name == tmp.Name &&
storePolicy.Version == tmp.Version {
// Update name in place if it changed, without bumping revision
if storePolicy.Name != tmp.Name {
nameUpdate := s.getQueryBuilder().
Update("AccessControlPolicies").
Set("Name", storePolicy.Name).
Where(sq.Eq{"ID": policy.ID})
if _, err = tx.ExecBuilder(nameUpdate); err != nil {
if IsUniqueConstraintError(err, []string{"Name", "idx_accesscontrolpolicies_name_type"}) {
return nil, store.NewErrConflict("AccessControlPolicy", err, "name="+policy.Name)
}
return nil, errors.Wrapf(err, "failed to update name for policy with id=%s", policy.ID)
}
existingPolicy.Name = storePolicy.Name
if err = tx.Commit(); err != nil {
return nil, errors.Wrap(err, "commit_transaction")
}
}
return existingPolicy, nil
}
@ -255,6 +272,9 @@ func (s *SqlAccessControlPolicyStore) Save(rctx request.CTX, policy *model.Acces
_, err = tx.ExecBuilder(query)
if err != nil {
if IsUniqueConstraintError(err, []string{"Name", "idx_accesscontrolpolicies_name_type"}) {
return nil, store.NewErrConflict("AccessControlPolicy", err, "name="+policy.Name)
}
return nil, errors.Wrapf(err, "failed to save policy with id=%s", policy.ID)
}

View file

@ -15,6 +15,7 @@ import (
func TestAccessControlPolicyStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) {
t.Run("Save", func(t *testing.T) { testAccessControlPolicyStoreSaveAndGet(t, rctx, ss) })
t.Run("SaveDuplicateName", func(t *testing.T) { testAccessControlPolicyStoreSaveDuplicateName(t, rctx, ss) })
t.Run("Delete", func(t *testing.T) { testAccessControlPolicyStoreDelete(t, rctx, ss) })
t.Run("SetActive", func(t *testing.T) { testAccessControlPolicyStoreSetActive(t, rctx, ss) })
t.Run("SetActiveMultiple", func(t *testing.T) { testAccessControlPolicyStoreSetActiveMultiple(t, rctx, ss) })
@ -27,7 +28,7 @@ func testAccessControlPolicyStoreSaveAndGet(t *testing.T, rctx request.CTX, ss s
t.Run("Save parent policy", func(t *testing.T) {
policy := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Name",
Name: "Save Parent " + model.NewId(),
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
@ -133,11 +134,245 @@ func testAccessControlPolicyStoreSaveAndGet(t *testing.T, rctx request.CTX, ss s
})
}
func testAccessControlPolicyStoreSaveDuplicateName(t *testing.T, rctx request.CTX, ss store.Store) {
t.Run("Duplicate parent policy name should fail", func(t *testing.T) {
policy1 := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Unique Policy Name",
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
Version: model.AccessControlPolicyVersionV0_2,
Imports: []string{},
Rules: []model.AccessControlPolicyRule{
{
Actions: []string{"action"},
Expression: "user.properties.program == \"engineering\"",
},
},
}
policy1, err := ss.AccessControlPolicy().Save(rctx, policy1)
require.NoError(t, err)
require.NotNil(t, policy1)
t.Cleanup(func() {
deleteErr := ss.AccessControlPolicy().Delete(rctx, policy1.ID)
require.NoError(t, deleteErr)
})
policy2 := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Unique Policy Name",
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
Version: model.AccessControlPolicyVersionV0_2,
Imports: []string{},
Rules: []model.AccessControlPolicyRule{
{
Actions: []string{"action"},
Expression: "user.properties.department == \"sales\"",
},
},
}
_, err = ss.AccessControlPolicy().Save(rctx, policy2)
require.Error(t, err)
var conflictErr *store.ErrConflict
require.ErrorAs(t, err, &conflictErr)
})
t.Run("Same name across different types should succeed", func(t *testing.T) {
parentPolicy := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Cross Type Name",
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
Version: model.AccessControlPolicyVersionV0_2,
Imports: []string{},
Rules: []model.AccessControlPolicyRule{
{
Actions: []string{"action"},
Expression: "user.properties.program == \"engineering\"",
},
},
}
parentPolicy, err := ss.AccessControlPolicy().Save(rctx, parentPolicy)
require.NoError(t, err)
require.NotNil(t, parentPolicy)
t.Cleanup(func() {
deleteErr := ss.AccessControlPolicy().Delete(rctx, parentPolicy.ID)
require.NoError(t, deleteErr)
})
channelPolicy := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Cross Type Name",
Type: model.AccessControlPolicyTypeChannel,
Active: true,
Revision: 1,
Version: model.AccessControlPolicyVersionV0_2,
Imports: []string{},
Rules: []model.AccessControlPolicyRule{
{
Actions: []string{"action"},
Expression: "user.properties.program == \"engineering\"",
},
},
}
channelPolicy, err = ss.AccessControlPolicy().Save(rctx, channelPolicy)
require.NoError(t, err)
require.NotNil(t, channelPolicy)
t.Cleanup(func() {
deleteErr := ss.AccessControlPolicy().Delete(rctx, channelPolicy.ID)
require.NoError(t, deleteErr)
})
})
t.Run("Updating same policy name should succeed", func(t *testing.T) {
policy := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Update Same Name",
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
Version: model.AccessControlPolicyVersionV0_2,
Imports: []string{},
Rules: []model.AccessControlPolicyRule{
{
Actions: []string{"action"},
Expression: "user.properties.program == \"engineering\"",
},
},
}
policy, err := ss.AccessControlPolicy().Save(rctx, policy)
require.NoError(t, err)
require.NotNil(t, policy)
require.Equal(t, 1, policy.Revision)
t.Cleanup(func() {
deleteErr := ss.AccessControlPolicy().Delete(rctx, policy.ID)
require.NoError(t, deleteErr)
})
// Update rules but keep same name — should succeed and bump revision
policy.Rules = []model.AccessControlPolicyRule{
{
Actions: []string{"action"},
Expression: "user.properties.department == \"sales\"",
},
}
policy, err = ss.AccessControlPolicy().Save(rctx, policy)
require.NoError(t, err)
require.NotNil(t, policy)
require.Equal(t, 2, policy.Revision)
})
t.Run("Changing policy name should not bump revision", func(t *testing.T) {
policy := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Original Name",
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
Version: model.AccessControlPolicyVersionV0_2,
Imports: []string{},
Rules: []model.AccessControlPolicyRule{
{
Actions: []string{"action"},
Expression: "user.properties.program == \"engineering\"",
},
},
}
policy, err := ss.AccessControlPolicy().Save(rctx, policy)
require.NoError(t, err)
require.Equal(t, 1, policy.Revision)
t.Cleanup(func() {
deleteErr := ss.AccessControlPolicy().Delete(rctx, policy.ID)
require.NoError(t, deleteErr)
})
// Change only the name — revision should NOT bump
policy.Name = "Renamed Policy"
policy, err = ss.AccessControlPolicy().Save(rctx, policy)
require.NoError(t, err)
require.NotNil(t, policy)
require.Equal(t, "Renamed Policy", policy.Name)
require.Equal(t, 1, policy.Revision)
// Verify the name persisted
fetched, err := ss.AccessControlPolicy().Get(rctx, policy.ID)
require.NoError(t, err)
require.Equal(t, "Renamed Policy", fetched.Name)
require.Equal(t, 1, fetched.Revision)
})
t.Run("Reusing name after deletion should succeed", func(t *testing.T) {
policy1 := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Reusable Name",
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
Version: model.AccessControlPolicyVersionV0_2,
Imports: []string{},
Rules: []model.AccessControlPolicyRule{
{
Actions: []string{"action"},
Expression: "user.properties.program == \"engineering\"",
},
},
}
policy1, err := ss.AccessControlPolicy().Save(rctx, policy1)
require.NoError(t, err)
err = ss.AccessControlPolicy().Delete(rctx, policy1.ID)
require.NoError(t, err)
policy2 := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Reusable Name",
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
Version: model.AccessControlPolicyVersionV0_2,
Imports: []string{},
Rules: []model.AccessControlPolicyRule{
{
Actions: []string{"action"},
Expression: "user.properties.department == \"sales\"",
},
},
}
policy2, err = ss.AccessControlPolicy().Save(rctx, policy2)
require.NoError(t, err)
require.NotNil(t, policy2)
t.Cleanup(func() {
deleteErr := ss.AccessControlPolicy().Delete(rctx, policy2.ID)
require.NoError(t, deleteErr)
})
})
}
func testAccessControlPolicyStoreDelete(t *testing.T, rctx request.CTX, ss store.Store) {
t.Run("Delete parent policy", func(t *testing.T) {
policy := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Name",
Name: "Delete Parent " + model.NewId(),
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
@ -251,7 +486,7 @@ func testAccessControlPolicyStoreGetAll(t *testing.T, rctx request.CTX, ss store
id := model.NewId()
parentPolicy := &model.AccessControlPolicy{
ID: id,
Name: "Name",
Name: "GetAll Parent " + id,
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
@ -297,7 +532,7 @@ func testAccessControlPolicyStoreGetAll(t *testing.T, rctx request.CTX, ss store
id3 := "zzz" + model.NewId()[3:] // ensure the order of the ID
parentPolicy2 := &model.AccessControlPolicy{
ID: id3,
Name: "Name",
Name: "Name2",
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
@ -554,7 +789,7 @@ func testAccessControlPolicyStoreGetPoliciesByFieldID(t *testing.T, rctx request
t.Helper()
policy := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Policy",
Name: "Policy " + model.NewId(),
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,
@ -654,7 +889,7 @@ func testAccessControlPolicyStoreGetPoliciesByFieldID(t *testing.T, rctx request
deletableField := model.NewId()
policy := &model.AccessControlPolicy{
ID: model.NewId(),
Name: "Policy",
Name: "Policy " + model.NewId(),
Type: model.AccessControlPolicyTypeParent,
Active: true,
Revision: 1,

View file

@ -7026,6 +7026,10 @@
"id": "app.pap.save_policy.app_error",
"translation": "Unable to save access control policy."
},
{
"id": "app.pap.save_policy.name_exists.app_error",
"translation": "A policy with this name already exists. Please choose a different name."
},
{
"id": "app.pap.search_access_control_policies.app_error",
"translation": "Could not search access control policies."

View file

@ -202,7 +202,11 @@ function PolicyDetails({
version: 'v0.2',
}).then((result) => {
if (result.error) {
setServerError(result.error.message);
if (result.error.server_error_id === 'app.pap.save_policy.name_exists.app_error') {
setServerError(formatMessage({id: 'admin.access_control.edit_policy.name_exists', defaultMessage: 'A policy with this name already exists. Please choose a different name.'}));
} else {
setServerError(result.error.message);
}
setShowConfirmationModal(false);
success = false;
return;

View file

@ -270,6 +270,7 @@
"admin.access_control.edit": "Edit",
"admin.access_control.edit_policy.apply_policy": "Apply policy",
"admin.access_control.edit_policy.cancel": "Cancel",
"admin.access_control.edit_policy.name_exists": "A policy with this name already exists. Please choose a different name.",
"admin.access_control.edit_policy.save": "Save",
"admin.access_control.edit_policy.save_policy": "Save policy",
"admin.access_control.edit_policy.serverError": "There are errors in the form above: {serverError}",