feat(scim): Default SCIM user creation to sole configured user type, advertise pagination support - #4769
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
d81093d to
37983ce
Compare
…rtise pagination support Allow POST /Users to omit the ThunderID extension schema URN when the request carries only core SCIM attributes. CreateUser then defaults to the sole configured user type via resolveDefaultEntityTypeName, erroring if zero or more than one type is configured. ReplaceUser (and Me PUT) still require the extension URN explicitly; no fallback on update. Also advertise the RFC 9865 pagination attribute on GET /ServiceProviderConfig: index-based pagination supported, cursor based pagination not implemented.
37983ce to
e46c4a7
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates ThunderID’s SCIM implementation to allow POST /Users requests that contain only SCIM core attributes (no ThunderID extension URN) to succeed by defaulting to the sole configured user entity type, and it extends GET /ServiceProviderConfig to advertise SCIM pagination capabilities per RFC 9865.
Changes:
- Relaxed SCIM user request validation to permit zero ThunderID extension URNs when the payload contains core attributes, and added service-layer default user type resolution for
CreateUser. - Kept update semantics explicit by requiring the custom schema URN for
ReplaceUser(and related PUT flows). - Added pagination capability fields to the ServiceProviderConfig model/response and expanded unit test coverage.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/internal/scim/users_service.go | Defaults CreateUser user type when omitted; enforces URN requirement on ReplaceUser. |
| backend/internal/scim/users_service_test.go | Adds unit tests for defaulting behavior and URN-required replace semantics. |
| backend/internal/scim/service.go | Adds pagination fields to config versioning and ServiceProviderConfig response; introduces resolveDefaultEntityTypeName. |
| backend/internal/scim/service_test.go | Extends capability assertions to include pagination fields. |
| backend/internal/scim/scim_validator.go | Relaxes extension URN requirement for core-only payloads while preserving multi-URN rejection. |
| backend/internal/scim/scim_validator_test.go | Adds coverage for core-only payload parsing with no ThunderID URN. |
| backend/internal/scim/model.go | Adds SCIMPaginationConfig and wires it into SCIMServiceProviderConfig. |
| backend/internal/scim/config/config.go | Defines pagination support constants and defaults used by ServiceProviderConfig. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| thunderPrefix := strings.ToLower(ThunderIDURNPrefix) | ||
| var thunderURNs []string | ||
| for _, urn := range schemas { | ||
| if strings.HasPrefix(strings.ToLower(strings.TrimSpace(urn)), thunderPrefix) { | ||
| thunderURNs = append(thunderURNs, urn) |
| require.Equal(t, scimconfig.ETagSupported, result.ETag.Supported) | ||
| require.Equal(t, scimconfig.ETagSupported, result.ETag.Supported) | ||
| require.Equal(t, scimconfig.PaginationCursorSupported, result.Pagination.Cursor) |
|
|
||
| // PaginationMaxPageSize is the maximum number of resources returned | ||
| // per page, regardless of the requested "count". | ||
| PaginationMaxPageSize = serverconst.MaxPageSize |
There was a problem hiding this comment.
PaginationMaxPageSize is serverconst.MaxPageSize (100), but the list handlers don't cap at 100 — they clamp count to scimconfig.FilterMaxResults (200):
users_handler.go:79—if count > scimconfig.FilterMaxResults { count = scimconfig.FilterMaxResults }users_handler.go:165(/.search) andgroup_handler.go:47do the same.
So GET /Users?count=150 returns 150 resources while GET /ServiceProviderConfig advertises pagination.maxPageSize: 100. The same response also advertises filter.maxResults: 200, so the document contradicts itself.
Either advertise the value that is actually enforced, or change the handlers to clamp at MaxPageSize.
| PaginationMaxPageSize = serverconst.MaxPageSize | |
| PaginationMaxPageSize = FilterMaxResults |
| ) (*SCIMUser, *tidcommon.ServiceError) { | ||
| logger := log.GetLogger().With(log.String(log.LoggerKeyComponentName, loggerComponentName)) | ||
|
|
||
| if payload.UserTypeName == "" { |
There was a problem hiding this comment.
Worth reconsidering this asymmetry. The target user's type is not actually ambiguous on update — ReplaceUser loads existingUser a few lines below and then enforces that the requested type equals existingUser.Type (ErrorImmutableUserType, line ~206). So on PUT the extension URN can only ever be the type the user already has; requiring it is ceremony rather than disambiguation.
Concrete cost: a client that created a user via the new core-only POST cannot do the natural GET → modify → PUT round trip with a core-only body, and PUT /Me is worse — a /Me client generally has no way to know its own type URN, yet it now must send one.
Suggest resolving the type from existingUser.Type when payload.UserTypeName == "" (keeping the mismatch check for the case where a URN is supplied), which keeps POST and PUT consistent.
| logger := log.GetLogger().With(log.String(log.LoggerKeyComponentName, loggerComponentName)) | ||
|
|
||
| if payload.UserTypeName == "" { | ||
| logger.Error(ctx, "SCIM ReplaceUser: custom schema URN required") |
There was a problem hiding this comment.
Logging a plain client-input error at Error level is inconsistent with the other 400 paths in this same function, which use logger.Debug (e.g. the conflicting core/custom value branch). Any client sending a core-only PUT will now generate error-level entries, which degrades error-rate alerting.
| return "", &ErrorMissingCustomSchema | ||
| } | ||
| if page.TotalResults != 1 || len(page.Types) != 1 { | ||
| return "", &ErrorMissingCustomSchema |
There was a problem hiding this comment.
The zero-types and multiple-types cases are collapsed into one client error, but they are quite different:
- More than one type configured — 400 is right; the client genuinely has to disambiguate.
- Zero types configured — this is a server configuration state. Returning 400 with "The request must include exactly one ThunderID custom user schema URN" tells the client to add a URN that does not exist, so a well-behaved client will retry indefinitely instead of surfacing a deployment problem. A server-side error (or at minimum a distinct, accurate message) would be more actionable.
Also worth noting these two branches are indistinguishable in the logs, since resolveDefaultEntityTypeName logs nothing.
| if svcErr.Type == tidcommon.ServerErrorType { | ||
| return "", &ErrorInternalServer | ||
| } | ||
| return "", &ErrorMissingCustomSchema |
There was a problem hiding this comment.
Every non-server error from GetEntityTypeList is flattened into ErrorMissingCustomSchema and the original is dropped without logging, so an authorization or validation failure from the entity type service surfaces to the caller as "Missing custom schema" and leaves no trace to diagnose. Consider logging svcErr here before mapping.
| coreAttrs[k] = v | ||
| } | ||
| if extensionURN == "" && len(coreAttrs) == 0 { | ||
| return nil, &ErrorMissingCustomSchema |
There was a problem hiding this comment.
ErrorMissingCustomSchema's description in error_constants.go still reads "The request must include exactly one ThunderID custom user schema URN", which this change makes inaccurate — zero URNs is now valid. The same error code is also reused for two further cases (empty payload here, and "multiple user types configured so the default is ambiguous" in resolveDefaultEntityTypeName), so clients cannot tell them apart and the message is wrong for all three.
Worth updating the description and/or splitting the ambiguous-default case into its own code.
| require.Equal(t, scimconfig.PaginationDefaultMethod, result.Pagination.DefaultPaginationMethod) | ||
| require.Equal(t, scimconfig.PaginationDefaultPageSize, result.Pagination.DefaultPageSize) | ||
| require.Equal(t, scimconfig.PaginationMaxPageSize, result.Pagination.MaxPageSize) | ||
|
|
There was a problem hiding this comment.
Duplicate of the assertion on the previous line (result.ETag.Supported is checked twice) — looks like a stray copy-paste; this line can just be deleted.
Purpose
POST /Users required the ThunderID extension schema URN even when the request only carried core SCIM attributes, forcing clients to know a custom type URN just to create a user. This PR lets CreateUser omit the extension URN and fall back to the sole configured user type. It also advertises RFC 9865 pagination capabilities on GET /ServiceProviderConfig, which was previously missing from the response.
Approach
scim_validator.go: relaxed validation so zero ThunderID extension URNs is allowed, but only when the payload carries core attributes (empty payloads still rejected viaErrorMissingCustomSchema). More than one extension URN remains rejected as before.service.go: addedresolveDefaultEntityTypeName, which looks up configured user entity types and returns the sole one's canonical name. Errors withErrorMissingCustomSchemaif zero or more than one type is configured (default would be ambiguous).users_service.go:CreateUsernow callsresolveDefaultEntityTypeNamewhenUserTypeNameis empty, instead of failing.ReplaceUser(and/MePUT) keep the extension URN mandatory, no fallback on update, since an update should be explicit about which type it targets.config.go/model.go: addedPaginationCursorSupported(false),PaginationIndexSupported(true), default method/page size constants, and theSCIMPaginationConfigresponse struct, wired intoGetServiceProviderConfig.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks