-
Notifications
You must be signed in to change notification settings - Fork 3
feat(api): Support self registered users/Feide users #2861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…rties-endpoint-for-systemusers
This comment was marked as resolved.
This comment was marked as resolved.
src/Digdir.Domain.Dialogporten.Application/Common/IUserRegistry.cs
Outdated
Show resolved
Hide resolved
…luentValidation/FluentValidationPartyIdentifierExtensions.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…tion/DecisionRequestHelper.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (1)
95-130: Self‑identified/Feide early‑return is sound but make assumptions explicitThe emulation path for
IdportenSelfIdentifiedUserIdentifier/AltinnSelfIdentifiedUserIdentifier/FeideUserIdentifierlooks correct and matches the documented limitation (no access‑management support; only end‑user contexts). Two minor points to consider:
- You rely on
_user.GetPrincipal()forPartyUuid/PartyIdwhile also accepting anauthenticatedPartyparameter; ifGetAuthorizedPartiesis ever called with anauthenticatedPartythat does not come from the current principal, this branch will silently mix identities. Either document this invariant on the interface or add a guard/assertion to fail fast if they diverge.- You call
_user.GetPrincipal()twice; capturing it once (e.g.var principal = _user.GetPrincipal();) would slightly improve clarity and avoid duplication.Functionally this looks fine and matches
AuthorizedPartiesHelper’sSelfIdentifiedhandling.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/AuthorizedPartiesResult.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (6)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/IntegrationTestUser.cs (1)
ClaimsPrincipal(29-29)src/Digdir.Domain.Dialogporten.GraphQL/Common/Authentication/LocalDevelopmentUser.cs (1)
ClaimsPrincipal(26-26)src/Digdir.Domain.Dialogporten.WebApi/Common/LocalDevelopmentUser.cs (1)
ClaimsPrincipal(26-26)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AuthorizedPartiesResultDto.cs (1)
AuthorizedPartiesResultDto(3-19)src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs (2)
TryGetPartyUuid(156-166)TryGetPartyId(168-178)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AuthorizedPartiesHelper.cs (1)
AuthorizedPartiesHelper(6-87)
🔇 Additional comments (4)
src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/AuthorizedPartiesResult.cs (1)
42-43: Verification confirms enum change is correctly implemented.All usages of
AuthorizedPartyTypeproperly handle the newSelfIdentifiedmember:
- Both switch expressions in
AuthorizedPartiesHelper.cs(lines 37–45 and 53–59) explicitly map the new member with default exception handling- AutoMapper automatically handles enum mapping in
MappingProfile.csfiles without requiring special configuration- Business logic properly integrates the new member (line 62:
IsCurrentEndUsercheck)- No serialization or database migration issues identified
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (3)
64-71: Switch toClaimsPrincipalis consistent with the new API surfaceUsing
_user.GetPrincipal()directly forDialogDetailsAuthorizationRequest.ClaimsPrincipalaligns with the newDecisionRequestHelper.CreateDialogDetailsRequestcontract and avoids duplicating claim extraction here. No issues from a correctness or maintainability perspective.
239-246: Comment clarifies expected behavior for empty authorized partiesThe new comment around system users vs. other user types clearly documents why an empty list is treated as an error except for
SystemUserIdentifier. This matches the subsequent guard and improves readability.
290-298:xacmlJsonResponserenaming and flow are correctRenaming to
xacmlJsonResponseand passing it consistently toLogIfIndeterminateandCreateDialogDetailsResponsepreserves behavior and clarifies the variable’s purpose. No functional concerns here.



Description
This introduces support for SI-users as well as other IDP authenticated persons (such as Feide) relying on userId/partyId only.
Note! Feide / ID-porten SRE users are currently disallowed in the validator (and relevant integration tests), pending resolution of identifier names. The rest of the implementation is included in this PR.
This includes:
Other
DialogDetailsAuthorizationRequestto contain reference to ClaimsPrincipal instead of a List copy, reducing allocations and required extension methods.Limitations: there is no support in accessmanagement or PDP to perform authorizations on the new external identifiers, so we are only able to support authorization scenarios where we have a authenticated enduser context. In practice, this means we cannot support ?EndUserId= on SO-API for anything other than ID-porten authenticated persons and system users.
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)