Frontend/auth updates#1727
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughOAuth authentication is reorganized around shared CSRF and PKCE utilities. Cognito clients, hosted-login URLs, and frontend routes now use application-specific callback paths with dedicated callback pages and shared token exchange handling. ChangesOAuth callback flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant PublicDashboard
participant Cognito
participant AuthCallback
PublicDashboard->>Browser: Generate CSRF state and PKCE challenge
Browser->>Cognito: Request hosted login with state and code challenge
Cognito-->>AuthCallback: Redirect with authorization code and state
AuthCallback->>Cognito: Exchange code with PKCE verifier
Cognito-->>AuthCallback: Return tokens
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
webroot/src/pages/AuthCallback/StaffCosmo/StaffCosmo.vue (1)
8-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared error markup.
This template is duplicated verbatim across
StaffCosmo.vue,StaffJcc.vue, and (per the stack outline)StaffSocialWork.vue/LicenseeJcc.vue. Each new page only needs to render this same error card, so extracting a sharedAuthCallbackErrorcomponent (or a slot in a common wrapper) would reduce duplication and centralize future error-copy changes.🤖 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 `@webroot/src/pages/AuthCallback/StaffCosmo/StaffCosmo.vue` around lines 8 - 20, Extract the duplicated error card markup from StaffCosmo.vue, StaffJcc.vue, StaffSocialWork.vue, and LicenseeJcc.vue into a shared AuthCallbackError component or common wrapper. Preserve the existing isError conditional, translations, styling classes, and DashboardPublic router link, then replace each page’s inline markup with the shared component.backend/social-work-app/docs/internal/postman/postman-collection.json (1)
164-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the PKCE and CSRF flow in these requests.
The authorize requests omit
state,code_challenge, andcode_challenge_method; the token request omitscode_verifier. Update the collection’s pre-request setup and exchange request so Postman validates the security contract introduced here.Also applies to: 240-247, 312-319
🤖 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 `@backend/social-work-app/docs/internal/postman/postman-collection.json` around lines 164 - 167, Update the Postman authorization-flow requests and shared pre-request setup to generate and send a CSRF state and PKCE code challenge with code_challenge_method, then persist the matching code_verifier for the flow. Add the stored state, challenge parameters, and verifier to the corresponding token exchange requests, including the sections identified by the repeated authorize/token request blocks.webroot/src/pages/AuthCallback/_mixins/handler.mixin.ts (3)
138-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out
toNativeexport.The commented-out
// export default toNative(MixinAuthCallbackHandler);is dead code. The direct class export on line 140 is correct for use withmixins()in subclasses likeStaffCosmo.ts.🧹 Proposed cleanup
- -// export default toNative(MixinAuthCallbackHandler); - export default MixinAuthCallbackHandler;🤖 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 `@webroot/src/pages/AuthCallback/_mixins/handler.mixin.ts` at line 138, Remove the commented-out toNative export near MixinAuthCallbackHandler, keeping the active direct class export unchanged for mixins() consumers.
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type annotations for
cognitoAuthDomainandcognitoClientIdparameters.Both
getTokensandfetchCognitoTokensleavecognitoAuthDomainandcognitoClientIduntyped, defaulting toany. Addingstringannotations would improve type safety and consistency with the typedauthTypeparameter.🔧 Proposed fix
- async getTokens(appMode: AppModes, authType: AuthTypes, cognitoAuthDomain, cognitoClientId): Promise<void> { + async getTokens(appMode: AppModes, authType: AuthTypes, cognitoAuthDomain: string, cognitoClientId: string): Promise<void> {- async fetchCognitoTokens(authType: AuthTypes, cognitoAuthDomain, cognitoClientId): Promise<void> { + async fetchCognitoTokens(authType: AuthTypes, cognitoAuthDomain: string, cognitoClientId: string): Promise<void> {Also applies to: 99-99
🤖 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 `@webroot/src/pages/AuthCallback/_mixins/handler.mixin.ts` at line 84, Add explicit string type annotations to the cognitoAuthDomain and cognitoClientId parameters in both getTokens and fetchCognitoTokens, preserving the existing appMode and authType types.
99-117: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout and error logging to the Cognito token exchange.
The
axios.post()call has no explicit timeout — if Cognito is slow or unresponsive, the user is stuck on a blank callback page until the browser's default timeout fires. Additionally, the.catch()on line 88 silently swallows the error with no logging, making production debugging difficult.⏱️ Proposed fix: add timeout and error logging
params.append('code_verifier', consumePkceCodeVerifier() || ''); - const { data } = await axios.post(`${cognitoAuthDomain}/oauth2/token`, params); + const { data } = await axios.post(`${cognitoAuthDomain}/oauth2/token`, params, { + timeout: 15000, + });And in
getTokens():- await this.fetchCognitoTokens(authType, cognitoAuthDomain, cognitoClientId).catch(() => { + await this.fetchCognitoTokens(authType, cognitoAuthDomain, cognitoClientId).catch((err) => { + console.error('Auth callback token exchange failed:', err); this.isError = true; });🤖 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 `@webroot/src/pages/AuthCallback/_mixins/handler.mixin.ts` around lines 99 - 117, Update fetchCognitoTokens to pass an explicit timeout option to the axios.post token request, using the project’s standard request timeout if available. In getTokens, replace the silent catch with error logging that includes the caught error and relevant Cognito token-exchange context before preserving the existing failure handling.webroot/src/pages/AuthCallback/StaffSocialWork/StaffSocialWork.spec.ts (1)
12-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSmoke test only covers mounting; consider testing auth callback behavior.
The test verifies the component mounts but doesn't exercise the CSRF state verification, token exchange, or error display paths that are the core purpose of this callback page. The mixin's
created()will setisError = truewhenverifyCsrfState()fails (no stored state in test), which is untested.💡 Suggested additional test cases
it('should display error when CSRF state verification fails', async () => { const wrapper = await mountShallow(StaffSocialWork); await wrapper.vm.$nextTick(); expect(wrapper.findComponent(StaffSocialWork).exists()).to.equal(true); // Assert isError is true and error UI is rendered }); it('should verify CSRF state and exchange tokens on valid callback', async () => { // Set up sessionStorage with matching state, route query with code+state // Assert token exchange was called and redirect occurred });🤖 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 `@webroot/src/pages/AuthCallback/StaffSocialWork/StaffSocialWork.spec.ts` around lines 12 - 19, Expand the StaffSocialWork tests beyond mounting by covering the auth callback behavior in the component’s created lifecycle and mixin. Add a test for failed verifyCsrfState() that waits for $nextTick and asserts isError plus the rendered error UI, and a valid callback test that configures matching sessionStorage state and route code/state, then verifies token exchange and redirect behavior using the relevant mocked methods.
🤖 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 `@webroot/src/pages/AuthCallback/_mixins/mixins.spec.ts`:
- Around line 43-50: Update the “should successfully get tokens” test to stub
the axios.post token exchange with a successful response and spy on the router
navigation method used by getTokens, then assert it is called with the expected
route instead of checking history.state.replaced. Ensure no real request to
http://localhost/oauth2/token can occur.
In `@webroot/src/pages/MfaResetConfirmLicensee/MfaResetConfirmLicensee.ts`:
- Around line 51-61: Handle failures from createPkceChallenge() within
MfaResetConfirmLicensee.created() using try/catch, preserve the existing CSRF
initialization, and surface a clear user-facing error or disable login when PKCE
generation fails so navigation cannot occur with an empty code_challenge.
- Line 92: Update the getHostedLoginUri call in MfaResetConfirmLicensee to pass
AppModes.JCC instead of this.appMode, ensuring the licensee callback path always
targets the supported JCC route.
In `@webroot/src/utils/auth.ts`:
- Around line 133-218: Resolve the LICENSEE callback mismatch in
getHostedLoginUri: either add the missing LicenseeCosmo and LicenseeSocialWork
callback pages and router entries for /auth/callback/licensee/cosmo and
/auth/callback/licensee/socialwork, or restrict callback-path generation so
LICENSEE only produces supported routes such as /auth/callback/licensee/jcc.
---
Nitpick comments:
In `@backend/social-work-app/docs/internal/postman/postman-collection.json`:
- Around line 164-167: Update the Postman authorization-flow requests and shared
pre-request setup to generate and send a CSRF state and PKCE code challenge with
code_challenge_method, then persist the matching code_verifier for the flow. Add
the stored state, challenge parameters, and verifier to the corresponding token
exchange requests, including the sections identified by the repeated
authorize/token request blocks.
In `@webroot/src/pages/AuthCallback/_mixins/handler.mixin.ts`:
- Line 138: Remove the commented-out toNative export near
MixinAuthCallbackHandler, keeping the active direct class export unchanged for
mixins() consumers.
- Line 84: Add explicit string type annotations to the cognitoAuthDomain and
cognitoClientId parameters in both getTokens and fetchCognitoTokens, preserving
the existing appMode and authType types.
- Around line 99-117: Update fetchCognitoTokens to pass an explicit timeout
option to the axios.post token request, using the project’s standard request
timeout if available. In getTokens, replace the silent catch with error logging
that includes the caught error and relevant Cognito token-exchange context
before preserving the existing failure handling.
In `@webroot/src/pages/AuthCallback/StaffCosmo/StaffCosmo.vue`:
- Around line 8-20: Extract the duplicated error card markup from
StaffCosmo.vue, StaffJcc.vue, StaffSocialWork.vue, and LicenseeJcc.vue into a
shared AuthCallbackError component or common wrapper. Preserve the existing
isError conditional, translations, styling classes, and DashboardPublic router
link, then replace each page’s inline markup with the shared component.
In `@webroot/src/pages/AuthCallback/StaffSocialWork/StaffSocialWork.spec.ts`:
- Around line 12-19: Expand the StaffSocialWork tests beyond mounting by
covering the auth callback behavior in the component’s created lifecycle and
mixin. Add a test for failed verifyCsrfState() that waits for $nextTick and
asserts isError plus the rendered error UI, and a valid callback test that
configures matching sessionStorage state and route code/state, then verifies
token exchange and redirect behavior using the relevant mocked methods.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85950346-9adc-4ed8-8df5-ef52b99643b1
📒 Files selected for processing (80)
backend/common-cdk/common_constructs/user_pool.pybackend/common-cdk/tests/test_user_pool.pybackend/compact-connect/docs/internal/postman/postman-collection.jsonbackend/compact-connect/docs/postman/postman-collection.jsonbackend/compact-connect/stacks/persistent_stack/staff_users.pybackend/compact-connect/stacks/provider_users/provider_users.pybackend/compact-connect/tests/app/base.pybackend/cosmetology-app/docs/internal/postman/postman-collection.jsonbackend/cosmetology-app/docs/postman/postman-collection.jsonbackend/cosmetology-app/docs/search-internal/postman/postman-collection.jsonbackend/cosmetology-app/stacks/persistent_stack/staff_users.pybackend/cosmetology-app/tests/app/base.pybackend/social-work-app/docs/internal/postman/postman-collection.jsonbackend/social-work-app/docs/postman/postman-collection.jsonbackend/social-work-app/docs/search-internal/postman/postman-collection.jsonbackend/social-work-app/stacks/persistent_stack/staff_users.pybackend/social-work-app/tests/app/base.pywebroot/src/app.config.tswebroot/src/components/App/App.spec.tswebroot/src/components/App/App.tswebroot/src/components/AutoLogout/AutoLogout.tswebroot/src/components/ChangePassword/ChangePassword.tswebroot/src/components/MilitaryAffiliationInfoBlock/MilitaryAffiliationInfoBlock.tswebroot/src/components/Page/PageMainNav/PageMainNav.tswebroot/src/components/StateSettingsList/StateSettingsList.tswebroot/src/components/UserAccount/UserAccount.tswebroot/src/models/LicenseeUser/LicenseeUser.model.spec.tswebroot/src/models/LicenseeUser/LicenseeUser.model.tswebroot/src/models/StaffUser/StaffUser.model.spec.tswebroot/src/models/StaffUser/StaffUser.model.tswebroot/src/models/User/User.model.tswebroot/src/network/licenseApi/data.api.tswebroot/src/network/licenseApi/interceptors.tswebroot/src/network/searchApi/interceptors.tswebroot/src/network/stateApi/interceptors.tswebroot/src/network/userApi/interceptors.tswebroot/src/pages/AuthCallback/AuthCallback.spec.tswebroot/src/pages/AuthCallback/AuthCallback.tswebroot/src/pages/AuthCallback/LicenseeJcc/LicenseeJcc.lesswebroot/src/pages/AuthCallback/LicenseeJcc/LicenseeJcc.spec.tswebroot/src/pages/AuthCallback/LicenseeJcc/LicenseeJcc.tswebroot/src/pages/AuthCallback/LicenseeJcc/LicenseeJcc.vuewebroot/src/pages/AuthCallback/StaffCosmo/StaffCosmo.lesswebroot/src/pages/AuthCallback/StaffCosmo/StaffCosmo.spec.tswebroot/src/pages/AuthCallback/StaffCosmo/StaffCosmo.tswebroot/src/pages/AuthCallback/StaffCosmo/StaffCosmo.vuewebroot/src/pages/AuthCallback/StaffJcc/StaffJcc.lesswebroot/src/pages/AuthCallback/StaffJcc/StaffJcc.spec.tswebroot/src/pages/AuthCallback/StaffJcc/StaffJcc.tswebroot/src/pages/AuthCallback/StaffJcc/StaffJcc.vuewebroot/src/pages/AuthCallback/StaffSocialWork/StaffSocialWork.lesswebroot/src/pages/AuthCallback/StaffSocialWork/StaffSocialWork.spec.tswebroot/src/pages/AuthCallback/StaffSocialWork/StaffSocialWork.tswebroot/src/pages/AuthCallback/StaffSocialWork/StaffSocialWork.vuewebroot/src/pages/AuthCallback/_mixins/handler.mixin.tswebroot/src/pages/AuthCallback/_mixins/mixins.spec.tswebroot/src/pages/CompactSettings/CompactSettings.tswebroot/src/pages/Home/Home.tswebroot/src/pages/Logout/Logout.tswebroot/src/pages/MfaResetConfirmLicensee/MfaResetConfirmLicensee.tswebroot/src/pages/MfaResetStartLicensee/MfaResetStartLicensee.tswebroot/src/pages/PrivilegeDetail/PrivilegeDetail.tswebroot/src/pages/PublicDashboard/PublicDashboard.spec.tswebroot/src/pages/PublicDashboard/PublicDashboard.tswebroot/src/pages/StateSettings/StateSettings.tswebroot/src/router/index.tswebroot/src/router/routes.tswebroot/src/store/global/global.mutations.tswebroot/src/store/global/global.spec.tswebroot/src/store/global/global.state.tswebroot/src/store/user/user.actions.tswebroot/src/store/user/user.mutations.tswebroot/src/store/user/user.spec.tswebroot/src/store/user/user.state.tswebroot/src/styles.common/_mixins.lesswebroot/src/styles.common/mixins/auth-error.lesswebroot/src/utils/auth.tswebroot/tests/helpers/setup.tswebroot/tsconfig.jsonwebroot/vue.config.js
💤 Files with no reviewable changes (2)
- webroot/src/pages/AuthCallback/AuthCallback.spec.ts
- webroot/src/pages/AuthCallback/AuthCallback.ts
landonshumway-ia
left a comment
landonshumway-ia
left a comment
There was a problem hiding this comment.
The backend changes for this look clean, the only thing missing is we need to keep the legacy callback URL in place to avoid downtime for JCC.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/common-cdk/common_constructs/user_pool.py (1)
259-281: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid adding duplicate or unexpected callback URLs.
When
callback_pathuses its default/auth/callback, lines 274 and 276 append the same URL twice. For custom paths, the implementation adds a third legacy URL, whilebackend/common-cdk/tests/test_user_pool.pyexpects only the custom domain and localhost URLs. This will break the supplied callback URL assertions and may complicate Cognito client configuration.Only append the legacy URL when
callback_path != '/auth/callback', or update the contract and tests deliberately.Proposed fix
if ui_domain_name is not None: callback_urls.append(f'https://{ui_domain_name}{callback_path}') - # TODO - remove after cutover to custom callback paths is deployed `#noqa`: FIX002 - callback_urls.append(f'https://{ui_domain_name}/auth/callback') + if callback_path != '/auth/callback': + # TODO - remove after cutover to custom callback paths is deployed `#noqa`: FIX002 + callback_urls.append(f'https://{ui_domain_name}/auth/callback')🤖 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 `@backend/common-cdk/common_constructs/user_pool.py` around lines 259 - 281, The callback URL construction in the app-client creation method duplicates the default URL and adds an unexpected legacy URL for custom paths. Update the logic around callback_path so the legacy /auth/callback URL is appended only when callback_path differs from /auth/callback, preserving the custom-domain and localhost URLs expected by the existing contract.
🤖 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.
Outside diff comments:
In `@backend/common-cdk/common_constructs/user_pool.py`:
- Around line 259-281: The callback URL construction in the app-client creation
method duplicates the default URL and adds an unexpected legacy URL for custom
paths. Update the logic around callback_path so the legacy /auth/callback URL is
appended only when callback_path differs from /auth/callback, preserving the
custom-domain and localhost URLs expected by the existing contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5efa48dd-8038-4063-b301-8fd64ea9b5b8
📒 Files selected for processing (1)
backend/common-cdk/common_constructs/user_pool.py
|
@jlkravitz This is ready for your review. |
3 participants
Requirements List
Description List
stateparameter to be a random-value CSRF protectionTesting List
yarn test:unit:allshould run without errors or warningsyarn serveshould run without errors or warningsyarn buildshould run without errors or warningsCloses #1641
Summary by CodeRabbit