Skip to content

fix(scim): dedupe users by userName (lowercased) and return scimType=uniqueness on 409#4067

Draft
Hedwig7 wants to merge 3 commits intoory:mainfrom
sharecal-io:v26.2.0-fix/scim-username-dedup-and-scimtype
Draft

fix(scim): dedupe users by userName (lowercased) and return scimType=uniqueness on 409#4067
Hedwig7 wants to merge 3 commits intoory:mainfrom
sharecal-io:v26.2.0-fix/scim-username-dedup-and-scimtype

Conversation

@Hedwig7
Copy link
Copy Markdown

@Hedwig7 Hedwig7 commented May 1, 2026

Summary

DirectoryUsers.create() deduplicates POST /Users on userAttributes.email, and Users indexes/searches records by raw email. Per RFC 7643 §4.1.1, this is the wrong attribute: emails[].value carries uniqueness=none; the canonical unique attribute on User is userName (uniqueness=server). Using a non-unique attribute as the unique key produces false 409 conflicts. The 409 response also omits scimType, which RFC 7644 §3.12 requires for uniqueness conflicts.

Changes:

  • Users.ts: Index by (raw.userName ?? email).toLowerCase() instead of email; lowercase the search input
  • DirectoryUsers.ts: Dedupe on body.userName instead of userAttributes.email; return scimType: "uniqueness" on 409

What does this PR do?

Fixes two SCIM 2.0 spec deviations in DirectoryUsers.create() and Users that produce false 409 conflicts and infinite IdP retry loops.

Problem Details

Per RFC 7643 §4.1.1:

Attribute uniqueness caseExact
userName server false
emails[].value none n/a

Polis was indexing User records by email and deduping POSTs on userAttributes.email. Using emails[].value (uniqueness=none) as the unique index produces false 409 conflicts whenever userName and emails[].value diverge — which is spec-compliant IdP behavior, not an edge case:

  • userName can change independently of email (renames, login changes).
  • userName is caseExact=false; IdPs may ship the same logical userName in different casing across requests.
  • emails[].value is not required to track changes to userName, and the spec doesn't promise it will.

Additionally, RFC 7644 §3.3 and §3.12 require scimType="uniqueness" on 409 responses for uniqueness conflicts so the IdP can switch to PATCH on the existing resource. The current implementation omits this field. IdPs that depend on it (Microsoft Entra in particular) retry POST indefinitely on legitimate already-exists conditions.

Real-World Impact

A POST /Users with a renamed userName but the same emails[0].value is rejected as a duplicate against the prior record:

Stored: userName=alice@example.com, emails[0].value=alice@example.com

Request: POST /Users with userName=alice2@example.com, emails[0].value=alice@example.com

Current (incorrect) response: HTTP 409

{
  "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"],
  "detail": "User already exists"
}

The IdP can't interpret the bare 409 (no scimType) and retries POST indefinitely. The legitimately-renamed user is permanently locked out.

Expected response (after this fix): HTTP 201 Created — the lookup is now by userName=alice2@example.com, finds no match, and creates the new resource. When a true duplicate is detected, the 409 includes scimType="uniqueness" so the IdP knows to switch to PATCH.

Solution

Users.ts (create):

// Before
const { directoryId, id, email } = user;
// ...
value: keyFromParts(directoryId, email),

// After
const { directoryId, id } = user;
// Index by userName (RFC 7643: uniqueness=server, caseExact=false) instead of email.
const indexValue = (user.raw?.userName ?? user.email).toLowerCase();
// ...
value: keyFromParts(directoryId, indexValue),

Users.ts (search):

// Before
value: keyFromParts(directoryId, userName),

// After
// Lowercase to match the index built in create() (RFC 7643: caseExact=false).
value: keyFromParts(directoryId, userName.toLowerCase()),

DirectoryUsers.ts (create):

// Before
const { data: users } = await this.users.search(userAttributes.email, directory.id);
if (users && users.length > 0) {
  return this.respondWithError({ code: 409, message: 'User already exists' });
}

// After
const { userName } = body as { userName: string };
// Dedupe by userName (uniqueness=server) instead of email (uniqueness=none).
const { data: users } = await this.users.search(userName, directory.id);
if (users && users.length > 0) {
  // RFC 7644 §3.12: scimType=uniqueness so the IdP retries as PATCH.
  return this.respondWithError({
    code: 409,
    message: 'User already exists',
    scimType: 'uniqueness',
  });
}

