Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

feat: Add SCIM V2#2309

Open
Bewinxed wants to merge 101 commits intomaster from
bewinxed/add-scim-v2
Open

feat: Add SCIM V2 #2309
Bewinxed wants to merge 101 commits intomaster from
bewinxed/add-scim-v2

Conversation

@Bewinxed
Copy link
Collaborator

@Bewinxed Bewinxed commented Dec 31, 2025 •
edited
Loading

What kind of change does this PR introduce?

Feature - adds SCIM v2 provisioning support for enterprise SSO providers

This is a complete, general implementation inspired by the needs of this PR #2115

What is the current behavior?

Currently there's no way for identity providers (Okta, Azure AD, OneLogin, etc.) to automatically provision and deprovision users. Admins have to manually manage user accounts when employees join or leave, which is error-prone and doesn't scale for enterprise customers.

What is the new behavior?

Adds full SCIM v2 (RFC 7644) support, allowing identity providers to:

  • User provisioning: Automatically create users when added to the IdP
  • User updates: Sync profile changes (name, email) from IdP
  • User deprovisioning: Soft-delete users via ban when removed from IdP (preserves data, terminates sessions)
  • Group management: Create/update/delete groups and manage group membership

Endpoints added:

  • GET/POST /scim/v2/Users - list and create users
  • GET/PUT/PATCH/DELETE /scim/v2/Users/{id} - manage individual users
  • GET/POST /scim/v2/Groups - list and create groups
  • GET/PUT/PATCH/DELETE /scim/v2/Groups/{id} - manage individual groups
  • Discovery endpoints: /scim/v2/ServiceProviderConfig, /scim/v2/Schemas, /scim/v2/ResourceTypes

Authentication: Bearer token per SSO provider (stored as SHA-256 hash). Tokens are 160 bits generated from crypto/rand with a scim_ prefix.

Filtering: Full RFC 7644 filter support using the scim2/filter-parser library - supports eq, ne, co, sw, ew, pr, gt, ge, lt, le operators with and/or/not logic.

IdP compatibility: Tested with Azure AD quirks (booleans as strings, case-insensitive displayName uniqueness).

SCIM routes skip isValidExternalHost middleware

SCIM routes intentionally do not go through the isValidExternalHost middleware. This is by design because:

  • SCIM is a server-to-server API (IdP → Auth), not a user-facing browser redirect flow
  • SCIM does not generate email redirect URLs, which is the purpose of isValidExternalHost (validating redirect destinations)
  • SCIM uses its own bearer token authentication scoped to a specific SSO provider, which is separate from the JWT-based auth used by other endpoints

Additional context

I tried my best to make the implementation fit within the current tenant/user model instead of new tables for everything, adding schema changes only when necessary.

Some compliance work might be needed for other nuances with other SCIM providers (I've tested Microsoft Azure).

Deviations from RFC 7643

Some deviations from the RFC for SCIM v2 were done that relates to Supabase Auth:

  • Email is REQUIRED, as required by our auth model.
  • Deprovisioning uses soft-delete via Ban with reason (the RFC leaves this implementation to the implementer).
  • Bulk operations not supported, yet in the initial implementation.

New dependencies

  • github.com/scim2/filter-parser/v2 - RFC 7644 SCIM filter parsing

Schema Changes

  • sso_providers gets 2 new columns: scim_enabled (boolean), scim_bearer_token_hash (text)
  • scim_groups - stores SCIM groups per SSO provider
  • scim_group_members - junction table for group membership
  • Bulk operations not supported in initial release

Testing

All tests were based on the assumptions that the Azure Validation Tool expects, currently all passing.

depthfirst-app[bot] reacted with thumbs up emoji depthfirst-app[bot] reacted with eyes emoji
@Bewinxed Bewinxed force-pushed the bewinxed/add-scim-v2 branch 2 times, most recently from f514de5 to 7a60f1b Compare January 19, 2026 10:13
@Bewinxed Bewinxed marked this pull request as ready for review January 19, 2026 10:15
@Bewinxed Bewinxed requested a review from a team as a code owner January 19, 2026 10:15
Copy link

coveralls commented Jan 19, 2026 •
edited
Loading

Pull Request Test Coverage Report for Build 21770818575

Details

  • 1428 of 2280 (62.63%) changed or added relevant lines in 15 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 68.325%
Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/errors.go 5 11 45.45%
internal/models/errors.go 2 8 25.0%
internal/models/sso.go 21 32 65.63%
internal/api/apierrors/apierrors.go 33 48 68.75%
internal/api/scim_types.go 23 38 60.53%
internal/api/scim_helpers.go 128 148 86.49%
internal/models/user.go 46 90 51.11%
internal/api/ssoadmin.go 0 68 0.0%
internal/models/scim_group.go 201 276 72.83%
internal/api/scim_filter.go 29 143 20.28%
Totals Coverage Status
Change from base Build 21722549935: -0.6%
Covered Lines: 16368
Relevant Lines: 23956
💛 - Coveralls

Bewinxed added 22 commits February 5, 2026 23:36 
- Add FindUsersByProviderWithFilter for SCIM user listing
- Add FindSCIMGroupsBySSOProviderWithFilter for group listing
- Make external_id nullable, add case-insensitive displayName index
- Validate user belongs to SSO provider before adding to group
Copy link
Contributor

@cstockton cstockton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't given this a full review yet, but one change we will need is to merge the migrations into one. This will lower the possibility of migration errors.

Bewinxed reacted with thumbs up emoji
return nil
}

