Skip to content

fix: return externalId in SCIM Groups responses#69

Merged
TerrifiedBug merged 1 commit intomainfrom
fix/scim-group-externalid
Mar 8, 2026
Merged

fix: return externalId in SCIM Groups responses#69
TerrifiedBug merged 1 commit intomainfrom
fix/scim-group-externalid

Conversation

@TerrifiedBug
Copy link
Copy Markdown
Owner

Summary

  • SCIM Groups endpoints (/Groups and /Groups/{id}) never included externalId in responses. SCIM clients (pocket-id, Entra, Okta) match remote resources by externalId during sync — without it, every group appears as an orphan (DELETE) and then as new (POST), causing a full teardown/rebuild on every background sync cycle
  • This destroyed group_mapping team memberships and flooded audit logs with group_deleted / group_adopted / user_updated entries on every sync
  • Also skips writing scim.group_adopted audit entries when POST re-encounters an existing group with no actual changes

Root cause

Pocket-id's SCIM client does proper incremental sync: GET /Groups → match by externalId → PUT/POST/DELETE as needed. Our Users endpoint returned externalId correctly, but Groups never did. This caused pocket-id to see all remote groups as unmatched orphans every sync cycle.

Test plan

  • Deploy and trigger SCIM sync from pocket-id
  • Verify groups are matched by externalId (no DELETE→POST cycle in logs)
  • Verify team memberships are preserved across sync cycles
  • Verify audit logs are not flooded with no-op entries
  • Verify legitimate group deletion still reconciles team memberships

SCIM Groups endpoints never included externalId in responses. SCIM clients
like pocket-id match remote resources by externalId during sync — without
it, every group appears as an orphan (→ DELETE) and then as new (→ POST),
causing a full teardown/rebuild cycle on every background sync. This
destroyed team memberships and flooded audit logs.

Also skip writing scim.group_adopted audit entries when POST re-encounters
an existing group with no actual changes, reducing noise during normal
sync cycles.
@github-actions github-actions bot added the fix label Mar 8, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 8, 2026

Greptile Summary

This PR fixes a root-cause SCIM sync issue where the /Groups and /Groups/{id} endpoints never returned externalId in their responses, causing SCIM clients (pocket-id, Entra, Okta) to treat every group as an unmatched orphan on each background sync cycle (DELETE → POST churn). It also tightens audit logging in the POST handler so scim.group_adopted is only emitted when the externalId actually changes, and no entry is written at all for fully no-op re-encounters.

Key changes:

  • toScimGroupResponse in both route files now conditionally spreads externalId when present, fixing the client-side matching failure
  • POST /Groups audit logic is refactored: the action variable ("scim.group_created" | "scim.group_adopted" | null) replaces the old boolean adopted/isNew pair — HTTP 201 vs 200 selection is correctly preserved
  • No-op POST re-encounters (existing group, same or absent externalId) now silently return 200 without writing an audit entry, stopping audit log flooding
  • externalId is already included by Prisma as a scalar field in all findMany/findUnique queries (no select exclusion), so no query changes are needed

Confidence Score: 5/5

  • This PR is safe to merge — the changes are targeted, correct, and address a well-diagnosed root cause.
  • The diff is small and focused. The externalId field is a Prisma scalar returned by default on all existing queries, so no query changes are needed. The HTTP status code logic (201 vs 200) is correctly preserved via the new auditAction variable. The audit suppression for no-op syncs is intentional and well-reasoned. No security or data integrity concerns are introduced.
  • No files require special attention.

Important Files Changed

Filename Overview
src/app/api/scim/v2/Groups/route.ts Adds externalId to GET list and POST Group responses; refines POST audit logic to only fire scim.group_adopted when externalId actually changes, and skips the audit entry entirely for no-op re-encounters. HTTP status code (201 vs 200) is now derived from auditAction rather than the old isNew flag — correctly preserves the same behavior.
src/app/api/scim/v2/Groups/[id]/route.ts Adds externalId to GET, PUT, and PATCH Group responses by widening the toScimGroupResponse parameter type. Minor cosmetic removal of a comment about crash safety on the DELETE transaction. No logic changes to the underlying operations.

Sequence Diagram

sequenceDiagram
    participant Client as SCIM Client (pocket-id)
    participant API as /Groups API
    participant DB as PostgreSQL

    Note over Client,DB: Before fix — every sync cycle
    Client->>API: GET /Groups
    API->>DB: SELECT scimGroups
    DB-->>API: groups (no externalId)
    API-->>Client: Resources[] — externalId missing
    Client->>API: DELETE /Groups/{id} (orphan)
    Client->>API: POST /Groups (re-create)

    Note over Client,DB: After fix — incremental sync
    Client->>API: GET /Groups
    API->>DB: SELECT scimGroups
    DB-->>API: groups (with externalId)
    API-->>Client: Resources[] — externalId present

    alt Group matched by externalId
        Client->>API: PUT /Groups/{id} (update only if changed)
        API-->>Client: 200 OK + externalId in response
    else Truly new group
        Client->>API: POST /Groups
        API->>DB: INSERT scimGroup
        API-->>Client: 201 Created + externalId
    else Re-encounter existing group, no change
        Client->>API: POST /Groups
        API->>DB: findUnique (exists, externalId same)
        API-->>Client: 200 OK (no audit log written)
    end
Loading

Last reviewed commit: 7e55de3

@TerrifiedBug TerrifiedBug merged commit 9819053 into main Mar 8, 2026
12 checks passed
@TerrifiedBug TerrifiedBug deleted the fix/scim-group-externalid branch March 8, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant