Skip to content

fix(auth): provision an organization on OAuth signup - #4123

Merged
PierreBrisorgueil merged 3 commits into
masterfrom
fix/4115-oauth-org-provisioning
Sep 25, 2026
Merged

PierreBrisorgueil merged 3 commits into
masterfrom
fix/4115-oauth-org-provisioning

Conversation

@PierreBrisorgueil

@PierreBrisorgueil PierreBrisorgueil commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

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.
  • The Google/Apple strategy wrappers relay that as info.created through passport's verify callback.
  • oauthCallback's passport callback is now async and reads that 3rd info argument. After the !user guard, it calls AuthOrganizationService.handleSignupOrganization(user) only when info.created is true (best-effort try/catch, same pattern as verifyEmail). 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.
  • The whole callback body is wrapped in an outer try/catch routed to oauthErrorRedirect: passport never awaits the callback's promise, so any other throw would otherwise become an unhandled rejection. That fallback redirect now also checks res.headersSent first, so a throw after the success response already started writing can't attempt a conflicting second redirect.
  • Error-logging for the callback deduped into one helper.

Tests

modules/auth/tests/auth.oauth.signup-org.unit.tests.js (9 cases): provisioning fires on info.created, is skipped for an existing user with no current org, skipped for an existing user with a current org, skipped when info is absent entirely, skipped on err / !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.js extended 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.
  • A provider-unverified email with the mailer on still gets 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.created instead.

Closes #4115

https://claude.ai/code/session_01TTK9g6SFCfjfuWvB3MLFr3

Summary by CodeRabbit

  • New Features
    • New users who sign up through Apple or Google OAuth are provisioned with an organization.
  • Bug Fixes
    • OAuth callback failures are handled more safely, avoiding a second redirect if the response has already been sent.
    • Organization provisioning errors no longer prevent successful sign-in.

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 49 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: pierreb-devkit/Node/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c092a993-5772-410c-99e0-b26d0bf37e58

📥 Commits

Reviewing files that changed from the base of the PR and between a7dcb10 and bb02343.

📒 Files selected for processing (6)
  • ERRORS.md
  • modules/auth/controllers/auth.controller.js
  • modules/auth/strategies/local/apple.js
  • modules/auth/strategies/local/google.js
  • modules/auth/tests/auth.oauth.signup-org.unit.tests.js
  • modules/auth/tests/auth.oauth.signup.analytics.unit.tests.js
ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pierreb-devkit/Node/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 5135b100-d366-4c07-800c-dc15060c458c

📥 Commits

Reviewing files that changed from the base of the PR and between 06ce893 and a7dcb10.

📒 Files selected for processing (6)
  • ERRORS.md
  • modules/auth/controllers/auth.controller.js
  • modules/auth/strategies/local/apple.js
  • modules/auth/strategies/local/google.js
  • modules/auth/tests/auth.oauth.signup-org.unit.tests.js
  • modules/auth/tests/auth.oauth.signup.analytics.unit.tests.js
Files not reviewed due to moderation or processing errors (6)
  • modules/auth/controllers/auth.controller.js
  • modules/auth/strategies/local/apple.js
  • modules/auth/strategies/local/google.js
  • modules/auth/tests/auth.oauth.signup.analytics.unit.tests.js
  • modules/auth/tests/auth.oauth.signup-org.unit.tests.js
  • ERRORS.md

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The 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.

Changes

OAuth signup provisioning

Layer / File(s) Summary
Mark and report new OAuth signups
modules/auth/controllers/auth.controller.js, modules/auth/strategies/local/apple.js, modules/auth/strategies/local/google.js, modules/auth/tests/auth.oauth.signup.analytics.unit.tests.js
Newly created OAuth users receive a non-enumerable _isOAuthSignup marker. Apple and Google pass its boolean value as callback info. Tests check the marker for new, existing, and linked users.
Provision organizations and handle callback errors
modules/auth/controllers/auth.controller.js, modules/auth/tests/auth.oauth.signup-org.unit.tests.js, ERRORS.md
The asynchronous callback attempts organization provisioning when info.created is true. Provisioning failures do not prevent the token cookie and /token redirect. Callback failures use a logging helper and do not trigger a fallback redirect after headers are sent. Tests cover provisioning and error paths.

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
Loading

