Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6e9f038a-fe65-4cb3-9bf6-5a457d477d26) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the organization synchronization logic in AuthService into a reusable syncUserOrganizations method, simplifies the organization and user creation flow, and adds email-based user lookup as a fallback. The review highlights three important issues: a critical bug where passing an existing orgId to createOrgAndUser can cause a database unique constraint violation, a high-severity security risk of OAuth Account Takeover if email verification is not checked, and a medium-severity issue where non-admin users can inadvertently rename organizations.
| const create = await this._organizationService.createOrgAndUser( | ||
| { | ||
| company: companyName, | ||
| email: providerUser.email, | ||
| password: '', | ||
| provider, | ||
| providerId: providerUser.id, | ||
| datafast_visitor_id: body.datafast_visitor_id || '', | ||
| orgId: firstOrg?.id, | ||
| }, | ||
| ip, | ||
| userAgent | ||
| ); |
There was a problem hiding this comment.
If firstOrg?.id already exists in the database (e.g., when a second user from the same organization registers), calling createOrgAndUser with orgId: firstOrg.id will throw a database unique constraint violation error on the organization.id primary key, causing the registration to fail completely.
Before calling createOrgAndUser, you must check if the organization already exists. If it does, you should create the user and associate them with the existing organization instead of attempting to recreate the organization.
| if (!checkExists && user.email) { | ||
| checkExists = await this._userService.getUserByEmail(user.email); | ||
| } |
There was a problem hiding this comment.
Logging in a user solely by matching their email address from an external OAuth provider without verifying that the email is verified by the provider can lead to Account Takeover (ATO) vulnerabilities. If a malicious actor registers an account on the OAuth provider using a victim's email address (without verification) and then logs in via OAuth, they will gain access to the victim's account on this platform.
Please ensure that:
- The OAuth provider's payload indicates that the email is verified (e.g.,
email_verified: trueor equivalent claim). - You link the provider to the user's account (updating
providerNameandproviderId) so that subsequent logins are securely mapped viagetUserByProviderrather than repeatedly falling back to email matching.
| if (existing) { | ||
| if (orgInfo.name && existing.name !== orgInfo.name) { | ||
| await this._organizationService | ||
| .updateOrganizationName(orgInfo.id, orgInfo.name) | ||
| .catch(() => {}); | ||
| } | ||
| } else { | ||
| const orgExistsInDb = await this._organizationService.getOrgById(orgInfo.id); | ||
| if (orgExistsInDb) { | ||
| if (orgInfo.name && orgExistsInDb.name !== orgInfo.name) { | ||
| await this._organizationService | ||
| .updateOrganizationName(orgInfo.id, orgInfo.name) | ||
| .catch(() => {}); | ||
| } | ||
| await this._organizationService | ||
| .addUserToOrg(userId, makeId(5), orgInfo.id, role === 'SUPERADMIN' ? 'ADMIN' : role) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
Updating the organization name should be restricted to users with administrative privileges (e.g., OWNER, SUPERADMIN, or ADMIN). Currently, any user logging in who has a matching organization in their OAuth claims will trigger an organization name update, regardless of their role. This could allow a regular member to inadvertently rename the organization for all users.
Consider checking the user's role before calling updateOrganizationName.
| if (existing) { | |
| if (orgInfo.name && existing.name !== orgInfo.name) { | |
| await this._organizationService | |
| .updateOrganizationName(orgInfo.id, orgInfo.name) | |
| .catch(() => {}); | |
| } | |
| } else { | |
| const orgExistsInDb = await this._organizationService.getOrgById(orgInfo.id); | |
| if (orgExistsInDb) { | |
| if (orgInfo.name && orgExistsInDb.name !== orgInfo.name) { | |
| await this._organizationService | |
| .updateOrganizationName(orgInfo.id, orgInfo.name) | |
| .catch(() => {}); | |
| } | |
| await this._organizationService | |
| .addUserToOrg(userId, makeId(5), orgInfo.id, role === 'SUPERADMIN' ? 'ADMIN' : role) | |
| .catch(() => {}); | |
| if (existing) { | |
| if (orgInfo.name && existing.name !== orgInfo.name && (role === 'SUPERADMIN' || role === 'ADMIN')) { | |
| await this._organizationService | |
| .updateOrganizationName(orgInfo.id, orgInfo.name) | |
| .catch(() => {}); | |
| } | |
| } else { | |
| const orgExistsInDb = await this._organizationService.getOrgById(orgInfo.id); | |
| if (orgExistsInDb) { | |
| if (orgInfo.name && orgExistsInDb.name !== orgInfo.name && (role === 'SUPERADMIN' || role === 'ADMIN')) { | |
| await this._organizationService | |
| .updateOrganizationName(orgInfo.id, orgInfo.name) | |
| .catch(() => {}); | |
| } | |
| await this._organizationService | |
| .addUserToOrg(userId, makeId(5), orgInfo.id, role === 'SUPERADMIN' ? 'ADMIN' : role) | |
| .catch(() => {}); |
1 participant
What kind of change does this PR introduce?
Bug fix & Ecosystem Auth: Backend (
apps/backend,libraries/nestjs-libraries). Fixes organization synchronization during generic OAuth login inAuthService.checkExists()and enables passing canonicalorgIdduring user/organization creation.Why was this change needed?
Previously, when an existing user logged in via DOS ID SSO,
AuthService.checkExists()immediately returned a JWT without synchronizing updated claims and organizations from the OIDC userinfo payload. Furthermore, initial registration created an auto-generated UUID for the organization instead of reusing the canonical ecosystemorgIdprovided by DOS.Me.Technical Details & Scope
apps/backend/src/services/auth/auth.service.ts:syncUserOrganizations(userId, organizations)method to handle organization discovery, name synchronization, and user role assignment.checkExists()to runsyncUserOrganizations()and update user personal details before issuing the session JWT.loginOrRegisterProvider()to pass canonicalorgIdwhen creating the initial organization and user.libraries/nestjs-libraries/src/database/prisma/organizations/organization.repository.ts&organization.service.ts:orgIdfield toCreateOrgUserDtoinput oncreateOrgAndUser(), allowing the first organization to inherit the exact UUID from DOS ID.Verification & Testing
pnpm --filter ./apps/backend run build.postschema.joy@dos.aihas 3 canonical organizations (JOY,DOS,Crove) with ULTIMATE subscriptions and correctly mapped OAuth applications.QA
post.crove.com/authorbeta-post.crove.com/authJOY,DOS,Crove) appear with their correct canonical names and Super-Admin rolesChecklist:
pnpm run build).Note
High Risk
Changes authentication linking, org membership, and role assignment on every OAuth checkExists/login path; incorrect sync or role mapping could grant wrong org access or duplicate orgs.
Overview
OAuth login now keeps users, org memberships, and org names in sync with the identity provider instead of only issuing a JWT for returning users.
A new
syncUserOrganizationshelper centralizes provider org claims: it renames orgs when names drift, joins users to orgs that already exist by canonical ID, or creates orgs with the provider’s ID. Role mapping treats OWNER/SUPERADMIN as super-admin internally and downgrades to ADMIN when callingaddUserToOrg(which only accepts USER/ADMIN).loginOrRegisterProvideruses this helper for both existing and newly registered users and passesorgIdfrom the first provider org intocreateOrgAndUserso the initial org isn’t a random UUID.checkExists(OAuth callback before full login) now resolves users by email when provider ID lookup fails, updates display name, runs org sync, then returns a JWT—closing the gap where DOS ID SSO skipped claim updates.createOrgAndUserin the organization repository/service accepts an optionalorgIdto set the organization primary key at creation time.Reviewed by Cursor Bugbot for commit 4bc0111. Configure here.