Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces synchronous party-to-instance-owner resolution with an async overload that accepts an authentication context, adds ExternalIdentifier to InstanceOwner responses and OpenAPI, introduces new URN constants, refactors instance-owner party-type logic, adds unit and layout tests, and bumps a storage-interface package version. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
|
/publish |
PR release:
|
HauklandJ
added
backport-ignore
|
/publish |
PR release:
|
HauklandJ
force-pushed
the
feat/email-user
branch
from
f4f3350 to
b096a3e
Compare
|
/publish |
PR release:
|
|
/publish |
PR release:
|
|
/publish |
PR release:
|
|
tested in tt02: 
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 308-309: In PostSimplified, replace the synchronous
PartyToInstanceOwner call with the async overload that accepts the
authentication context: await InstantiationHelper.PartyToInstanceOwner(party,
_authenticationContext). Locate the PostSimplified method (around where
LookupParty is awaited) and update the InstanceOwner assignment to use the async
method on InstantiationHelper so email-identified users get their email
populated, matching the behavior used in Post.
In `@src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cs`:
- Around line 227-239: The code calls
GetInstanceOwnerPartyType(Instance.InstanceOwner) without guarding for a null
Instance.InstanceOwner while other branches use Instance.InstanceOwner?; update
the call or signature to handle nulls: either check Instance.InstanceOwner for
null before calling (return "unknown" when null) or change
GetInstanceOwnerPartyType to accept a nullable InstanceOwner? and return
"unknown" when the parameter is null; update references to
GetInstanceOwnerPartyType accordingly so no NullReferenceException can occur
when key == "instanceOwnerPartyType".
🧹 Nitpick comments (4)
src/Altinn.App.Core/Helpers/InstantiationHelper.cs (2)
222-267: Code duplication between sync and asyncPartyToInstanceOwneroverloads.The SSN, OrgNumber, and default branches (lines 231–242, 262–267) are duplicated from the sync overload (lines 200–221). Consider having the async overload delegate to the sync one for non-
SelfIdentifiedcases to reduce drift risk:♻️ Suggested refactor
internal static async Task<InstanceOwner> PartyToInstanceOwner( Party party, IAuthenticationContext authenticationContext ) { - if (!string.IsNullOrEmpty(party.SSN)) - { - return new() { PartyId = party.PartyId.ToString(CultureInfo.InvariantCulture), PersonNumber = party.SSN }; - } - else if (!string.IsNullOrEmpty(party.OrgNumber)) - { - return new() - { - PartyId = party.PartyId.ToString(CultureInfo.InvariantCulture), - OrganisationNumber = party.OrgNumber, - }; - } - else if (party.PartyTypeName.Equals(PartyType.SelfIdentified)) + if (party.PartyTypeName.Equals(PartyType.SelfIdentified)) { string? externalIdentifier = await GetExternalIdentityForSelfIdentifiedParty(party, authenticationContext); - // external identifier is expected to be in the format "urn:altinn:person:idporten-email:<email>". const string emailPrefix = AltinnUrns.SelfIdentifiedEmail + ":"; if ( externalIdentifier is not null && externalIdentifier.StartsWith(emailPrefix, StringComparison.OrdinalIgnoreCase) ) { return new() { PartyId = party.PartyId.ToString(CultureInfo.InvariantCulture), Email = externalIdentifier[emailPrefix.Length..], }; } - // Support legacy self identification with username and password. return new() { PartyId = party.PartyId.ToString(CultureInfo.InvariantCulture), Username = party.Name }; } - return new() - { - PartyId = party.PartyId.ToString(CultureInfo.InvariantCulture), - // instanceOwnerPartyType == "unknown" - }; + + // SSN, OrgNumber, and unknown party types don't need auth context + return PartyToInstanceOwner(party); }
269-282: Consider adding telemetry for external identity resolution.This new code path is a key part of the email user instantiation feature. There's no OpenTelemetry instrumentation to track when external identity resolution is attempted, succeeds, or falls back to legacy username flow. Adding an activity/span or at minimum a metric counter would help with observability in production.
As per coding guidelines, "Comprehensive telemetry instrumentation should be included in feature implementations using OpenTelemetry".
test/Altinn.App.Core.Tests/Helpers/InstantiationHelperTests.cs (2)
11-11: Consider sealing the test class.Minor nit: the class could be
sealedto follow the project convention. As per coding guidelines, "Use sealed for classes unless inheritance is considered a valid use-case".
13-48: Good coverage of the email extraction path.The test correctly validates that the email is extracted from the URN and that
Usernameremains null. Consider adding an edge-case test whereExternalIdentityequals exactly the prefix ("urn:altinn:person:idporten-email:") with no email portion — in that scenario the helper (line 256 ofInstantiationHelper.cs) would set
HauklandJ
force-pushed
the
feat/email-user
branch
from
5578117 to
be40330
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Altinn.App.Api/Models/InstanceResponse.cs`:
- Around line 163-166: The ExternalIdentifier property is documented as nullable
but the file has "#nullable disable"; re-enable nullable reference types for the
file (replace "#nullable disable" with "#nullable enable" or add "#nullable
enable" at the top) and update the property signature to reflect nullability:
change ExternalIdentifier to "public string? ExternalIdentifier { get; init; }"
(referencing the ExternalIdentifier property in InstanceResponse) so the type
matches the XML docs and project nullable guidelines.
In `@src/Altinn.App.Core/Helpers/InstantiationHelper.cs`:
- Around line 264-276: GetExternalIdentityForSelfIdentifiedParty dereferences
profile without guarding against a null return from user.LookupProfile(); change
the method to check the awaited LookupProfile() result (from
user.LookupProfile()) for null and return null if it's null, or return the
ExternalIdentity only when profile is non-null (e.g., use a null-check or the
null-conditional on profile before returning profile.ExternalIdentity) so no
NullReferenceException occurs.
|
2 participants
Description
Add email to InstanceOwner when the party is self-identified and contains the email urn for external identifier
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores