fix(oidc): fail sign-in when register self-identified provisioning fails#2004
Open
TheTechArch wants to merge 1 commit into
Open
fix(oidc): fail sign-in when register self-identified provisioning fails#2004TheTechArch wants to merge 1 commit into
TheTechArch wants to merge 1 commit into
Conversation
When register provisioning returned null (non-200, network error, or exception swallowed by the client), the OIDC sign-in path returned an unpopulated UserAuthenticationModel and continued building a session for UserID 0 / PartyID 0. The real failure was masked by a downstream session-creation error. Provisioning failure now throws RegisterUserProvisioningException, which HandleUpstreamCallback converts into a 502 LocalError so the actual cause is logged and surfaced instead of hidden. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR introduces exception-based error handling for OIDC self-identified user provisioning. A new ChangesOIDC Provisioning Exception Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Authentication/Services/OidcServerService.cs (2)
1652-1657:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize exception text and normalize client failures to the same exception type.
Line 1657 includes raw
externalIdentityin the exception message, which is logged at Line 323 and can leak identifiers. Also, exceptions thrown by_registerUserProvisioningClient.GetOrCreateUser(...)currently bypass this mapping path. Use a generic message and wrap non-cancellation exceptions asRegisterUserProvisioningException.Proposed fix
- var response = await _registerUserProvisioningClient.GetOrCreateUser(request, cancellationToken); + SelfIdentifiedUser? response; + try + { + response = await _registerUserProvisioningClient.GetOrCreateUser(request, cancellationToken); + } + catch (OperationCanceledException) + { + throw; + } + catch (Exception ex) + { + throw new RegisterUserProvisioningException( + "Register self-identified provisioning failed; sign-in cannot complete.", + ex); + } if (response is null) { throw new RegisterUserProvisioningException( - $"Register self-identified provisioning returned no result for externalIdentity '{externalIdentity}'; sign-in cannot complete."); + "Register self-identified provisioning returned no result; sign-in cannot complete."); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Authentication/Services/OidcServerService.cs` around lines 1652 - 1657, Replace the raw externalIdentity leak and normalize client failures by changing the call to _registerUserProvisioningClient.GetOrCreateUser(request, cancellationToken) so that any non-cancellation exception is caught and rethrown as a RegisterUserProvisioningException with a generic message (do not include externalIdentity); also when the returned response is null throw a RegisterUserProvisioningException with a generic message (no sensitive data). Preserve and rethrow OperationCanceledException/TaskCanceledException unchanged so cancellation semantics remain intact.
1637-1643:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle the new provisioning exception in all entry-point flows.
After this contract change, failures now throw instead of returning null.
HandleUpstreamCallbackhandles that, but the calls at Line 152 and Line 781 do not, which can still surface as unhandled 500s on affected ticket paths. Add consistent catch/mapping in those entry points as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Authentication/Services/OidcServerService.cs` around lines 1637 - 1643, Get the new provisioning exception handled consistently: surround every call site of GetOrCreateSelfIdentifiedUserViaRegister (including the two entry-point flows outside HandleUpstreamCallback that currently call it without protection) with a try/catch that mirrors HandleUpstreamCallback’s behavior—catch the provisioning-specific exception type thrown by GetOrCreateSelfIdentifiedUserViaRegister and map it to the same response/status/logging used in HandleUpstreamCallback so those flows no longer surface unhandled 500s. Ensure you update both entry-point handlers that invoke GetOrCreateSelfIdentifiedUserViaRegister (the other two call sites in this file) to use the same catch-and-map logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Authentication/Services/OidcServerService.cs`:
- Around line 1652-1657: Replace the raw externalIdentity leak and normalize
client failures by changing the call to
_registerUserProvisioningClient.GetOrCreateUser(request, cancellationToken) so
that any non-cancellation exception is caught and rethrown as a
RegisterUserProvisioningException with a generic message (do not include
externalIdentity); also when the returned response is null throw a
RegisterUserProvisioningException with a generic message (no sensitive data).
Preserve and rethrow OperationCanceledException/TaskCanceledException unchanged
so cancellation semantics remain intact.
- Around line 1637-1643: Get the new provisioning exception handled
consistently: surround every call site of
GetOrCreateSelfIdentifiedUserViaRegister (including the two entry-point flows
outside HandleUpstreamCallback that currently call it without protection) with a
try/catch that mirrors HandleUpstreamCallback’s behavior—catch the
provisioning-specific exception type thrown by
GetOrCreateSelfIdentifiedUserViaRegister and map it to the same
response/status/logging used in HandleUpstreamCallback so those flows no longer
surface unhandled 500s. Ensure you update both entry-point handlers that invoke
GetOrCreateSelfIdentifiedUserViaRegister (the other two call sites in this file)
to use the same catch-and-map logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d545f848-fe39-4f5c-8e2c-8bad2dc1b543
📒 Files selected for processing (2)
src/Authentication/Exceptions/RegisterUserProvisioningException.cssrc/Authentication/Services/OidcServerService.cs
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nullbyRegisterUserProvisioningClient) causedIdentifyOrCreateAltinnUserto return an unpopulatedUserAuthenticationModeland continue building an Altinn session/cookie for UserID 0 / PartyID 0. The real failure was masked by a downstream session-creation error, making the actual cause hard to diagnose (the original CodeRabbit concern on feat(oidc): provision self-identified users via register (feature-fla… #1982).GetOrCreateSelfIdentifiedUserViaRegisternow returns a non-nullableSelfIdentifiedUserand throws a newRegisterUserProvisioningExceptionwhen provisioning fails, instead of returning the empty model.HandleUpstreamCallbackcatches that exception and returns a502 Bad GatewayLocalError(register is a failed downstream dependency), so the cause is logged and surfaced rather than hidden.if (provisioned is null) return userAuthenticationModel;blocks now that the helper can't return null.Notes
IdentifyOrCreateAltinnUserbut requireUserID > 0and normally route to the existing-user branch; in the rare case a ticket carries anExternalIdentitywith the flag on and register is down, the exception propagates as an unhandled 500 — still surfacing the real cause in logs rather than masking it. Can add explicit handling there if desired.Test plan
RegisterSelfIdentifiedUserProvisioningon and register returning a non-200 / unreachable, OIDC sign-in returns 502 with the register failure logged (no session/cookie issued)🤖 Generated with Claude Code
Summary by CodeRabbit