Feature/altinn3 consent message notify#273
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds Altinn 3 consent flow alongside existing Altinn 2: new Altinn3ConsentService, token/Maskinporten handling, template Markdown for A3 correspondence, DI/KeyVault/settings updates, many service integrations, .NET target/framework and package upgrades, new tests, and minor templates/text changes. No public API removals. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR migrates consent initiation/receipts, correspondence, and notifications toward an Altinn 3–based flow, including retrieving consent tokens from Maskinporten, and updates the solution’s .NET/runtime dependencies to support the new integrations.
Changes:
- Introduces
Altinn3ConsentServiceand DI wiring, and routes consent initiation/checking toward Altinn 3 where applicable. - Adds Maskinporten “consent token” retrieval path and updates evidence harvesting to use it.
- Updates service-context text templates and rendering to support an Altinn 3 correspondence body variant.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| Dan.Core/Services/Altinn3ConsentService.cs | New Altinn 3 consent initiation, correspondence sending, and token/claims handling |
| Dan.Core/Services/TokenRequesterService.cs | Adds Maskinporten consent-token request support + cache key changes |
| Dan.Core/Services/EvidenceHarvesterService.cs | Changes evidence status check mode and consent token plumbing |
| Dan.Core/Services/EvidenceStatusService.cs | Adds Altinn 3 consent service dependency and routing |
| Dan.Core/Helpers/TextTemplateProcessor.cs | Adds Altinn 3 rendering mode + markdown button link support |
| Dan.Core/FuncAuthorization.cs | Initiates Altinn 3 vs Altinn 2 consents based on AltinnResource |
| Dan.Core/FuncConsentReceipt.cs | Adjusts receipt flow for Altinn 3 status handling |
| Dan.Common/Interfaces/IServiceContextTextTemplate.cs + templates | Adds CorrespondenceBodyA3 and updates templates accordingly |
| *.csproj files | Updates target framework and package versions across projects |
| Dan.Core.UnitTest/A3ConsentServiceTest.cs | Adds unit tests for the new Altinn 3 consent service |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+165
to
+184
| public async Task<string> GetJwt(Accreditation accreditation, EvidenceCode evidenceCode) | ||
| { | ||
| if (string.IsNullOrEmpty(accreditation.Altinn3ConsentId) && string.IsNullOrEmpty(accreditation.AuthorizationCode)) | ||
| { | ||
| throw new RequiresConsentException("The accreditation is missing a valid Altinn consent id."); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| // The consentId can be either the Altinn3ConsentId or the old AuthorizationCode which is migrated over to Altinn 3 for retrieval | ||
| var consentId = !string.IsNullOrEmpty(accreditation.Altinn3ConsentId) ? accreditation.Altinn3ConsentId : accreditation.AuthorizationCode; | ||
|
|
||
| var token = await _tokenRequesterService.GetMaskinportenConsentToken(consentId, accreditation.SubjectParty.GetAsString(false), evidenceCode); | ||
|
|
||
| if (token == null) | ||
| { | ||
| throw new ServiceNotAvailableException("Unable to parse consent token from Maskinporten"); | ||
| } | ||
|
|
||
| return token; |
Comment on lines
+119
to
+123
| var jwt = await GetJwt(accreditation, evidenceCodeWithConsent); | ||
|
|
||
| var payload = jwt.Split('.')[1]; | ||
| var token = Base64Url.Decode(payload); | ||
| if (token == null) return null; |
Comment on lines
+407
to
+410
| request.JsonContent(consentRequest); | ||
|
|
||
| _logger.LogInformation(JsonConvert.SerializeObject(consentRequest)); | ||
|
|
Comment on lines
+381
to
+382
| var resourceIdentifier = ec.AuthorizationRequirements.Where(z => z is ConsentRequirement).Select(z => (ConsentRequirement)z).Single().AltinnResource; | ||
|
|
|
|
||
| public LocalizedString CorrespondenceBodyA3 => new() | ||
| { | ||
| NoNb = $"For at {TextMacros.RequestorName} skal kunne gjennomføre en kvalifiseringsprosess må det utstedes fullmakt til å hente ut opplysninger om {TextMacros.SubjectName}.<br><br>Referanse: {TextMacros.EbevisReference} <br><br>Klikk på knappen under for å vite mer om hvilke opplysninger som er forespurt før du eventuelt gir fullmakten.<br> {TextMacros.Button}" |
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <TargetFramework>net8.0</TargetFramework> | ||
| <TargetFramework>net10.0</TargetFramework> |
Comment on lines
+11
to
12
| Task<string> GetMaskinportenConsentToken(string consentId, string offeredby, EvidenceCode evidenceCodes); | ||
| } |
| input = !string.IsNullOrEmpty(acc.ConsentReference) ? input.Replace(TextMacros.ConsentReference, acc.ConsentReference, StringComparison.InvariantCultureIgnoreCase) : input; | ||
| input = !string.IsNullOrEmpty(acc.ExternalReference) ? input.Replace(TextMacros.ExternalReference, acc.ExternalReference, StringComparison.InvariantCultureIgnoreCase) : input; | ||
| input = !string.IsNullOrEmpty(buttonText) ? input.Replace(TextMacros.Button, GetConsentButton(buttonText, consentUrl), StringComparison.InvariantCultureIgnoreCase) : input; | ||
| input = !string.IsNullOrEmpty(buttonText) ? (isAltinn3 ? input.Replace(TextMacros.Button, GetConsentButtonMd(buttonText, consentUrl), StringComparison.InvariantCultureIgnoreCase) : GetConsentButton(buttonText, consentUrl)) : input; |
Comment on lines
48
to
+60
| else if (_consentService.EvidenceCodeRequiresConsent(evidenceCode)) | ||
| { | ||
| var consentStatus = await _consentService.Check(accreditation, onlyLocalChecks); | ||
| status = MapConsentStatusToEvidenceStatusCode(consentStatus); | ||
| isConsentRequest = true; | ||
| if (accreditation.Altinn3ConsentId != null) | ||
| { | ||
| var consentStatus = await _consentServiceA3.Check(accreditation, onlyLocalChecks); | ||
| status = MapConsentStatusToEvidenceStatusCode(consentStatus); | ||
| isConsentRequest = true; | ||
|
|
||
| } else | ||
| { | ||
| var consentStatus = await _consentService.Check(accreditation, onlyLocalChecks); | ||
| status = MapConsentStatusToEvidenceStatusCode(consentStatus); | ||
| isConsentRequest = true; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dan.Core/Services/ServiceContextService.cs (1)
11-229:⚠️ Potential issue | 🟡 MinorRemove orphaned
OkinfoServiceContextTextTemplate.csclass.The removal of
okinfo-productanddigokfriv-productfrom the service registry was intentional (marked as "unused" in commit 326e347) and no active references remain in the codebase. However,OkinfoServiceContextTextTemplate.csstill exists as dead code and should be deleted to keep the codebase clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/ServiceContextService.cs` around lines 11 - 229, Remove the dead OkinfoServiceContextTextTemplate class file: locate the symbol OkinfoServiceContextTextTemplate (file OkinfoServiceContextTextTemplate.cs), delete that file, and remove any project file (.csproj) references or using/imports that reference it; confirm there are no remaining references to the removed service IDs (okinfo-product, digokfriv-product) in ServiceContextService.GetRegisteredServiceContexts or elsewhere and run a build to verify no compile errors remain.
🧹 Nitpick comments (20)
Dan.Common/Dan.Common.csproj (1)
37-38: Review the changelog for significant package updates.The
Microsoft.Azure.Functions.Workerpackage jumped from 2.1.0 to 2.51.0, andApplicationInsightsfrom 2.0.0 to 2.50.0. While these are substantial version increments, the v2.51.0 release notes indicate the primary change is an update to OpenTelemetry support (updated to v1.1.0 with Semantic Conventions v1.37 alignment). Verify that these changes are compatible with your application's observability requirements and that no other updates in intermediate versions require attention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Common/Dan.Common.csproj` around lines 37 - 38, The package bumps for Microsoft.Azure.Functions.Worker and Microsoft.Azure.Functions.Worker.ApplicationInsights in Dan.Common.csproj (the PackageReference entries for those two packages) are large; review the CHANGELOG/release notes for v2.1.0→v2.51.0 and v2.0.0→v2.50.0 to confirm OpenTelemetry changes (OTel v1.1.0 and Semantic Conventions v1.37) and any breaking or behavioral changes, run integration tests exercising observability (traces/metrics/logging) against ApplicationInsights and the Functions host, and either pin to a safer intermediate version or add configuration adjustments (OTel SDK/semantic convention mappings) in your telemetry initialization code if incompatibilities are found.Dan.Core/Services/Interfaces/IAltinn3ConsentService.cs (1)
1-9: Remove unused imports.
Dan.Common.EnumsandDan.Common.Modelsare imported but not used in this interface definition. The empty marker interface itself is fine for DI purposes.🧹 Suggested cleanup
-using Dan.Common.Enums; -using Dan.Common.Models; - namespace Dan.Core.Services.Interfaces; public interface IAltinn3ConsentService: IConsentService { - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/Interfaces/IAltinn3ConsentService.cs` around lines 1 - 9, Remove the two unused using directives by deleting the imports for Dan.Common.Enums and Dan.Common.Models at the top of the IAltinn3ConsentService file; leave the public interface IAltinn3ConsentService : IConsentService declaration intact (the marker interface is fine for DI) so only the unused using statements are removed.Dan.Core/ServiceContextTexts/DanTestServiceContextTextTemplate.cs (1)
24-28: Minor formatting inconsistency: missing blank line beforeCorrespondenceSender.Other property blocks in this file are separated by blank lines. Adding one after
CorrespondenceBodyA3would maintain consistency.🧹 Suggested formatting fix
public LocalizedString CorrespondenceBodyA3 => new() { NoNb = $"For at {TextMacros.RequestorName} skal kunne gjennomføre testprosessen må det utstedes fullmakt for {TextMacros.SubjectName}." }; + public LocalizedString CorrespondenceSender => new()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/ServiceContextTexts/DanTestServiceContextTextTemplate.cs` around lines 24 - 28, Add a blank line between the property definitions CorrespondenceBodyA3 and CorrespondenceSender to match the file's existing formatting convention; locate the CorrespondenceBodyA3 getter and insert one blank line before the start of the CorrespondenceSender property so all property blocks remain consistently separated.Dan.Core.UnitTest/EvidenceStatusServiceTest.cs (1)
22-23: Add one test that actually exercises the Altinn 3 status path.This file now wires
_mockConsentServiceA3intoEvidenceStatusService, but none of the updated cases setAltinn3ConsentIdor assert that_mockConsentServiceA3.Check(...)is used. One focused branch test here would lock down the new routing logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core.UnitTest/EvidenceStatusServiceTest.cs` around lines 22 - 23, Add a unit test in EvidenceStatusServiceTest that sets up a request/entity with Altinn3ConsentId populated so the EvidenceStatusService routes to the Altinn3 path; arrange the fake IAltinn3ConsentService (_mockConsentServiceA3) to return a known result from its Check(...) method, call the EvidenceStatusService method under test, then assert that _mockConsentServiceA3.Check(...) was invoked and that the service returned/handled the expected result (and optionally assert that the other consent service was not called) to lock down the new routing logic.Dan.Core/Config/KeyVault.cs (1)
60-65: SimplifyGetCertificateBase64to avoid unnecessary async overhead.The method wraps a simple
Get()call and usesawait Task.FromResult()which adds unnecessary overhead. SinceGet()is already async and returns the base64 string directly, this method could be simplified.♻️ Suggested simplification
public async Task<string> GetCertificateBase64(string key) { - var base64Certificate = await Get(key); - - return await Task.FromResult(base64Certificate); + return await Get(key); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Config/KeyVault.cs` around lines 60 - 65, The GetCertificateBase64 method unnecessarily uses async/await and Task.FromResult around an already-async Get(string) call; change GetCertificateBase64 to directly return the Task<string> from Get(key) (i.e., remove the async modifier, remove awaiting Task.FromResult, and simply return Get(key)) so there’s no extra state machine or overhead while preserving the same signature and behavior.Dan.Core/Program.cs (2)
261-277: Consider extracting environment detection to reduce repetition and improve robustness.The URL string check
Settings.MaskinportenUrl.Contains("test")is repeated three times and determines the environment. This pattern is fragile and could be simplified.♻️ Suggested refactor
void AddAltinn3Messaging(IServiceCollection services) { var encodedCert = Settings.AltinnCertificateb64; + var isTestEnvironment = Settings.MaskinportenUrl.Contains("test"); services.AddDdCorrespondenceService(options => { options.MaskinportenSettings = new MaskinportenSettings { ClientId = Settings.MaskinportenClientId, - Environment = Settings.MaskinportenUrl.Contains("test") ? "test" : "prod", - EnableDebugLogging = Settings.MaskinportenUrl.Contains("test") ? true : false, + Environment = isTestEnvironment ? "test" : "prod", + EnableDebugLogging = isTestEnvironment, EncodedX509 = encodedCert }; options.ResourceId = "digdir-data-altinn-no-melding"; - options.Environment = Settings.MaskinportenUrl.Contains("test") ? ApiEnvironment.Staging : ApiEnvironment.Production; + options.Environment = isTestEnvironment ? ApiEnvironment.Staging : ApiEnvironment.Production; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Program.cs` around lines 261 - 277, The repeated Settings.MaskinportenUrl.Contains("test") check in AddAltinn3Messaging is fragile; extract the environment detection into a single local variable or helper (e.g. bool isTestEnvironment = MaskinportenUrlContainsTest(...) or a method GetMaskinportenEnvironment()) and reuse it to set MaskinportenSettings.Environment, EnableDebugLogging, and the options.Environment (ApiEnvironment.Staging/Production), and use that single source when setting EncodedX509 and ResourceId in AddDdCorrespondenceService so the logic is centralized and consistent (refer to AddAltinn3Messaging, MaskinportenSettings, Settings.MaskinportenUrl, and ApiEnvironment).
33-33: Unused import.
System.Runtime.ConstrainedExecutiondoes not appear to be used in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Program.cs` at line 33, The using directive System.Runtime.ConstrainedExecution in Program.cs is unused; remove that using line (the System.Runtime.ConstrainedExecution import) from the file and then run the build or apply "Organize Usings" / remove unused usings to ensure no other unused imports remain.Dan.Core/Helpers/TextTemplateProcessor.cs (1)
73-73: Complex ternary reduces readability.This line has nested ternary operators which makes it hard to follow. Consider extracting to a clearer conditional.
♻️ Suggested refactor for readability
- input = !string.IsNullOrEmpty(buttonText) ? (isAltinn3 ? input.Replace(TextMacros.Button, GetConsentButtonMd(buttonText, consentUrl), StringComparison.InvariantCultureIgnoreCase) : input.Replace(TextMacros.Button, GetConsentButton(buttonText, consentUrl))) : input; + if (!string.IsNullOrEmpty(buttonText)) + { + var buttonHtml = isAltinn3 + ? GetConsentButtonMd(buttonText, consentUrl) + : GetConsentButton(buttonText, consentUrl); + input = input.Replace(TextMacros.Button, buttonHtml, StringComparison.InvariantCultureIgnoreCase); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Helpers/TextTemplateProcessor.cs` at line 73, The line in TextTemplateProcessor that assigns to input uses a nested ternary which hurts readability; instead extract the replacement logic into a small conditional or local variable: check if buttonText is null/empty first, then if isAltinn3 choose GetConsentButtonMd(buttonText, consentUrl) and call input.Replace(TextMacros.Button, ... , StringComparison.InvariantCultureIgnoreCase), otherwise call input.Replace(TextMacros.Button, GetConsentButton(buttonText, consentUrl)); update the input variable with the result—this keeps TextMacros.Button, GetConsentButtonMd, GetConsentButton, buttonText, isAltinn3, consentUrl and input semantics the same but much clearer.Dan.Core/Services/EvidenceHarvesterService.cs (1)
301-318: Dead code:GetConsentTokenmethod is never called.The
GetConsentTokenmethod appears to be replaced byGetMaskinportenConsentTokenbut was not removed. Consider removing this dead code.🧹 Remove unused method
- private async Task<string> GetConsentToken(EvidenceCode evidenceCode, Accreditation accreditation) - { - using (var _ = _log.Timer("jwt-fetch")) - { - _log.LogInformation( - "Getting JWT | aid={accreditationId}, evidenceCode={evidenceCodeName}, authCode={authorizationCode}", - accreditation.AccreditationId, evidenceCode.EvidenceCodeName, accreditation.AuthorizationCode); - - string jwt = await _a3ConsentService.GetJwt(accreditation, evidenceCode); - var response = JsonConvert.DeserializeObject<Dictionary<string, string>>(jwt); - _log.LogInformation( - "Completed JWT | aid={accreditationId}, evidenceCode={evidenceCodeName}, authCode={authorizationCode}, jwt={jwt}", - accreditation.AccreditationId, evidenceCode.EvidenceCodeName, accreditation.AuthorizationCode, - jwt); - - return jwt; - } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/EvidenceHarvesterService.cs` around lines 301 - 318, Remove the dead GetConsentToken method (private async Task<string> GetConsentToken(...)) since it is no longer used and has been superseded by GetMaskinportenConsentToken; delete the entire method body and its Timer/logging block and ensure no remaining references to GetConsentToken exist in EvidenceHarvesterService or tests, and if that removal makes any using directives or private members unused (e.g., the local Timer usage), clean those up as well.Dan.Core/FuncAuthorization.cs (1)
110-130: Logging inconsistency: Altinn 3 consent completion doesn't include elapsed time.The Altinn 2 consent completion log (line 128) includes
elapsedMs={elapsedMs}but the Altinn 3 completion log (line 119) does not. Consider adding elapsed time for consistency in observability.Also, the filter logic could use pattern matching for cleaner code:
♻️ Suggested improvements
- var consentRequirementsA3 = evidenceCodes.SelectMany(x => x.AuthorizationRequirements.Where(r => r is ConsentRequirement && !string.IsNullOrEmpty(((ConsentRequirement)r).AltinnResource)).ToList()).ToList(); + var consentRequirementsA3 = evidenceCodes.SelectMany(x => x.AuthorizationRequirements.Where(r => r is ConsentRequirement cr && !string.IsNullOrEmpty(cr.AltinnResource)).ToList()).ToList(); if (consentRequirementsA3.Any()) { _logger.LogInformation("Initiating Altinn3 consent for aid={accreditationId}", accreditation.AccreditationId); await _a3ConsentService.Initiate(accreditation, authRequest.SkipAltinnNotification); - _logger.LogInformation("Completed initiating Altinn3 consent for aid={accreditationId}", accreditation.AccreditationId); + _logger.LogInformation("Completed initiating Altinn3 consent for aid={accreditationId} elapsedMs={elapsedMs}", accreditation.AccreditationId, t.ElapsedMilliseconds); } - var consentRequirementsA2 = evidenceCodes.SelectMany(x => x.AuthorizationRequirements.Where(r => r is ConsentRequirement && string.IsNullOrEmpty(((ConsentRequirement)r).AltinnResource)).ToList()).ToList(); + var consentRequirementsA2 = evidenceCodes.SelectMany(x => x.AuthorizationRequirements.Where(r => r is ConsentRequirement cr && string.IsNullOrEmpty(cr.AltinnResource)).ToList()).ToList();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/FuncAuthorization.cs` around lines 110 - 130, The Altinn3 consent path (consentRequirementsA3 / _a3ConsentService.Initiate) logs start and completion but omits elapsedMs; start or reuse the same stopwatch (t) around the _a3ConsentService.Initiate call and include elapsedMs={elapsedMs} in the completion log to match the Altinn2 pattern, and while here simplify the LINQ filters by using pattern matching (e.g., AuthorizationRequirements.Where(r => r is ConsentRequirement cr && !string.IsNullOrEmpty(cr.AltinnResource)) and similarly for the Altinn2 branch) to make the consentRequirementsA3/consentRequirementsA2 declarations cleaner.Dan.Core/Services/TokenRequesterService.cs (1)
130-135: Minor: Cache key collision potential with underscore-separated components.If
offeredbyorevidenceCodeNamecontain underscores, the cache key could theoretically collide with other combinations. This is low risk but worth noting.♻️ Consider using a delimiter less likely to appear in values
- return "mpt_" + Settings.MaskinportenClientId + "_" + (consumerOrgNo ?? string.Empty) + "_" + (offeredby ?? string.Empty) + "_" + (evidenceCodeName ?? string.Empty) + scopeDigest; + return $"mpt|{Settings.MaskinportenClientId}|{consumerOrgNo ?? string.Empty}|{offeredby ?? string.Empty}|{evidenceCodeName ?? string.Empty}|{scopeDigest}";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/TokenRequesterService.cs` around lines 130 - 135, GetCacheKey can produce ambiguous keys because components (offeredby, evidenceCodeName, consumerOrgNo) are concatenated with underscores; instead ensure uniqueness by encoding or delimiting components unambiguously (e.g., Base64/Hex-encode each component or prefix each with its length) before building the final key in GetCacheKey so underscores in values cannot cause collisions; update the GetCacheKey implementation to transform consumerOrgNo, offeredby, and evidenceCodeName (and optionally Settings.MaskinportenClientId) with a reversible/unique encoding or length-prefixing and then concatenate with a stable separator and the scopeDigest.Dan.Core/Services/Altinn3ConsentService.cs (9)
219-219: Remove or document commented-out code.The commented-out
EnsureSrrRightscall should either be removed if no longer needed, or have a TODO/explanation for when it should be re-enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/Altinn3ConsentService.cs` at line 219, Remove or document the commented-out EnsureSrrRights call in Altinn3ConsentService: either delete the commented line "// await _altinnServiceOwnerApiService.EnsureSrrRights(...)" if it is obsolete, or replace it with a short TODO comment explaining why it is disabled and under what conditions it should be re-enabled (include the method name EnsureSrrRights, the involved variables accreditation.Requestor and accreditation.ValidTo, and evidenceCodesRequiringConsent in the comment for clarity).
412-412: Remove commented-out code.The commented-out ApiKey header should be removed if no longer needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/Altinn3ConsentService.cs` at line 412, Remove the leftover commented-out header line in Altinn3ConsentService: delete the commented call to request.Headers.TryAddWithoutValidation("ApiKey", Settings.AltinnApiKey) since it's unused; ensure no other references to Settings.AltinnApiKey remain in the same method so the code is clean.
257-261: Stub implementation should have a tracking marker.Consider adding a
// TODO:comment to make this easier to track for future implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/Altinn3ConsentService.cs` around lines 257 - 261, Add a TODO tracking marker to the stubbed LogUse method so it’s easy to find and implement later: modify the LogUse(Accreditation accreditation, EvidenceCode evidence, DateTime? dateTime) method in Altinn3ConsentService to include a clear `// TODO:` comment above or inside the method (e.g. `// TODO: Implement LogUse for Altinn3 - currently returns default true`) while keeping the existing Task.FromResult(true) return so behavior remains unchanged.
25-25: Unused field_noCertHttpClient.The
_noCertHttpClientfield is injected but never used in this class. Remove it if not needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/Altinn3ConsentService.cs` at line 25, The private field _noCertHttpClient in class Altinn3ConsentService is injected but never referenced; remove the unused field and any corresponding constructor parameter or DI registration if present (search for _noCertHttpClient and the Altinn3ConsentService constructor) so the class only keeps used dependencies; if the constructor still requires an HttpClient parameter, drop that parameter and update callers/DI to stop providing it.
326-329: Remove unnecessary catch-and-rethrow block.This catch block only rethrows the exception without any additional handling. Use an exception filter or remove it entirely.
Suggested fix
- catch (ServiceNotAvailableException ex) - { - throw; - } catch (Exception ex)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/Altinn3ConsentService.cs` around lines 326 - 329, The catch block that catches ServiceNotAvailableException and immediately rethrows it in Altinn3ConsentService (the catch (ServiceNotAvailableException ex) { throw; } block) is unnecessary; remove that catch clause so the exception will propagate naturally, or if special handling/logging was intended use an exception filter (e.g., catch (ServiceNotAvailableException ex) when (LogAndReturnFalse(ex)) or add the specific handling) — locate the catch in the Altinn3ConsentService method and delete the redundant catch-and-rethrow.
389-389:Single()may throw unhelpful exception.If multiple consent requirements exist for an evidence code within the same service context (violating the stated invariant),
Single()throwsInvalidOperationExceptionwith a generic message. Consider adding validation with a clearer error message.Suggested fix
- var resourceIdentifier = ec.AuthorizationRequirements.Where(z => z is ConsentRequirement).Select(z => (ConsentRequirement)z).Single().AltinnResource; + var consentRequirements = ec.AuthorizationRequirements.OfType<ConsentRequirement>().ToList(); + if (consentRequirements.Count != 1) + { + throw new InvalidOperationException($"Expected exactly one ConsentRequirement for evidence code {ec.EvidenceCodeName}, found {consentRequirements.Count}"); + } + var resourceIdentifier = consentRequirements[0].AltinnResource;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/Altinn3ConsentService.cs` at line 389, The current use of Single() on ec.AuthorizationRequirements can throw a generic InvalidOperationException; replace it with explicit validation: extract the consent requirements via ec.AuthorizationRequirements.OfType<ConsentRequirement>().ToList(), check that the list.Count == 1, and if not throw a new InvalidOperationException with a clear message including ec (or its id) and the actual count, otherwise read the single element's AltinnResource; update the reference to ConsentRequirement and AltinnResource accordingly so the error is informative when the invariant is violated.
26-26: Logger type parameter mismatch.The logger is typed as
ILogger<ConsentService>but this class isAltinn3ConsentService. Log entries will show the wrong source type, making debugging harder.Suggested fix
- private readonly ILogger<ConsentService> _logger; + private readonly ILogger<Altinn3ConsentService> _logger;And update the constructor parameter accordingly on line 46.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/Altinn3ConsentService.cs` at line 26, The logger generic is incorrect: change the field and constructor parameter from ILogger<ConsentService> to ILogger<Altinn3ConsentService> so the logger reflects this class; update the private readonly field _logger and the Altinn3ConsentService constructor parameter type to ILogger<Altinn3ConsentService> (and adjust any assignments) to ensure logs are attributed to Altinn3ConsentService.
288-291: Fragile environment detection via URL string.Checking for "tt02" in the URL to determine environment behavior is fragile and could break if URLs change. Consider using an explicit configuration setting for environment detection.
Minor improvement for case-insensitive comparison
- if (!Settings.AltinnServiceOwnerApiUri.ToLower().Contains("tt02")) + if (!Settings.AltinnServiceOwnerApiUri.Contains("tt02", StringComparison.OrdinalIgnoreCase))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/Altinn3ConsentService.cs` around lines 288 - 291, The code currently detects environment by checking if Settings.AltinnServiceOwnerApiUri contains "tt02", which is fragile; replace this URL string sniff with an explicit configuration flag or environment setting (e.g., add Settings.IsAltinnTestEnvironment or Settings.AltinnEnvironment) and use that boolean/value in the Altinn3ConsentService decision instead of Contains("tt02"); also, if you must compare strings, use a case-insensitive comparison (e.g., StringComparison.OrdinalIgnoreCase or ToLowerInvariant) when checking the environment value. Ensure the change affects the conditional that currently calls GetOrganizationName(subject) when the URI does not contain "tt02".
393-394: Use domain-specific exceptions instead of genericException.Lines 393 and 444 throw generic
Exception. Use a more specific exception type for better error handling upstream.Suggested fix for line 393
- throw new Exception($"AltinnResource was not defined on the ConsentRequirement for evidence code {ec.EvidenceCodeName}, which is required for creating the consent request"); + throw new InvalidOperationException($"AltinnResource was not defined on the ConsentRequirement for evidence code {ec.EvidenceCodeName}, which is required for creating the consent request");Suggested fix for line 444
- throw new Exception("Deserialize returned null"); + throw new ServiceNotAvailableException("Failed to deserialize consent response from Altinn");Also applies to: 444-444
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dan.Core/Services/Altinn3ConsentService.cs` around lines 393 - 394, Replace the generic throws in Altinn3ConsentService with a domain-specific exception: instead of throwing new Exception(...) in the blocks that validate ConsentRequirement (the two occurrences inside the method that references ec.EvidenceCodeName and AltinnResource), throw a custom exception such as InvalidConsentRequirementException or ConsentConfigurationException (or use InvalidOperationException if you prefer a BCL type), and include the same descriptive message; add the new exception class to the domain (if creating one) and update any callers or unit tests that expect the previous generic Exception to handle the specific error type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dan.Core/Config/Settings.cs`:
- Around line 106-109: The AltinnCertificateb64 getter directly calls
KeyVault.GetCertificateBase64 every time which bypasses the same source-path
logic used by AltinnCertificate and forces Key Vault access on each call; modify
AltinnCertificateb64 to follow the same resolution path as AltinnCertificate
(respecting IsUnitTest and SelfSignedCert branches) and return the base64 value
derived from that resolved certificate, and ensure the value is fetched/cached
once (e.g., compute once on first access or reuse the already-resolved
certificate) instead of calling KeyVault.GetCertificateBase64 on every access;
reference AltinnCertificateb64, AltinnCertificate, KeyVaultSslCertificate,
KeyVault.GetCertificateBase64, IsUnitTest and SelfSignedCert when making the
change.
In `@Dan.Core/Dan.Core.csproj`:
- Line 36: The project is referencing a pre-release package: replace or pin the
PackageReference Include="Altinn.Dd.Correspondence" Version="2.0.10-alpha" in
Dan.Core.csproj to a non-prerelease/stable version (or a specific released
version) so we don't depend on alpha artifacts; if you must continue using the
alpha temporarily, add a short code comment and a tracking TODO to update
PackageReference Version to the next stable release and consider adding a
lockfile or explicit version range to avoid unintended upgrades.
In `@Dan.Core/FuncConsentReceipt.cs`:
- Line 29: Remove the unused private field _consentService from the
FuncConsentReceipt class: locate the declaration "private readonly
IConsentService _consentService;" in FuncConsentReceipt and delete it (also
remove any now-unused using/import for IConsentService if present); ensure no
constructor parameter or assignment references remain and run a build to confirm
no other references to _consentService exist.
In `@Dan.Core/ServiceContextTexts/DrosjeloyveServiceContextTextTemplate.cs`:
- Around line 26-29: CorrespondenceBodyA3 currently includes HTML <br> tags in
both NoNb and NoNn variants; change these to plain-text newlines by replacing
each HTML break sequence (e.g., "<br>", "<br></br>") with "\n\n" so the property
remains a plain-text LocalizedString for the new correspondence channel—update
the string literals in the CorrespondenceBodyA3 getter (the NoNb and NoNn values
referencing TextMacros.RequestorName, TextMacros.SubjectName and
TextMacros.Button) to use "\n\n" instead of any HTML tags.
In `@Dan.Core/Services/Altinn3ConsentService.cs`:
- Line 84: The comparison of accreditation.ValidTo uses inconsistent time
sources (DateTime.Now at one place and DateTime.UtcNow elsewhere) which can
cause incorrect expiry checks; update all comparisons in Altinn3ConsentService
that reference accreditation.ValidTo (e.g., the if at accreditation.ValidTo <
DateTime.Now and the check around line 95) to use a single consistent time
basis—prefer DateTime.UtcNow—so both expiry checks compare UTC times
consistently.
- Around line 69-72: The code logs an error when
evidenceCodesRequiringConsent.Count == 0 but continues execution (in
Altinn3ConsentService when handling accreditation), which can produce invalid
downstream behavior; replace the current _logger.LogError(...) path with an
explicit failure: either throw a descriptive exception (e.g.,
InvalidOperationException) including accreditation.AccreditationId, or return a
defined failure/NoConsentRequired status from the method (choose the approach
consistent with the method's signature) so execution stops or the caller can
handle the absence of consent; update references to
evidenceCodesRequiringConsent and accreditation accordingly.
- Line 346: The IdempotencyKey currently uses Guid.NewGuid() in
SendCorrespondence which breaks idempotency; change it to a deterministic GUID
derived from stable identifiers (e.g., AccreditationId or a concatenation of
AccreditationId+recipient+template) instead of Guid.NewGuid(); add a helper like
GenerateDeterministicGuid(string input) in Altinn3ConsentService (or a utility
class) that computes a hash-based GUID (MD5/SHA1 of the input bytes) and use
GenerateDeterministicGuid(accreditationIdString) when setting IdempotencyKey in
SendCorrespondence so retries produce the same key.
- Around line 248-252: The catch in Altinn3ConsentService.cs currently logs a
misleading message and swallows the exception—fix by correcting the typo and
either actually removing the orphaned consent or surfacing the failure: update
the _logger.LogInformation call to a clear error (use _logger.LogError) with the
corrected message "Correspondence failed to send..." including
accreditation.AccreditationId, accreditation.SubjectParty.GetAsString(), and
consentresponse.Id, then either call the existing Altinn removal method (invoke
the method that removes consent requests for the given consentresponse.Id)
before returning, or rethrow the exception (throw;) after logging so the caller
is aware of the failure; ensure the change is made in the catch(Exception ex)
block where _logger.LogInformation currently resides.
- Around line 411-414: The token deserialization in Altinn3ConsentService (where
you call JsonConvert.DeserializeObject<Dictionary<string,string>>(await
_tokenRequesterService.GetMaskinportenToken("altinn:consentrequests.org")) and
then access token["access_token"]) lacks null and key checks; update the code to
safely parse and validate the result from GetMaskinportenToken (handle
invalid/empty JSON and JsonConvert returning null), verify
token.ContainsKey("access_token") before using it, and on failure log a clear
error (including the raw response or parse exception) and throw or return a
meaningful error instead of letting a
KeyNotFoundException/NullReferenceException bubble up. Ensure you reference the
token variable and the _tokenRequesterService.GetMaskinportenToken call when
implementing the checks and logging.
- Line 417: The current call
_logger.LogInformation(JsonConvert.SerializeObject(consentRequest)) in
Altinn3ConsentService risks logging PII; replace it by logging only
non-sensitive fields (e.g., consentRequest.Id, consentRequest.Status,
timestamps) or use structured logging with explicit properties and a redaction
helper instead of serializing the entire consentRequest object; update the code
path that references JsonConvert.SerializeObject(consentRequest) to construct a
safe anonymous/DTO with allowed fields or call a
redactConsentRequest(consentRequest) helper and pass those explicit fields to
_logger.LogInformation to avoid exposing organization numbers or subject
identifiers.
- Around line 99-100: The code reads the exp claim with
claims.FindFirst(ConsentJwtValidtoKey)?.Value and passes it to Convert.ToInt64,
which masks a missing or non-numeric claim and yields the epoch (1970) date;
update the logic in Altinn3ConsentService to safely handle a missing or invalid
exp: retrieve the Claim via claims.FindFirst(ConsentJwtValidtoKey), check for
null and try to parse its Value (e.g., long.TryParse) into secSince1970, and if
parsing fails return/throw a clear error or handle as expired instead of
silently producing 1970-01-01; update the variables secSince1970 and validTo
computation accordingly so validTo is only computed from a valid parsed value.
- Around line 121-124: The code in Altinn3ConsentService.cs directly accesses
jwt.Split('.')[1] when parsing the token; add bounds validation in the method
(the JWT parsing block) to ensure jwt is not null/empty and that jwt.Split('.')
yields at least 2 (preferably 3) parts before accessing index 1, and handle
malformed input by returning null or logging and returning null; update the code
around the localClaimsIdentityModel creation (the Base64Url.Decode and
JsonConvert.DeserializeObject calls) to only run after this validation to avoid
IndexOutOfRangeException.
In `@Dan.Core/Services/ConsentService.cs`:
- Around line 290-293: GetClaims currently calls GetJwt(accreditation,
accreditation.EvidenceCodes.First()), which forces an arbitrary evidence code
and triggers an InvalidOperationException when EvidenceCodes is empty; instead
update GetJwt to accept an optional evidenceCode (nullable or default parameter)
and change the call in GetClaims to call GetJwt(accreditation) (or pass null) so
you no longer call EvidenceCodes.First(); ensure the updated GetJwt
implementation and all its callers handle a null/absent evidenceCode and
preserve the original Check() behavior (i.e., log when no consent-requiring
evidence codes exist rather than throwing).
In `@Dan.Core/Services/EvidenceHarvesterService.cs`:
- Around line 320-338: GetMaskinportenConsentToken currently logs when the
deserialized jwt is null or missing "access_token" but continues and will cause
a NullReferenceException; update the method (around the call to
_a3ConsentService.GetJwt and the jwt variable) to validate that jwt is not null
AND jwt.ContainsKey("access_token") (or use TryGetValue), and if the token is
missing log the full response and then throw a clear exception (or return a
deterministic failure) instead of proceeding to return jwt["access_token"];
ensure the log includes the response and accreditation identifiers to aid
debugging.
In `@Dan.Core/Services/Interfaces/IConsentService.cs`:
- Around line 38-40: The XML doc for GetJwt has a mismatched param name: change
the <param> element from "evidenceCodes" to "evidenceCode" to match the method
signature Task<string> GetJwt(Accreditation accreditation, EvidenceCode
evidenceCode); in IConsentService (update the <param name="..."> token only).
In `@Dan.Core/Services/Interfaces/ITokenRequesterService.cs`:
- Line 11: The method signature in ITokenRequesterService for
GetMaskinportenConsentToken has a misleading parameter name: change the
parameter named evidenceCodes to the singular evidenceCode to match its type
EvidenceCode; also rename offeredby to offeredBy to follow C# naming
conventions. Update the interface method signature for
GetMaskinportenConsentToken and then update all implementations/usages of
ITokenRequesterService (methods named GetMaskinportenConsentToken and any
callers) to use the new parameter names and corresponding variable names and XML
docs so compilation and references remain consistent.
---
Outside diff comments:
In `@Dan.Core/Services/ServiceContextService.cs`:
- Around line 11-229: Remove the dead OkinfoServiceContextTextTemplate class
file: locate the symbol OkinfoServiceContextTextTemplate (file
OkinfoServiceContextTextTemplate.cs), delete that file, and remove any project
file (.csproj) references or using/imports that reference it; confirm there are
no remaining references to the removed service IDs (okinfo-product,
digokfriv-product) in ServiceContextService.GetRegisteredServiceContexts or
elsewhere and run a build to verify no compile errors remain.
---
Nitpick comments:
In `@Dan.Common/Dan.Common.csproj`:
- Around line 37-38: The package bumps for Microsoft.Azure.Functions.Worker and
Microsoft.Azure.Functions.Worker.ApplicationInsights in Dan.Common.csproj (the
PackageReference entries for those two packages) are large; review the
CHANGELOG/release notes for v2.1.0→v2.51.0 and v2.0.0→v2.50.0 to confirm
OpenTelemetry changes (OTel v1.1.0 and Semantic Conventions v1.37) and any
breaking or behavioral changes, run integration tests exercising observability
(traces/metrics/logging) against ApplicationInsights and the Functions host, and
either pin to a safer intermediate version or add configuration adjustments
(OTel SDK/semantic convention mappings) in your telemetry initialization code if
incompatibilities are found.
In `@Dan.Core.UnitTest/EvidenceStatusServiceTest.cs`:
- Around line 22-23: Add a unit test in EvidenceStatusServiceTest that sets up a
request/entity with Altinn3ConsentId populated so the EvidenceStatusService
routes to the Altinn3 path; arrange the fake IAltinn3ConsentService
(_mockConsentServiceA3) to return a known result from its Check(...) method,
call the EvidenceStatusService method under test, then assert that
_mockConsentServiceA3.Check(...) was invoked and that the service
returned/handled the expected result (and optionally assert that the other
consent service was not called) to lock down the new routing logic.
In `@Dan.Core/Config/KeyVault.cs`:
- Around line 60-65: The GetCertificateBase64 method unnecessarily uses
async/await and Task.FromResult around an already-async Get(string) call; change
GetCertificateBase64 to directly return the Task<string> from Get(key) (i.e.,
remove the async modifier, remove awaiting Task.FromResult, and simply return
Get(key)) so there’s no extra state machine or overhead while preserving the
same signature and behavior.
In `@Dan.Core/FuncAuthorization.cs`:
- Around line 110-130: The Altinn3 consent path (consentRequirementsA3 /
_a3ConsentService.Initiate) logs start and completion but omits elapsedMs; start
or reuse the same stopwatch (t) around the _a3ConsentService.Initiate call and
include elapsedMs={elapsedMs} in the completion log to match the Altinn2
pattern, and while here simplify the LINQ filters by using pattern matching
(e.g., AuthorizationRequirements.Where(r => r is ConsentRequirement cr &&
!string.IsNullOrEmpty(cr.AltinnResource)) and similarly for the Altinn2 branch)
to make the consentRequirementsA3/consentRequirementsA2 declarations cleaner.
In `@Dan.Core/Helpers/TextTemplateProcessor.cs`:
- Line 73: The line in TextTemplateProcessor that assigns to input uses a nested
ternary which hurts readability; instead extract the replacement logic into a
small conditional or local variable: check if buttonText is null/empty first,
then if isAltinn3 choose GetConsentButtonMd(buttonText, consentUrl) and call
input.Replace(TextMacros.Button, ... ,
StringComparison.InvariantCultureIgnoreCase), otherwise call
input.Replace(TextMacros.Button, GetConsentButton(buttonText, consentUrl));
update the input variable with the result—this keeps TextMacros.Button,
GetConsentButtonMd, GetConsentButton, buttonText, isAltinn3, consentUrl and
input semantics the same but much clearer.
In `@Dan.Core/Program.cs`:
- Around line 261-277: The repeated Settings.MaskinportenUrl.Contains("test")
check in AddAltinn3Messaging is fragile; extract the environment detection into
a single local variable or helper (e.g. bool isTestEnvironment =
MaskinportenUrlContainsTest(...) or a method GetMaskinportenEnvironment()) and
reuse it to set MaskinportenSettings.Environment, EnableDebugLogging, and the
options.Environment (ApiEnvironment.Staging/Production), and use that single
source when setting EncodedX509 and ResourceId in AddDdCorrespondenceService so
the logic is centralized and consistent (refer to AddAltinn3Messaging,
MaskinportenSettings, Settings.MaskinportenUrl, and ApiEnvironment).
- Line 33: The using directive System.Runtime.ConstrainedExecution in Program.cs
is unused; remove that using line (the System.Runtime.ConstrainedExecution
import) from the file and then run the build or apply "Organize Usings" / remove
unused usings to ensure no other unused imports remain.
In `@Dan.Core/ServiceContextTexts/DanTestServiceContextTextTemplate.cs`:
- Around line 24-28: Add a blank line between the property definitions
CorrespondenceBodyA3 and CorrespondenceSender to match the file's existing
formatting convention; locate the CorrespondenceBodyA3 getter and insert one
blank line before the start of the CorrespondenceSender property so all property
blocks remain consistently separated.
In `@Dan.Core/Services/Altinn3ConsentService.cs`:
- Line 219: Remove or document the commented-out EnsureSrrRights call in
Altinn3ConsentService: either delete the commented line "// await
_altinnServiceOwnerApiService.EnsureSrrRights(...)" if it is obsolete, or
replace it with a short TODO comment explaining why it is disabled and under
what conditions it should be re-enabled (include the method name
EnsureSrrRights, the involved variables accreditation.Requestor and
accreditation.ValidTo, and evidenceCodesRequiringConsent in the comment for
clarity).
- Line 412: Remove the leftover commented-out header line in
Altinn3ConsentService: delete the commented call to
request.Headers.TryAddWithoutValidation("ApiKey", Settings.AltinnApiKey) since
it's unused; ensure no other references to Settings.AltinnApiKey remain in the
same method so the code is clean.
- Around line 257-261: Add a TODO tracking marker to the stubbed LogUse method
so it’s easy to find and implement later: modify the LogUse(Accreditation
accreditation, EvidenceCode evidence, DateTime? dateTime) method in
Altinn3ConsentService to include a clear `// TODO:` comment above or inside the
method (e.g. `// TODO: Implement LogUse for Altinn3 - currently returns default
true`) while keeping the existing Task.FromResult(true) return so behavior
remains unchanged.
- Line 25: The private field _noCertHttpClient in class Altinn3ConsentService is
injected but never referenced; remove the unused field and any corresponding
constructor parameter or DI registration if present (search for
_noCertHttpClient and the Altinn3ConsentService constructor) so the class only
keeps used dependencies; if the constructor still requires an HttpClient
parameter, drop that parameter and update callers/DI to stop providing it.
- Around line 326-329: The catch block that catches ServiceNotAvailableException
and immediately rethrows it in Altinn3ConsentService (the catch
(ServiceNotAvailableException ex) { throw; } block) is unnecessary; remove that
catch clause so the exception will propagate naturally, or if special
handling/logging was intended use an exception filter (e.g., catch
(ServiceNotAvailableException ex) when (LogAndReturnFalse(ex)) or add the
specific handling) — locate the catch in the Altinn3ConsentService method and
delete the redundant catch-and-rethrow.
- Line 389: The current use of Single() on ec.AuthorizationRequirements can
throw a generic InvalidOperationException; replace it with explicit validation:
extract the consent requirements via
ec.AuthorizationRequirements.OfType<ConsentRequirement>().ToList(), check that
the list.Count == 1, and if not throw a new InvalidOperationException with a
clear message including ec (or its id) and the actual count, otherwise read the
single element's AltinnResource; update the reference to ConsentRequirement and
AltinnResource accordingly so the error is informative when the invariant is
violated.
- Line 26: The logger generic is incorrect: change the field and constructor
parameter from ILogger<ConsentService> to ILogger<Altinn3ConsentService> so the
logger reflects this class; update the private readonly field _logger and the
Altinn3ConsentService constructor parameter type to
ILogger<Altinn3ConsentService> (and adjust any assignments) to ensure logs are
attributed to Altinn3ConsentService.
- Around line 288-291: The code currently detects environment by checking if
Settings.AltinnServiceOwnerApiUri contains "tt02", which is fragile; replace
this URL string sniff with an explicit configuration flag or environment setting
(e.g., add Settings.IsAltinnTestEnvironment or Settings.AltinnEnvironment) and
use that boolean/value in the Altinn3ConsentService decision instead of
Contains("tt02"); also, if you must compare strings, use a case-insensitive
comparison (e.g., StringComparison.OrdinalIgnoreCase or ToLowerInvariant) when
checking the environment value. Ensure the change affects the conditional that
currently calls GetOrganizationName(subject) when the URI does not contain
"tt02".
- Around line 393-394: Replace the generic throws in Altinn3ConsentService with
a domain-specific exception: instead of throwing new Exception(...) in the
blocks that validate ConsentRequirement (the two occurrences inside the method
that references ec.EvidenceCodeName and AltinnResource), throw a custom
exception such as InvalidConsentRequirementException or
ConsentConfigurationException (or use InvalidOperationException if you prefer a
BCL type), and include the same descriptive message; add the new exception class
to the domain (if creating one) and update any callers or unit tests that expect
the previous generic Exception to handle the specific error type.
In `@Dan.Core/Services/EvidenceHarvesterService.cs`:
- Around line 301-318: Remove the dead GetConsentToken method (private async
Task<string> GetConsentToken(...)) since it is no longer used and has been
superseded by GetMaskinportenConsentToken; delete the entire method body and its
Timer/logging block and ensure no remaining references to GetConsentToken exist
in EvidenceHarvesterService or tests, and if that removal makes any using
directives or private members unused (e.g., the local Timer usage), clean those
up as well.
In `@Dan.Core/Services/Interfaces/IAltinn3ConsentService.cs`:
- Around line 1-9: Remove the two unused using directives by deleting the
imports for Dan.Common.Enums and Dan.Common.Models at the top of the
IAltinn3ConsentService file; leave the public interface IAltinn3ConsentService :
IConsentService declaration intact (the marker interface is fine for DI) so only
the unused using statements are removed.
In `@Dan.Core/Services/TokenRequesterService.cs`:
- Around line 130-135: GetCacheKey can produce ambiguous keys because components
(offeredby, evidenceCodeName, consumerOrgNo) are concatenated with underscores;
instead ensure uniqueness by encoding or delimiting components unambiguously
(e.g., Base64/Hex-encode each component or prefix each with its length) before
building the final key in GetCacheKey so underscores in values cannot cause
collisions; update the GetCacheKey implementation to transform consumerOrgNo,
offeredby, and evidenceCodeName (and optionally Settings.MaskinportenClientId)
with a reversible/unique encoding or length-prefixing and then concatenate with
a stable separator and the scopeDigest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53379a4b-92b3-4db5-b6aa-6cef22d07175
📒 Files selected for processing (35)
Dan.Common.UnitTest/Dan.Common.UnitTest.csprojDan.Common/Dan.Common.csprojDan.Common/Interfaces/IServiceContextTextTemplate.csDan.Common/Models/Party.csDan.Core.UnitTest/A3ConsentServiceTest.csDan.Core.UnitTest/Dan.Core.UnitTest.csprojDan.Core.UnitTest/EvidenceHarvesterServiceTest.csDan.Core.UnitTest/EvidenceStatusServiceTest.csDan.Core.UnitTest/appsettings.unittest.jsonDan.Core/Config/KeyVault.csDan.Core/Config/Settings.csDan.Core/Dan.Core.csprojDan.Core/FuncAuthorization.csDan.Core/FuncConsentReceipt.csDan.Core/FuncEvidenceHarvester.csDan.Core/Helpers/TextTemplateProcessor.csDan.Core/Models/Altinn3ConsentError.csDan.Core/Program.csDan.Core/ServiceContextTexts/DanTestServiceContextTextTemplate.csDan.Core/ServiceContextTexts/DrosjeloyveServiceContextTextTemplate.csDan.Core/ServiceContextTexts/EBevisServiceContextTextTemplate.csDan.Core/ServiceContextTexts/EuresServiceContextTextTemplate.csDan.Core/ServiceContextTexts/OkinfoServiceContextTextTemplate.csDan.Core/ServiceContextTexts/RenderedServiceContextTexts.csDan.Core/ServiceContextTexts/SeriositetServiceContextTextTemplate.csDan.Core/Services/Altinn3ConsentService.csDan.Core/Services/ConsentService.csDan.Core/Services/EvidenceHarvesterService.csDan.Core/Services/EvidenceStatusService.csDan.Core/Services/Interfaces/IAltinn3ConsentService.csDan.Core/Services/Interfaces/IConsentService.csDan.Core/Services/Interfaces/ITokenRequesterService.csDan.Core/Services/ServiceContextService.csDan.Core/Services/TokenRequesterService.csDan.PluginTest/Dan.PluginTest.csproj
Comment on lines
+106
to
+109
| /// <summary> | ||
| /// Altinn EC Certificate in base64 | ||
| /// </summary> | ||
| public static string AltinnCertificateb64 => KeyVault.GetCertificateBase64(KeyVaultSslCertificate).Result; |
Contributor
There was a problem hiding this comment.
Keep AltinnCertificateb64 on the same source path as AltinnCertificate.
This accessor always hits Key Vault, so the Altinn 3 correspondence setup can bypass the existing IsUnitTest / SelfSignedCert branch and resolve a different certificate than the rest of the app. That also means a fresh Key Vault fetch on every access.
Suggested fix
@@
private static string? _maskinportenClientId;
private static List<string>? _consentValidationSecrets;
+ private static string? _altinnCertificateb64;
@@
- public static string AltinnCertificateb64 => KeyVault.GetCertificateBase64(KeyVaultSslCertificate).Result;
+ public static string AltinnCertificateb64 =>
+ _altinnCertificateb64 ??= IsUnitTest
+ ? GetSetting("SelfSignedCert")
+ : KeyVault.GetCertificateBase64(KeyVaultSslCertificate).Result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dan.Core/Config/Settings.cs` around lines 106 - 109, The AltinnCertificateb64
getter directly calls KeyVault.GetCertificateBase64 every time which bypasses
the same source-path logic used by AltinnCertificate and forces Key Vault access
on each call; modify AltinnCertificateb64 to follow the same resolution path as
AltinnCertificate (respecting IsUnitTest and SelfSignedCert branches) and return
the base64 value derived from that resolved certificate, and ensure the value is
fetched/cached once (e.g., compute once on first access or reuse the
already-resolved certificate) instead of calling KeyVault.GetCertificateBase64
on every access; reference AltinnCertificateb64, AltinnCertificate,
KeyVaultSslCertificate, KeyVault.GetCertificateBase64, IsUnitTest and
SelfSignedCert when making the change.
| </ItemGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="Azure.Identity" Version="1.16.0" /> | ||
| <PackageReference Include="Altinn.Dd.Correspondence" Version="2.0.10-alpha" /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Altinn.Dd.Correspondence NuGet latest version
💡 Result:
The latest version of the Altinn.Dd.Correspondence NuGet package is 2.0.10-alpha.
Citations:
- 1: https://nugetprodusnc.azure-api.net/packages/Altinn.Dd.Correspondence/2.0.10-alpha
- 2: https://nugetprodusnc.azure-api.net/packages/Altinn.Dd.Correspondence/2.0.3-alpha
Alpha package in use.
Altinn.Dd.Correspondence version 2.0.10-alpha is a pre-release package, and only alpha versions are currently available. Alpha versions may have stability issues or breaking changes without notice. Ensure the team is aware of the stability implications and monitor for future stable releases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dan.Core/Dan.Core.csproj` at line 36, The project is referencing a
pre-release package: replace or pin the PackageReference
Include="Altinn.Dd.Correspondence" Version="2.0.10-alpha" in Dan.Core.csproj to
a non-prerelease/stable version (or a specific released version) so we don't
depend on alpha artifacts; if you must continue using the alpha temporarily, add
a short code comment and a tracking TODO to update PackageReference Version to
the next stable release and consider adding a lockfile or explicit version range
to avoid unintended upgrades.
Comment on lines
+69
to
+72
| if (evidenceCodesRequiringConsent.Count == 0) | ||
| { | ||
| _logger.LogError("Expected at least one evidencecode in the accreditation requiring consent in accredition {accreditationId}", accreditation.AccreditationId); | ||
| } |
Contributor
There was a problem hiding this comment.
Error logged but execution continues without consent requirement.
When no evidence codes require consent, an error is logged but the method continues. This could lead to unexpected behavior downstream. Consider whether this should throw an exception or return a specific status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dan.Core/Services/Altinn3ConsentService.cs` around lines 69 - 72, The code
logs an error when evidenceCodesRequiringConsent.Count == 0 but continues
execution (in Altinn3ConsentService when handling accreditation), which can
produce invalid downstream behavior; replace the current _logger.LogError(...)
path with an explicit failure: either throw a descriptive exception (e.g.,
InvalidOperationException) including accreditation.AccreditationId, or return a
defined failure/NoConsentRequired status from the method (choose the approach
consistent with the method's signature) so execution stops or the caller can
handle the absence of consent; update references to
evidenceCodesRequiringConsent and accreditation accordingly.
| request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {token["access_token"]}"); | ||
| request.JsonContent(consentRequest); | ||
|
|
||
| _logger.LogInformation(JsonConvert.SerializeObject(consentRequest)); |
Contributor
There was a problem hiding this comment.
Potential PII exposure in logs.
Logging the full serialized consentRequest may expose sensitive data including organization numbers and subject identifiers. Consider logging only non-sensitive fields or using structured logging with redaction.
Suggested fix
- _logger.LogInformation(JsonConvert.SerializeObject(consentRequest));
+ _logger.LogInformation("Creating consent request for AccreditationId={accreditationId}, ConsentRequestId={consentRequestId}",
+ accreditation.AccreditationId, consentRequest.Id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _logger.LogInformation(JsonConvert.SerializeObject(consentRequest)); | |
| _logger.LogInformation("Creating consent request for AccreditationId={accreditationId}, ConsentRequestId={consentRequestId}", | |
| accreditation.AccreditationId, consentRequest.Id); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dan.Core/Services/Altinn3ConsentService.cs` at line 417, The current call
_logger.LogInformation(JsonConvert.SerializeObject(consentRequest)) in
Altinn3ConsentService risks logging PII; replace it by logging only
non-sensitive fields (e.g., consentRequest.Id, consentRequest.Status,
timestamps) or use structured logging with explicit properties and a redaction
helper instead of serializing the entire consentRequest object; update the code
path that references JsonConvert.SerializeObject(consentRequest) to construct a
safe anonymous/DTO with allowed fields or call a
redactConsentRequest(consentRequest) helper and pass those explicit fields to
_logger.LogInformation to avoid exposing organization numbers or subject
identifiers.
Comment on lines
290
to
293
| private async Task<ClaimsIdentity?> GetClaims(Accreditation accreditation) | ||
| { | ||
| var jwt = await GetJwt(accreditation); | ||
| var jwt = await GetJwt(accreditation, accreditation.EvidenceCodes.First()); //evidencecode is not needed for getting the JWT, so we can just pass one of them in | ||
|
|
Contributor
There was a problem hiding this comment.
Don't call First() on EvidenceCodes just to satisfy the new GetJwt signature.
Check() only logs when no consent-requiring evidence codes are present. This change turns that case into an InvalidOperationException, and it also picks an arbitrary evidence code if the first entry does not require consent.
Suggested fix
private async Task<ClaimsIdentity?> GetClaims(Accreditation accreditation)
{
- var jwt = await GetJwt(accreditation, accreditation.EvidenceCodes.First()); //evidencecode is not needed for getting the JWT, so we can just pass one of them in
+ var evidenceCode = GetEvidenceCodesRequiringConsentForActiveContext(accreditation).FirstOrDefault();
+ if (evidenceCode == null)
+ {
+ _logger.LogError("Unable to resolve a consent-requiring evidence code for AccreditationId={accreditationId}", accreditation.AccreditationId);
+ return null;
+ }
+
+ var jwt = await GetJwt(accreditation, evidenceCode);
if (string.IsNullOrEmpty(jwt))
{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dan.Core/Services/ConsentService.cs` around lines 290 - 293, GetClaims
currently calls GetJwt(accreditation, accreditation.EvidenceCodes.First()),
which forces an arbitrary evidence code and triggers an
InvalidOperationException when EvidenceCodes is empty; instead update GetJwt to
accept an optional evidenceCode (nullable or default parameter) and change the
call in GetClaims to call GetJwt(accreditation) (or pass null) so you no longer
call EvidenceCodes.First(); ensure the updated GetJwt implementation and all its
callers handle a null/absent evidenceCode and preserve the original Check()
behavior (i.e., log when no consent-requiring evidence codes exist rather than
throwing).
Comment on lines
+320
to
+338
| private async Task<string> GetMaskinportenConsentToken(EvidenceCode evidenceCode, Accreditation accreditation) | ||
| { | ||
| using (var _ = _log.Timer("jwt-fetch")) | ||
| { | ||
| _log.LogInformation( | ||
| "Getting JWT | aid={accreditationId}, evidenceCode={evidenceCodeName}, authCode={authorizationCode}, a3consentid={a3consentid}", | ||
| accreditation.AccreditationId, evidenceCode.EvidenceCodeName, accreditation.AuthorizationCode, accreditation.Altinn3ConsentId); | ||
|
|
||
| string response = await _a3ConsentService.GetJwt(accreditation, evidenceCode); | ||
| var jwt = JsonConvert.DeserializeObject<Dictionary<string, string>>(response); | ||
|
|
||
| if (jwt?["access_token"] == null) | ||
| _log.LogInformation( | ||
| "Completed JWT | aid={accreditationId}, evidenceCode={evidenceCodeName}, authCode={authorizationCode}, jwt={jwt}", | ||
| accreditation.AccreditationId, evidenceCode.EvidenceCodeName, accreditation.AuthorizationCode, jwt); | ||
|
|
||
| return jwt["access_token"]; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Null check doesn't prevent NullReferenceException.
The null check at line 331 logs information but doesn't return or throw, allowing execution to continue to line 336 where jwt["access_token"] will throw a NullReferenceException if jwt is null or the key is missing.
🐛 Fix the null handling
private async Task<string> GetMaskinportenConsentToken(EvidenceCode evidenceCode, Accreditation accreditation)
{
using (var _ = _log.Timer("jwt-fetch"))
{
_log.LogInformation(
"Getting JWT | aid={accreditationId}, evidenceCode={evidenceCodeName}, authCode={authorizationCode}, a3consentid={a3consentid}",
accreditation.AccreditationId, evidenceCode.EvidenceCodeName, accreditation.AuthorizationCode, accreditation.Altinn3ConsentId);
string response = await _a3ConsentService.GetJwt(accreditation, evidenceCode);
var jwt = JsonConvert.DeserializeObject<Dictionary<string, string>>(response);
- if (jwt?["access_token"] == null)
+ if (jwt == null || !jwt.ContainsKey("access_token") || jwt["access_token"] == null)
+ {
+ _log.LogError(
+ "Failed parsing consent token | aid={accreditationId}, evidenceCode={evidenceCodeName}",
+ accreditation.AccreditationId, evidenceCode.EvidenceCodeName);
+ throw new ServiceNotAvailableException("Failed to retrieve consent token from Maskinporten");
+ }
+
_log.LogInformation(
- "Completed JWT | aid={accreditationId}, evidenceCode={evidenceCodeName}, authCode={authorizationCode}, jwt={jwt}",
- accreditation.AccreditationId, evidenceCode.EvidenceCodeName, accreditation.AuthorizationCode, jwt);
+ "Completed JWT | aid={accreditationId}, evidenceCode={evidenceCodeName}, authCode={authorizationCode}",
+ accreditation.AccreditationId, evidenceCode.EvidenceCodeName, accreditation.AuthorizationCode);
return jwt["access_token"];
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dan.Core/Services/EvidenceHarvesterService.cs` around lines 320 - 338,
GetMaskinportenConsentToken currently logs when the deserialized jwt is null or
missing "access_token" but continues and will cause a NullReferenceException;
update the method (around the call to _a3ConsentService.GetJwt and the jwt
variable) to validate that jwt is not null AND jwt.ContainsKey("access_token")
(or use TryGetValue), and if the token is missing log the full response and then
throw a clear exception (or return a deterministic failure) instead of
proceeding to return jwt["access_token"]; ensure the log includes the response
and accreditation identifiers to aid debugging.
Comment on lines
+38
to
+40
| /// <param name="evidenceCodes">Evidence code requiring consent</param> | ||
| /// <returns>A JWT</returns> | ||
| Task<string> GetJwt(Accreditation accreditation); | ||
| Task<string> GetJwt(Accreditation accreditation, EvidenceCode evidenceCode); |
Contributor
There was a problem hiding this comment.
Fix XML documentation parameter name.
The <param> element says evidenceCodes (plural) but the parameter is evidenceCode (singular).
📝 Proposed fix
- /// <param name="evidenceCodes">Evidence code requiring consent</param>
+ /// <param name="evidenceCode">Evidence code requiring consent</param>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// <param name="evidenceCodes">Evidence code requiring consent</param> | |
| /// <returns>A JWT</returns> | |
| Task<string> GetJwt(Accreditation accreditation); | |
| Task<string> GetJwt(Accreditation accreditation, EvidenceCode evidenceCode); | |
| /// <param name="evidenceCode">Evidence code requiring consent</param> | |
| /// <returns>A JWT</returns> | |
| Task<string> GetJwt(Accreditation accreditation, EvidenceCode evidenceCode); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dan.Core/Services/Interfaces/IConsentService.cs` around lines 38 - 40, The
XML doc for GetJwt has a mismatched param name: change the <param> element from
"evidenceCodes" to "evidenceCode" to match the method signature Task<string>
GetJwt(Accreditation accreditation, EvidenceCode evidenceCode); in
IConsentService (update the <param name="..."> token only).
| public interface ITokenRequesterService | ||
| { | ||
| Task<string> GetMaskinportenToken(string scopes, string? consumerOrgNo = null); | ||
| Task<string> GetMaskinportenConsentToken(string consentId, string offeredby, EvidenceCode evidenceCodes); |
Contributor
There was a problem hiding this comment.
Parameter naming inconsistency: evidenceCodes should be singular.
The parameter type is EvidenceCode (singular), but the name evidenceCodes implies a collection. This inconsistency could confuse callers.
✏️ Suggested fix
- Task<string> GetMaskinportenConsentToken(string consentId, string offeredby, EvidenceCode evidenceCodes);
+ Task<string> GetMaskinportenConsentToken(string consentId, string offeredBy, EvidenceCode evidenceCode);Note: Also consider using PascalCase for offeredBy to follow C# naming conventions for parameters (though camelCase is also acceptable).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Task<string> GetMaskinportenConsentToken(string consentId, string offeredby, EvidenceCode evidenceCodes); | |
| Task<string> GetMaskinportenConsentToken(string consentId, string offeredBy, EvidenceCode evidenceCode); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dan.Core/Services/Interfaces/ITokenRequesterService.cs` at line 11, The
method signature in ITokenRequesterService for GetMaskinportenConsentToken has a
misleading parameter name: change the parameter named evidenceCodes to the
singular evidenceCode to match its type EvidenceCode; also rename offeredby to
offeredBy to follow C# naming conventions. Update the interface method signature
for GetMaskinportenConsentToken and then update all implementations/usages of
ITokenRequesterService (methods named GetMaskinportenConsentToken and any
callers) to use the new parameter names and corresponding variable names and XML
docs so compilation and references remain consistent.
SondreJDigdir
approved these changes
Mar 25, 2026
|
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
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.
Description
move consent, correspondence and notifications to altinn 3, also consent tokens are now retrieved from maskinporten
Documentation
Summary by CodeRabbit
New Features
Chores
Tests