feat: improve SCIM implementation#2351
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
WalkthroughAdds organization member active flag and invitation last-sent timestamp; introduces a transactional UserInviteService and Keycloak helper; refactors invite and SCIM flows to use the service with optional mailer and audit logging; updates repositories, types, migration, tests, and audit log actions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
controlplane/src/core/repositories/OrganizationRepository.ts (1)
242-254:isMemberOfdoes not constrain membership to the given organizationThe join currently does:
.from(organizationsMembers) .innerJoin(organizations, eq(organizations.id, input.organizationId)) .innerJoin(users, eq(users.id, organizationsMembers.userId)) .where(and(eq(users.id, input.userId), eq(organizationsMembers.active, true)))
organizations.idis compared only to the literalinput.organizationId, never toorganizationsMembers.organizationId. As long as the organization exists and the user has any active membership in any org, this query will return a row andisMemberOfwill incorrectly returntrue.You likely want to either:
- Join on the membership relation and filter by org:
.from(organizationsMembers) .innerJoin(organizations, eq(organizations.id, organizationsMembers.organizationId)) .innerJoin(users, eq(users.id, organizationsMembers.userId)) .where( and( eq(users.id, input.userId), eq(organizationsMembers.organizationId, input.organizationId), eq(organizationsMembers.active, true), ), );or use an equivalent condition in
where. This is a behavioral bug and should be fixed before relying onisMemberOffor authorization or SCIM logic.controlplane/src/core/controllers/scim.ts (2)
157-260: Use organization member'sactivefield consistently across all GET endpointsThe inconsistency is confirmed. Your GET /Users list endpoint returns:
active: keycloakUsers[0].enabled ?? true,while GET /Users/:userID returns:
active: user.active,This divergence is problematic because PUT and PATCH both treat the org member's
user.activefield as the source of truth for the active status (e.g., line 456 comparesuser.active !== activeand updates accordingly). However, the list endpoint ignores this and pulls from Keycloak instead, causing the same user to report differentactivevalues depending on which endpoint is called.Update the list endpoint to use
member.activeanduser.active(instead ofkeycloakUsers[0].enabled) on both the filtered (line 214) and unfiltered (line 244) branches to maintain consistency.
317-396: HTTP and SCIM status codes must be aligned per RFC 7644In the POST
/Usershandler (lines 374-379), error status codes are misaligned:if (isPublicError(err)) { return res.code(400).send( ScimError({ detail: err.message, status: err.code === EnumStatusCode.ERR_ALREADY_EXISTS ? 409 : 500, }), ); }Issues confirmed:
ERR_ALREADY_EXISTS: HTTP status is 400, SCIM status is 409 — violates RFC 7644 Section 3.12, which requires these to match. Should be HTTP 409.- Other
PublicErrorcodes (ERR_NOT_FOUND,ERR_LIMIT_REACHED, etc.): HTTP status is 400, SCIM status is 500 — violates alignment requirement.ERR_NOT_FOUNDshould be 404;ERR_LIMIT_REACHEDshould be 429 or 400 with appropriate scimType.Correct the error handling to align HTTP and SCIM status codes:
- Return HTTP 409 for
ERR_ALREADY_EXISTSwith SCIM status 409 (conflict/uniqueness).- Return HTTP 404 for
ERR_NOT_FOUNDwith SCIM status 404.- Return HTTP 400 (or 429) for
ERR_LIMIT_REACHEDwith SCIM status matching the HTTP code.
🧹 Nitpick comments (10)
controlplane/src/db/models.ts (1)
100-177: SCIM audit actions wiring looks consistent with existing modelThe new
AuditLogFullActionvariants for SCIM andnamespace_proposal_config.updatedintegrate cleanly into the union and match how audit actions are modeled elsewhere. Only minor nit:scim.organization_invitation_createduses an underscore beforecreatedwhereas most other actions followresource.action(organization_invitation.created). If consistency with existing naming matters for downstream consumers or analytics, consider aligning the naming scheme; otherwise this is fine as-is.controlplane/src/db/schema.ts (1)
1388-1401: NewactiveandlastSentAtcolumns are modeled correctly; consider active semantics for aggregations
organizationsMembers.active: boolean('active').notNull().default(true)andorganizationInvitations.lastSentAt: timestamp('last_sent_at', { withTimezone: true })are modeled in line with existing schema (e.g.,users.active, other timestamps) and match the repository usage in this PR.- With the new
activeflag onorganization_members, some read paths now filter onactive = truewhile others (e.g.,memberCount) still count all memberships. Double-check whether the intended behavior is “all members ever” vs “currently active members” and align those callers if necessary.Also applies to: 1591-1607
controlplane/src/core/repositories/OrganizationRepository.ts (1)
259-291: Active membership flag is wired through DTOs correctly; tighten a couple of behaviors
memberships,getOrganizationMember,getOrganizationMemberByEmail, andgetMembersnow all surfaceorganizationsMembers.activeinto DTOs. This aligns nicely with the new per-membership activity concept and keeps repo code data-focused, with authorization still handled at the service layer (as per prior learnings).- In
memberships, filtering onorganizationsMembers.active = trueis consistent with the idea that only active memberships should be treated as current when building auth context and RBAC evaluators.A few follow-ups to consider:
Clarify which paths should ignore inactive memberships
memberCountandgetOrganizationAdmins(which callsgetMembers) currently include inactive members. If SCIM deactivation is meant to remove users from “admin” lists and member counts, you may want to addeq(organizationsMembers.active, true)to those queries as well. If not, a short comment near those methods clarifying intent (“counts all, including inactive”) would help avoid regressions later.
setOrganizationMemberActiveexecution semanticspublic setOrganizationMemberActive(input: { id: string; organizationId: string; active: boolean }) { return this.db .update(organizationsMembers) .set({ active: input.active }) .where(and(eq(organizationsMembers.organizationId, input.organizationId), eq(organizationsMembers.id, input.id))); }This returns a Drizzle update query. It will only run if call sites
awaitthe return value (since Drizzle’s query builders arePromiseLike), but the method itself is notasync, unlike most other mutators in this repository.For consistency and to make misuse harder, consider making it explicitly async and executing it here:
public async setOrganizationMemberActive(input: { id: string; organizationId: string; active: boolean }) { await this.db .update(organizationsMembers) .set({ active: input.active }) .where( and( eq(organizationsMembers.organizationId, input.organizationId), eq(organizationsMembers.id, input.id), ), ); }This keeps call sites from accidentally forgetting to await the query.
Group resolution and inactive members
getOrganizationMemberGroupsdoes not filter onorganizationsMembers.active, which is fine if it’s only called from flows that already restrict to active members (e.g.,memberships,getMembers). If you plan to use it in contexts where a user might be inactive in the organization, consider either adding anactive = truecondition or documenting that callers must enforce that themselves.Also applies to: 360-395, 398-439, 441-492, 494-505, 507-512, 1592-1617
controlplane/src/core/repositories/OrganizationInvitationRepository.ts (1)
165-208: Invitation creation &lastSentAtupdates are mostly solid; consider tightening update scope
inviteUsernow bails out of group insertion when either the insert yields no rows orinput.groups.length === 0. That safely supports “invite without groups” and avoids inserting an emptyvalues([])set.
updateLastSentToNowcorrectly updateslastSentAtonly for non-accepted invitations for the specified user and organization:.where( and( eq(organizationInvitations.organizationId, organizationId), eq(organizationInvitations.userId, userId), eq(organizationInvitations.accepted, false), ), )which is exactly what the resend-throttling logic in
UserInviteServiceneeds.One small robustness improvement: if your data model ever allowed multiple pending invitations per (organizationId, userId), this update would touch all of them while
getPendingOrganizationInvitationonly reads one (the earliest bycreatedAt). To keep the write/read symmetry tight, you could optionally narrowupdateLastSentToNowby invitation ID when you have it available (e.g., passinvitationIdfrom the pending-invitation lookup). Not required if you guarantee at most one pending invite per org/user pair.Also applies to: 263-275
controlplane/src/core/bufservices/user/inviteUser.ts (1)
9-42: Good service extraction and transactional invite flow
- Delegating the invitation logic to
UserInviteServiceinside a singleopts.db.transactionis a solid refactor: the handler now focuses on auth, wiring, and audit logging, while the service encapsulates the complex invite/user/group/Keycloak logic.- Injecting
db: tx,logger,keycloakRealm,keycloak, andmailerinto the service matches how the service is designed in the rest of the PR and keeps concerns separated.- Audit logging still records
organization_invitation.createdwithorganizationSlugfrom the auth context, which is appropriate for this non-SCIM path.If the
InviteUserRequestproto later grows optional fields like first/last name or a password that the service can use, consider threading those through toservice.inviteUserhere as well to avoid having multiple invite entry points behave differently.Also applies to: 44-47
controlplane/test/scim.test.ts (1)
116-159: Make the pending invitations assertion resilient to existing stateThis test assumes there are zero pending invitations before creating the new user and therefore asserts
totalCount === 1. Because the DB and server are shared across tests in thisdescribe, any earlier test that leaves a pending invitation will make this brittle.Consider either:
- Capturing the initial
totalCountbefore the POST and asserting that it increased by one, or- Filtering
pendingInvitationsbytotalCount.controlplane/src/core/services/UserInviteService.ts (4)
66-77: Member limit check should not rely on truthiness oflimit
limitis computed then used as:const limit = usersFeature?.limit === -1 ? undefined : usersFeature?.limit; if (limit && memberCount >= limit) { throw new PublicError(EnumStatusCode.ERR_LIMIT_REACHED, ...); }If
limitis ever0, this condition will not trigger and the organization will be treated as having no limit. Safer is to check explicitly forundefined:if (limit !== undefined && memberCount >= limit) { ... }This keeps semantics correct even if a “zero‑member” plan is ever introduced.
96-118: Consider auto‑provisioning the DB user when Keycloak user exists but DB user is missingCurrently:
- If the Keycloak user is missing, you create it and immediately add a DB user.
- If the Keycloak user exists but
userRepo.byIdreturnsnull, you throwPublicError('User not found').If there are valid scenarios where a Keycloak user may pre‑exist without a DB row (e.g., manually created in Keycloak or migrated from another system), this will prevent invitations from being sent.
A more forgiving behavior would be to create the DB user in that second case as well, mirroring the “user doesn’t exist in Keycloak” branch, and only throw if that insertion fails.
141-162: Group validation logic is strict for emptygroupsarraysThe logic:
const groups: string[] = []; if (input.groups) { ... } if (input.groups && groups.length === 0) { throw new PublicError(EnumStatusCode.ERR, 'No group was provided'); }Means that if
input.groupsis an empty array, you’ll throw'No group was provided'. That’s a valid design, but differs from the SCIM controller, which currently never suppliesgroupsintoinviteUser(so this branch is effectively unused).If you plan to wire SCIM
groupsintoinviteUser, consider whether an empty array should be allowed (no groups) or rejected, and encode that explicitly (e.g.,if (input.groups && input.groups.length === 0)or by validating at the controller layer).
188-236: Throttle interval comment doesn’t match implementation; also only applies to mailer pathInside
#sendInvitation:if (pendingInvitation?.lastSentAt && pendingInvitation.lastSentAt.getTime() + 1000 * 60 * 30 > Date.now()) { // We are not sending the invitation more than once an hour return; }
1000 * 60 * 30is 30 minutes, not one hour, so the comment is misleading.- This throttle only applies to the custom
mailerpath. If the user has no memberships yet, you always send a new Keycloak “execute actions” email regardless oflastSentAt.If the intent is a 30‑minute throttle for custom mail plus unlimited Keycloak emails, just fix the comment. If you actually want a one‑hour throttle or want it applied to both paths, consider adjusting the multiplier and/or factoring the
lastSentAtcheck up a level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
controlplane/migrations/0133_foamy_vargas.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/core/bufservices/user/inviteUser.ts(2 hunks)controlplane/src/core/build-server.ts(1 hunks)controlplane/src/core/controllers/auth.ts(2 hunks)controlplane/src/core/controllers/scim.ts(8 hunks)controlplane/src/core/errors/errors.ts(1 hunks)controlplane/src/core/repositories/OrganizationInvitationRepository.ts(4 hunks)controlplane/src/core/repositories/OrganizationRepository.ts(6 hunks)controlplane/src/core/repositories/UserRepository.ts(0 hunks)controlplane/src/core/services/Keycloak.ts(1 hunks)controlplane/src/core/services/UserInviteService.ts(1 hunks)controlplane/src/db/models.ts(1 hunks)controlplane/src/db/schema.ts(2 hunks)controlplane/src/types/index.ts(1 hunks)controlplane/test/scim.test.ts(9 hunks)studio/src/components/audit-log-table.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- controlplane/src/core/repositories/UserRepository.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/repositories/OrganizationRepository.tscontrolplane/src/core/controllers/auth.ts
📚 Learning: 2025-09-10T11:15:52.157Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/ProposalRepository.ts:562-572
Timestamp: 2025-09-10T11:15:52.157Z
Learning: The getLatestCheckForProposal function in controlplane/src/core/repositories/ProposalRepository.ts is only called during proposal creation or updates, so proposal match error checking (hasProposalMatchError) is not needed since the proposal is being modified itself rather than being matched against.
Applied to files:
controlplane/src/core/repositories/OrganizationInvitationRepository.ts
🧬 Code graph analysis (7)
controlplane/src/core/services/UserInviteService.ts (8)
controlplane/src/core/services/Keycloak.ts (1)
Keycloak(8-442)controlplane/src/core/services/Mailer.ts (1)
Mailer(13-101)controlplane/src/core/repositories/OrganizationRepository.ts (3)
OrganizationRepository(50-1670)memberCount(341-358)memberships(259-339)controlplane/src/core/repositories/UserRepository.ts (1)
UserRepository(18-178)controlplane/src/core/repositories/OrganizationInvitationRepository.ts (1)
OrganizationInvitationRepository(12-276)controlplane/src/core/errors/errors.ts (1)
PublicError(17-17)controlplane/src/core/repositories/OrganizationGroupRepository.ts (1)
OrganizationGroupRepository(9-356)controlplane/src/types/index.ts (1)
OrganizationInvitationDTO(313-319)
controlplane/src/core/repositories/OrganizationRepository.ts (1)
controlplane/src/db/schema.ts (3)
users(1134-1140)organizationsMembers(1387-1409)organizations(1261-1284)
controlplane/src/core/controllers/auth.ts (1)
controlplane/src/db/schema.ts (1)
organizationsMembers(1387-1409)
controlplane/src/core/controllers/scim.ts (4)
controlplane/src/core/services/Mailer.ts (1)
Mailer(13-101)controlplane/src/core/services/UserInviteService.ts (1)
UserInviteService(15-237)controlplane/src/core/repositories/AuditLogRepository.ts (2)
AuditLogRepository(28-137)AddAuditLogInput(6-23)controlplane/src/core/errors/errors.ts (1)
isPublicError(33-35)
controlplane/test/scim.test.ts (1)
controlplane/src/core/test-util.ts (1)
TestAuthenticator(223-226)
controlplane/src/core/bufservices/user/inviteUser.ts (1)
controlplane/src/core/services/UserInviteService.ts (1)
UserInviteService(15-237)
controlplane/src/core/repositories/OrganizationInvitationRepository.ts (1)
controlplane/src/db/schema.ts (1)
organizationInvitations(1591-1615)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (17)
controlplane/src/core/controllers/auth.ts (2)
4-4: LGTM: Import added for new active filtering logic.The import of
andfrom drizzle-orm is necessary for the compound where clause added at Line 274.
274-274: LGTM: Active membership filtering properly implemented.The addition of
eq(organizationsMembers.active, true)ensures that only active organization memberships are considered when determining if a user already belongs to an organization. This correctly aligns with the newactivefield introduced in the schema and prevents treating inactive memberships as valid.studio/src/components/audit-log-table.tsx (1)
80-82: LGTM: SCIM invitation audit action properly integrated.The addition of
auditAction === "scim.organization_invitation_created"appropriately extends the UI formatting logic to handle SCIM-based invitation audit events consistently with existing invitation actions.controlplane/src/core/errors/errors.ts (1)
33-35: LGTM: Type signature appropriately widened.Changing the parameter type from
Errortounknownis a standard best practice for type guards. This allows callers to pass any error type without type assertions while preserving the runtime instanceof check.controlplane/migrations/meta/_journal.json (1)
935-942: LGTM: Migration journal entry properly added.The new journal entry follows the established pattern and correctly references the corresponding migration file
0133_foamy_vargas.sql.controlplane/src/types/index.ts (1)
318-318: LGTM: DTO properly extended with invitation tracking field.The optional
lastSentAtfield correctly represents the new database column and aligns with the invitation tracking functionality introduced in this PR.controlplane/migrations/0133_foamy_vargas.sql (1)
1-2: LGTM: Schema migration properly structured.Both column additions are non-breaking:
last_sent_atis nullable, appropriately handling existing invitation records.activehas a sensible default oftruewith NOT NULL constraint, ensuring existing members remain active.controlplane/src/core/build-server.ts (1)
463-463: LGTM: Mailer properly wired to SCIM controller.The mailer client is correctly passed to the SCIM controller registration, enabling email-based invitation functionality when SMTP is configured.
controlplane/src/core/services/Keycloak.ts (1)
95-104: LGTM: User lookup method properly implemented.The
findUserByEmailmethod is correctly implemented with:
- Exact email matching to avoid false positives.
- Efficient single-result query (
max: 1).- Consistent realm fallback pattern matching other methods in the class.
- Clear return semantics (user object or undefined).
controlplane/src/core/repositories/OrganizationInvitationRepository.ts (1)
111-149: ExposinglastSentAton pending invitations aligns with resend throttlingSelecting
schema.organizationInvitations.lastSentAtand threading it into theOrganizationInvitationDTOgives theUserInviteServiceenough context to avoid resending invitations too frequently. The null/undefined handling is straightforward:lastSentAt: orgMember[0].lastSentAt || undefined,Assuming the DTO is typed to accept
Date | undefined, this is consistent with how other timestamp fields are handled in the codebase (e.g., usingtoISOString()only when serializing externally). Looks good.controlplane/test/scim.test.ts (3)
7-8: Authenticator wiring in tests looks correctImporting
TestAuthenticatorand pullingsetupDetails.authenticatorinto a sharedauthenticatorvariable matches the intended pattern for switching user contexts in later tests. No issues here.Also applies to: 21-25, 39-45
288-388: Update‑after‑acceptance flow looks correct and realisticThe flow of:
- Removing the user from the organization,
- Inviting via SCIM,
- Accepting the invitation as the invited user (via
authenticator.changeUserWithSuppliedContext+acceptOrDeclineInvitation),- Switching back to the original admin context and updating the SCIM user,
matches the controller behavior and validates that updates work only after the invitation is accepted. Assertions on the final GET (name and
activeflag) look consistent with the SCIM controller implementation.
390-473: Patch‑after‑acceptance test is aligned with PATCH handler semanticsThis test correctly:
- Invites the existing user,
- Has the invited user accept the organization invitation,
- Patches
active: falseusing a SCIM PatchOp body,- Verifies via a subsequent GET that
activeisfalse.This mirrors the
PATCH /Users/:userIDhandler’sreplace/value.activebranch and provides good coverage for the new activation/deactivation path.controlplane/src/core/controllers/scim.ts (4)
294-303: Using organization‑levelactivein GET /Users/:userID is consistent with SCIM flowsSwitching to
active: user.activehere aligns this endpoint with your new activation/deactivation logic (PUT/PATCH). As long as the list endpoint is updated similarly, this will give SCIM clients a consistent view of per‑organization membership activity.
317-371: Good use of UserInviteService and audit logging in user creationThe create handler:
- Wraps the invitation flow in a DB transaction,
- Delegates user provisioning/invitation to
UserInviteService,- Records an audit log entry (
scim.organization_invitation_created) with relevant actor and API key metadata,- Returns a SCIM-compliant User representation including the newly created
id.This is a clean separation of concerns and a solid improvement over controller-level orchestration.
399-489: PUT /Users/:userID update path matches SCIM expectations and adds useful audit logsThe PUT handler:
- Validates org membership and the backing Keycloak user,
- Updates Keycloak user details (name + groups),
- Writes an audit log for the update,
- Conditionally updates the org member’s
activeflag and logs activation/deactivation separately,- Returns an updated SCIM User representation.
This matches the SCIM full‑replace semantics and cleanly ties into your new
activefield and audit infrastructure.
491-569: PATCH handler correctly supportsactivereplace operations, but only at org levelThe PATCH handler:
- Validates that the user is an org member,
- Ignores non‑
replaceoperations,- Handles both
path: 'active'with string value and object{ value: { active: boolean } }forms,- Updates
setOrganizationMemberActiveand batches corresponding audit logs.Two points to be aware of:
- It only toggles the organization member’s
activeflag; Keycloak’senabledstate is left unchanged. If that’s intentional (SCIMactive== org membership status, not IdP status), this is fine but should be documented.- Non‑
replaceoperations are silently ignored, which is permissible but may surprise clients expecting errors for unsupported ops.Functionally, this is a reasonable and focused implementation of SCIM PATCH for activation/deactivation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
controlplane/test/scim.test.ts (2)
211-288: Fix duplicate invitation test to use email-filtered assertionsLine 282 has the same fragile
totalCount === 1assertion. To verify that no duplicate invitation was created for the same email, filter the invitations by email:const pendingOrgMembers = await client.getPendingOrganizationMembers({}); expect(pendingOrgMembers.response?.code).toBe(EnumStatusCode.OK); -expect(pendingOrgMembers.totalCount).toBe(1); const emails = pendingOrgMembers.pendingInvitations.map((inv) => inv.email); -const exists = emails.includes(email); - -expect(exists).toBe(true); +const invitationsForEmail = emails.filter(e => e === email); +expect(invitationsForEmail).toHaveLength(1);This verifies that exactly one invitation exists for the test email, regardless of other pending invitations in the organization.
163-209: Fix pending invitation assertions to be email-filtered instead of relying on totalCountLine 203 asserts
totalCount === 1, which is fragile because tests share organization state without inter-test cleanup. If prior tests create pending invitations (e.g., the skipped test at line 116), this assertion will fail.You already filter by email at lines 205-208 to verify the specific invitation exists. Replace the totalCount assertion with a filtered check:
const pendingOrgMembers = await client.getPendingOrganizationMembers({}); expect(pendingOrgMembers.response?.code).toBe(EnumStatusCode.OK); -expect(pendingOrgMembers.totalCount).toBe(1); const emails = pendingOrgMembers.pendingInvitations.map((inv) => inv.email); -const exists = emails.includes(email); - -expect(exists).toBe(true); +expect(emails).toContain(email);This ensures the test verifies only its own invitation state.
🧹 Nitpick comments (2)
controlplane/src/core/services/UserInviteService.ts (2)
49-58: Advisory lock is enforced, but improve error messageThe advisory lock is properly enforced by throwing an error when
acquiredis false. However, the error message "Slow down" is not user-friendly or descriptive.Consider improving the error message:
if (!advisoryLockRows?.[0]?.acquired) { - // Another request already acquired the lock for this invitation - throw new PublicError(EnumStatusCode.ERR, 'Slow down'); + throw new PublicError( + EnumStatusCode.ERR, + 'An invitation for this user is already being processed. Please try again in a moment.' + ); }Additionally, consider adding a database UNIQUE constraint on
organization_invitations(organization_id, user_id)as a safety net against concurrent races if the advisory lock fails.
224-224: Consider injecting WEB_BASE_URL as configurationThe service directly accesses
process.env.WEB_BASE_URLat lines 224 and 238. Consider passing this as a configuration parameter in the constructor to improve testability and reduce coupling to environment variables:constructor(input: { db: PostgresJsDatabase<typeof schema>; logger: FastifyBaseLogger; keycloakRealm: string | undefined; keycloak: Keycloak; mailer: Mailer | undefined; webBaseUrl: string; // Add this }) { // ... this.webBaseUrl = input.webBaseUrl; }Then use
this.webBaseUrlinstead ofprocess.env.WEB_BASE_URL.Also applies to: 238-238
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controlplane/src/core/services/UserInviteService.ts(1 hunks)controlplane/test/scim.test.ts(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-22T11:13:45.617Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2224
File: router/pkg/authentication/keyfunc/keyfunc_test.go:142-154
Timestamp: 2025-09-22T11:13:45.617Z
Learning: When reviewing forked code, especially test files with test fixtures like JWT tokens, avoid suggesting modifications to maintain alignment with upstream and preserve the original author's structure. Test fixtures that are clearly marked as such (not real secrets) should generally be left unchanged in forked implementations.
Applied to files:
controlplane/test/scim.test.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/services/UserInviteService.ts
🧬 Code graph analysis (2)
controlplane/test/scim.test.ts (1)
controlplane/src/core/test-util.ts (1)
TestAuthenticator(223-226)
controlplane/src/core/services/UserInviteService.ts (6)
controlplane/src/core/services/Keycloak.ts (1)
Keycloak(8-442)controlplane/src/core/services/Mailer.ts (1)
Mailer(13-101)controlplane/src/core/repositories/OrganizationRepository.ts (3)
OrganizationRepository(50-1670)memberCount(341-358)memberships(259-339)controlplane/src/core/repositories/UserRepository.ts (1)
UserRepository(18-178)controlplane/src/core/repositories/OrganizationInvitationRepository.ts (1)
OrganizationInvitationRepository(12-276)controlplane/src/core/repositories/OrganizationGroupRepository.ts (1)
OrganizationGroupRepository(9-356)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
controlplane/test/scim.test.ts (4)
7-7: LGTM: TestAuthenticator wiringThe TestAuthenticator import and setup properly enable multi-user test flows for invitation acceptance scenarios.
Also applies to: 21-21, 39-39
116-161: Verify that skipped test doesn't pollute subsequent test stateThe skipped test would create a pending invitation if it ran. While it's appropriately skipped due to the studio client limitation, ensure that if this test is re-enabled in the future, it includes proper cleanup to avoid affecting subsequent assertions on
totalCount.
290-390: LGTM: Invitation acceptance and update flowThe test properly orchestrates the multi-user flow by switching authenticator contexts to accept the invitation before updating the user. The assertions correctly verify the update operations.
392-475: LGTM: Invitation acceptance and patch flowSimilar to the PUT test, this correctly verifies the PATCH operation after invitation acceptance using proper authenticator context switching.
controlplane/src/core/services/UserInviteService.ts (7)
15-34: LGTM: Clean dependency injectionThe constructor follows a clear dependency injection pattern with properly typed dependencies.
60-78: LGTM: Organization validation and limit enforcementThe organization lookup, member count checking, and feature limit enforcement are well-structured and provide clear error messages.
80-88: LGTM: Existing member checkThe check prevents attempting to invite users who are already organization members.
90-128: LGTM: Keycloak-DB synchronization with defensive checksThe flow properly synchronizes user state between Keycloak and the local database. Lines 120-127 are defensive checks that ensure the user exists after all creation logic, which adds safety at minimal cost.
130-149: LGTM: Pending invitation handlingThe logic correctly handles existing pending invitations by resending rather than creating duplicates.
151-172: LGTM: Group validationThe group validation ensures that all provided groups belong to the organization and throws a clear error if invalid groups are provided.
174-196: LGTM: Invitation creation and sendingThe flow cleanly separates invitation creation (DB operation) from notification sending (email/Keycloak).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
controlplane/test/scim.test.ts (1)
154-162: Remove fragile totalCount assertion; email-based check at lines 158-161 is sufficient.Line 156 asserts
totalCount === 1, which assumes this is the only pending invitation in the organization. This is fragile because:
- Test execution order can vary
- Other tests in the same organization can leave pending invitations
- The random email created at line 117 is never cleaned up, affecting subsequent tests if they check totalCount
Lines 158-161 already correctly verify that the specific email exists in
pendingInvitationswithout relying on the total count. This is the proper approach for test isolation.Apply this diff:
const pendingOrgMembers = await client.getPendingOrganizationMembers({}); expect(pendingOrgMembers.response?.code).toBe(EnumStatusCode.OK); -expect(pendingOrgMembers.totalCount).toBe(1); const emails = pendingOrgMembers.pendingInvitations.map((inv) => inv.email); const exists = emails.includes(email); expect(exists).toBe(true);
🧹 Nitpick comments (2)
controlplane/test/scim.test.ts (2)
118-119: Consider using mockResolvedValue for async Keycloak methods.
spy.mockImplementation(vi.fn())works but is unconventional. IfexecuteActionsEmailis async (returns a Promise), usemockResolvedValuefor clarity:-const spy = vi.spyOn(keycloakClient, 'executeActionsEmail'); -spy.mockImplementation(vi.fn()); +const spy = vi.spyOn(keycloakClient, 'executeActionsEmail').mockResolvedValue(undefined);This makes the intent explicit and avoids potential issues if the method expects a Promise return.
290-295: Strengthen assertion to verify no duplicate invitations.The test title states "does not create multiple invitations," but lines 293-295 only check that the email exists without verifying there's exactly one invitation for that email.
Apply this diff to explicitly verify idempotency:
const pendingOrgMembers = await client.getPendingOrganizationMembers({}); expect(pendingOrgMembers.response?.code).toBe(EnumStatusCode.OK); -const emails = pendingOrgMembers.pendingInvitations.map((inv) => inv.email); -const exists = emails.includes(email); -expect(exists).toBe(true); +const invitationsForEmail = pendingOrgMembers.pendingInvitations.filter(inv => inv.email === email); +expect(invitationsForEmail.length).toBe(1);This directly asserts that only one invitation exists for the email, clearly demonstrating idempotency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/test/scim.test.ts(14 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-22T11:13:45.617Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2224
File: router/pkg/authentication/keyfunc/keyfunc_test.go:142-154
Timestamp: 2025-09-22T11:13:45.617Z
Learning: When reviewing forked code, especially test files with test fixtures like JWT tokens, avoid suggesting modifications to maintain alignment with upstream and preserve the original author's structure. Test fixtures that are clearly marked as such (not real secrets) should generally be left unchanged in forked implementations.
Applied to files:
controlplane/test/scim.test.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/test/scim.test.ts
🧬 Code graph analysis (1)
controlplane/test/scim.test.ts (1)
controlplane/src/core/test-util.ts (1)
TestAuthenticator(223-226)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (3)
controlplane/test/scim.test.ts (3)
2-2: LGTM! Clean test infrastructure additions.The addition of
vifor spying andTestAuthenticatorfor user context switching appropriately supports the new invitation flow tests. The setup properly wires the authenticator from test configuration.Also applies to: 7-7, 21-21, 39-39
167-215: LGTM! Proper email-based assertion for test isolation.This test correctly verifies the invitation exists by filtering
pendingInvitationsby email (lines 212-214) rather than checkingtotalCount. The cleanup-at-start pattern (lines 171-175) with user context switching viaauthenticator.changeUserWithSuppliedContextproperly ensures a clean slate for the invitation flow test.
298-402: LGTM! Comprehensive invitation acceptance workflow coverage.Both tests properly verify the complete invitation-based membership flow:
- Clean up prior state by rejecting invitations and removing memberships
- Create invitation via SCIM
- Accept invitation from the invited user's context
- Perform updates/patches once membership is established
The user context switching via
authenticator.changeUserWithSuppliedContextcorrectly simulates multi-user workflows, and the assertions verify the expected state at each step.Also applies to: 404-491
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/src/core/controllers/scim.ts (1)
169-176:activeflag semantics are inconsistent between list and getYou’ve moved
GET /Users/:userIDto useuser.active(organization membership) as the SCIMactiveflag, andPATCH /Users/:userID/PUT /Users/:userIDnow updatesetOrganizationMemberActive, which is good.However, in
GET /Userstheactivefield is still derived fromkeycloakUsers[0].enabled ?? truefor each member, so:
GET /Usersmay returnactive: truefor a member whose org membership has been deactivated via SCIM.GET /Users/:userIDwill simultaneously returnactive: falsefor the same user.If
activeis intended to represent membership activity (as the update/patch paths suggest), consider switching list responses to use the same membershipactiveflag (assuminggetMembersexposes it) to keep SCIM views consistent.Also applies to: 210-219, 294-303, 453-473
🧹 Nitpick comments (1)
controlplane/src/core/services/UserInviteService.ts (1)
203-251: Invitation sending and throttling behavior is reasonable, consider logging when mailer is missingThe membership‑based branch (Keycloak action email for first‑time users vs. custom mail for users with memberships) and 30‑minute resend throttle are coherent. When
maileris undefined, the method returns early and never updateslastSentAt; this is acceptable but might be opaque operationally.Consider adding a debug/warn log before the early
returnso operators can see why invitations are not being emailed when a mailer is not configured.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controlplane/src/core/bufservices/user/inviteUser.ts(2 hunks)controlplane/src/core/controllers/scim.ts(8 hunks)controlplane/src/core/services/UserInviteService.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/services/UserInviteService.ts
🧬 Code graph analysis (3)
controlplane/src/core/services/UserInviteService.ts (8)
controlplane/src/core/services/Keycloak.ts (1)
Keycloak(8-442)controlplane/src/core/services/Mailer.ts (1)
Mailer(13-101)controlplane/src/core/repositories/OrganizationRepository.ts (3)
OrganizationRepository(50-1670)memberCount(341-358)memberships(259-339)controlplane/src/core/repositories/UserRepository.ts (1)
UserRepository(18-178)controlplane/src/core/repositories/OrganizationInvitationRepository.ts (1)
OrganizationInvitationRepository(12-276)controlplane/src/core/errors/errors.ts (1)
PublicError(17-17)controlplane/src/types/index.ts (2)
UserDTO(284-287)OrganizationInvitationDTO(313-319)controlplane/src/core/repositories/OrganizationGroupRepository.ts (1)
OrganizationGroupRepository(9-356)
controlplane/src/core/bufservices/user/inviteUser.ts (1)
controlplane/src/core/services/UserInviteService.ts (1)
UserInviteService(15-252)
controlplane/src/core/controllers/scim.ts (5)
controlplane/src/core/services/Mailer.ts (1)
Mailer(13-101)controlplane/src/core/services/UserInviteService.ts (1)
UserInviteService(15-252)controlplane/src/core/repositories/AuditLogRepository.ts (2)
AuditLogRepository(28-137)AddAuditLogInput(6-23)controlplane/src/core/errors/errors.ts (1)
isPublicError(33-35)controlplane/src/db/schema.ts (1)
auditLogs(1926-1962)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
controlplane/src/core/bufservices/user/inviteUser.ts (1)
27-40: Delegation toUserInviteServicelooks correctService construction and usage (including awaiting
service.inviteUserand passinggroups/organizationSlugfromauthContextare consistent with the new service-based flow and audit model. No issues from this wrapper.Also applies to: 42-45
controlplane/src/core/services/UserInviteService.ts (1)
94-132: User + Keycloak existence flow is solidThe logic to (a) ensure the Keycloak user exists, (b) mirror the user into the local
userstable, and (c) re‑fetch to guarantee presence before proceeding is sound and transactionally safe with the current repository patterns.Based on learnings
controlplane/src/core/controllers/scim.ts (3)
27-80: SCIM request typing and content-type parser are appropriateTyped
FastifyRequestaliases for list/get/create/update/patch and the dedicatedapplication/scim+jsonparser (parsing as string thenJSON.parse) match SCIM usage and keep handlers nicely typed. No issues here.
396-487: User update flow (PUT /Users/:userID) is coherent with new audit modelThe update route correctly:
- Ensures the user belongs to the organization and exists in Keycloak.
- Updates basic user details and groups in Keycloak.
- Writes an audit log for the general update, and a second audit entry when
activechanges, tied to membership viasetOrganizationMemberActive.The structure and audit logging match the expected patterns in this codebase.
Based on learnings
489-567: SCIMPATCH /Users/:userIDlogic is clear and well-scopedThe patch handler:
- Validates the organization member exists.
- Restricts behavior to
replaceoperations onactive(either aspath: "active"with string value or in a value object).- Updates membership via
setOrganizationMemberActiveand batches audit log entries with a shared partial payload.This is a good, minimal SCIM PATCH implementation focused on activation/deactivation semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/src/core/controllers/scim.ts (1)
162-260: Inconsistentactivestatus source between list and get endpoints.The
GET /Users(list) endpoint reads theactivestatus from Keycloak'senabledfield (lines 214, 244), whileGET /Users/:userIDreturns the databaseuser.activefield (line 302). This inconsistency could cause the same user to show different active states in list vs. detail views.Consider using a consistent source—either always read from the database
user.activefield (recommended, as it's what PUT/PATCH update), or always from Keycloak'senabledfield.Apply this diff to make the list endpoint consistent with the detail endpoint:
users.push({ id: user.userID, emails: [{ value: user.email }], userName: user.email, - active: keycloakUsers[0].enabled ?? true, + active: user.active, name: { givenName: keycloakUsers[0].firstName || '', familyName: keycloakUsers[0].lastName || '',and apply the same change around line 244:
users.push({ id: member.userID, emails: [{ value: member.email }], userName: member.email, - active: keycloakUsers[0].enabled ?? true, + active: member.active, name: { givenName: keycloakUsers[0].firstName || '', familyName: keycloakUsers[0].lastName || '',
♻️ Duplicate comments (1)
controlplane/src/core/controllers/scim.ts (1)
370-386: Refine error status mapping for better SCIM compliance.The error handling still maps most
PublicErrorcases to HTTP 400 with SCIMstatus500, while onlyERR_ALREADY_EXISTSgets status 409. This loses useful semantics for SCIM clients and creates a mismatch between the HTTP code (400) and SCIMstatus(500).Consider mapping
EnumStatusCodevalues more explicitly:
ERR_ALREADY_EXISTS→ HTTP 409 / SCIMstatus: 409ERR_LIMIT_REACHED→ HTTP 429 / SCIMstatus: 429ERR_NOT_FOUND→ HTTP 404 / SCIMstatus: 404ERR(throttle "Slow down") → HTTP 429 / SCIMstatus: 429- Fallback → HTTP 500 / SCIM
status: 500Ensure the outer HTTP status always matches the SCIM
statusfield.Apply this diff to improve error mapping:
} catch (err: unknown) { if (isPublicError(err)) { + let httpStatus = 500; + let scimStatus = 500; + + if (err.code === EnumStatusCode.ERR_ALREADY_EXISTS) { + httpStatus = 409; + scimStatus = 409; + } else if (err.code === EnumStatusCode.ERR_LIMIT_REACHED) { + httpStatus = 429; + scimStatus = 429; + } else if (err.code === EnumStatusCode.ERR_NOT_FOUND) { + httpStatus = 404; + scimStatus = 404; + } else if (err.message === 'Slow down') { + httpStatus = 429; + scimStatus = 429; + } + - return res.code(400).send( + return res.code(httpStatus).send( ScimError({ detail: err.message, - status: err.code === EnumStatusCode.ERR_ALREADY_EXISTS ? 409 : 500, + status: scimStatus, }), );
🧹 Nitpick comments (1)
controlplane/src/core/controllers/scim.ts (1)
530-544: Consider more robust string-to-boolean conversion.Line 532 uses
operation.value?.toLowerCase() === 'true'to parse the active flag. This only recognizes the literal string "true" (case-insensitive). While this may match the SCIM spec, it's brittle if clients send variations like"1","yes", or if the value is accidentally sent as a boolean that gets stringified.Consider a more defensive approach:
if ('path' in operation) { if (operation.path.toLowerCase() === 'active') { - const active = operation.value?.toLowerCase() === 'true'; + const active = operation.value?.toLowerCase() === 'true' || operation.value === 'True'; await opts.organizationRepository.setOrganizationMemberActive({ id: orgMember.orgMemberID, organizationId: authContext.organizationId, active, });Alternatively, if you want to strictly enforce SCIM compliance, consider adding validation that rejects non-compliant values with a descriptive error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/controllers/scim.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controlplane/src/core/controllers/scim.ts (5)
controlplane/src/core/services/Mailer.ts (1)
Mailer(13-101)controlplane/src/core/services/UserInviteService.ts (1)
UserInviteService(15-252)controlplane/src/core/repositories/AuditLogRepository.ts (2)
AuditLogRepository(28-137)AddAuditLogInput(6-23)controlplane/src/core/errors/errors.ts (1)
isPublicError(33-35)controlplane/src/db/schema.ts (1)
auditLogs(1926-1962)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
🔇 Additional comments (4)
controlplane/src/core/controllers/scim.ts (4)
1-80: LGTM!The new imports and type definitions are well-structured. The optional
mailerparameter inScimControllerOptionscorrectly aligns with theUserInviteServicedesign, and the internal request types improve type safety throughout the endpoints.
98-105: LGTM!The updated content type parser signature correctly ignores the unused request parameter. The parser only needs the body to parse JSON, so this is a valid simplification.
397-487: LGTM!The PUT endpoint implementation is well-structured:
- Updates Keycloak user details appropriately
- Comprehensive audit logging for both general updates and active status changes
- Conditional check (line 454) prevents redundant database updates when active status hasn't changed
- Returns proper SCIM-formatted response
561-564: LGTM!The batch audit logging pattern is efficient and correct. Collecting audit log entries and writing them in a single call reduces database round trips, and the conditional check prevents unnecessary operations.
2 participants
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist