Conversation
Implement JIT organization sync from userinfo claims, mapping canonical DOS-Me organization IDs and roles to internal Organization and UserOrganization records. Co-authored-by: Cursor <cursoragent@cursor.com>
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_0708439f-3849-46c9-b02c-c5f5c19ef33f) |
There was a problem hiding this comment.
Code Review
This pull request updates the authentication service to synchronize provider-associated organizations for both existing and registering users, allowing optional organization IDs to be passed down during creation. The review feedback highlights a major redundancy in auth.service.ts where identical branches are executed regardless of the organization's existence, and points out a redundant ternary expression when determining user roles.
| let create: any; | ||
| if (firstOrg?.id) { | ||
| const orgExists = await this._organizationService.getOrgById(firstOrg.id); | ||
| if (!orgExists) { | ||
| create = await this._organizationService.createOrgAndUser( | ||
| { | ||
| company: companyName, | ||
| email: providerUser.email, | ||
| password: '', | ||
| provider, | ||
| providerId: providerUser.id, | ||
| datafast_visitor_id: body.datafast_visitor_id || '', | ||
| }, | ||
| ip, | ||
| userAgent | ||
| ); | ||
| } else { | ||
| create = await this._organizationService.createOrgAndUser( | ||
| { | ||
| company: companyName, | ||
| email: providerUser.email, | ||
| password: '', | ||
| provider, | ||
| providerId: providerUser.id, | ||
| datafast_visitor_id: body.datafast_visitor_id || '', | ||
| }, | ||
| ip, | ||
| userAgent | ||
| ); | ||
| } | ||
| } else { | ||
| create = await this._organizationService.createOrgAndUser( | ||
| { | ||
| company: companyName, | ||
| email: providerUser.email, | ||
| password: '', | ||
| provider, | ||
| providerId: providerUser.id, | ||
| datafast_visitor_id: body.datafast_visitor_id || '', | ||
| }, | ||
| ip, | ||
| userAgent | ||
| ); | ||
| } |
There was a problem hiding this comment.
The if (firstOrg?.id) block contains three identical branches that call this._organizationService.createOrgAndUser with the exact same arguments. This redundancy makes the code hard to maintain and suggests a logical error, as the first organization's canonical ID is never used during registration.
We can simplify this by removing the redundant conditional branches and making a single call to createOrgAndUser. The actual synchronization of the first organization (including its canonical ID) can then be handled by updating the subsequent loop to start at index 0 instead of 1.
const create = await this._organizationService.createOrgAndUser(
{
company: companyName,
email: providerUser.email,
password: '',
provider,
providerId: providerUser.id,
datafast_visitor_id: body.datafast_visitor_id || '',
},
ip,
userAgent
);| for (const orgInfo of providerUser.organizations) { | ||
| if (!existingOrgIds.has(orgInfo.id)) { | ||
| const role = orgInfo.role === 'MEMBER' ? 'USER' : 'ADMIN'; | ||
| const orgExists = await this._organizationService.getOrgById(orgInfo.id); | ||
| if (orgExists) { | ||
| await this._organizationService | ||
| .addUserToOrg(user.id, makeId(5), orgInfo.id, role) | ||
| .catch(() => {}); | ||
| } else { | ||
| await this._organizationService | ||
| .createOrgForExistingUser(user.id, orgInfo.name, role === 'ADMIN' ? 'ADMIN' : 'USER', orgInfo.id) | ||
| .catch(() => {}); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The ternary expression role === 'ADMIN' ? 'ADMIN' : 'USER' on line 177 is redundant because role is already defined as either 'USER' or 'ADMIN' on line 169. We can simplify this by passing role directly.
| for (const orgInfo of providerUser.organizations) { | |
| if (!existingOrgIds.has(orgInfo.id)) { | |
| const role = orgInfo.role === 'MEMBER' ? 'USER' : 'ADMIN'; | |
| const orgExists = await this._organizationService.getOrgById(orgInfo.id); | |
| if (orgExists) { | |
| await this._organizationService | |
| .addUserToOrg(user.id, makeId(5), orgInfo.id, role) | |
| .catch(() => {}); | |
| } else { | |
| await this._organizationService | |
| .createOrgForExistingUser(user.id, orgInfo.name, role === 'ADMIN' ? 'ADMIN' : 'USER', orgInfo.id) | |
| .catch(() => {}); | |
| } | |
| } | |
| } | |
| for (const orgInfo of providerUser.organizations) { | |
| if (!existingOrgIds.has(orgInfo.id)) { | |
| const role = orgInfo.role === 'MEMBER' ? 'USER' : 'ADMIN'; | |
| const orgExists = await this._organizationService.getOrgById(orgInfo.id); | |
| if (orgExists) { | |
| await this._organizationService | |
| .addUserToOrg(user.id, makeId(5), orgInfo.id, role) | |
| .catch(() => {}); | |
| } else { | |
| await this._organizationService | |
| .createOrgForExistingUser(user.id, orgInfo.name, role, orgInfo.id) | |
| .catch(() => {}); | |
| } | |
| } | |
| } |
What kind of change does this PR introduce?
Feature & Identity Sync
Why was this change needed?
Implements Section 3 (Inbound JIT Sync) from the DOS / Crove Organization Management & Sync Standard:
organizationsarray fromhttps://api.dos.me/sso/userinfois evaluated.OWNER/ADMIN->ADMIN/SUPERADMIN,MEMBER->USER).id,name, and user role assignment.Other information:
Checklist:
Note
Medium Risk
Changes auth-time org membership and ID assignment on every SSO login/registration; silent failures could leave users out of expected orgs, and role mapping may not match all claim types.
Overview
Adds just-in-time organization sync during DOS ID (SSO) login and registration so local org membership and IDs stay aligned with
userinfoorganization claims.For returning users, any claim org the user is not already in is linked: existing local orgs get
addUserToOrgwith mapped roles (MEMBER→USER, otherwiseADMIN); missing orgs are created viacreateOrgForExistingUserusing the claim’s canonicalidandname. Errors on individual orgs are swallowed with.catch(() => {}).For new provider registrations, the first claim org still drives the default company name; additional orgs use the same exists-vs-create branching instead of always creating new orgs by name only.
createOrgForExistingUser(repository + service) now accepts an optionalorgIdso new organizations can be persisted with the external SSOT identifier.Reviewed by Cursor Bugbot for commit d141027. Bugbot is set up for automated code reviews on this repo. Configure here.