Merge Risk: ⚪ Minimal · up to a7dcb

No confirmed issue currently blocks merging. Complete the normal checks for OAuth signup and organization provisioning.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a7dcb

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The newly reachable persistent effect is creation of a free workspace and owner membership for a newly created OAuth identity, with a possible organization-scoped signup grant. The inspected flow does not add an existing-user or existing-organization provisioning path.

Security Findings and Attack Paths

  • inferred — No introduced identity-selection or organization-policy bypass was established in the inspected path: the callback uses Passport’s resolved user, the marker originates in the user-creation branch, and organization policy remains enforced by the provisioning service. Provider-library validation was not independently verified.

Trust Boundaries and Controls

  • observed — The public GET and Apple POST callback routes enter Passport authentication. The controller consumes the provider-resolved user rather than selecting an identity from callback request-body fields; signup eligibility checks precede creation.

Resilience and Maintainability Implications

  • inferred — Authentication can succeed without a workspace after a provisioning error or a verification-required result. The active-membership check supports sequential convergence, but its find-then-create sequence is not an atomic per-user guarantee; the inspected new-account marker limits, but does not prove the absence of, overlapping provisioning from other entry paths.

Hardening Proposals

  • proposed — Consider a policy-aware reconciliation path for newly created accounts whose workspace provisioning fails, preserving the verification gate and avoiding reprovisioning users deliberately left without an organization.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning 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 requir… 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 c…
Linked Issues check ⚠️ Warning Issue #4115 requires provisioning when the resolved OAuth user has no current organization. The implementation provisions only when info.created is true. It therefore provisions a new OAuth user, bu… Change the OAuth callback condition to provision when !user.currentOrganization, as required by the issue's updated scope. Keep the best-effort error handling. Update the tests to require provisioning for an existing org-less user and to …
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: provisioning an organization during OAuth signup.
Out of Scope Changes check ✅ Passed The changes stay within issue #4115. The _isOAuthSignup marker and provider info.created plumbing support OAuth signup detection. The provisioning call and tests implement the workspace requiremen…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 5…
Full details: Description check

Explanation

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 check

Explanation

Issue #4115 requires provisioning when the resolved OAuth user has no current organization. The implementation provisions only when info.created is true. It therefore provisions a new OAuth user, but it does not provision an existing OAuth user with currentOrganization unset. The new unit test explicitly verifies this omission. The best-effort failure handling and callback error handling meet the related coding requirements.

Resolution

Change the OAuth callback condition to provision when !user.currentOrganization, as required by the issue's updated scope. Keep the best-effort error handling. Update the tests to require provisioning for an existing org-less user and to skip provisioning only when the user has a current organization or the callback has an error or no user.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@PierreBrisorgueil
PierreBrisorgueil force-pushed the fix/4115-oauth-org-provisioning branch from a8049e6 to 6fa4599 Compare September 25, 2026 08:09
@codecov

codecov Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.37%. Comparing base (e408163) to head (bb02343).

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              
Flag Coverage Δ
integration 63.94% <71.42%> (-0.04%) ⬇️
unit 79.34% <100.00%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e408163...bb02343. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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
@PierreBrisorgueil
PierreBrisorgueil force-pushed the fix/4115-oauth-org-provisioning branch from a7dcb10 to bb02343 Compare September 25, 2026 08:38
@PierreBrisorgueil
PierreBrisorgueil merged commit 41ff740 into master Sep 25, 2026
8 checks passed
@PierreBrisorgueil
PierreBrisorgueil deleted the fix/4115-oauth-org-provisioning branch September 25, 2026 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 OAuth signup never provisions an organization

1 participant