Conversation
6e4416d to
edff202
Compare
f689fc8 to
fec7a7d
Compare
3538a0b to
77782ab
Compare
4709682 to
e114c8e
Compare
a9ac782 to
316aa74
Compare
eb969a8 to
86c6ef9
Compare
7294ee5 to
e6690b1
Compare
fc3d2c3 to
29c862c
Compare
56f3655 to
3e66f1d
Compare
annabkr
left a comment
There was a problem hiding this comment.
question: could we break this up for easier review? I find Claude is pretty good at doing that, if it feels tedious to do yourself
hf
left a comment
There was a problem hiding this comment.
Looks good, some minor clarifications not blocking from my POV.
| Name string `json:"name"` | ||
| Type AttributeType `json:"type"` | ||
| MultiValued bool `json:"multiValued"` | ||
| Description string `json:"description"` | ||
| Required bool `json:"required"` | ||
| CanonicalValues []string `json:"canonicalValues,omitempty"` | ||
| CaseExact bool `json:"caseExact"` | ||
| Mutability Mutability `json:"mutability"` | ||
| Returned Returned `json:"returned"` | ||
| Uniqueness Uniqueness `json:"uniqueness"` | ||
| ReferenceTypes []ReferenceType `json:"referenceTypes,omitempty"` | ||
| SubAttributes []*Attribute `json:"subAttributes,omitempty"` |
There was a problem hiding this comment.
Shouldn't all of these have omitempty?
There was a problem hiding this comment.
Shouldn't all of these have omitempty?
RFC 7643 says:
Unlike other core resources, the "Schema" resource MAY contain a complex object within a sub-attribute, and all attributes are REQUIRED unless otherwise specified.
So I opted to not add the omitempty so that they get the default zero values which would be false for all booleans.
| field | required |
|---|---|
| name | Y |
| name | Y |
| type | Y |
| multiValued | Y |
| description | Y |
| required | Y |
| caseExact | Y |
| mutability | Y |
| returned | Y |
| uniqueness | Y |
| canonicalValues | N |
| referenceTypes | N |
| subAttributes | N |
| if values.Get("sortBy") != "" { | ||
| return SortAscending, nil | ||
| } |
There was a problem hiding this comment.
How is ?sortBy (without =true) handled here?
There was a problem hiding this comment.
How is ?sortBy (without =true) handled here?
I think this will return as a default sort order and then the default sorting will kick in. I'll double check though.
9f216cd to
adc4c77
Compare
adc4c77 to
26eceaf
Compare
26eceaf to
ab06b11
Compare
7f512ea to
3c58fe8
Compare
This comment has been minimized.
This comment has been minimized.
| case models.LinkAccount: | ||
| if _, err = p.api.createNewIdentity(tx, user, providerType, identityData(input)); err != nil { |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
SCIM's LinkAccount branch attaches an sso:<provider> identity to an existing password user but leaves user.IsSSOUser false. That user passes the SSO-only passkey-registration guard, can register a credential while provisioned, and later use passkey login after SCIM deactivation to obtain a fresh session, bypassing deprovisioning.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: In the models.LinkAccount case within the link function, after attaching the SSO identity to an existing password user, explicitly set user.IsSSOUser = true and persist it to the database using tx.UpdateOnly(user, "is_sso_user"). This ensures that: (1) the passkey-registration guard in passkey_registration.go correctly blocks the now-SSO-linked user from registering passkeys, and (2) the SCIM deprovisioning check in tokens/service.go (if user.IsSSOUser { ... IsSCIMUserDeprovisionedForUpdate ... }) is evaluated for this user after a SCIM deactivation, preventing bypass of deprovisioning via passkey login.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| case models.LinkAccount: | |
| if _, err = p.api.createNewIdentity(tx, user, providerType, identityData(input)); err != nil { | |
| case models.LinkAccount: | |
| if !user.IsSSOUser { | |
| user.IsSSOUser = true | |
| if err = tx.UpdateOnly(user, "is_sso_user"); err != nil { | |
| return nil, false, err | |
| } | |
| } | |
| if _, err = p.api.createNewIdentity(tx, user, providerType, identityData(input)); err != nil { |
e5af232 to
74fac8f
Compare
74fac8f to
67b7e96
Compare
| err := conn.Transaction(func(tx *storage.Connection) error { | ||
| var terr error | ||
|
|
||
| if config.SSO.SCIM.Enabled && user.IsSSOUser { |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
When SCIM links a provisioned record to an existing password account, user.IsSSOUser remains false. After the IdP sets active:false, this guard skips the SCIM deprovision check; password or refresh-token authentication can then issue a new session for the offboarded user.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Remove the user.IsSSOUser condition from the SCIM deprovision guard. The IsSCIMUserDeprovisionedForUpdate function already safely handles users who have no SCIM records by returning false when len(rows) == 0 (line 301 of scim_user.go). By keeping user.IsSSOUser in the guard, users whose password accounts were linked to a SCIM provisioned record (where IsSSOUser remains false) bypass the deprovisioning check entirely. Changing the condition to if config.SSO.SCIM.Enabled { ensures all users are checked against SCIM deprovisioning status when SCIM is active, regardless of whether they are flagged as SSO users.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| if config.SSO.SCIM.Enabled && user.IsSSOUser { | |
| if config.SSO.SCIM.Enabled { |
796e977 to
099c7b3
Compare
What kind of change does this PR introduce?
Feature. SCIM 2.0 provisioning for SSO providers: Users, Groups, per-provider tokens, admin endpoints, and audit events.
What is the current behavior?
/scim/v2serves discovery metadata only, behindGOTRUE_EXPERIMENTAL_SCIM_ENABLED. There is no way to provision users or groups from an IdP.What is the new behavior?
SCIM endpoints, authenticated with a bearer token that is scoped to one SSO provider:
/scim/v2/UsersuserName eq,externalId eq), sort, paginate/scim/v2/Users/scim/v2/Users/{id}/scim/v2/GroupsdisplayName eq,externalId eq), paginate/scim/v2/Groups/scim/v2/Groups/{id}/scim/v2/ServiceProviderConfig,/scim/v2/ResourceTypes[/{id}],/scim/v2/Schemas[/{id}]Admin endpoints under
/admin/sso/providers/{idp_id}/scim:/enabled,base_url, active tokens//tokens/tokensexpires_at/tokens/{prefix}Audit events:
scim_user_created,scim_user_updated,scim_user_deactivated,scim_user_reactivated,scim_user_deletedscim_group_created,scim_group_updated,scim_group_deleted,scim_group_member_added,scim_group_member_removedscim_enabled,scim_disabled,scim_token_created,scim_token_revoked,scim_users_bannedConfiguration:
GOTRUE_SSO_SCIM_ENABLEDfalseGOTRUE_EXPERIMENTAL_SCIM_ENABLED, which is removedGOTRUE_RATE_LIMIT_SCIM3000Additional context
Extracted from #2731