WEB-978: [Playwright] Multi-role auth storageState foundation#3644
Open
devvaansh wants to merge 1 commit into
Open
WEB-978: [Playwright] Multi-role auth storageState foundation#3644devvaansh wants to merge 1 commit into
devvaansh wants to merge 1 commit into
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Auth role configuration contract playwright/config/roles.ts |
AuthRole interface and ROLES registry define default, admin, and restricted roles with environment variable keys, default credentials, storage paths, and type-safe RoleId union. |
Auth helpers with storage mirroring and verification playwright/auth-helpers.ts |
authenticateRole now mirrors auth state from sessionStorage to localStorage when missing, uses route-portable ROUTES.home for verification, and guarantees context cleanup via try/finally. |
Role-specific setup files playwright/auth.admin.setup.ts, playwright/auth.restricted.setup.ts |
New setup hooks authenticate as ROLES.admin and ROLES.restricted, generating role-specific persisted storage state; restricted enforces env var credentials. |
Default setup refactoring playwright/auth.setup.ts |
Setup simplified to delegate to authenticateRole(ROLES.default, ...), removing local automation and storage logic while preserving legacy storage path compatibility. |
Playwright config with conditional role projects playwright.config.ts |
Filesystem detection and environment overrides conditionally enable setup-admin/setup-restricted and chromium-admin/chromium-restricted projects wired to role-specific storage files. |
Possibly related PRs
- openMF/web-app#3627: Modifies
playwright/auth-helpers.tsandauthenticateRoleflow with storageState persistence and sessionStorage→localStorage mirroring refactoring. - openMF/web-app#3457: Wires
playwright/auth.setup.tssetup projects intoplaywright.config.tswith persisted storageState; this PR extends that pattern to role-specific admin and restricted setups.
Suggested reviewers
- alberto-art3ch
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately summarizes the main change: establishing a multi-role authentication framework with role-specific storage state management in Playwright configuration. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 @coderabbitai help to get the list of available commands and usage tips.
devvaansh
changed the title
devvaansh
force-pushed
the
WEB-978-playwright-multi-role-auth-foundation
branch
from
b72b4b6 to
6e9a067
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
eslint-rules/no-bare-wait-for-timeout.js (1)
48-52: 💤 Low valueConsider handling computed property access.
The current check
callee.property.name !== 'waitForTimeout'only catches identifier-based member access (page.waitForTimeout()). Computed property access likepage['waitForTimeout']()would havecallee.property.type === 'Literal'with nonamefield, so the rule returns early and doesn't flag it.In practice, this syntax is rare for Playwright methods, but for completeness you could add:
const propName = callee.property.type === 'Identifier' ? callee.property.name : (callee.property.type === 'Literal' ? callee.property.value : null); if (propName !== 'waitForTimeout') return;🤖 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 `@eslint-rules/no-bare-wait-for-timeout.js` around lines 48 - 52, The CallExpression visitor only checks callee.property.name which misses computed access like page['waitForTimeout']; update the check in the CallExpression handler to derive the property name from callee.property by handling Identifier and Literal (computed) cases (e.g., get propName from callee.property.name or callee.property.value) and then compare propName to 'waitForTimeout' before calling context.report({ node, messageId: 'forbidden' }); ensure you reference the existing callee and context.report usage so other logic is unchanged.playwright/utils/sleep.ts (1)
76-95: 💤 Low valueOptional: Validate parsed registry entries.
Line 85 uses
as LoggedSleepEntry[]without verifying that each entry has the expected shape. Ifsleeps.jsonis manually edited or corrupted, downstream code might see unexpected data.The existing try/catch prevents crashes, so this is advisory. If you want stronger guarantees, you could add a runtime validator (e.g., checking that each entry has
ms,reason,recordedAt).🤖 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 `@playwright/utils/sleep.ts` around lines 76 - 95, The recordSleep function currently casts parsed JSON to LoggedSleepEntry[] without validating shape, which can allow corrupted or manually edited REGISTRY_PATH data to flow downstream; update recordSleep to validate that parsed is an array and that each element contains the required keys/types (ms: number, reason: string, recordedAt: string or valid date) before assigning to existing, skipping or sanitizing invalid entries and optionally logging a warning; keep the surrounding try/catch so failures still don't break tests and reference REGISTRY_PATH, recordSleep, and LoggedSleepEntry when making the checks.
🤖 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.
Inline comments:
In `@playwright.config.ts`:
- Around line 159-198: The default 'chromium' project currently has no
exclusions so admin/restricted specs can run twice; update the 'chromium'
project config (the object with name: 'chromium') to exclude those paths by
adding a testIgnore (or testMatch) entry that excludes
/tests\/admin\/.*\.spec\.ts/ and /tests\/restricted\/.*\.spec\.ts/ so
admin/restricted suites only run under 'chromium-admin' and
'chromium-restricted' respectively.
In `@playwright/auth-helpers.ts`:
- Around line 79-93: login attempts are being retried even when credentials are
invalid because loginAndWaitForDashboard only times out; modify the login flow
to surface explicit auth failures (e.g., have loginAndWaitForDashboard return an
auth response or throw a specific error like InvalidCredentialsError when you
detect a 401/403 or an explicit "invalid credentials" signal) and update the
retry usage around retry(async () => { await loginPage.navigate(); await
loginPage.loginAndWaitForDashboard(username, password); }, { label:
`auth:${role.id}:login`, ... }) so that the retry logic treats that
InvalidCredentialsError (or a non-retriable auth-status return) as
permanent—either by adding a classifier/shouldRetry that returns false for that
error or by catching it and rethrowing without passing it to retry.
In `@playwright/utils/sleep.ts`:
- Around line 97-110: The skip logic in resolveCaller is too broad because
line.includes('sleep.ts') can match substrings; update resolveCaller to extract
the filename from each stack frame (using the same regex already used to get
match[1]) and then skip only when the filename endsWith('sleep.ts') or when the
function name equals 'resolveCaller'; this ensures you still ignore internal
frames but won't falsely skip callers like 'my-sleep.ts' or paths containing
that substring.
---
Nitpick comments:
In `@eslint-rules/no-bare-wait-for-timeout.js`:
- Around line 48-52: The CallExpression visitor only checks callee.property.name
which misses computed access like page['waitForTimeout']; update the check in
the CallExpression handler to derive the property name from callee.property by
handling Identifier and Literal (computed) cases (e.g., get propName from
callee.property.name or callee.property.value) and then compare propName to
'waitForTimeout' before calling context.report({ node, messageId: 'forbidden'
}); ensure you reference the existing callee and context.report usage so other
logic is unchanged.
In `@playwright/utils/sleep.ts`:
- Around line 76-95: The recordSleep function currently casts parsed JSON to
LoggedSleepEntry[] without validating shape, which can allow corrupted or
manually edited REGISTRY_PATH data to flow downstream; update recordSleep to
validate that parsed is an array and that each element contains the required
keys/types (ms: number, reason: string, recordedAt: string or valid date) before
assigning to existing, skipping or sanitizing invalid entries and optionally
logging a warning; keep the surrounding try/catch so failures still don't break
tests and reference REGISTRY_PATH, recordSleep, and LoggedSleepEntry when making
the checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1bcf8d91-6085-475d-b9d7-b5fdebac33f4
📒 Files selected for processing (16)
eslint-rules/README.mdeslint-rules/index.jseslint-rules/no-bare-wait-for-timeout.jseslint-rules/no-direct-login-goto.jseslint.config.jsplaywright.config.tsplaywright/auth-helpers.tsplaywright/auth.admin.setup.tsplaywright/auth.restricted.setup.tsplaywright/auth.setup.tsplaywright/config/roles.tsplaywright/sleeps.jsonplaywright/utils/retry.spec.tsplaywright/utils/retry.tsplaywright/utils/sleep.spec.tsplaywright/utils/sleep.ts
Comment on lines
159
to
+198
| { | ||
| name: 'chromium', | ||
| use: { | ||
| ...devices['Desktop Chrome'], | ||
| storageState: 'playwright/.auth/user.json', | ||
| storageState: ROLES.default.storageStateFile, | ||
| // Launch options for handling SSL in headed mode | ||
| launchOptions: { | ||
| args: ['--ignore-certificate-errors'] | ||
| } | ||
| }, | ||
| dependencies: ['setup'] | ||
| } | ||
| }, | ||
| ...(includeAdminProjects | ||
| ? [ | ||
| { | ||
| name: 'chromium-admin', | ||
| testMatch: /tests\/admin\/.*\.spec\.ts/, | ||
| use: { | ||
| ...devices['Desktop Chrome'], | ||
| storageState: ROLES.admin.storageStateFile, | ||
| launchOptions: { args: ['--ignore-certificate-errors'] } | ||
| }, | ||
| dependencies: ['setup-admin'] | ||
| } | ||
| ] | ||
| : []), | ||
| ...(includeRestrictedProjects | ||
| ? [ | ||
| { | ||
| name: 'chromium-restricted', | ||
| testMatch: /tests\/restricted\/.*\.spec\.ts/, | ||
| use: { | ||
| ...devices['Desktop Chrome'], | ||
| storageState: ROLES.restricted.storageStateFile, | ||
| launchOptions: { args: ['--ignore-certificate-errors'] } | ||
| }, | ||
| dependencies: ['setup-restricted'] | ||
| } | ||
| ] | ||
| : []) |
There was a problem hiding this comment.
Default chromium project will also execute admin/restricted specs.
Because the default project at Line 160 has no exclusion, files under playwright/tests/admin/** and playwright/tests/restricted/** can run twice (default + role project) and may run with the wrong storage state.
Suggested fix
{
name: 'chromium',
+ testIgnore: [/tests\/admin\/.*\.spec\.ts/, /tests\/restricted\/.*\.spec\.ts/],
use: {
...devices['Desktop Chrome'],
storageState: ROLES.default.storageStateFile,
// Launch options for handling SSL in headed mode
launchOptions: {
args: ['--ignore-certificate-errors']
}
},
dependencies: ['setup']
},🤖 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 `@playwright.config.ts` around lines 159 - 198, The default 'chromium' project
currently has no exclusions so admin/restricted specs can run twice; update the
'chromium' project config (the object with name: 'chromium') to exclude those
paths by adding a testIgnore (or testMatch) entry that excludes
/tests\/admin\/.*\.spec\.ts/ and /tests\/restricted\/.*\.spec\.ts/ so
admin/restricted suites only run under 'chromium-admin' and
'chromium-restricted' respectively.
Comment on lines
+79
to
+93
| await retry( | ||
| async () => { | ||
| await loginPage.navigate(); | ||
| await loginPage.loginAndWaitForDashboard(username, password); | ||
| }, | ||
| { | ||
| label: `auth:${role.id}:login`, | ||
| onRetry: (info) => { | ||
| console.warn( | ||
| `[auth:${role.id}] login attempt ${info.attempt} failed (${info.classification}); ` + | ||
| `retrying in ${info.delayMs}ms` | ||
| ); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
Bad-credential failures are likely misclassified as transient retries.
At Line 82, loginAndWaitForDashboard only waits for navigation; when credentials are wrong it typically fails via timeout, and retry treats TimeoutError as transient. That means permanent auth failures are retried instead of failing fast.
Please surface auth response status (or explicit invalid-credential signal) from the login flow and classify 401/403 as permanent in this retry path.
🤖 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 `@playwright/auth-helpers.ts` around lines 79 - 93, login attempts are being
retried even when credentials are invalid because loginAndWaitForDashboard only
times out; modify the login flow to surface explicit auth failures (e.g., have
loginAndWaitForDashboard return an auth response or throw a specific error like
InvalidCredentialsError when you detect a 401/403 or an explicit "invalid
credentials" signal) and update the retry usage around retry(async () => { await
loginPage.navigate(); await loginPage.loginAndWaitForDashboard(username,
password); }, { label: `auth:${role.id}:login`, ... }) so that the retry logic
treats that InvalidCredentialsError (or a non-retriable auth-status return) as
permanent—either by adding a classifier/shouldRetry that returns false for that
error or by catching it and rethrowing without passing it to retry.
Implements GSoC 2026 proposal item WA-2.2. playwright/config/roles.ts - ROLES registry (default, admin, restricted). Each role declares env-var names, fallback credentials, storageState path, and a description. The 'restricted' role has no fallback to fail loudly when env vars are missing. Shape is framework-agnostic so the React repo can adopt the same registry unchanged. playwright/auth-helpers.ts - Single authenticateRole(role, page, browser) procedure used by every auth.<role>.setup.ts. Mirrors the legacy single-role auth.setup.ts but is now parameterized by an AuthRole. Improvements: (1) the sessionStorage->localStorage mirror short- circuits when the key already exists in localStorage, making the helper portable between Angular (sessionStorage) and React (localStorage) without code changes; (2) verification navigation uses ROUTES.home from the Layer-2 registry instead of a hard-coded '/#/' string; (3) the verification context is closed in a try/finally block so a failed assertion never leaks a Chromium process. playwright/auth.setup.ts - Refactored to a one-liner calling authenticateRole(ROLES.default, ...). Legacy playwright/.auth/user.json path preserved so zero specs need updating. playwright/auth.admin.setup.ts - Per-role setup for the admin role. Defaults to mifos/password locally; production CI uses E2E_ADMIN_USERNAME / E2E_ADMIN_PASSWORD. playwright/auth.restricted.setup.ts - Per-role setup for the restricted role. No default credentials - missing env vars produce a fast, actionable error. playwright.config.ts - Multi-role project wiring: (1) chromium project uses ROLES.default.storageStateFile; (2) setup-admin/chromium-admin and setup-restricted/chromium- restricted are conditionally registered via hasSpecFiles() so they are truly zero-cost until a spec is dropped under playwright/tests/admin/** or playwright/tests/restricted/**; (3) both role groups can also be force-enabled via PLAYWRIGHT_ENABLE_ADMIN_PROJECTS=1 / PLAYWRIGHT_ENABLE_RESTRICTED_PROJECTS=1 for CI matrices. Verified live against Fineract https://localhost:8443 + Angular dev server: setup project passes storageState verification. Epic: WEB-967 - Production-Grade E2E and Unit Testing Infrastructure
devvaansh
force-pushed
the
WEB-978-playwright-multi-role-auth-foundation
branch
from
6e9a067 to
8ce61bf
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
playwright.config.ts (1)
127-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefault
chromiumproject will execute admin/restricted specs with wrong credentials.The default
chromiumproject (lines 147-158) has notestIgnoreortestMatchfilter, so it will run ALL specs including those undertests/admin/**andtests/restricted/**. These specs will ALSO run under their dedicatedchromium-admin/chromium-restrictedprojects (lines 159-186), resulting in:
- Duplicate execution: Same spec runs twice.
- Wrong auth context: Admin/restricted specs run once with
ROLES.default.storageStateFile(incorrect) and once with role-specific storage (correct).This defeats the purpose of role isolation and will produce confusing test results when admin/restricted tests fail due to permission mismatches.
🔒 Proposed fix to exclude role-specific specs from default project
{ name: 'chromium', + testIgnore: [/tests\/admin\/.*\.spec\.ts/, /tests\/restricted\/.*\.spec\.ts/], use: { ...devices['Desktop Chrome'], storageState: ROLES.default.storageStateFile,🤖 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 `@playwright.config.ts` around lines 127 - 186, The default "chromium" project currently has no test filter and therefore runs admin/restricted specs with ROLES.default; update the "chromium" project definition in playwright.config.ts to exclude role-specific tests (e.g., add testIgnore: ['**/tests/admin/**', '**/tests/restricted/**'] or tighten testMatch) so that admin specs only run under "chromium-admin" and restricted specs only under "chromium-restricted"; ensure the filters reference the same paths used by the chromium-admin/chromium-restricted projects and keep existing ROLES.default.storageStateFile and dependencies intact.
🤖 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.
Duplicate comments:
In `@playwright.config.ts`:
- Around line 127-186: The default "chromium" project currently has no test
filter and therefore runs admin/restricted specs with ROLES.default; update the
"chromium" project definition in playwright.config.ts to exclude role-specific
tests (e.g., add testIgnore: ['**/tests/admin/**', '**/tests/restricted/**'] or
tighten testMatch) so that admin specs only run under "chromium-admin" and
restricted specs only under "chromium-restricted"; ensure the filters reference
the same paths used by the chromium-admin/chromium-restricted projects and keep
existing ROLES.default.storageStateFile and dependencies intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66281da7-d417-4295-8227-13c2dc43a242
📒 Files selected for processing (6)
playwright.config.tsplaywright/auth-helpers.tsplaywright/auth.admin.setup.tsplaywright/auth.restricted.setup.tsplaywright/auth.setup.tsplaywright/config/roles.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- playwright/auth.admin.setup.ts
- playwright/auth.restricted.setup.ts
- playwright/auth.setup.ts
- playwright/config/roles.ts
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
1 participant
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
Implements GSoC 2026 proposal item ** Multi-role
storageStatefoundation** from Epic WEB-967. Builds on WEB-975 (#3639 — retry infrastructure).Converts the existing single-role auth setup into a reusable, role-driven auth helper that supports parallel credential provisioning for RBAC testing. The default
chromiumproject andplaywright/.auth/user.jsonpath are fully preserved — zero changes required to any existing spec.Changes
playwright/config/roles.ts(new)ROLESregistry withdefault,admin, andrestrictedentriesusernameEnv/passwordEnv, fallback credentials,storageStateFile, anddescriptionrestrictedhas no fallback — missing env vars produce a loud error at setup time rather than a silent failure mid-suiteplaywright/auth-helpers.ts(refactored)authenticateRole(role, page, browser)used by everyauth.<role>.setup.tsretry()(WA-2.1) — transient Fineract 5xx / ECONNRESET during warm-up no longer kills auth setuplocalStorage(React); falls back to mirroring fromsessionStorage(Angular). Works against both repos without branching.ROUTES.homefrom the Layer-2 registry instead of a hard-coded/#/try/finally— no leaked Chromium processes on assertion failureplaywright/auth.setup.ts(refactored)await authenticateRole(ROLES.default, page, browser)playwright/.auth/user.jsonoutput path preserved — zero spec changes requiredplaywright/auth.admin.setup.ts(new)authenticateRole(ROLES.admin, ...). Defaults locally; CI overrides viaE2E_ADMIN_USERNAME/E2E_ADMIN_PASSWORDplaywright/auth.restricted.setup.ts(new)authenticateRole(ROLES.restricted, ...). No credential defaults — fails loudly when env vars are unsetplaywright.config.ts(updated)hasSpecFiles(dir)helper — conditionally registers admin/restricted projects only when matching spec files existsetup-admin/chromium-adminandsetup-restricted/chromium-restrictedare zero-cost until specs land underplaywright/tests/admin/**orplaywright/tests/restricted/**PLAYWRIGHT_ENABLE_ADMIN_PROJECTS=1/PLAYWRIGHT_ENABLE_RESTRICTED_PROJECTS=1How to verify
Related issues and discussion
https://mifosforge.jira.com/browse/WEB-978
Parent Epic: https://mifosforge.jira.com/browse/WEB-967
Depends on: #3639 (WEB-975 — retry infrastructure, must merge first)
Screenshots, if any
No UI changes — Playwright infrastructure only.
Checklist
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit