Skip to content

Task/fix oauth for existing users#406

Open
TySmith17 wants to merge 3 commits into
Openpanel-dev:mainfrom
gigsmart:task/fix-oauth-for-existing-users
Open

Task/fix oauth for existing users#406
TySmith17 wants to merge 3 commits into
Openpanel-dev:mainfrom
gigsmart:task/fix-oauth-for-existing-users

Conversation

@TySmith17

@TySmith17 TySmith17 commented Jun 29, 2026

Copy link
Copy Markdown

Summary by CodeRabbit

  • New Features

    • Registration-related checks are now available more broadly across the app, improving access to sign-up flows.
  • Bug Fixes

    • Invite-based registration is no longer blocked by a restrictive environment setting in some cases.
    • OAuth sign-in now follows the updated registration-eligibility logic more consistently.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

getIsRegistrationAllowed is exported from packages/trpc/src/routers/auth.ts and re-exported alongside TRPCAccessError from the package index. The ALLOW_INVITATION environment variable check inside the invite branch and the registration-allowed gate in signInOAuth are removed. A blank line is added in the OAuth callback controller.

Changes

Registration Allowance Refactor

Layer / File(s) Summary
Export and logic changes in auth router + package index
packages/trpc/src/routers/auth.ts, packages/trpc/index.ts
getIsRegistrationAllowed is changed from a local to an exported function. The ALLOW_INVITATION === 'false' early return in the invite branch is removed. The signInOAuth mutation no longer calls getIsRegistrationAllowed and throws TRPCAccessError when disallowed. Both getIsRegistrationAllowed and TRPCAccessError are added as public re-exports from the package index.
Whitespace-only edit
apps/api/src/controllers/oauth-callback.controller.tsx
Single blank line inserted between the existingUser guard and db.user.create in handleNewUser.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A gate was removed, an export set free,
No more ALLOW_INVITATION blocking the spree,
Invites flow onward, unblocked by the env,
The rabbit hops forward and checks once again,
With TRPCAccessError now out in the light,
The warren of exports is tidy and right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.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 matches the OAuth-focused changes and is reasonably aligned with the existing-user sign-in flow updates.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch task/fix-oauth-for-existing-users

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/trpc/src/routers/auth.ts (1)

92-96: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Keep the OAuth registration check on the callback’s new-user path.

Line 94 now lets every OAuth flow start, but the supplied stack says the OAuth callback only got a whitespace change. That means self-hosted deployments with registration disabled can still create new OAuth users. Move getIsRegistrationAllowed(inviteId) to the OAuth callback branch that creates a user when no existing account is found.

🤖 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/trpc/src/routers/auth.ts` around lines 92 - 96, The registration
gate for OAuth was moved too early in signInOAuth, which now blocks or allows
the entire OAuth start flow instead of only the new-user callback path. Keep
getIsRegistrationAllowed(inviteId) in the OAuth callback branch that handles the
no-existing-account case, and only apply it right before creating a new user
there so existing users can still complete sign-in while registration-disabled
deployments cannot create new OAuth accounts.
apps/api/src/controllers/oauth-callback.controller.tsx (1)

100-114: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Link OAuth accounts instead of rejecting existing users apps/api/src/controllers/oauth-callback.controller.tsx:100-114
handleNewUser still throws when the email already exists, so a user who signed up with email/password can’t finish GitHub/Google OAuth. This path should attach the OAuth account to the matched user instead of erroring.

🤖 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 `@apps/api/src/controllers/oauth-callback.controller.tsx` around lines 100 -
114, The existing-user branch in handleNewUser currently throws a LogError when
db.user.findFirst returns a match, which blocks OAuth sign-in for users whose
email already exists. Update handleNewUser to link the OAuth provider account to
the matched user instead of rejecting it, using the existingUser, oauthUser, and
providerName data in apps/api/src/controllers/oauth-callback.controller.tsx, and
only fall back to an error if account linking cannot be completed.
🤖 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 `@apps/api/src/controllers/oauth-callback.controller.tsx`:
- Around line 100-114: The existing-user branch in handleNewUser currently
throws a LogError when db.user.findFirst returns a match, which blocks OAuth
sign-in for users whose email already exists. Update handleNewUser to link the
OAuth provider account to the matched user instead of rejecting it, using the
existingUser, oauthUser, and providerName data in
apps/api/src/controllers/oauth-callback.controller.tsx, and only fall back to an
error if account linking cannot be completed.

In `@packages/trpc/src/routers/auth.ts`:
- Around line 92-96: The registration gate for OAuth was moved too early in
signInOAuth, which now blocks or allows the entire OAuth start flow instead of
only the new-user callback path. Keep getIsRegistrationAllowed(inviteId) in the
OAuth callback branch that handles the no-existing-account case, and only apply
it right before creating a new user there so existing users can still complete
sign-in while registration-disabled deployments cannot create new OAuth
accounts.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 109a2641-b9fe-40e1-bf63-8981b8cd099d

📥 Commits

Reviewing files that changed from the base of the PR and between 4ed4d39 and 2b445ee.

📒 Files selected for processing (3)
  • apps/api/src/controllers/oauth-callback.controller.tsx
  • packages/trpc/index.ts
  • packages/trpc/src/routers/auth.ts

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.

2 participants