fix(auth): provision an organization on OAuth signup - #4123
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: pierreb-devkit/Node/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pierreb-devkit/Node/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Files not reviewed due to moderation or processing errors (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe OAuth strategies now report whether a user was newly created. The OAuth callback uses that status to request organization provisioning and adds error handling for provisioning and callback failures. ChangesOAuth signup provisioning
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant OAuthStrategy
participant oauthCallback
participant AuthOrganizationService
participant HTTPResponse
OAuthStrategy->>oauthCallback: Pass user and info.created
oauthCallback->>AuthOrganizationService: Provision organization when info.created is true
AuthOrganizationService-->>oauthCallback: Return or reject provisioning request
oauthCallback->>HTTPResponse: Set token cookie and redirect to /token
Merge Risk: ⚪ Minimal · up to No confirmed issue currently blocks merging. Complete the normal checks for OAuth signup and organization provisioning. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new workspace-creation path is limited to newly created OAuth accounts, and the organization service retains its verification and membership checks. No new privilege bypass was established. Provisioning can still fail without preventing sign-in, and some security coverage remains incomplete. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains what changed, why it changed, implementation details, tests, known gaps, and the related issue. However, it does not follow the repository template because it omits the required Summary, Scope, Validation, Guardrails checkboxes, and Notes for reviewers sections. Resolution Restructure the description to include the template sections. Add module impact, cross-module impact, risk level, completed validation checks, guardrail confirmations, and reviewer notes. Preserve the existing implementation details, test coverage, known gaps, and issue reference under the appropriate sections. Full details: Linked Issues checkExplanation Issue Resolution Change the OAuth callback condition to provision when ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review coverage is incomplete: 6 files could not be fully reviewed. Findings from completed review steps are included; see review info for details. 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 |
a8049e6 to
6fa4599
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4123 +/- ##
==========================================
+ Coverage 94.36% 94.37% +0.01%
==========================================
Files 173 173
Lines 6016 6027 +11
Branches 1937 1938 +1
==========================================
+ Hits 5677 5688 +11
Misses 276 276
Partials 63 63
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
OAuth (Google/Apple) signup resolved a user but never called AuthOrganizationService.handleSignupOrganization — only local signup and verifyEmail did, so an OAuth user landed on the org-required page with no workspace, unlike email signup. oauthCallback's passport callback is now async: after the !user guard, it calls handleSignupOrganization (best-effort, same pattern as verifyEmail) whenever the resolved user has no currentOrganization — covers new OAuth signups and any account left orphaned by this bug (self-heals, no backfill needed), and is a no-op on a normal login that already has a workspace. Wrapped the whole callback body in a try/catch: passport.authenticate() invokes this callback fire-and-forget and never awaits its returned promise, so any other throw here would otherwise become a silent unhandled rejection instead of the existing error redirect. Refs #3762, #3765, #3680 Claude-Session: https://claude.ai/code/session_01TTK9g6SFCfjfuWvB3MLFr3
…etup - auth.controller.js: extract logOAuthCallbackFailure() so the three OAuth-callback failure branches (passport err, !user, outer catch-all) share one logger.error call instead of repeating it verbatim. - auth.oauth.signup-org.unit.tests.js: extract the shared jest mock registration into one registerMocks() helper reused by beforeEach and the jwt-throw test, instead of duplicating ~80 lines of module mocks. Claude-Session: https://claude.ai/code/session_01TTK9g6SFCfjfuWvB3MLFr3
Gating org provisioning on `!user.currentOrganization` re-provisioned any existing org-less user (removed from org, org deleted, pending join) on every OAuth login. Gate on `info.created` instead, set by checkOAuthUserProfile's create branch and relayed through passport's verify-callback info argument to oauthCallback. Also guards the outer-catch fallback redirect with res.headersSent, so a throw after the success response starts writing can't attempt a second, conflicting redirect. Claude-Session: https://claude.ai/code/session_01TTK9g6SFCfjfuWvB3MLFr3
a7dcb10 to
bb02343
Compare
What
OAuth signup (Google / Apple) now provisions the user's workspace, like email signup does — but only for a genuine new signup.
Why
OAuth users landed on the org-required page and had to create a workspace by hand; many stopped there.
How
checkOAuthUserProfile's create branch (new account) marks its result so the caller can tell a brand-new signup apart from an existing/linked user.info.createdthrough passport's verify callback.oauthCallback's passport callback is nowasyncand reads that 3rdinfoargument. After the!userguard, it callsAuthOrganizationService.handleSignupOrganization(user)only wheninfo.createdis true (best-effort try/catch, same pattern asverifyEmail). An existing user — whether on a normal login or one with no current org right now (removed from their org, org deleted, a pending join request) — is never re-provisioned.oauthErrorRedirect: passport never awaits the callback's promise, so any other throw would otherwise become an unhandled rejection. That fallback redirect now also checksres.headersSentfirst, so a throw after the success response already started writing can't attempt a conflicting second redirect.Tests
modules/auth/tests/auth.oauth.signup-org.unit.tests.js(9 cases): provisioning fires oninfo.created, is skipped for an existing user with no current org, skipped for an existing user with a current org, skipped wheninfois absent entirely, skipped onerr/!user, a provisioning rejection still sets the cookie and redirects, a throw past provisioning hits the outer catch, and a throw after headers are already sent does not attempt a second redirect.auth.oauth.signup.analytics.unit.tests.jsextended to assert the new create-branch marker is set only on branch 4.Known gaps (out of scope)
suggestedJoin(domain-match hint) is not carried through the OAuth redirect.emailVerificationRequired.Reviewer: /critical-review fallback (CodeRabbit rate-limited on this repo's Free review quota during this PR's review window). Finding: gating provisioning on "no current org" would have re-provisioned any existing org-less user on every OAuth login — fixed by gating on
info.createdinstead.Closes #4115
https://claude.ai/code/session_01TTK9g6SFCfjfuWvB3MLFr3
Summary by CodeRabbit