fix(dashboard): Use updateActiveAdministrator for profile page - #5055
Open
Ryrahul wants to merge 1 commit into
Open
fix(dashboard): Use updateActiveAdministrator for profile page#5055Ryrahul wants to merge 1 commit into
Ryrahul wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe profile GraphQL document now uses Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
michaelbromley
added
the
T1: Fast track
Collaborator
|
Thanks @Ryrahul, checked out the branch and reviewed it. The change is correct:
Two things before merge: 1. The commit message says 2. Needs a test. The existing I wrote one locally and confirmed it fails on // #5037 — an admin without UpdateAdministrator permission must still be able to
// edit their own profile (the page uses updateActiveAdministrator, gated by Owner)
test('should allow an admin without UpdateAdministrator permission to update own profile', async ({
page,
browser,
}) => {
const suffix = Date.now();
const emailAddress = `restricted-${suffix}@test.com`;
const password = 'test-password';
const client = new VendureAdminClient(page);
await client.login();
const { createRole } = await client.gql(
`mutation ($input: CreateRoleInput!) { createRole(input: $input) { id } }`,
{
input: {
code: `restricted-${suffix}`,
description: 'No admin permissions',
permissions: ['ReadCatalog'],
},
},
);
await client.gql(
`mutation ($input: CreateAdministratorInput!) { createAdministrator(input: $input) { id } }`,
{
input: {
firstName: 'Restricted',
lastName: 'Admin',
emailAddress,
password,
roleIds: [createRole.id],
},
},
);
// Fresh context so we browse as the restricted admin, not the superadmin
const context = await browser.newContext({ storageState: { cookies: [], origins: [] } });
const restrictedPage = await context.newPage();
await restrictedPage.goto('/login');
await restrictedPage.getByPlaceholder('Email').fill(emailAddress);
await restrictedPage.getByPlaceholder('Password').fill(password);
await restrictedPage.getByRole('button', { name: 'Sign in' }).click();
await expect(restrictedPage).not.toHaveURL(/\/login/, { timeout: 15_000 });
await restrictedPage.goto('/profile');
const firstNameField = restrictedPage.locator('[data-slot="field"]').filter({
has: restrictedPage.locator('[data-slot="field-label"]').getByText('First name', { exact: true }),
});
await firstNameField.getByRole('textbox').fill('Renamed');
await restrictedPage.getByRole('button', { name: 'Update' }).click();
await expect(
restrictedPage.locator('[data-sonner-toast]').filter({ hasText: 'Successfully updated profile' }),
).toBeVisible({ timeout: 10_000 });
await restrictedPage.reload();
await expect(firstNameField.getByRole('textbox')).toHaveValue('Renamed');
await context.close();
});Run with: CI=true VITE_TEST_PORT=5176 bunx playwright test --config e2e/playwright.config.ts e2e/tests/settings/profile.spec.ts --reporter=listHappy to approve once the test is in and the commit trailer is fixed. |
The profile page used the updateAdministrator mutation which requires Permission.UpdateAdministrator, preventing administrators without that permission from editing their own profile (name, email, password). Switch to updateActiveAdministrator which is gated by Permission.Owner and resolves the target from ctx.activeUserId, so any authenticated administrator can edit their own profile without needing elevated permissions. - Replace updateAdministratorDocument with updateActiveAdministratorDocument - Remove id from setValuesForUpdate (UpdateActiveAdministratorInput has no id field; the server infers it from the active user) - Add e2e test verifying a restricted admin (without UpdateAdministrator) can update their own profile Fixes vendurehq#5037
Ryrahul
force-pushed
the
fix/profile-update-permission
branch
from
July 31, 2026 10:30
0ab495e to
7b8450d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/dashboard/e2e/tests/settings/profile.spec.ts (2)
103-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider cleaning up the created role and administrator.
The test creates a role and an administrator through
client.gqlcalls but never deletes them afterward. Over repeated CI runs, this accumulates test-only administrators and roles in the database, unless the test environment resets between runs.Add
deleteAdministratoranddeleteRolecalls at the end of the test to keep the test data isolated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/e2e/tests/settings/profile.spec.ts` around lines 103 - 126, The test setup creates persistent administrator and role records without cleanup. At the end of the test containing the createAdministrator and createRole mutations, add client.gql calls for deleteAdministrator and deleteRole using the created administrator and role IDs, ensuring cleanup runs after the test assertions complete.
129-152: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
context.close()with try/finally.
context.close()at line 151 only runs if every prior assertion (lines 131-150) succeeds. If any assertion fails, the browser context created at line 129 stays open for the rest of the test run.Wrap the interactions in a
try/finallyblock so the context is always closed, even when an assertion throws.♻️ Proposed fix
const context = await browser.newContext({ storageState: { cookies: [], origins: [] } }); - const restrictedPage = await context.newPage(); - await restrictedPage.goto('/login'); - await restrictedPage.getByPlaceholder('Email').fill(emailAddress); - await restrictedPage.getByPlaceholder('Password').fill(password); - await restrictedPage.getByRole('button', { name: 'Sign in' }).click(); - await expect(restrictedPage).not.toHaveURL(/\/login/, { timeout: 15_000 }); - - await restrictedPage.goto('/profile'); - const firstNameField = restrictedPage.locator('[data-slot="field"]').filter({ - has: restrictedPage.locator('[data-slot="field-label"]').getByText('First name', { exact: true }), - }); - await firstNameField.getByRole('textbox').fill('Renamed'); - await restrictedPage.getByRole('button', { name: 'Update' }).click(); - - await expect( - restrictedPage.locator('[data-sonner-toast]').filter({ hasText: 'Successfully updated profile' }), - ).toBeVisible({ timeout: 10_000 }); - - await restrictedPage.reload(); - await expect(firstNameField.getByRole('textbox')).toHaveValue('Renamed'); - - await context.close(); + try { + const restrictedPage = await context.newPage(); + await restrictedPage.goto('/login'); + await restrictedPage.getByPlaceholder('Email').fill(emailAddress); + await restrictedPage.getByPlaceholder('Password').fill(password); + await restrictedPage.getByRole('button', { name: 'Sign in' }).click(); + await expect(restrictedPage).not.toHaveURL(/\/login/, { timeout: 15_000 }); + + await restrictedPage.goto('/profile'); + const firstNameField = restrictedPage.locator('[data-slot="field"]').filter({ + has: restrictedPage.locator('[data-slot="field-label"]').getByText('First name', { exact: true }), + }); + await firstNameField.getByRole('textbox').fill('Renamed'); + await restrictedPage.getByRole('button', { name: 'Update' }).click(); + + await expect( + restrictedPage.locator('[data-sonner-toast]').filter({ hasText: 'Successfully updated profile' }), + ).toBeVisible({ timeout: 10_000 }); + + await restrictedPage.reload(); + await expect(firstNameField.getByRole('textbox')).toHaveValue('Renamed'); + } finally { + await context.close(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/e2e/tests/settings/profile.spec.ts` around lines 129 - 152, Wrap the restrictedPage login, profile update, assertions, and reload flow in a try/finally block, keeping context.close() in the finally clause. Preserve the existing test interactions and assertions while ensuring the browser context created by browser.newContext is closed even when any step fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/dashboard/e2e/tests/settings/profile.spec.ts`:
- Around line 103-126: The test setup creates persistent administrator and role
records without cleanup. At the end of the test containing the
createAdministrator and createRole mutations, add client.gql calls for
deleteAdministrator and deleteRole using the created administrator and role IDs,
ensuring cleanup runs after the test assertions complete.
- Around line 129-152: Wrap the restrictedPage login, profile update,
assertions, and reload flow in a try/finally block, keeping context.close() in
the finally clause. Preserve the existing test interactions and assertions while
ensuring the browser context created by browser.newContext is closed even when
any step fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e48dae48-31ea-4173-a676-fb995eed8cda
📒 Files selected for processing (3)
packages/dashboard/e2e/tests/settings/profile.spec.tspackages/dashboard/src/app/routes/_authenticated/_profile/profile.graphql.tspackages/dashboard/src/app/routes/_authenticated/_profile/profile.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/dashboard/src/app/routes/_authenticated/_profile/profile.graphql.ts
- packages/dashboard/src/app/routes/_authenticated/_profile/profile.tsx
Ryrahul
changed the title
Ryrahul
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The profile page (
/profile) uses theupdateAdministratormutation, which requiresPermission.UpdateAdministrator. This means any administrator whose role does not include that permission cannot edit their own first name, last name, email, or password — the form loads fine (the query usesactiveAdministratorgated byPermission.Owner), but saving fails with aFORBIDDENerror.The Admin API already has
updateActiveAdministratorfor exactly this case — it's gated byPermission.Ownerand resolves the target fromctx.activeUserId, so it can only ever modify the calling user's own record.This PR switches the profile page to use
updateActiveAdministrator.Fixes #5037
What changed
profile.graphql.tsupdateAdministrator→updateActiveAdministrator(mutation name + input type)profile.tsxupdateDocumentreference, removedidfromsetValuesForUpdate(UpdateActiveAdministratorInputhas noidfield — the server infers it from the active user)Why this is safe
updateActiveAdministratoris@Allow(Permission.Owner)— any authenticated admin can call itctx.activeUserId(cannot modify other admins)UpdateActiveAdministratorInputhas noroleIdsfield — prevents privilege escalationcustomFieldsis dynamically added viagraphql-custom-fields.tsextend input, so custom fields on the profile page continue to work/administrators/:id) still usesupdateAdministratorwithPermission.UpdateAdministrator— unchangedBreaking changes
None.
Checklist
📌 Always:
👍 Most of the time:
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.