respondWithError signature is widened to accept an optional scimType passthrough.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

  1. Test Case 1 — Rename with shared email:

    Pre-populate: userName=alice@example.com, emails[0].value=alice@example.com
    POST /Users with userName=alice2@example.com, emails[0].value=alice@example.com
    Expected: 201 Created (no false 409)
    
  2. Test Case 2 — Same userName, different casing:

    Pre-populate: userName=Alice@example.com
    POST /Users with userName=alice@example.com (different case)
    Expected: 409 with scimType="uniqueness" (case-insensitive match per caseExact=false)
    
  3. Test Case 3 — Bona fide duplicate:

    POST /Users with userName=alice@example.com (twice)
    Expected: First → 201; second → 409 with scimType="uniqueness"
    
  • Existing unit tests (no changes needed; behavior change requires new tests)
  • e2e tests in e2e/api/scim/v2.0/users.spec.ts — happy to add if maintainers prefer

Migration / compatibility

Existing records stay keyed under their original casing; this change does not migrate the store. The next re-provision under the same canonical userName produces one transitional record under the new lowercased index, after which the record is stable. Operators wanting to clean up orphan records can do so via a one-off script — out of scope for this PR.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code and corrected any misspellings
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective — happy to add e2e tests if maintainers prefer
  • Existing unit tests pass locally with my changes

Additional Notes

  • This is a SCIM 2.0 spec compliance fix. The fix changes which attribute Polis uses for the server-side uniqueness check, aligning with RFC 7643's specification that userName is the canonical unique attribute on User.
  • The case-sensitivity portion of this same root cause was reported in Cannot push a new group with the old name of a renamed group #3559 for Groups. The Groups fix is mechanically equivalent but kept out of scope here for review tractability — happy to follow up with a Groups PR after this lands.

Hedwig7 added 2 commits April 30, 2026 16:14
Per RFC 7643 §4.1.1, the SCIM userName attribute carries uniqueness=server and caseExact=false; emails[].value carries uniqueness=none. Users were indexed and searched by email, which is the wrong attribute: using a uniqueness=none field as the unique key produces false 409 conflicts whenever userName and emails[].value diverge. That is spec-compliant IdP behavior, not an edge case — userName can change independently of email (rename), and IdPs may ship userName in different casing across requests (caseExact=false).

Index on (raw.userName ?? email).toLowerCase() in Users.create() and lowercase the lookup in Users.search(). The lowercase normalization is required by caseExact=false on userName.

Existing records keyed under the legacy raw casing remain in the store and are not migrated by this change; subsequent re-provisions produce one transitional record under the new lowercased index, after which the record is stable.
…ess on 409

Two SCIM-spec deviations in DirectoryUsers.create() / respondWithError:

1) DirectoryUsers.create() searches the users store by userAttributes.email (= emails[0].value || userName). Per RFC 7643 §4.1.1 the canonical unique attribute on User is userName (uniqueness=server); emails[].value is uniqueness=none. The fix pivots dedupe to body.userName so the server-side uniqueness check matches the spec-defined unique attribute.

2) The 409 response body did not include scimType. RFC 7644 §3.3 / §3.12 require scimType="uniqueness" on uniqueness conflicts so the IdP can recognize the conflict and switch to PATCH on the existing resource. Without it, IdPs that loop on bare 409s (Microsoft Entra in particular) retry POST indefinitely.

Dedupe on body.userName (which Users.search now lowercases internally) and add scimType="uniqueness" to the 409 body. respondWithError signature is widened to accept an optional scimType passthrough.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 1, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 28f87b87-1948-4cf8-bd57-0441846f66e2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…p tests

- Users.update() now passes directoryIdUsername and directoryId indexes
  to put(), preventing stale index entries when userName changes via
  PUT or PATCH. Previously only create() maintained indexes.

- DirectoryUsers.create() validates body.userName presence and returns
  400 instead of crashing with a TypeError on undefined.

- Changed ?? to || for userName fallback so empty-string userNames
  don't collapse into a single index bucket.

- Added 6 deterministic tests for the userName-based dedup contract:
  scimType=uniqueness on 409, same-userName-different-email 409,
  different-userName-same-email 201 (LEV-956 case), case-insensitive
  match, PUT-rename-then-POST stale-index fix, missing-userName 400.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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