if err := user.Ban(tx, time.Duration(math.MaxInt64), &scimDeprovisionedReason); err != nil {
Copy link
Contributor

@depthfirst-app depthfirst-app bot Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Severity: HIGH

Privilege escalation: scimDeleteUser unconditionally overwrites existing ban reasons with SCIM_DEPROVISIONED. A SCIM admin can bypass a Super Admin's platform-wide ban by deleting then re-creating the user, which triggers an unban because the reason now matches the SCIM deprovisioning tag.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: Prevent SCIM from overwriting non-SCIM ban reasons to avoid privilege escalation. Before calling user.Ban() with scimDeprovisionedReason, check if the user is already banned with a different reason. If the user has been banned by a Super Admin for security reasons (e.g., 'Compromised Account'), SCIM should not be allowed to overwrite that ban reason. This prevents a SCIM admin from bypassing the platform-wide ban by deprovisioning (which would overwrite the reason) and then re-provisioning (which triggers an unban because the reason now matches scimDeprovisionedReason). The same vulnerability exists in scimPatchUser and scimReplaceUser functions and should be addressed there as well.

⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.

Suggested change
iferr:=user.Ban(tx, time.Duration(math.MaxInt64), &scimDeprovisionedReason); err!=nil {
// Prevent overwriting non-SCIM ban reasons to avoid privilege escalation
if user.IsBanned() && (user.BannedReason == nil || *user.BannedReason != scimDeprovisionedReason) {
return apierrors.NewSCIMBadRequestError("User is banned by administrator and cannot be deprovisioned via SCIM", "mutability")
}
if err := user.Ban(tx, time.Duration(math.MaxInt64), &scimDeprovisionedReason); err != nil {
return apierrors.NewSCIMInternalServerError("Error deprovisioning user").WithInternalError(err)
}

Comment on lines 544 to 548
if user.Identities[i].Provider == providerType {
if user.Identities[i].IdentityData != nil {
delete(user.Identities[i].IdentityData, "external_id")
}
if err := tx.UpdateOnly(&user.Identities[i], "identity_data"); err != nil {
Copy link
Contributor

@depthfirst-app depthfirst-app bot Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Severity: MEDIUM

Stale authentication mapping: Removing externalid via PATCH only deletes it from IdentityData but leaves the provider_id column intact in the identities table. The user remains able to authenticate using the identifier the administrator intended to revoke.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: When removing the externalid via SCIM PATCH, both the identity_data JSON field and the provider_id column must be cleared. Currently only identity_data is updated, leaving the provider_id intact, which allows the user to continue authenticating with the old identifier. Clear the provider_id field by setting it to an empty string and include both "provider_id" and "identity_data" in the UpdateOnly call, mirroring the pattern used in the 'add' operation at lines 565-571.

⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.

Suggested change
if user.Identities[i].Provider == providerType {
if user.Identities[i].IdentityData != nil {
delete(user.Identities[i].IdentityData, "external_id")
}
if err := tx.UpdateOnly(&user.Identities[i], "identity_data"); err != nil {
if user.Identities[i].Provider == providerType {
user.Identities[i].ProviderID = ""
if user.Identities[i].IdentityData != nil {
delete(user.Identities[i].IdentityData, "external_id")
}
if err := tx.UpdateOnly(&user.Identities[i], "provider_id", "identity_data"); err != nil {
return apierrors.NewSCIMInternalServerError("Error updating identity").WithInternalError(err)
}
break
}

Bewinxed added 18 commits February 7, 2026 04:15
return apierrors.NewSCIMInternalServerError("Error checking existing SSO user").WithInternalError(err)
}

if len(ssoUsers) > 0 {
Copy link
Contributor

@depthfirst-app depthfirst-app bot Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Severity: HIGH

Reactivating users based only on email allows data takeover. In enterprise environments where emails are recycled, a new IdP user with a previously used email will inherit the user_id and all associated application data of the old user. SCIM reactivation must verify that the externalId also matches.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: Before reactivating a deprovisioned user, verify that the incoming externalId matches the existing identity's ProviderID. After line 162 where 'candidate := deprovisioned[0]' is assigned, add validation logic: iterate through candidate.Identities to find the identity with the matching providerType, then check if params.ExternalID is non-empty and different from the existing ProviderID. If they don't match, return a SCIM conflict error indicating that the email matches an existing user with a different external ID. This prevents account takeover when email addresses are recycled at the Identity Provider, ensuring that a new employee with a recycled email cannot inherit the previous employee's user_id and application data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@depthfirst-app depthfirst-app[bot] depthfirst-app[bot] left review comments

@cstockton cstockton cstockton requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Comments

AltStyle によって変換されたページ (->オリジナル) /