feat: add SCIM 2.0 user provisioning (EE)#1306
Open
brendan-kellam wants to merge 7 commits into
Open
Conversation
Adds a SCIM 2.0 server so an IdP (Okta, Entra) can provision, update, and deprovision org members. Users-only scope; deprovisioning soft-deactivates the membership (forces logout + revokes tokens) rather than deleting it, and JIT auto-join is suppressed when SCIM is enabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThis PR implements an EE SCIM 2.0 server for automated user provisioning and deprovisioning. It adds SCIM-specific database columns ( ChangesEE SCIM 2.0 Provisioning Server and Membership Service Refactor
Sequence Diagram(s)sequenceDiagram
participant IdP as Identity Provider (Okta/Entra)
participant SCIMRoute as SCIM API Route
participant withScimAuth
participant MembershipService as membership.service
participant Database
IdP->>SCIMRoute: POST /scim/v2/Users (Bearer sbscim_...)
SCIMRoute->>withScimAuth: authenticate request
withScimAuth->>Database: lookup ScimToken by hash
withScimAuth->>withScimAuth: check scim entitlement + org.isScimEnabled
withScimAuth-->>SCIMRoute: ScimAuthContext { org, prisma }
SCIMRoute->>Database: findOrCreate User by email
SCIMRoute->>Database: check existing userToOrg membership
alt user already active
SCIMRoute-->>IdP: 409 Conflict
else user inactive
SCIMRoute->>MembershipService: setMemberActive(true)
SCIMRoute-->>IdP: 200 + SCIM User
else no membership
SCIMRoute->>MembershipService: addMember(orgId, userId)
SCIMRoute-->>IdP: 201 + SCIM User + Location header
end
IdP->>SCIMRoute: PATCH /scim/v2/Users/{id}
SCIMRoute->>withScimAuth: authenticate request
SCIMRoute->>MembershipService: setMemberActive(false) if active=false
SCIMRoute->>Database: update user name/email
SCIMRoute-->>IdP: 200 + updated SCIM User
IdP->>SCIMRoute: DELETE /scim/v2/Users/{id}
SCIMRoute->>withScimAuth: authenticate request
SCIMRoute->>MembershipService: removeMember(orgId, userId)
MembershipService->>Database: Serializable tx — delete userToOrg + revoke credentials
SCIMRoute-->>IdP: 204 No Content
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
brendan-kellam
commented
Jun 16, 2026
| /// SCIM soft-deactivation flag. When false, the membership is suspended by | ||
| /// the IdP: the user is treated as a non-member for auth purposes (see | ||
| /// `getAuthContext`) but the row is preserved so the IdP can reactivate it. | ||
| isActive Boolean @default(true) |
Contributor
Author
There was a problem hiding this comment.
tbd: do we actually need to store a isActive field here?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/app/invite/page.tsx (1)
45-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not treat inactive memberships as active members in redirect logic.
At Line 46,
if (membership)redirects users with SCIM-deactivated rows (isActive: false) as if they were active members. This diverges from the auth contract where only active memberships confer access.Suggested fix
- if (membership) { + if (membership?.isActive) { redirect(`/`); }🤖 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/web/src/app/invite/page.tsx` around lines 45 - 47, The redirect logic in the invite page only checks for the existence of a membership object, but does not verify that it is active. Modify the condition in the if statement that checks `membership` to also verify that the membership's active status is true (check the isActive property). This ensures that users with deactivated SCIM memberships are not incorrectly treated as active members and redirected, maintaining consistency with the authentication contract.
🟡 Minor comments (8)
packages/web/src/app/(app)/settings/security/components/scimProvisioningSettings.tsx-125-127 (1)
125-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd accessible labels to icon-only action buttons.
Lines 125, 170, and 230 render icon-only buttons without accessible names, which makes these actions ambiguous for screen-reader users.
Also applies to: 170-172, 230-236
🤖 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/web/src/app/`(app)/settings/security/components/scimProvisioningSettings.tsx around lines 125 - 127, The icon-only action buttons in the scimProvisioningSettings component lack accessible labels for screen readers. Add aria-label attributes to the Button components at handleCopyBaseUrl (around line 125-127), the button around line 170-172, and the button around line 230-236 to provide descriptive accessible names that explain what each button does when activated by assistive technology users.packages/web/src/features/membership/components/joinOrganizationCard.tsx-56-63 (1)
56-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace CSS variable color classes with Tailwind semantic color classes.
text-[var(--muted-foreground)]andtext-[var(--primary-foreground)]should use direct Tailwind classes in this TSX file.Suggested fix
- <p className="text-[var(--muted-foreground)] text-[15px] leading-6"> + <p className="text-muted-foreground text-[15px] leading-6"> Welcome to Sourcebot! Click the button below to join this organization. </p> @@ - className="w-full h-11 bg-primary hover:bg-primary/90 text-[var(--primary-foreground)] transition-all duration-200 font-medium" + className="w-full h-11 bg-primary hover:bg-primary/90 text-primary-foreground transition-all duration-200 font-medium"As per coding guidelines,
**/*.{tsx,jsx,mdx}must use Tailwind color classes directly instead of CSS variable syntax.🤖 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/web/src/features/membership/components/joinOrganizationCard.tsx` around lines 56 - 63, In the joinOrganizationCard component, replace the CSS variable syntax in the className attributes with direct Tailwind semantic color classes. Change `text-[var(--muted-foreground)]` to `text-muted-foreground` in the paragraph element and change `text-[var(--primary-foreground)]` to `text-primary-foreground` in the Button component's className to align with the coding guidelines that require TSX files to use Tailwind color classes directly instead of CSS variable syntax.Source: Coding guidelines
packages/web/src/features/membership/components/submitJoinRequestCard.tsx-60-70 (1)
60-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Tailwind color tokens instead of CSS-variable utility syntax.
Replace
text-[var(--...)]with token classes (text-primary-foreground,text-foreground,text-muted-foreground) for consistency with repo standards.As per coding guidelines,
**/*.{tsx,jsx,mdx}: Use Tailwind color classes directly instead of CSS variable syntax (e.g., useborder-borderinstead ofborder-[var(--border)]).🤖 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/web/src/features/membership/components/submitJoinRequestCard.tsx` around lines 60 - 70, Replace all CSS variable utility syntax with Tailwind color token classes in the submitJoinRequestCard component. Specifically, update the className attributes in the svg element and the h1 and p elements to use text-primary-foreground, text-foreground, and text-muted-foreground instead of text-[var(--primary-foreground)], text-[var(--foreground)], and text-[var(--muted-foreground)] respectively. This aligns with the repository's coding standards for consistent use of Tailwind color classes rather than CSS variable syntax.Source: Coding guidelines
packages/web/src/app/(app)/settings/members/components/membersList.tsx-217-217 (1)
217-217:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a fallback label when name is missing.
Line 217 renders
member.namedirectly, so members without a profile name show a blank primary label. Use email fallback to keep identity visible.Suggested fix
- <span className="font-medium truncate">{member.name}</span> + <span className="font-medium truncate">{member.name ?? member.email}</span>🤖 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/web/src/app/`(app)/settings/members/components/membersList.tsx at line 217, The span element rendering member.name on line 217 of membersList.tsx displays blank when the name property is missing or empty, making member identification difficult. Modify the content of the span to use a fallback expression that displays member.email when member.name is not available, such as using a logical OR operator or conditional expression to ensure every member row has a visible identifier.packages/web/src/app/api/(server)/ee/scim/v2/Users/[id]/route.ts-74-75 (1)
74-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unsafe non-null assertions after reload.
A concurrent delete between update and reload can make
refreshednull;refreshed!then produces a 500 instead of a SCIM 404.Suggested fix
- const refreshed = await loadMembership(prisma, org.id, id); - return scimJson(toScimUser(refreshed!), 200); + const refreshed = await loadMembership(prisma, org.id, id); + if (!refreshed) { + return scimError(404, `User ${id} not found`); + } + return scimJson(toScimUser(refreshed), 200);Also applies to: 113-114
🤖 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/web/src/app/api/`(server)/ee/scim/v2/Users/[id]/route.ts around lines 74 - 75, The non-null assertion on the `refreshed` variable returned from loadMembership can cause a 500 error if a concurrent delete occurs between the update and reload, when it should return a SCIM 404 instead. Remove the non-null assertion operator (!) after `refreshed` in the return statement at the scimJson call, and add a null check immediately after the loadMembership assignment. If refreshed is null, return an appropriate SCIM 404 response, otherwise proceed with toScimUser(refreshed) only when the value is confirmed to be non-null. Apply this same fix at the other location mentioned in the comment (lines 113-114).packages/web/src/ee/features/scim/withScimAuth.ts-26-34 (1)
26-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAccept case-insensitive bearer auth schemes.
The current check rejects
bearer/mixed-case schemes, which can break otherwise valid IdP requests.Suggested fix
- const authorization = request.headers.get("Authorization") ?? undefined; - if (!authorization?.startsWith("Bearer ")) { + const authorization = request.headers.get("Authorization") ?? undefined; + const [scheme, bearer] = authorization?.split(/\s+/, 2) ?? []; + if (scheme?.toLowerCase() !== "bearer" || !bearer) { return scimError(401, "Missing or malformed Authorization header"); } - - const bearer = authorization.slice("Bearer ".length);🤖 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/web/src/ee/features/scim/withScimAuth.ts` around lines 26 - 34, The authorization header check for the Bearer token scheme is case-sensitive and only accepts "Bearer " with that exact capitalization, which will reject valid IdP requests that use lowercase "bearer" or mixed case variants. Convert the authorization header value to lowercase before checking if it starts with "Bearer " (or check against "bearer " in lowercase), and adjust the subsequent slice operation that extracts the bearer token to use the lowercased authorization value to ensure the token extraction works correctly regardless of the case used in the Authorization header.CHANGELOG.md-42-42 (1)
42-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPlace this entry under
[Unreleased]instead of a released version block.Line 42 is currently under
## [5.0.3]. The project changelog policy requires PR entries to be added under[Unreleased]until release cut.As per coding guidelines,
CHANGELOG.md: “Update CHANGELOG.md with an entry under[Unreleased]linking to the new PR.”🤖 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 `@CHANGELOG.md` at line 42, The SCIM 2.0 server provisioning entry is currently placed under the [5.0.3] released version section in CHANGELOG.md. Move this entry from the [5.0.3] section to the [Unreleased] section at the top of the changelog. The entry should maintain its exact format and content, just relocated to comply with the project's changelog policy that requires new PR entries to be added under [Unreleased] until the next release is cut.Source: Coding guidelines
packages/web/src/ee/features/scim/actions.ts-73-85 (1)
73-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize and validate token names on the server boundary.
Line 73 and Line 115 accept
nameas-is. Without trimming/non-empty validation, you can create hard-to-manage tokens (e.g., whitespace-only or visually duplicate names with trailing spaces).Proposed fix
export const generateScimToken = async (name: string): Promise<{ token: string } | ServiceError> => sew(() => withAuth(async ({ org, user, role, prisma }) => withMinimumOrgRole(role, OrgRole.OWNER, async () => { + const normalizedName = name.trim(); + if (!normalizedName) { + return { + statusCode: StatusCodes.BAD_REQUEST, + errorCode: ErrorCode.INVALID_REQUEST, + message: "SCIM token name cannot be empty", + } satisfies ServiceError; + } if (!await hasEntitlement('scim')) { return scimNotAvailable(); } const existing = await prisma.scimToken.findFirst({ where: { orgId: org.id, - name, + name: normalizedName, }, }); @@ const scimToken = await prisma.scimToken.create({ data: { - name, + name: normalizedName, hash, orgId: org.id, }, }); @@ - metadata: { scim_token: name }, + metadata: { scim_token: normalizedName }, });Also applies to: 115-126
🤖 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/web/src/ee/features/scim/actions.ts` around lines 73 - 85, The generateScimToken function accepts the name parameter without normalization or validation, allowing whitespace-only or duplicate names with trailing spaces to be created. Add validation at the beginning of the function to trim the name parameter and verify it is not empty after trimming, returning an appropriate error if validation fails. Apply the same normalization and validation logic to the other function around line 115 that also handles scim token names.
🧹 Nitpick comments (1)
packages/db/prisma/schema.prisma (1)
436-437: ⚡ Quick winRemove redundant uniqueness on primary key
hash.
hashis already unique because it is the model@id; keeping@uniqueis redundant and can lead to unnecessary duplicate index artifacts in migrations.Suggested schema cleanup
- hash String `@id` `@unique` + hash String `@id`🤖 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/db/prisma/schema.prisma` around lines 436 - 437, Remove the redundant `@unique` annotation from the hash field definition in the Prisma schema. Since hash is already defined as the primary key with `@id`, the `@unique` annotation is unnecessary and can create duplicate indexes. Modify the hash field to only include `@id` `@unique`, removing the `@unique` directive entirely so it reads as hash String `@id`.
🤖 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.
Inline comments:
In `@packages/db/prisma/schema.prisma`:
- Around line 410-413: Replace the non-unique index annotation @@index([orgId,
scimExternalId]) with a unique constraint @@unique([orgId, scimExternalId]) on
the scimExternalId field to enforce per-organization uniqueness of SCIM external
identifiers. This prevents duplicate IdP identities within the same org and
ensures unambiguous identity resolution in SCIM flows.
In `@packages/web/src/app/`(app)/layout.tsx:
- Around line 81-87: The gate logic currently only checks for the absence of a
membership but does not account for inactive memberships. When a membership row
exists with `isActive: false`, execution falls through and later assigns a role
from that inactive membership. Modify the membership validation check to ensure
that inactive memberships are treated the same as non-existent memberships by
adding a check for the `isActive` property. Update all conditions that check for
`!membership` to also verify `membership.isActive` is true, so that both missing
and inactive memberships are handled consistently as non-members in this gate.
In
`@packages/web/src/app/`(app)/settings/security/components/scimProvisioningSettings.tsx:
- Around line 102-110: The handleRevokeToken function calls the async
revokeScimToken action without a try/catch block, which means any exceptions
thrown by that function will escape unhandled and the user won't see an error
toast. Wrap the await revokeScimToken(name) call in a try/catch block, and in
the catch clause, display a destructive toast with an appropriate error message
to ensure all failure scenarios (both service errors and thrown exceptions) are
properly communicated to the user.
In `@packages/web/src/app/`(app)/settings/security/page.tsx:
- Around line 32-33: The SCIM token fetch logic in the security page converts
ServiceError results into an empty array, which masks retrieval failures as "no
tokens exist" in the UI. Instead of using the conditional assignment that checks
isServiceError(scimTokensResult) and defaults to an empty array, you need to
handle the error case separately so that the UI can distinguish between a
successful fetch with zero tokens versus a failed fetch. Create separate state
or variables to track both the scimTokens array and whether an error occurred
during the fetch, allowing the downstream UI to appropriately display either an
empty token list or an error message.
In `@packages/web/src/app/api/`(server)/ee/scim/v2/Users/[id]/route.ts:
- Around line 62-75: The code currently updates the user profile via
prisma.user.update() before validating the membership state change via
applyActive(), which can lead to partial updates if the membership change fails.
Reorder the operations so that applyActive() is called before
prisma.user.update() to validate the membership state change constraints first,
or alternatively wrap both the prisma.user.update() and applyActive() calls in a
database transaction to ensure that either both operations succeed or both fail
atomically. Apply the same fix to the similar code block mentioned at lines
98-115.
- Around line 64-67: The prisma.user.update() calls in both the PUT handler
(lines 64-67) and PATCH handler (lines 99-105) lack error handling for unique
email constraint violations. Wrap both prisma.user.update() calls in try-catch
blocks and catch errors related to unique constraint violations on the email
field. When such an error occurs, return a SCIM 409 Conflict response with
scimType set to "uniqueness" to match the existing error handling pattern
already implemented in the POST handler. Reference the POST handler's error
handling approach as a template for the proper response format.
In `@packages/web/src/app/api/`(server)/ee/scim/v2/Users/route.ts:
- Around line 21-25: The GET request handler is manually parsing query
parameters using parseInt, Math.max, and Math.min instead of using a Zod schema
for validation. Create a Zod schema that defines the shape and validation rules
for the query parameters (filter, startIndex, and count), including type
coercion and default values. Replace the manual parsing logic starting with the
params variable assignment through the count variable assignment with a single
Zod schema parse operation that validates and transforms the incoming
request.nextUrl.searchParams data, ensuring all validation logic is centralized
in the schema definition rather than scattered throughout the handler code.
- Around line 66-69: The current findUnique followed by create pattern for user
provisioning has a race condition where concurrent requests can both pass the
existence check before either executes the create, causing a unique constraint
violation on the email field. Replace the separate findUnique and create calls
with a single prisma.user.upsert operation that atomically checks for existence
and creates the user in one transaction, using email as the unique identifier
and providing both the create and update data objects with the email and name
fields.
In `@packages/web/src/app/invite/page.tsx`:
- Around line 54-55: The JoinOrganizationCard component is being rendered
without passing the validated inviteLinkId as a prop, which causes the
joinOrganization function to fail invite-link validation for organizations that
require member approval. Pass the inviteLinkId that should be available in the
page context as a prop to the JoinOrganizationCard component so it can be used
for proper validation when the user attempts to join the organization.
In `@packages/web/src/app/redeem/page.tsx`:
- Around line 45-48: The membership redirect check at the `if (membership)`
condition is catching both active and inactive memberships, including
SCIM-deactivated ones where isActive is false. This prevents the intended
not-provisioned flow from executing. Modify the condition to check not only that
membership exists but also that membership.isActive is true before redirecting
to the home page, ensuring only active members are redirected while deactivated
members proceed through the redemption flow.
In `@packages/web/src/ee/features/scim/actions.ts`:
- Around line 24-31: The getScimBaseUrl function and the other read operations
at lines 37-44 and 152-169 are implemented as server actions (wrapped with sew,
withAuth, withMinimumOrgRole), but they only perform data fetching without
mutations, which violates the project guideline that server actions should only
be used for POST/PUT/DELETE operations. Extract these read operations into
separate utility functions that return the data directly without server action
wrappers, then import and call them as regular async functions from client
components or other modules. Keep the authorization and entitlement checks, but
move them outside the server action pattern.
- Around line 80-103: Add a database-level unique constraint to prevent race
conditions in the SCIM token creation logic. In the ScimToken model located in
packages/db/prisma/schema.prisma, add a composite unique constraint using the
@@unique directive that combines the orgId and name fields. Then create a new
Prisma migration to apply this database constraint, which will ensure that the
pre-check validation in the create action is backed by database enforcement and
prevent concurrent requests from creating duplicate tokens with the same name
within the same organization.
In `@packages/web/src/ee/features/scim/schemas.ts`:
- Around line 22-30: The scimUserCreateSchema currently accepts emails through
the emails array without validating that they are actual email addresses before
they are persisted as user.email in downstream handlers. Update the
scimEmailSchema definition to include proper email format validation (ensure the
email field uses z.string().email() or equivalent). Additionally, locate the
other email validation location mentioned around line 65-66 and apply the same
email format validation to prevent blank or non-email identifiers from being
stored in the database.
In `@packages/web/src/features/membership/actions/accountRequests.ts`:
- Around line 65-125: Remove the redundant if (!existingRequest) conditional
check in the createAccountRequest function since an early return at line 57-63
already handles the case when existingRequest is truthy. Unindent all code that
was nested inside this conditional block by one level and remove the closing
brace at line 125. Additionally, in the approveAccountRequest function, add
deletion of the accountRequest record after successfully calling addMember
(around line 181) to match the cleanup pattern used in rejectAccountRequest,
ensuring that approved account requests do not leave orphaned records in the
database.
In `@packages/web/src/features/membership/components/submitJoinRequestCard.tsx`:
- Around line 18-45: The handleSubmit function in submitJoinRequestCard.tsx does
not have proper error handling for exceptions thrown by createAccountRequest().
If createAccountRequest() throws an error, setIsSubmitting(false) will never
execute, leaving the button locked. Refactor the handleSubmit function to wrap
the createAccountRequest() call and subsequent logic in a try/catch/finally
block, placing the setIsSubmitting(false) call in the finally block to ensure it
always executes. In the catch block, display an error toast with the caught
error message so users are informed of any unexpected failures.
In `@packages/web/src/features/membership/membership.service.ts`:
- Around line 47-49: The seat capacity check using orgHasAvailability() is
performed outside the database transaction in both the membership add and
reactivation code paths, allowing concurrent requests to bypass the seat limit.
Move the orgHasAvailability() check inside the same database transaction that
performs the actual insert or reactivate operation for the membership record,
ensuring the capacity validation and data persistence happen atomically
together. Apply this transactional pattern consistently to both the add path
(around the seatLimitReached() check) and the reactivation path to prevent race
conditions where multiple concurrent requests can each pass the check separately
and then both persist, exceeding the seat cap.
In `@packages/web/src/features/membership/onCreateUser.ts`:
- Around line 43-50: The first-user OWNER role assignment in the onCreateUser
function is prone to race conditions because it uses a non-atomic read of the
members array to determine if this is the first user. Replace the current
approach where __unsafePrisma.org.findUnique loads the full members array and
members.length is checked at line 76-81 with a single atomic transaction in the
membership layer that performs both a count query (instead of loading full
members) and the membership creation in one serializable unit. This ensures
concurrent user creations cannot both observe empty membership and both assign
OrgRole.OWNER.
---
Outside diff comments:
In `@packages/web/src/app/invite/page.tsx`:
- Around line 45-47: The redirect logic in the invite page only checks for the
existence of a membership object, but does not verify that it is active. Modify
the condition in the if statement that checks `membership` to also verify that
the membership's active status is true (check the isActive property). This
ensures that users with deactivated SCIM memberships are not incorrectly treated
as active members and redirected, maintaining consistency with the
authentication contract.
---
Minor comments:
In `@CHANGELOG.md`:
- Line 42: The SCIM 2.0 server provisioning entry is currently placed under the
[5.0.3] released version section in CHANGELOG.md. Move this entry from the
[5.0.3] section to the [Unreleased] section at the top of the changelog. The
entry should maintain its exact format and content, just relocated to comply
with the project's changelog policy that requires new PR entries to be added
under [Unreleased] until the next release is cut.
In `@packages/web/src/app/`(app)/settings/members/components/membersList.tsx:
- Line 217: The span element rendering member.name on line 217 of
membersList.tsx displays blank when the name property is missing or empty,
making member identification difficult. Modify the content of the span to use a
fallback expression that displays member.email when member.name is not
available, such as using a logical OR operator or conditional expression to
ensure every member row has a visible identifier.
In
`@packages/web/src/app/`(app)/settings/security/components/scimProvisioningSettings.tsx:
- Around line 125-127: The icon-only action buttons in the
scimProvisioningSettings component lack accessible labels for screen readers.
Add aria-label attributes to the Button components at handleCopyBaseUrl (around
line 125-127), the button around line 170-172, and the button around line
230-236 to provide descriptive accessible names that explain what each button
does when activated by assistive technology users.
In `@packages/web/src/app/api/`(server)/ee/scim/v2/Users/[id]/route.ts:
- Around line 74-75: The non-null assertion on the `refreshed` variable returned
from loadMembership can cause a 500 error if a concurrent delete occurs between
the update and reload, when it should return a SCIM 404 instead. Remove the
non-null assertion operator (!) after `refreshed` in the return statement at the
scimJson call, and add a null check immediately after the loadMembership
assignment. If refreshed is null, return an appropriate SCIM 404 response,
otherwise proceed with toScimUser(refreshed) only when the value is confirmed to
be non-null. Apply this same fix at the other location mentioned in the comment
(lines 113-114).
In `@packages/web/src/ee/features/scim/actions.ts`:
- Around line 73-85: The generateScimToken function accepts the name parameter
without normalization or validation, allowing whitespace-only or duplicate names
with trailing spaces to be created. Add validation at the beginning of the
function to trim the name parameter and verify it is not empty after trimming,
returning an appropriate error if validation fails. Apply the same normalization
and validation logic to the other function around line 115 that also handles
scim token names.
In `@packages/web/src/ee/features/scim/withScimAuth.ts`:
- Around line 26-34: The authorization header check for the Bearer token scheme
is case-sensitive and only accepts "Bearer " with that exact capitalization,
which will reject valid IdP requests that use lowercase "bearer" or mixed case
variants. Convert the authorization header value to lowercase before checking if
it starts with "Bearer " (or check against "bearer " in lowercase), and adjust
the subsequent slice operation that extracts the bearer token to use the
lowercased authorization value to ensure the token extraction works correctly
regardless of the case used in the Authorization header.
In `@packages/web/src/features/membership/components/joinOrganizationCard.tsx`:
- Around line 56-63: In the joinOrganizationCard component, replace the CSS
variable syntax in the className attributes with direct Tailwind semantic color
classes. Change `text-[var(--muted-foreground)]` to `text-muted-foreground` in
the paragraph element and change `text-[var(--primary-foreground)]` to
`text-primary-foreground` in the Button component's className to align with the
coding guidelines that require TSX files to use Tailwind color classes directly
instead of CSS variable syntax.
In `@packages/web/src/features/membership/components/submitJoinRequestCard.tsx`:
- Around line 60-70: Replace all CSS variable utility syntax with Tailwind color
token classes in the submitJoinRequestCard component. Specifically, update the
className attributes in the svg element and the h1 and p elements to use
text-primary-foreground, text-foreground, and text-muted-foreground instead of
text-[var(--primary-foreground)], text-[var(--foreground)], and
text-[var(--muted-foreground)] respectively. This aligns with the repository's
coding standards for consistent use of Tailwind color classes rather than CSS
variable syntax.
---
Nitpick comments:
In `@packages/db/prisma/schema.prisma`:
- Around line 436-437: Remove the redundant `@unique` annotation from the hash
field definition in the Prisma schema. Since hash is already defined as the
primary key with `@id`, the `@unique` annotation is unnecessary and can create
duplicate indexes. Modify the hash field to only include `@id` `@unique`, removing
the `@unique` directive entirely so it reads as hash String `@id`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0afa80cc-c399-4e0f-9b31-b82624e66b85
📒 Files selected for processing (74)
CHANGELOG.mdpackages/db/prisma/migrations/20260619214548_add_scim_users_support/migration.sqlpackages/db/prisma/schema.prismapackages/shared/src/constants.tspackages/shared/src/crypto.tspackages/shared/src/entitlements.tspackages/shared/src/index.server.tspackages/web/next.config.mjspackages/web/src/__mocks__/prisma.tspackages/web/src/actions.tspackages/web/src/app/(app)/@sidebar/components/defaultSidebar/index.tsxpackages/web/src/app/(app)/components/submitAccountRequestButton.tsxpackages/web/src/app/(app)/components/submitJoinRequest.tsxpackages/web/src/app/(app)/layout.tsxpackages/web/src/app/(app)/settings/components/settingsCard.tsxpackages/web/src/app/(app)/settings/layout.tsxpackages/web/src/app/(app)/settings/members/components/inviteMemberCard.tsxpackages/web/src/app/(app)/settings/members/components/invitesList.tsxpackages/web/src/app/(app)/settings/members/components/membersList.tsxpackages/web/src/app/(app)/settings/members/components/requestsList.tsxpackages/web/src/app/(app)/settings/members/page.tsxpackages/web/src/app/(app)/settings/security/components/inviteLinkEnabledSettingsCard.tsxpackages/web/src/app/(app)/settings/security/components/memberApprovalRequiredSettingsCard.tsxpackages/web/src/app/(app)/settings/security/components/scimEnabledSettingsCard.tsxpackages/web/src/app/(app)/settings/security/components/scimProvisioningSettings.tsxpackages/web/src/app/(app)/settings/security/components/scimUpsellCard.tsxpackages/web/src/app/(app)/settings/security/page.tsxpackages/web/src/app/api/(server)/ee/scim/v2/ResourceTypes/route.tspackages/web/src/app/api/(server)/ee/scim/v2/Schemas/route.tspackages/web/src/app/api/(server)/ee/scim/v2/ServiceProviderConfig/route.tspackages/web/src/app/api/(server)/ee/scim/v2/Users/[id]/route.tspackages/web/src/app/api/(server)/ee/scim/v2/Users/route.tspackages/web/src/app/components/joinOrganizationButton.tsxpackages/web/src/app/components/joinOrganizationCard.tsxpackages/web/src/app/components/logoutEscapeHatch.tsxpackages/web/src/app/invite/actions.tspackages/web/src/app/invite/page.tsxpackages/web/src/app/redeem/components/acceptInviteCard.tsxpackages/web/src/app/redeem/page.tsxpackages/web/src/auth.tspackages/web/src/ee/features/audit/types.tspackages/web/src/ee/features/membership/actions.tspackages/web/src/ee/features/scim/actions.tspackages/web/src/ee/features/scim/constants.tspackages/web/src/ee/features/scim/mapper.tspackages/web/src/ee/features/scim/schemas.test.tspackages/web/src/ee/features/scim/schemas.tspackages/web/src/ee/features/scim/withScimAuth.tspackages/web/src/ee/features/sso/sso.tspackages/web/src/ee/features/userManagement/actions.tspackages/web/src/features/membership/actions/accountRequests.tspackages/web/src/features/membership/actions/index.tspackages/web/src/features/membership/actions/invites.tspackages/web/src/features/membership/actions/members.tspackages/web/src/features/membership/components/deactivatedMemberBadge.tsxpackages/web/src/features/membership/components/joinOrganizationCard.tsxpackages/web/src/features/membership/components/managedByScimBadge.tsxpackages/web/src/features/membership/components/managedByScimNotice.tsxpackages/web/src/features/membership/components/notProvisionedCard.tsxpackages/web/src/features/membership/components/pendingApprovalCard.tsxpackages/web/src/features/membership/components/submitJoinRequestCard.tsxpackages/web/src/features/membership/errors.tspackages/web/src/features/membership/logger.tspackages/web/src/features/membership/membership.service.test.tspackages/web/src/features/membership/membership.service.tspackages/web/src/features/membership/onCreateUser.tspackages/web/src/features/membership/utils.tspackages/web/src/features/scim/utils.tspackages/web/src/features/userManagement/actions.tspackages/web/src/lib/authUtils.tspackages/web/src/lib/errorCodes.tspackages/web/src/lib/posthogEvents.tspackages/web/src/middleware/withAuth.test.tspackages/web/src/middleware/withAuth.ts
💤 Files with no reviewable changes (8)
- packages/web/src/app/(app)/components/submitAccountRequestButton.tsx
- packages/web/src/app/(app)/components/submitJoinRequest.tsx
- packages/web/src/ee/features/userManagement/actions.ts
- packages/web/src/features/userManagement/actions.ts
- packages/web/src/app/components/joinOrganizationCard.tsx
- packages/web/src/app/components/joinOrganizationButton.tsx
- packages/web/src/app/invite/actions.ts
- packages/web/src/lib/authUtils.ts
Comment on lines
+410
to
+413
| scimExternalId String? | ||
|
|
||
| @@id([orgId, userId]) | ||
| @@index([orgId, scimExternalId]) |
Contributor
There was a problem hiding this comment.
Enforce per-org uniqueness for SCIM externalId.
scimExternalId currently has only a non-unique index, which permits duplicate IdP identities inside the same org and can make SCIM identity resolution/update flows ambiguous.
Suggested schema fix
- @@index([orgId, scimExternalId])
+ @@unique([orgId, scimExternalId])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| scimExternalId String? | |
| @@id([orgId, userId]) | |
| @@index([orgId, scimExternalId]) | |
| scimExternalId String? | |
| @@id([orgId, userId]) | |
| @@unique([orgId, scimExternalId]) |
🤖 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/db/prisma/schema.prisma` around lines 410 - 413, Replace the
non-unique index annotation @@index([orgId, scimExternalId]) with a unique
constraint @@unique([orgId, scimExternalId]) on the scimExternalId field to
enforce per-organization uniqueness of SCIM external identifiers. This prevents
duplicate IdP identities within the same org and ensures unambiguous identity
resolution in SCIM flows.
Comment on lines
+81
to
+87
| if (await isScimEnabled(org)) { | ||
| return <NotProvisionedCard />; | ||
| } | ||
|
|
||
| if (!isMemberApprovalRequired(org)) { | ||
| return ( | ||
| <div className="min-h-screen flex items-center justify-center p-6"> | ||
| <LogoutEscapeHatch className="absolute top-0 right-0 p-6" /> | ||
| <JoinOrganizationCard /> | ||
| </div> | ||
| ) | ||
| } else { | ||
| const hasPendingApproval = await __unsafePrisma.accountRequest.findFirst({ | ||
| where: { | ||
| orgId: org.id, | ||
| requestedById: session.user.id | ||
| } | ||
| }); | ||
|
|
||
| if (hasPendingApproval) { | ||
| return <PendingApprovalCard /> | ||
| } else { | ||
| return <SubmitJoinRequest /> | ||
| return <JoinOrganizationCard />; | ||
| } |
Contributor
There was a problem hiding this comment.
Treat inactive memberships as non-members in this gate.
This branch only handles !membership. If a row exists with isActive: false, execution falls through and later assigns a role from that inactive membership.
Suggested fix
- if (!membership) {
+ if (!membership || !membership.isActive) {
if (await isScimEnabled(org)) {
return <NotProvisionedCard />;
}
if (!isMemberApprovalRequired(org)) {
return <JoinOrganizationCard />;
}Also applies to: 96-99
🤖 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/web/src/app/`(app)/layout.tsx around lines 81 - 87, The gate logic
currently only checks for the absence of a membership but does not account for
inactive memberships. When a membership row exists with `isActive: false`,
execution falls through and later assigns a role from that inactive membership.
Modify the membership validation check to ensure that inactive memberships are
treated the same as non-existent memberships by adding a check for the
`isActive` property. Update all conditions that check for `!membership` to also
verify `membership.isActive` is true, so that both missing and inactive
memberships are handled consistently as non-members in this gate.
Comment on lines
+102
to
+110
| const handleRevokeToken = async (name: string) => { | ||
| const result = await revokeScimToken(name); | ||
| if (isServiceError(result)) { | ||
| toast({ title: "Error", description: `Failed to revoke SCIM token: ${result.message}`, variant: "destructive" }); | ||
| return; | ||
| } | ||
| router.refresh(); | ||
| toast({ description: "SCIM token revoked" }); | ||
| }; |
Contributor
There was a problem hiding this comment.
Handle revoke action exceptions explicitly.
Line 103 calls an async server action without a try/catch. If it throws, the rejection escapes and users get no failure toast.
Suggested fix
const handleRevokeToken = async (name: string) => {
- const result = await revokeScimToken(name);
- if (isServiceError(result)) {
- toast({ title: "Error", description: `Failed to revoke SCIM token: ${result.message}`, variant: "destructive" });
- return;
- }
- router.refresh();
- toast({ description: "SCIM token revoked" });
+ try {
+ const result = await revokeScimToken(name);
+ if (isServiceError(result)) {
+ toast({ title: "Error", description: `Failed to revoke SCIM token: ${result.message}`, variant: "destructive" });
+ return;
+ }
+ router.refresh();
+ toast({ description: "SCIM token revoked" });
+ } catch (error) {
+ console.error("Failed to revoke SCIM token:", error);
+ toast({ title: "Error", description: "Failed to revoke SCIM token", variant: "destructive" });
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleRevokeToken = async (name: string) => { | |
| const result = await revokeScimToken(name); | |
| if (isServiceError(result)) { | |
| toast({ title: "Error", description: `Failed to revoke SCIM token: ${result.message}`, variant: "destructive" }); | |
| return; | |
| } | |
| router.refresh(); | |
| toast({ description: "SCIM token revoked" }); | |
| }; | |
| const handleRevokeToken = async (name: string) => { | |
| try { | |
| const result = await revokeScimToken(name); | |
| if (isServiceError(result)) { | |
| toast({ title: "Error", description: `Failed to revoke SCIM token: ${result.message}`, variant: "destructive" }); | |
| return; | |
| } | |
| router.refresh(); | |
| toast({ description: "SCIM token revoked" }); | |
| } catch (error) { | |
| console.error("Failed to revoke SCIM token:", error); | |
| toast({ title: "Error", description: "Failed to revoke SCIM token", variant: "destructive" }); | |
| } | |
| }; |
🤖 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/web/src/app/`(app)/settings/security/components/scimProvisioningSettings.tsx
around lines 102 - 110, The handleRevokeToken function calls the async
revokeScimToken action without a try/catch block, which means any exceptions
thrown by that function will escape unhandled and the user won't see an error
toast. Wrap the await revokeScimToken(name) call in a try/catch block, and in
the catch clause, display a destructive toast with an appropriate error message
to ensure all failure scenarios (both service errors and thrown exceptions) are
properly communicated to the user.
Comment on lines
+32
to
+33
| const scimTokensResult = hasScimEntitlement ? await getScimTokens() : []; | ||
| const scimTokens = isServiceError(scimTokensResult) ? [] : scimTokensResult; |
Contributor
There was a problem hiding this comment.
Don’t collapse SCIM token fetch errors into an empty token list.
Lines 32-33 convert ServiceError to [], which makes downstream UI look like “no tokens exist” instead of “token retrieval failed.” That can lead owners to create unnecessary extra tokens.
Suggested fix
- const scimTokensResult = hasScimEntitlement ? await getScimTokens() : [];
- const scimTokens = isServiceError(scimTokensResult) ? [] : scimTokensResult;
+ const scimTokensResult = hasScimEntitlement ? await getScimTokens() : [];
+ const scimTokensError = isServiceError(scimTokensResult) ? scimTokensResult : null;
+ const scimTokens = scimTokensError ? [] : scimTokensResult;- {scimEnabled && (
+ {scimTokensError && (
+ <Alert className="items-center p-4">
+ <Info className="w-4 h-4 text-muted-foreground" />
+ <AlertDescription>{scimTokensError.message}</AlertDescription>
+ </Alert>
+ )}
+ {scimEnabled && !scimTokensError && (
<ScimProvisioningSettings baseUrl={scimBaseUrl} tokens={scimTokens} />
)}🤖 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/web/src/app/`(app)/settings/security/page.tsx around lines 32 - 33,
The SCIM token fetch logic in the security page converts ServiceError results
into an empty array, which masks retrieval failures as "no tokens exist" in the
UI. Instead of using the conditional assignment that checks
isServiceError(scimTokensResult) and defaults to an empty array, you need to
handle the error case separately so that the UI can distinguish between a
successful fetch with zero tokens versus a failed fetch. Create separate state
or variables to track both the scimTokens array and whether an error occurred
during the fetch, allowing the downstream UI to appropriately display either an
empty token list or an error message.
Comment on lines
+62
to
+75
| const name = payload.name?.formatted ?? payload.displayName ?? undefined; | ||
| const email = resolveEmail(payload); | ||
| await prisma.user.update({ | ||
| where: { id }, | ||
| data: { name, email }, | ||
| }); | ||
|
|
||
| const activeError = await applyActive(org.id, id, membership.isActive, coerceActive(payload.active)); | ||
| if (activeError) { | ||
| return activeError; | ||
| } | ||
|
|
||
| const refreshed = await loadMembership(prisma, org.id, id); | ||
| return scimJson(toScimUser(refreshed!), 200); |
Contributor
There was a problem hiding this comment.
Prevent partial resource updates in PUT/PATCH.
Both handlers persist profile fields before setMemberActive. If membership state change fails (seat/owner constraints), the request returns an error after partially committing changes.
Also applies to: 98-115
🤖 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/web/src/app/api/`(server)/ee/scim/v2/Users/[id]/route.ts around
lines 62 - 75, The code currently updates the user profile via
prisma.user.update() before validating the membership state change via
applyActive(), which can lead to partial updates if the membership change fails.
Reorder the operations so that applyActive() is called before
prisma.user.update() to validate the membership state change constraints first,
or alternatively wrap both the prisma.user.update() and applyActive() calls in a
database transaction to ensure that either both operations succeed or both fail
atomically. Apply the same fix to the similar code block mentioned at lines
98-115.
Comment on lines
+22
to
+30
| export const scimUserCreateSchema = z.object({ | ||
| userName: z.string(), | ||
| externalId: z.string().optional(), | ||
| name: scimNameSchema.optional(), | ||
| emails: z.array(scimEmailSchema).optional(), | ||
| // `active` may arrive as a boolean or a string ("true"/"false"). | ||
| active: z.union([z.boolean(), z.string()]).optional(), | ||
| displayName: z.string().optional(), | ||
| }).passthrough(); |
Contributor
There was a problem hiding this comment.
Validate SCIM email identifiers before persisting.
Line 23 and Line 65 currently accept arbitrary strings and then persist them as user.email in downstream handlers. That allows blank/non-email identifiers to be stored, which can corrupt account identity semantics.
Proposed fix
export const scimUserCreateSchema = z.object({
- userName: z.string(),
+ userName: z.string().trim().min(1),
externalId: z.string().optional(),
name: scimNameSchema.optional(),
emails: z.array(scimEmailSchema).optional(),
// `active` may arrive as a boolean or a string ("true"/"false").
active: z.union([z.boolean(), z.string()]).optional(),
displayName: z.string().optional(),
-}).passthrough();
+}).passthrough().superRefine((data, ctx) => {
+ const candidate = data.emails?.find((e) => e.primary)?.value ?? data.emails?.[0]?.value ?? data.userName;
+ if (!candidate || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(candidate)) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ message: "A valid email is required in emails[].value or userName",
+ });
+ }
+});Also applies to: 63-66
🤖 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/web/src/ee/features/scim/schemas.ts` around lines 22 - 30, The
scimUserCreateSchema currently accepts emails through the emails array without
validating that they are actual email addresses before they are persisted as
user.email in downstream handlers. Update the scimEmailSchema definition to
include proper email format validation (ensure the email field uses
z.string().email() or equivalent). Additionally, locate the other email
validation location mentioned around line 65-66 and apply the same email format
validation to prevent blank or non-email identifiers from being stored in the
database.
Comment on lines
+65
to
+125
| if (!existingRequest) { | ||
| await __unsafePrisma.accountRequest.create({ | ||
| data: { | ||
| requestedById: user.id, | ||
| orgId: org.id, | ||
| }, | ||
| }); | ||
|
|
||
| const smtpConnectionUrl = getSMTPConnectionURL(); | ||
| if (smtpConnectionUrl && env.EMAIL_FROM_ADDRESS) { | ||
| // TODO: This is needed because we can't fetch the origin from the request headers when this is called | ||
| // on user creation (the header isn't set when next-auth calls onCreateUser for some reason) | ||
| const deploymentUrl = env.AUTH_URL; | ||
|
|
||
| const owners = await __unsafePrisma.user.findMany({ | ||
| where: { | ||
| orgs: { | ||
| some: { | ||
| orgId: org.id, | ||
| role: "OWNER", | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| if (owners.length === 0) { | ||
| logger.error(`Failed to find any owners for org ${org.id} when drafting email for account request from ${user.id}`); | ||
| } else { | ||
| const html = await render(JoinRequestSubmittedEmail({ | ||
| baseUrl: deploymentUrl, | ||
| requestor: { | ||
| name: user.name ?? undefined, | ||
| email: user.email, | ||
| avatarUrl: user.image ?? undefined, | ||
| }, | ||
| orgName: org.name, | ||
| orgImageUrl: org.imageUrl ?? undefined, | ||
| })); | ||
|
|
||
| const ownerEmails = owners | ||
| .map((owner) => owner.email) | ||
| .filter((email): email is string => email !== null); | ||
|
|
||
| const transport = createTransport(smtpConnectionUrl); | ||
| const result = await transport.sendMail({ | ||
| to: ownerEmails, | ||
| from: env.EMAIL_FROM_ADDRESS, | ||
| subject: `New account request for ${org.name} on Sourcebot`, | ||
| html, | ||
| text: `New account request for ${org.name} on Sourcebot by ${user.name ?? user.email}`, | ||
| }); | ||
|
|
||
| const failed = result.rejected.concat(result.pending).filter(Boolean); | ||
| if (failed.length > 0) { | ||
| logger.error(`Failed to send account request email to ${ownerEmails.join(', ')}: ${failed}`); | ||
| } | ||
| } | ||
| } else { | ||
| logger.warn(`SMTP_CONNECTION_URL or EMAIL_FROM_ADDRESS not set. Skipping account request email to owner`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Redundant conditional and missing account request cleanup.
-
The
if (!existingRequest)check at line 65 is redundant since the function already returns at lines 57-63 whenexistingRequestis truthy. -
After successfully approving an account request (in
approveAccountRequest), theaccountRequestrecord is not deleted. UnlikerejectAccountRequestwhich properly deletes the record, approvals leave orphaned request records.
🔧 Proposed fixes
Remove redundant conditional in createAccountRequest:
- if (!existingRequest) {
- await __unsafePrisma.accountRequest.create({
+ await __unsafePrisma.accountRequest.create({And ensure the closing brace is also removed at line 125.
Add account request deletion in approveAccountRequest after successful addMember (around line 181):
if (isServiceError(addUserToOrgRes)) {
await failAuditCallback(addUserToOrgRes.message);
return addUserToOrgRes;
}
+ await prisma.accountRequest.delete({
+ where: {
+ id: requestId,
+ },
+ });
await createAudit({🤖 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/web/src/features/membership/actions/accountRequests.ts` around lines
65 - 125, Remove the redundant if (!existingRequest) conditional check in the
createAccountRequest function since an early return at line 57-63 already
handles the case when existingRequest is truthy. Unindent all code that was
nested inside this conditional block by one level and remove the closing brace
at line 125. Additionally, in the approveAccountRequest function, add deletion
of the accountRequest record after successfully calling addMember (around line
181) to match the cleanup pattern used in rejectAccountRequest, ensuring that
approved account requests do not leave orphaned records in the database.
Comment on lines
+18
to
+45
| const handleSubmit = async () => { | ||
| setIsSubmitting(true) | ||
| const result = await createAccountRequest() | ||
| if (!isServiceError(result)) { | ||
| if (result.existingRequest) { | ||
| toast({ | ||
| title: "Request Already Submitted", | ||
| description: "Your request to join the organization has already been submitted. Please wait for it to be approved.", | ||
| variant: "default", | ||
| }) | ||
| } else { | ||
| toast({ | ||
| title: "Request Submitted", | ||
| description: "Your request to join the organization has been submitted.", | ||
| variant: "default", | ||
| }) | ||
| } | ||
| // Refresh the page to trigger layout re-render and show PendingApprovalCard | ||
| router.refresh() | ||
| } else { | ||
| toast({ | ||
| title: "Failed to Submit", | ||
| description: `There was an error submitting your request. Reason: ${result.message}`, | ||
| variant: "destructive", | ||
| }) | ||
| } | ||
| setIsSubmitting(false) | ||
| } |
Contributor
There was a problem hiding this comment.
Always clear submitting state when the action throws.
createAccountRequest() can throw; without try/finally, isSubmitting can stay true and lock the button.
Suggested fix
const handleSubmit = async () => {
- setIsSubmitting(true)
- const result = await createAccountRequest()
- if (!isServiceError(result)) {
- if (result.existingRequest) {
- toast({
- title: "Request Already Submitted",
- description: "Your request to join the organization has already been submitted. Please wait for it to be approved.",
- variant: "default",
- })
- } else {
- toast({
- title: "Request Submitted",
- description: "Your request to join the organization has been submitted.",
- variant: "default",
- })
- }
- // Refresh the page to trigger layout re-render and show PendingApprovalCard
- router.refresh()
- } else {
- toast({
- title: "Failed to Submit",
- description: `There was an error submitting your request. Reason: ${result.message}`,
- variant: "destructive",
- })
- }
- setIsSubmitting(false)
+ setIsSubmitting(true)
+ try {
+ const result = await createAccountRequest()
+ if (!isServiceError(result)) {
+ if (result.existingRequest) {
+ toast({
+ title: "Request Already Submitted",
+ description: "Your request to join the organization has already been submitted. Please wait for it to be approved.",
+ variant: "default",
+ })
+ } else {
+ toast({
+ title: "Request Submitted",
+ description: "Your request to join the organization has been submitted.",
+ variant: "default",
+ })
+ }
+ router.refresh()
+ } else {
+ toast({
+ title: "Failed to Submit",
+ description: `There was an error submitting your request. Reason: ${result.message}`,
+ variant: "destructive",
+ })
+ }
+ } finally {
+ setIsSubmitting(false)
+ }
}🤖 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/web/src/features/membership/components/submitJoinRequestCard.tsx`
around lines 18 - 45, The handleSubmit function in submitJoinRequestCard.tsx
does not have proper error handling for exceptions thrown by
createAccountRequest(). If createAccountRequest() throws an error,
setIsSubmitting(false) will never execute, leaving the button locked. Refactor
the handleSubmit function to wrap the createAccountRequest() call and subsequent
logic in a try/catch/finally block, placing the setIsSubmitting(false) call in
the finally block to ensure it always executes. In the catch block, display an
error toast with the caught error message so users are informed of any
unexpected failures.
Comment on lines
+47
to
+49
| if (!(await orgHasAvailability(orgId))) { | ||
| return seatLimitReached(); | ||
| } |
Contributor
There was a problem hiding this comment.
Seat-cap enforcement is non-atomic and can over-provision memberships.
At Line 47 and Line 269, capacity is checked before the write transaction. Concurrent add/reactivate requests can both pass the pre-check and then both persist, exceeding the seat cap.
Proposed direction
- if (!(await orgHasAvailability(orgId))) {
- return seatLimitReached();
- }
- const membership = await prisma.$transaction(async (tx) => {
+ const membership = await prisma.$transaction(async (tx) => {
+ const seatCap = getSeatCap();
+ if (seatCap) {
+ const activeUserCount = await tx.userToOrg.count({
+ where: { orgId, isActive: true },
+ });
+ if (activeUserCount >= seatCap) {
+ return seatLimitReached();
+ }
+ }
const created = await tx.userToOrg.create({ ... });
...Apply the same pattern to the reactivation path so the check and update are in one transaction.
Also applies to: 51-70, 269-271, 273-279
🤖 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/web/src/features/membership/membership.service.ts` around lines 47 -
49, The seat capacity check using orgHasAvailability() is performed outside the
database transaction in both the membership add and reactivation code paths,
allowing concurrent requests to bypass the seat limit. Move the
orgHasAvailability() check inside the same database transaction that performs
the actual insert or reactivate operation for the membership record, ensuring
the capacity validation and data persistence happen atomically together. Apply
this transactional pattern consistently to both the add path (around the
seatLimitReached() check) and the reactivation path to prevent race conditions
where multiple concurrent requests can each pass the check separately and then
both persist, exceeding the seat cap.
Comment on lines
+43
to
+50
| const defaultOrg = await __unsafePrisma.org.findUnique({ | ||
| where: { | ||
| id: SINGLE_TENANT_ORG_ID, | ||
| }, | ||
| include: { | ||
| members: true, | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
First-user OWNER bootstrap is race-prone and can create multiple owners.
Line 76 derives “first user” from a non-atomic read (members.length from Line 43). Concurrent user creations can both observe empty membership and both assign OrgRole.OWNER.
Move first-user role resolution into a single transaction in the membership layer (count + create in one serializable unit), and use a count query instead of loading full members.
Also applies to: 76-81
🤖 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/web/src/features/membership/onCreateUser.ts` around lines 43 - 50,
The first-user OWNER role assignment in the onCreateUser function is prone to
race conditions because it uses a non-atomic read of the members array to
determine if this is the first user. Replace the current approach where
__unsafePrisma.org.findUnique loads the full members array and members.length is
checked at line 76-81 with a single atomic transaction in the membership layer
that performs both a count query (instead of loading full members) and the
membership creation in one serializable unit. This ensures concurrent user
creations cannot both observe empty membership and both assign OrgRole.OWNER.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a SCIM 2.0 server (EE, gated by the new
scimentitlement) so an identity provider (Okta, Entra) can provision and deprovision Sourcebot members.Scope
/scim/v2Users endpoints: discovery, list+filter, create, get, replace, PATCH (activetoggle), delete. Groups deferred.UserToOrg.isActive): forces logout viasessionVersionand revokes API/OAuth tokens, but preserves the row so the IdP can reactivate.ScimToken(bearer auth viawithScimAuth); managed under Settings → Security.Note: the
scimentitlement must be added to the lighthouse entitlements list and deployed before online licenses will grant it.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements