Skip to content

feat: SCIM reconciliation redesign#64

Merged
TerrifiedBug merged 22 commits intomainfrom
feat/unified-group-mapping
Mar 8, 2026
Merged

feat: SCIM reconciliation redesign#64
TerrifiedBug merged 22 commits intomainfrom
feat/unified-group-mapping

Conversation

@TerrifiedBug
Copy link
Copy Markdown
Owner

Summary

Replaces the event-driven, additive-only SCIM group membership system with a state-reconciliation model that correctly handles adds, removes, multi-group overlap, group deletion, and role changes.

  • ScimGroupMember table — tracks "IdP says user X is in group Y" (provenance)
  • TeamMember.source field — distinguishes manual (admin-assigned) from group_mapping (automation-managed)
  • Single reconciliation functionreconcileUserTeamMemberships() computes desired state from group names + mapping config, diffs against current, applies adds/updates/removes
  • SCIM endpoints rewritten — all mutations go through ScimGroupMember + reconciliation (PATCH remove now works, PUT does full sync, DELETE cascades)
  • OIDC login updated — two modes: OIDC-only (token groups) and SCIM+OIDC (union of ScimGroupMember + token groups). Both add AND remove stale memberships
  • Bulk reconciliation — when admin saves group mappings in SCIM mode, all SCIM-managed users are reconciled immediately
  • Manual assignments immutable — automation never touches source: "manual" TeamMembers

What was broken before

  • PATCH remove member was a silent no-op
  • PUT full-sync was additive-only (couldn't remove members)
  • DELETE group didn't cascade to team memberships
  • OIDC login could add but never remove stale memberships
  • Multiple groups mapping to the same team caused incorrect behavior

Design doc

docs/plans/2026-03-08-scim-reconciliation-design.md

Test plan

  • SCIM sync from IdP creates ScimGroupMember records and maps users to teams via reconciliation
  • OIDC login maps user to correct team based on token groups
  • Removing user from IdP group + SCIM sync removes team membership
  • Deleting SCIM group cascades and removes team memberships
  • Manual team assignments are preserved through SCIM/OIDC sync
  • Changing group mappings in settings immediately reconciles SCIM-managed users
  • Run database migration (20260308050000_add_scim_group_member_and_source)

- POST: return updated adopted record instead of stale pre-update object
- PUT: change from destructive full-sync to additive-only member sync
  to prevent cross-group deprovisioning in multi-group scenarios
- DELETE: remove team member cascade that would wipe all members from
  mapped teams regardless of membership provenance
- PATCH member remove is now a no-op: without membership provenance
  tracking, removing would silently revoke access granted by other
  groups, OIDC, or manual assignment (same safeguard as DELETE/PUT)
- Remove removeMappedMemberships helper (no longer used)
- Process displayName rename before member ops and re-resolve mappings,
  preventing stale mapping context when rename + member ops are batched
…rships

- PUT handler now re-resolves groupMappings after displayName rename,
  matching the fix already applied to PATCH
- applyMappedMemberships only upgrades roles, never downgrades — prevents
  a lower-role group sync from overwriting a higher role granted by
  another group, OIDC, or manual assignment
Introduces provenance tracking for SCIM group memberships.
ScimGroupMember tracks which IdP groups each user belongs to.
TeamMember.source distinguishes manual from group_mapping assignments.
Single reconcileUserTeamMemberships() function replaces all direct
TeamMember manipulation. Computes desired state from group names +
mappings, diffs against current source=group_mapping members, and
applies adds/updates/removes. Manual assignments are never touched.
…ation

POST now creates ScimGroupMember records for each member and calls
reconcileUserTeamMemberships instead of directly creating TeamMembers.
GET returns actual ScimGroupMember data in the members array.
…ciliation

PATCH add/remove members now creates/deletes ScimGroupMembers and
reconciles. PUT does full member sync (adds missing, removes absent).
DELETE cascades ScimGroupMembers and reconciles affected users.
displayName rename reconciles all group members.
In SCIM mode, reconciles all users with ScimGroupMember records
when admin updates group mappings. Changes take effect immediately.
In OIDC-only mode, changes take effect on next login (no bulk data).
OIDC-only mode: uses token groups directly, removes stale memberships.
SCIM+OIDC mode: uses union of ScimGroupMember + token groups.
OIDC login never writes to ScimGroupMember (avoids Azure AD token limit).
Default team fallback preserved for users with no group matches.
Document two-mode behavior (OIDC-only vs SCIM+OIDC), reconciliation
semantics, manual assignment immutability, and corrected SCIM Group
lifecycle (remove, full sync, delete cascade).
Group create/adopt and member processing now share one transaction.
Previously, the group would commit independently, so if member
processing failed the IdP would get a 4xx despite the group being
committed — SCIM clients treat 4xx as permanent and won't retry.
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 8, 2026

Greptile Summary

This PR replaces the additive-only, event-driven SCIM group membership system with a full state-reconciliation model. It introduces ScimGroupMember as a provenance table, adds a source field to TeamMember to protect manual assignments, and funnels all SCIM endpoints and OIDC login through a single reconcileUserTeamMemberships function that correctly computes desired state and diffs against current state.

The overall design is solid and the migration is safe (existing rows default to source: 'manual'). Two issues were found:

  • PATCH remove filter regex is case-sensitive — RFC 7644 requires filter operators to be case-insensitive. The regex /^members\[value eq "([^"]+)"\]$/ in Groups/[id]/route.ts has no /i flag, so an IdP sending EQ instead of eq would silently drop the remove operation — the very bug this PR is intended to fix.
  • Default team fallback race condition in auth.ts — the findFirst check and subsequent teamMember.create are not atomic. Concurrent OIDC logins for the same user can both see no membership and both attempt create, causing a unique constraint error that fails one login.

Confidence Score: 3/5

  • Safe to merge after fixing the two correctness issues: case-sensitive PATCH remove regex and the default team fallback race condition.
  • The core reconciliation design is correct and the migration is safe. Two concrete bugs were verified: (1) the PATCH remove filter regex is case-sensitive, which violates RFC 7644 and could silently drop removes from some IdPs—directly undermining the PR's stated goal of fixing PATCH remove; (2) the default team fallback uses a non-atomic read-check-create pattern that can fail under concurrent OIDC logins. Both are real correctness issues that should be addressed before merge.
  • src/app/api/scim/v2/Groups/[id]/route.ts (case-sensitive filter regex), src/auth.ts (default team fallback race)

Sequence Diagram

sequenceDiagram
    participant IdP as Identity Provider
    participant SCIM as SCIM Endpoint
    participant SGM as ScimGroupMember
    participant Reconcile as reconcileUserTeamMemberships
    participant TM as TeamMember

    IdP->>SCIM: PATCH /Groups/:id (add/remove/rename)
    SCIM->>SGM: upsert / deleteMany ScimGroupMember
    SCIM->>SGM: getScimGroupNamesForUser(userId)
    SGM-->>SCIM: [groupName1, groupName2, ...]
    SCIM->>Reconcile: reconcileUserTeamMemberships(tx, userId, groupNames)
    Reconcile->>Reconcile: loadGroupMappings()
    Reconcile->>TM: findMany source="group_mapping"
    Reconcile->>TM: create / update / delete (diff desired vs current)
    Note over Reconcile,TM: Never touches source="manual" records

    IdP->>SCIM: OIDC Login (token with groups claim)
    SCIM->>SGM: findMany ScimGroupMember (SCIM+OIDC mode)
    SCIM->>Reconcile: reconcileUserTeamMemberships(tx, userId, union(scim+token))
    Reconcile->>TM: diff and apply changes
Loading

Comments Outside Diff (2)

  1. src/app/api/scim/v2/Groups/[id]/route.ts, line 127-128 (link)

    RFC 7644 Section 3.4.2 requires filter operators to be case-insensitive. This regex will not match members[value EQ "userId"] (uppercase EQ), causing the remove to silently fall through to the second branch. Add the /i flag:

  2. src/auth.ts, line 280-295 (link)

    The findFirst check and the subsequent create are not atomic. If two OIDC logins for the same user occur concurrently (e.g., two browser tabs opening simultaneously), both threads can observe hasMembership = null and both attempt to create a TeamMember with the same userId + teamId pair. The second insert will hit the unique constraint and throw an error, failing that login session.

    Use upsert to make this atomic:

    if (settings.oidcDefaultTeamId) {
      const defaultRole = settings.oidcDefaultRole ?? "VIEWER";
      await prisma.teamMember.upsert({
        where: { userId_teamId: { userId: dbUser.id, teamId: settings.oidcDefaultTeamId } },
        create: { userId: dbUser.id, teamId: settings.oidcDefaultTeamId, role: defaultRole, source: "group_mapping" },
        update: {},
      });
    }

Last reviewed commit: 54d454d

PATCH remove: handle both RFC 7644 forms — filter notation
(members[value eq "userId"]) used by Okta/Azure and array-value form.
Without this, single-member removals via filter notation were a no-op.

Default team: check if user has any memberships after reconciliation,
not whether they have groups. A user with unmatched groups should
still get the default team.
@TerrifiedBug
Copy link
Copy Markdown
Owner Author

@greptile re-review.

  1. PATCH remove filter notation — Added handling for members[value eq "userId"] (Okta/Azure
    format) alongside the array-value form. Without this, single-member removals from those IdPs
    would silently no-op.
  2. Default team fallback — Changed from userGroupNames.length === 0 to just checking if the
    user has any memberships after reconciliation. A user with groups like ["contractors"] that
    don't match any mapping should still get the default team.

…pping

# Conflicts:
#	docs/public/operations/authentication.md
#	prisma/schema.prisma
#	src/app/api/scim/v2/Groups/[id]/route.ts
#	src/app/api/scim/v2/Groups/route.ts
#	src/server/services/group-mappings.ts
…ation

The withAudit middleware fires after the mutation completes, but by then
the user record is already deleted — causing AuditLog_userId_fkey FK
violations. Instead, write the audit log manually before the deletion
transaction with userId: null, capturing the deleted user's identity in
metadata fields.
Wrap scimGroup.delete and user reconciliation in a single transaction
so a crash between them can't leave stale group_mapping TeamMembers.
@TerrifiedBug
Copy link
Copy Markdown
Owner Author

@greptile re-review.

Finding 1: DELETE endpoint not atomic — Valid, fixed. Wrapped scimGroup.delete and user
reconciliation in a single $transaction so a crash can't leave phantom team access.

Finding 2: loadGroupMappings() reads outside transaction — Not a real bug, skipped. Greptile
acknowledges this themselves: "there is no practical bug today." All callers commit settings
changes before calling reconciliation. Threading a tx parameter through loadGroupMappings
would add complexity for zero practical benefit. This is a theoretical concern, not a
correctness issue.

…llback

Add /i flag to PATCH remove filter regex per RFC 7644 case-insensitive
operator requirement. Use upsert for default team assignment to handle
concurrent OIDC logins gracefully.
@TerrifiedBug TerrifiedBug merged commit e880c08 into main Mar 8, 2026
3 checks passed
@TerrifiedBug TerrifiedBug deleted the feat/unified-group-mapping branch March 8, 2026 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant