| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting. Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds SCIM 2.0 endpoints, token management, and SCIM-aware membership state. It also splits membership actions into feature modules, updates login and join flows, and changes settings, billing, chat, and API responses to use active or suspended membership data. ChangesSCIM provisioning and membership architecture update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labelssourcebot-team Suggested reviewers
❌ Failed checks (1 warning)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
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 win
Do 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🤖 Prompt for AI Agents- if (membership) { + if (membership?.isActive) { redirect(`/`); }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.
packages/web/src/app/(app)/settings/security/components/scimProvisioningSettings.tsx-125-127 (1)🧹 Nitpick comments (1)packages/web/src/features/membership/components/joinOrganizationCard.tsx-56-63 (1)125-127: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Add 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 AgentsVerify 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/submitJoinRequestCard.tsx-60-70 (1)56-63: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Replace CSS variable color classes with Tailwind semantic color classes.
text-[var(--muted-foreground)] and text-[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 AgentsVerify 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/app/(app)/settings/members/components/membersList.tsx-217-217 (1)60-70: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Use 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., use border-border instead of border-[var(--border)]).
🤖 Prompt for AI AgentsVerify 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/api/(server)/ee/scim/v2/Users/[id]/route.ts-74-75 (1)217-217: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Use a fallback label when name is missing.
Line 217 renders member.name directly, so members without a profile name show a blank primary label. Use email fallback to keep identity visible.
Suggested fix🤖 Prompt for AI Agents- <span className="font-medium truncate">{member.name}</span> + <span className="font-medium truncate">{member.name ?? member.email}</span>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/ee/features/scim/withScimAuth.ts-26-34 (1)74-75: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Remove unsafe non-null assertions after reload.
A concurrent delete between update and reload can make refreshed null; 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 AgentsVerify 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).CHANGELOG.md-42-42 (1)26-34: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Accept case-insensitive bearer auth schemes.
The current check rejects bearer/mixed-case schemes, which can break otherwise valid IdP requests.
Suggested fix🤖 Prompt for AI Agents- 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);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.packages/web/src/ee/features/scim/actions.ts-73-85 (1)42-42: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Place 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 AgentsVerify 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
73-85: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Normalize and validate token names on the server boundary.
Line 73 and Line 115 accept name as-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 fixexport 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 AgentsVerify 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.
packages/db/prisma/schema.prisma (1)🤖 Prompt for all review comments with AI agents436-437: ⚡ Quick win
Remove redundant uniqueness on primary key hash.
hash is already unique because it is the model @id; keeping @unique is redundant and can lead to unnecessary duplicate index artifacts in migrations.
Suggested schema cleanup🤖 Prompt for AI Agents- hash String `@id` `@unique` + hash String `@id`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`.
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`.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0afa80cc-c399-4e0f-9b31-b82624e66b85
📥 CommitsReviewing files that changed from the base of the PR and between 26435a4 and 199c2bd.
📒 Files selected for processing (74)
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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/web/src/features/membership/membership.service.ts`: - Around line 40-47: In the ensureActiveMember function, when an existing active membership is found (the first if condition checking existing && existing.isActive), add a check to see if the incoming scimExternalId differs from the existing membership's scimExternalId. If they differ, delegate to setMemberActive with the active flag set to true and the provided scimExternalId, rather than returning the existing membership immediately. This ensures the SCIM external ID is persisted consistently across all code paths.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f28d6b43-479d-4001-b746-c114dfaf7c40
📥 CommitsReviewing files that changed from the base of the PR and between 199c2bd and 260b789.
📒 Files selected for processing (21)
Sorry, something went wrong.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Sorry, something went wrong.
…mbers Add a per-membership lastActiveAt to UserToOrg, stamped in getAuthContext alongside the existing global User.lastActiveAt. The migration backfills it from User.lastActiveAt so existing members are not reset to "never active". Switch the service ping's DAU/WAU/MAU and billed user count onto the per-org timestamp. billedUserCount now counts only active (non-suspended) members who have been active in the org at least once, so provisioned-but-never-signed-in members no longer consume a seat. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)packages/web/src/middleware/withAuth.test.ts (1)🤖 Prompt for all review comments with AI agents486-552: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add regression coverage for suspended activity updates.
The suspended-member tests should also assert prisma.userToOrg.updateMany is not called, so the auth denial cannot still mark deprovisioned memberships active.
Also applies to: 930-986
🤖 Prompt for AI AgentsVerify 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/middleware/withAuth.test.ts` around lines 486 - 552, The suspended-member auth tests in withAuth.test.ts currently verify role denial but do not cover the side effect on membership activity. Update the suspended-membership cases in getAuthContext to also assert prisma.userToOrg.updateMany is never called, including the API-key path, so suspended or deprovisioned memberships cannot be marked active when access is denied.
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/web/src/app/`(app)/settings/apiKeys/layout.tsx: - Around line 5-12: The layout props type in authenticatedPage currently references React.ReactNode without React in scope, which will fail type-checking under the current JSX settings. Update the apiKeys layout to import type ReactNode and use that type for the children prop in the authenticatedPage generic, keeping the existing SettingsContainer and notFound logic unchanged. In `@packages/web/src/app/`(app)/settings/members/membersTable.tsx: - Around line 367-382: Biome is flagging the intentional trigger dependencies in the two `useLayoutEffect` hooks inside `membersTable.tsx`. Keep the current scroll restore/top behavior in `membersTable` by adding targeted Biome dependency ignores for the `sorting` effect and the `filter, searchQuery` effect, rather than removing those dependencies. Make the ignores as narrow as possible and keep the rest of the hook logic unchanged. In `@packages/web/src/app/`(app)/settings/members/membersTableActions.tsx: - Around line 285-312: The promote/demote actions in membersTableActions and the corresponding membership mutations in actions.ts are still available for SCIM-managed orgs, unlike the other membership changes. Add the same SCIM guard used elsewhere so promoteToOwner and demoteToMember are disabled or blocked when the org is SCIM-managed, and make sure the server-side actions also enforce this restriction rather than relying only on the UI. In `@packages/web/src/app/api/`(server)/ee/users/route.ts: - Line 59: The EE user-list public API in the route backed by publicEeUserListItemSchema now exposes suspendedAt instead of isActive, which is a breaking response-shape change for consumers. Update the route and any related schema references so the exported user list contract is consistent, then regenerate the OpenAPI output with the web workspace openapi generation task and refresh docs/api-reference/sourcebot-public.openapi.json. Also add a brief migration/release-notes entry calling out that isActive was replaced by suspendedAt. In `@packages/web/src/features/billing/servicePing.ts`: - Around line 48-65: The DAU/WAU/MAU queries in servicePing still count suspended memberships because they only filter by lastActiveAt; update the count conditions to use the same active-membership criteria as activeUserCount, likely by reusing activeMembershipWhere() or adding an equivalent suspendedAt exclusion alongside the existing orgId and activity cutoff checks in the servicePing metrics query. In `@packages/web/src/features/membership/membership.service.test.ts`: - Around line 20-23: Add activeMembershipWhere to the `@/features/membership/utils` mock because the current vi.mock replacement omits it, causing countActiveOwners to see it as undefined during removeMember and setMemberRole tests. Update the membership.service.test.ts mock around the membership utils override so it either exports activeMembershipWhere alongside orgHasAvailability and unsuspendedMembershipWhere, or spreads the real module and overrides only the needed helpers, keeping countActiveOwners functional. In `@packages/web/src/middleware/withAuth.ts`: - Around line 113-115: Skip updating membership activity when the membership is suspended. In withAuth, before calling updateMembershipLastActiveAt(membership), add a guard using the membership record so suspended rows are ignored and only active memberships update UserToOrg.lastActiveAt. Keep the change localized to the membership handling block in withAuth and use the existing membership object to detect suspension. --- Nitpick comments: In `@packages/web/src/middleware/withAuth.test.ts`: - Around line 486-552: The suspended-member auth tests in withAuth.test.ts currently verify role denial but do not cover the side effect on membership activity. Update the suspended-membership cases in getAuthContext to also assert prisma.userToOrg.updateMany is never called, including the API-key path, so suspended or deprovisioned memberships cannot be marked active when access is denied.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4af5ee55-42e0-4afc-879c-f28e8800efe1
📥 CommitsReviewing files that changed from the base of the PR and between 2234f8d and 12355df.
📒 Files selected for processing (49)
Sorry, something went wrong.
There was a problem hiding this comment.
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/middleware/withAuth.ts (1)119-130: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Re-read membership before granting access after pending activation.
role is computed from the pre-transaction row. If the guarded activation updates 0 rows because the membership was concurrently suspended/deleted, this still refreshes activity and returns the stale role. Have activatePendingMembership return the activated/current row or re-fetch the membership here before deriving role.
🤖 Prompt for AI AgentsVerify 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/middleware/withAuth.ts` around lines 119 - 130, The access flow in withAuth is using a stale membership row after activatePendingMembership, so role can be granted from pre-transaction data. Update the withAuth path to re-read the membership after activation, or have activatePendingMembership return the current activated row, and then derive role only from that refreshed membership before calling updateMembershipLastActiveAt or returning access.
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 `@packages/web/src/middleware/withAuth.ts`: - Around line 119-130: The access flow in withAuth is using a stale membership row after activatePendingMembership, so role can be granted from pre-transaction data. Update the withAuth path to re-read the membership after activation, or have activatePendingMembership return the current activated row, and then derive role only from that refreshed membership before calling updateMembershipLastActiveAt or returning access.
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52a663a3-17e6-42c6-b2d4-099de06aa499
📥 CommitsReviewing files that changed from the base of the PR and between 12355df and 1d3a2fd.
📒 Files selected for processing (6)
Sorry, something went wrong.
License Audit❌ Status: FAIL
Fail Reasons
Unresolved Packages
Weak Copyleft Packages (informational)
|
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Adds a SCIM 2.0 server (EE, gated by the new scim entitlement) so an identity provider (Okta, Entra) can provision and deprovision Sourcebot members.
Scope
Note: the scim entitlement must be added to the lighthouse entitlements list and deployed before online licenses will grant it.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes