Implement AuthenticationMethod support in IInstanceDataAccessor - #1472
Conversation
📝 WalkthroughWalkthroughAdds IAppMetadata injections across signing components and the SigningController; introduces per-data-type storage authentication overrides on IInstanceDataAccessor and implements them in the unit-of-work; adds extensions to apply service-owner auth for restricted data types; wires these into signing flows and updates tests and public API snapshots. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
/publish |
danielskovli
added
bugfix
danielskovli
added
kind/incident-B
|
/publish |
PR release:
|
… Move shared core for this into IInstanceDataAccessor extension method.
|
/publish |
PR release:
|
|
/publish |
PR release:
|
danielskovli
changed the title
AuthenticationMethod support in IInstanceDataAccessor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (1)
23-29: Filter first, then iterate (tiny readability win).Inline restriction filtering makes the loop intent explicit.
-foreach (DataType signatureDataType in signatureDataTypes) -{ - if (IsRestrictedDataType(signatureDataType)) - { - accessor.SetAuthenticationMethod(signatureDataType, StorageAuthenticationMethod.ServiceOwner()); - } -} +foreach (DataType dt in signatureDataTypes.Where(IsRestrictedDataType)) +{ + accessor.SetAuthenticationMethod(dt, StorageAuthenticationMethod.ServiceOwner()); +}
🧹 Nitpick comments (11)
src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs (1)
85-98: Remove unused helpers (duplication + dead code)
GetDataType,GetDataTypes, andIsRestrictedDataTypeare not used here and duplicate logic under SigningInstanceDataAccessorExtensions. Keep single source to avoid drift and drop the unusedSystem.Diagnostics.CodeAnalysisusing.-using System.Diagnostics.CodeAnalysis; @@ - private static DataType? GetDataType(ApplicationMetadata appMetadata, string? dataTypeId) => - dataTypeId is null - ? null - : appMetadata.DataTypes.FirstOrDefault(x => x.Id.Equals(dataTypeId, StringComparison.OrdinalIgnoreCase)); - - private static IEnumerable<DataType> GetDataTypes( - ApplicationMetadata appMetadata, - IEnumerable<string?> dataTypeIds - ) => dataTypeIds.Select(dataTypeId => GetDataType(appMetadata, dataTypeId)).OfType<DataType>(); - - private static bool IsRestrictedDataType([NotNullWhen(true)] DataType? dataType) => - !string.IsNullOrWhiteSpace(dataType?.ActionRequiredToRead) - || !string.IsNullOrWhiteSpace(dataType?.ActionRequiredToWrite); + // Removed unused helpers; keep this class focused on task orchestration.test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs (1)
98-101: Test double should not throw; implement no-op or store overridesThrowing in the fake can break tests that invoke the new API indirectly. Make it harmless.
- public void SetAuthenticationMethod(DataType dataType, StorageAuthenticationMethod method) - { - throw new NotImplementedException(); - } + public void SetAuthenticationMethod(DataType dataType, StorageAuthenticationMethod method) + { + // No-op in tests; can be extended to track overrides if needed. + _authOverrides[dataType.Id] = method; + }Add a backing field (outside this hunk) to retain calls:
// near other fields private readonly Dictionary<string, StorageAuthenticationMethod> _authOverrides = new();src/Altinn.App.Api/Controllers/SigningController.cs (1)
38-45: Constructor injects IAppMetadata but doesn't use itThe new dependency is unused; consider either wiring it for future use or dropping it to avoid API surface churn. If you intend to keep it, assign to a field to document intent and silence analyzers.
@@ public class SigningController : ControllerBase { private readonly IInstanceClient _instanceClient; private readonly IProcessReader _processReader; private readonly IAuthenticationContext _authenticationContext; + private readonly IAppMetadata _appMetadata; private readonly ILogger<SigningController> _logger; @@ IAuthenticationContext authenticationContext, IAppMetadata appMetadata, ILogger<SigningController> logger ) { _instanceClient = instanceClient; _processReader = processReader; _authenticationContext = authenticationContext; + _appMetadata = appMetadata; _logger = logger;Confirm this public API change is intentional for apps consuming Altinn.App.Api; otherwise revert and keep the dependency internal to services.
src/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cs (2)
21-26: Use the correct logger categoryLogger is typed as ILogger. Prefer ILogger for accurate logging scopes.
-internal sealed class SignDocumentManager( +internal sealed class SignDocumentManager( IAltinnPartyClient altinnPartyClient, IAppMetadata appMetadata, - ILogger<SigningService> logger, + ILogger<SignDocumentManager> logger, Telemetry? telemetry = null ) : ISignDocumentManager { - private readonly ILogger<SigningService> _logger = logger; + private readonly ILogger<SignDocumentManager> _logger = logger;Also applies to: 38-39
148-151: Avoid extra allocation when decoding UTF-8You can decode from ReadOnlySpan without ToArray().
- string signDocumentSerialized = Encoding.UTF8.GetString(data.ToArray()); + string signDocumentSerialized = Encoding.UTF8.GetString(data.Span);src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs (1)
113-116: Unreachable null-coalescing after First(...)First(...) throws if not found; the null-coalescing throw never runs. Use FirstOrDefault and then null-check so the intended ApplicationConfigException is thrown.
- DataType signatureDateType = - appMetadataResult.Ok.DataTypes.First(x => x.Id == signingConfiguration.SignatureDataType) - ?? throw new ApplicationConfigException("Didn't find signature data type in app metadata"); + DataType signatureDateType = + appMetadataResult.Ok.DataTypes.FirstOrDefault(x => x.Id == signingConfiguration.SignatureDataType) + ?? throw new ApplicationConfigException("Didn't find signature data type in app metadata");src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1)
119-121: Consistent propagation of authenticationMethod — minor nits.
- Good: All storage ops now carry the resolved auth method.
- Nit: Name the boolean argument in DeleteData for clarity.
- Optional: Consider disposing the temporary stream locally to avoid ownership ambiguity.
- await _dataClient.DeleteData(..., - false, - authenticationMethod: GetAuthenticationMethod(change.DataElementIdentifier) - ); + await _dataClient.DeleteData(..., + hard: false, + authenticationMethod: GetAuthenticationMethod(change.DataElementIdentifier) + );If DataClient does not take ownership of the stream, prefer:
- new MemoryAsStream(bytes), + using var stream = new MemoryAsStream(bytes); + // pass 'stream' belowPlease confirm DataClient’s ownership contract before changing.
Also applies to: 389-391, 412-414, 451-453
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs (1)
26-45: Good test adaptation; add a case for restricted types.Nice: injecting IAppMetadata and centralizing SignatureDataTypeId. Add a test where the data type is “restricted” (ActionRequiredToRead/Write set) and assert that SetAuthenticationMethod is invoked with ServiceOwner before downloads.
I can draft a new test that configures ApplicationMetadata with a restricted DataType and verifies a Mock receives SetAuthenticationMethod(ServiceOwner()).
Also applies to: 197-198, 272-273, 306-312
src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (1)
15-19: Accept IEnumerable for flexibility and add null guards.Avoids forcing array allocations at call sites and validates inputs.
-public static void SetServiceOwnerAuthForRestrictedDataTypes( - this IInstanceDataAccessor accessor, - ApplicationMetadata appMetadata, - string?[] dataTypeIds -) +public static void SetServiceOwnerAuthForRestrictedDataTypes( + this IInstanceDataAccessor accessor, + ApplicationMetadata appMetadata, + IEnumerable<string?> dataTypeIds +) { - IEnumerable<DataType> signatureDataTypes = GetDataTypes(appMetadata, dataTypeIds); + ArgumentNullException.ThrowIfNull(appMetadata); + ArgumentNullException.ThrowIfNull(dataTypeIds); + IEnumerable<DataType> signatureDataTypes = GetDataTypes(appMetadata, dataTypeIds); } -private static IEnumerable<DataType> GetDataTypes( +private static IEnumerable<DataType> GetDataTypes( ApplicationMetadata appMetadata, - IEnumerable<string?> dataTypeIds + IEnumerable<string?> dataTypeIds ) => dataTypeIds.Select(dataTypeId => GetDataType(appMetadata, dataTypeId)).OfType<DataType>();Also applies to: 32-35
src/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs (2)
143-158: Consider person email fallback to mirror SMS fallback.You already use org or person mobile for SMS. For email, also consider person email if available to improve delivery.
Please confirm whether Person has an email property in Altinn.Platform.Register.Models (naming may be Email or EMailAddress) before adding the fallback.
192-193: Log with context (data type id).Including the SigneeStatesDataTypeId helps troubleshooting.
-logger.LogInformation("Didn't find any signee states for task."); +logger.LogInformation("Didn't find any signee states for task. DataTypeId: {DataTypeId}", signeeStatesDataTypeId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/Altinn.App.Api/Controllers/SigningController.cs(1 hunks)src/Altinn.App.Core/Features/IInstanceDataAccessor.cs(1 hunks)src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs(1 hunks)src/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cs(3 hunks)src/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs(8 hunks)src/Altinn.App.Core/Features/Signing/Services/SigningService.cs(2 hunks)src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs(2 hunks)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs(7 hunks)src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs(2 hunks)test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs(4 hunks)test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs(9 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cssrc/Altinn.App.Api/Controllers/SigningController.cssrc/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/IInstanceDataAccessor.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cssrc/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cssrc/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cstest/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cstest/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cssrc/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cssrc/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cssrc/Altinn.App.Api/Controllers/SigningController.cssrc/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/IInstanceDataAccessor.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cssrc/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cssrc/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cstest/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cstest/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cssrc/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cssrc/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cstest/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cstest/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Api/Controllers/SigningController.cssrc/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/IInstanceDataAccessor.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cssrc/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cssrc/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cssrc/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cssrc/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs
src/Altinn.App.Core/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Files:
src/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/IInstanceDataAccessor.cssrc/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cssrc/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cssrc/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cssrc/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs
test/Altinn.App.Core.Tests/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Files:
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cstest/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs
🧠 Learnings (7)
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/**/*.cs : New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Applied to files:
src/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cstest/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cssrc/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cstest/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cssrc/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cstest/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cssrc/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs : Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Applied to files:
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Integration.Tests/**/*.cs : Include integration tests for platform service interactions (using Docker Testcontainers)
Applied to files:
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Core.Tests/Features/**/*.cs : Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧬 Code graph analysis (12)
test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs (3)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (3)
SetAuthenticationMethod(48-48)DataType(41-41)DataType(60-75)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (5)
SetAuthenticationMethod(83-86)DataType(140-140)DataType(547-556)StorageAuthenticationMethod(558-563)StorageAuthenticationMethod(565-566)src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs (2)
SetAuthenticationMethod(94-95)DataType(92-92)
src/Altinn.App.Core/Features/Signing/Services/SigningService.cs (1)
src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (1)
SetServiceOwnerAuthForRestrictedDataTypes(15-30)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (3)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (5)
SetAuthenticationMethod(83-86)DataType(140-140)DataType(547-556)StorageAuthenticationMethod(558-563)StorageAuthenticationMethod(565-566)src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs (2)
SetAuthenticationMethod(94-95)DataType(92-92)test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs (2)
SetAuthenticationMethod(98-101)DataType(87-96)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (3)
SetAuthenticationMethod(48-48)DataType(41-41)DataType(60-75)src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs (2)
SetAuthenticationMethod(94-95)DataType(92-92)
src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs (3)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (3)
SetAuthenticationMethod(48-48)DataType(41-41)DataType(60-75)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (5)
SetAuthenticationMethod(83-86)DataType(140-140)DataType(547-556)StorageAuthenticationMethod(558-563)StorageAuthenticationMethod(565-566)test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs (2)
SetAuthenticationMethod(98-101)DataType(87-96)
src/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cs (1)
src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (1)
SetServiceOwnerAuthForRestrictedDataTypes(15-30)
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs (1)
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs (1)
SigneeContext(182-191)
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs (2)
src/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs (1)
SigneeContextsManager(20-204)src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs (1)
DataType(85-88)
src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs (2)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (4)
InstanceDataUnitOfWork(22-639)InstanceDataUnitOfWork(54-78)DataType(140-140)DataType(547-556)src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (3)
DataType(37-40)IEnumerable(32-35)IsRestrictedDataType(42-44)
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs (2)
src/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cs (1)
SignDocumentManager(21-337)src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs (2)
DataType(85-88)AltinnSignatureConfiguration(155-169)
src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (1)
src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs (3)
IEnumerable(90-93)DataType(85-88)IsRestrictedDataType(95-97)
src/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs (3)
src/Altinn.App.Core/Internal/Sign/SignatureContext.cs (1)
Signee(85-109)src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (1)
SetServiceOwnerAuthForRestrictedDataTypes(15-30)src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs (1)
IEnumerable(90-93)
🪛 GitHub Check: CodeQL
src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs
[notice] 23-29: Missed opportunity to use Where
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
🔇 Additional comments (13)
src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs (1)
60-78: Config var rename and guard condition look goodRenaming to
signingConfigurationand the null-guard for delegated signing reads clearer and preserves behavior.src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (1)
42-48: Public interface expansion — confirm compatibility and intentAdding SetAuthenticationMethod to the public IInstanceDataAccessor is a breaking change for external implementers. In-repo scan reports NO_DIRECT_IMPLEMENTERS (only internal references and tests). Either annotate the interface as ImplementableByApps if apps are expected to implement it, or confirm this change is safe to merge/backport and provide a migration/backport plan.
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
1259-1259: Breaking API: IInstanceDataAccessor now requires SetAuthenticationMethod(DataType, StorageAuthenticationMethod).Confirmed implementations/tests in this repo: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (interface), src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs, src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs, test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs.
- Confirm DI registrations still resolve and that any external consumers/implementers are updated.
- Add a migration note / CHANGELOG entry and bump the package version.
- Consider adding a convenience extension overload: SetAuthenticationMethod(this IInstanceDataAccessor, string dataTypeId, StorageAuthenticationMethod) to avoid repeated lookups/null-checks.
src/Altinn.App.Core/Features/Signing/Services/SigningService.cs (1)
7-7: LGTM: extension importImport is correct and scoped to the call site usage.
src/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cs (1)
51-53: Good: apply ServiceOwner auth before listing/downloading signature elementsThis correctly ensures restricted signature data types are accessed with the right authentication.
Please confirm IAppMetadata.GetApplicationMetadata() is cached internally; this path may be hit on every request.
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs (2)
93-96: Fail-open on app metadata fetch — confirm desired behaviorOn metadata read failure you log and return no issues, effectively passing validation. Confirm this is the intended product decision for signing tasks.
103-106: Fail-open on signee contexts read — confirm desired behaviorSimilarly, failures in GetSigneeContexts log and pass validation. Ensure this is acceptable; alternatively emit a ValidationIssue to block task completion when signatures cannot be evaluated.
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs (1)
39-45: LGTM: tests updated for app metadata–driven SigneeStatesSolid wiring of IAppMetadata in tests and consistent use of SigneeStatesDataTypeId constant. Mocks and DI setup look correct.
Also applies to: 54-59, 62-67, 118-119, 220-220, 287-287, 317-317, 377-377, 408-414
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
50-53: Per‑data‑type auth override looks good.Thread‑safe dictionary + sensible default are appropriate here.
558-567: Helper resolution reads clean and efficient.The accessor-based resolution and fallback are straightforward.
src/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs (2)
20-26: Primary‑ctor refactor LGTM.Sealed type with DI parameters simplifies state; telemetry usage remains scoped.
180-185: Correct: set service‑owner auth for potentially restricted data.Fetching metadata and applying overrides before read is appropriate.
test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
568-568: IAppMetadata is already registered in DI — no action required.IAppMetadata is registered via
services.TryAddSingleton<IAppMetadata, AppMetadata>()in src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (around line 172).
|
/publish |
PR release:
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1)
82-86: Add null guards and ensure all implementers override this (no throws).Defensive checks avoid hidden NREs and make misconfigurations fail fast. Also ensure every IInstanceDataAccessor implementer provides a real implementation (no NotImplementedException).
Apply:
public void OverrideAuthenticationMethod(DataType dataType, StorageAuthenticationMethod method) { + ArgumentNullException.ThrowIfNull(dataType); + ArgumentNullException.ThrowIfNull(method); _authenticationMethodOverrides[dataType.Id] = method; }Run to verify no NotImplementedException remains:
#!/bin/bash rg -nP --type=cs -C2 '\bclass\s+\w+\s*:\s*[^{]*\bIInstanceDataAccessor\b' rg -nP --type=cs -C2 'OverrideAuthenticationMethod\s*\(.*\)\s*=>\s*throw\s+new\s+NotImplementedException' rg -nP --type=cs -C3 'OverrideAuthenticationMethod\s*\(.*\)\s*{\s*throw\s+new\s+NotImplementedException'src/Altinn.App.Core/Features/Signing/Services/SigningService.cs (1)
74-76: Apply ServiceOwner auth before delete/read to avoid 403 on restricted types.You still configure auth after calling RemoveSigneeState; move it before any read/write/delete on restricted types.
Apply:
@@ - //TODO: Can be removed when AddBinaryDataElement supports setting generatedFromTask, because then it will be automatically deleted in ProcessTaskInitializer. - RemoveSigneeState(instanceDataMutator, signeeStateDataTypeId); + // Ensure ServiceOwner auth is applied for restricted types before any reads/writes/deletes. + ApplicationMetadata applicationMetadata = await _appMetadata.GetApplicationMetadata(); + instanceDataMutator.OverrideAuthenticationMethodForRestrictedDataTypes( + applicationMetadata, + [signeeStateDataTypeId], + StorageAuthenticationMethod.ServiceOwner() + ); + + // TODO: Can be removed when AddBinaryDataElement supports setting generatedFromTask, because then it will be automatically deleted in ProcessTaskInitializer. + RemoveSigneeState(instanceDataMutator, signeeStateDataTypeId); @@ - ApplicationMetadata applicationMetadata = await _appMetadata.GetApplicationMetadata(); - instanceDataMutator.OverrideAuthenticationMethodForRestrictedDataTypes( - applicationMetadata, - [signeeStateDataTypeId], - StorageAuthenticationMethod.ServiceOwner() - ); + // (moved earlier)Consider doing the same before deletes in AbortRuntimeDelegatedSigning.
Also applies to: 143-149
🧹 Nitpick comments (6)
test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs (2)
8-9: Prefer internal sealed for test utility class.Aligns with guidelines: default internal, sealed unless inheritance is intended.
-public class InstanceDataAccessorFake : IInstanceDataAccessor, IEnumerable<KeyValuePair<DataElement?, object>> +internal sealed class InstanceDataAccessorFake : IInstanceDataAccessor, IEnumerable<KeyValuePair<DataElement?, object>>If other test projects reflectively instantiate this, keep it public; otherwise switch to internal.
87-96: Remove unreachable null-check on _applicationMetadata.Field is non-nullable, assigned in ctor (with fallback), so this branch cannot execute.
- if (_applicationMetadata is null) - { - throw new InvalidOperationException("Application metadata not set for InstanceDataAccessorFake"); - }test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
1259-1260: Define scope/thread-safety and lifecycle of auth overridesPlease document whether overrides are per‑accessor instance, per‑request, and thread‑safe. Also clarify reset semantics (e.g., auto‑reset after operation or explicit reset API) to avoid bleed‑through across operations. XML docs on the interface method would suffice.
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1)
558-567: Helper methods look good; consider aligning ID comparisons across codebase.If other lookups are case-sensitive (e.g., GetDataTypeByString), decide on one policy. No change required here.
test/Altinn.App.Core.Tests/Features/Validators/Default/SigningTaskValidatorTests.cs (1)
151-153: Tests now expect exceptions; rename for clarity and assert message.Method names still say “LogError/ReturnEmptyList”. Rename to reflect throws and optionally assert the exception message for stronger intent.
Example:
var ex = await Assert.ThrowsAsync<Exception>(() => _validator.Validate(dataAccessorMock.Object, taskId, null!)); Assert.Equal("Error fetching metadata", ex.Message);Also applies to: 178-180
src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (1)
15-28: Add null guards and use params for convenience.Safer API and simpler call sites.
Apply:
- public static void OverrideAuthenticationMethodForRestrictedDataTypes( + public static void OverrideAuthenticationMethodForRestrictedDataTypes( this IInstanceDataAccessor accessor, ApplicationMetadata appMetadata, - string?[] dataTypeIds, + params string?[] dataTypeIds, StorageAuthenticationMethod authenticationMethod ) { + ArgumentNullException.ThrowIfNull(accessor); + ArgumentNullException.ThrowIfNull(appMetadata); + ArgumentNullException.ThrowIfNull(dataTypeIds); + ArgumentNullException.ThrowIfNull(authenticationMethod);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs(1 hunks)src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs(1 hunks)src/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cs(3 hunks)src/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs(8 hunks)src/Altinn.App.Core/Features/Signing/Services/SigningService.cs(2 hunks)src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs(2 hunks)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs(7 hunks)src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs(1 hunks)test/Altinn.App.Core.Tests/Features/Validators/Default/SigningTaskValidatorTests.cs(2 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Altinn.App.Core/Features/IInstanceDataAccessor.cs
- src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs
- src/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cs
- src/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
test/Altinn.App.Core.Tests/Features/Validators/Default/SigningTaskValidatorTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cssrc/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Core.Tests/Features/Validators/Default/SigningTaskValidatorTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cssrc/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs
test/Altinn.App.Core.Tests/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Files:
test/Altinn.App.Core.Tests/Features/Validators/Default/SigningTaskValidatorTests.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/Features/Validators/Default/SigningTaskValidatorTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cssrc/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs
src/Altinn.App.Core/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Files:
src/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cssrc/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs
🧠 Learnings (4)
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/**/*.cs : New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Applied to files:
src/Altinn.App.Core/Features/Signing/Services/SigningService.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Core/Features/Signing/Services/SigningService.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Core/Features/Signing/Services/SigningService.cssrc/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs
🧬 Code graph analysis (5)
test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs (3)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (3)
OverrideAuthenticationMethod(48-48)DataType(41-41)DataType(60-75)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (5)
OverrideAuthenticationMethod(83-86)DataType(140-140)DataType(547-556)StorageAuthenticationMethod(558-563)StorageAuthenticationMethod(565-566)src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs (2)
OverrideAuthenticationMethod(94-95)DataType(92-92)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (3)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (3)
OverrideAuthenticationMethod(48-48)DataType(41-41)DataType(60-75)src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs (2)
OverrideAuthenticationMethod(94-95)DataType(92-92)test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs (2)
OverrideAuthenticationMethod(98-101)DataType(87-96)
src/Altinn.App.Core/Features/Signing/Services/SigningService.cs (2)
src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (1)
OverrideAuthenticationMethodForRestrictedDataTypes(15-28)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
StorageAuthenticationMethod(558-563)StorageAuthenticationMethod(565-566)
src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (3)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (5)
StorageAuthenticationMethod(558-563)StorageAuthenticationMethod(565-566)DataType(140-140)DataType(547-556)OverrideAuthenticationMethod(83-86)src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (9)
IEnumerable(98-101)IEnumerable(106-109)IEnumerable(117-130)IEnumerable(135-147)IEnumerable(152-165)IEnumerable(170-182)DataType(41-41)DataType(60-75)OverrideAuthenticationMethod(48-48)test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs (2)
DataType(87-96)OverrideAuthenticationMethod(98-101)
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs (3)
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs (2)
SigneeContext(182-191)SignDocument(80-91)src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (1)
DataType(35-38)src/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cs (1)
SignDocument(180-199)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
🔇 Additional comments (6)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (3)
50-53: Good: sensible defaults for auth overrides.ConcurrentDictionary keyed by dataType.Id with CurrentUser() default keeps behavior stable for non-restricted types.
119-121: Passing auth through read path is correct.This ensures per-type overrides are honored for GetDataBytes.
451-453: Good: delete path also honors auth override.src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs (1)
90-96: Direct async calls + exception propagation: OK.Matches the updated tests’ expectations.
src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (2)
30-39: Lookup helpers are fine; consistent case-insensitive matching.No change required.
40-43: Restriction check reads clearly.Good use of NotNullWhen for flow analysis.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs (1)
102-104: Fix compile-time issue: MinCount is nullable; treat null as 0 and reuse counts
signatureDataType.MinCountis nullable;signedCount >= signatureDataType.MinCountyields a nullable bool and won’t compile. Also, reusesignedCountto compute “all signed.”Apply this diff:
- int signedCount = signeeContextsResult.Count(signeeContext => signeeContext.SignDocument is not null); - bool haveMinimumAmountOfSignatures = signedCount >= signatureDataType.MinCount; - bool allSigneesHaveSigned = signeeContextsResult.All(signeeContext => signeeContext.SignDocument is not null); + int minRequired = signatureDataType.MinCount ?? 0; + int signedCount = signeeContextsResult.Count(ctx => ctx.SignDocument is not null); + bool haveMinimumAmountOfSignatures = signedCount >= minRequired; + bool allSigneesHaveSigned = signedCount == signeeContextsResult.Count;
🧹 Nitpick comments (2)
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs (2)
90-96: Optional: Avoid CancellationToken.NoneIf feasible in this code path, prefer threading through a CancellationToken to
_appMetadata.GetApplicationMetadata()and_signingService.GetSigneeContexts(...)instead ofCancellationToken.None.
106-107: Confirm intended semantics: require all signees or allow min-count?The current condition requires both minimum count and that every signee has signed. If the intent is “all parties must sign,”
allSigneesHaveSignedalone is sufficient; if min-count should allow completion earlier, use||.Example (choose per spec):
- if (haveMinimumAmountOfSignatures && allSigneesHaveSigned) + if (allSigneesHaveSigned)or
- if (haveMinimumAmountOfSignatures && allSigneesHaveSigned) + if (haveMinimumAmountOfSignatures || allSigneesHaveSigned)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs
src/Altinn.App.Core/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Files:
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs
🧠 Learnings (1)
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs
🧬 Code graph analysis (1)
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs (4)
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs (2)
SigneeContext(182-191)SignDocument(80-91)src/Altinn.App.Core/Features/Signing/Extensions/SigningInstanceDataAccessorExtensions.cs (1)
DataType(35-38)src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (2)
DataType(41-41)DataType(60-75)src/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cs (1)
SignDocument(180-199)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
🔇 Additional comments (1)
src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs (1)
2-2: LGTM: Needed usingImport is required for SigneeContext and related models.
ivarne
left a comment
ivarne
left a comment
There was a problem hiding this comment.
Quick read through of IInstanceDataAccessor related code found just a few minor nits
…a types are set as restricted.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs (1)
231-236: Fix mock setup: wrong GetBinaryData overloadProduction calls GetBinaryData(DataElement), but the test sets up the DataElementIdentifier overload. This will return default data and can break the test.
Apply this diff to set up the correct overloads:
- cachedInstanceMutator - .Setup(x => x.GetBinaryData(new DataElementIdentifier(signDocumentDataElement1.Id))) - .ReturnsAsync(new ReadOnlyMemory<byte>(ToBytes(signDocument1))); + cachedInstanceMutator + .Setup(x => x.GetBinaryData(signDocumentDataElement1)) + .ReturnsAsync(new ReadOnlyMemory<byte>(ToBytes(signDocument1))); - cachedInstanceMutator - .Setup(x => x.GetBinaryData(new DataElementIdentifier(signDocumentDataElement2.Id))) - .ReturnsAsync(new ReadOnlyMemory<byte>(ToBytes(signDocument2))); + cachedInstanceMutator + .Setup(x => x.GetBinaryData(signDocumentDataElement2)) + .ReturnsAsync(new ReadOnlyMemory<byte>(ToBytes(signDocument2)));Alternatively, if you prefer the identifier overload, relax the matcher instead of constructing a new identifier instance:
- .Setup(x => x.GetBinaryData(new DataElementIdentifier(signDocumentDataElement1.Id))) + .Setup(x => x.GetBinaryData(It.Is<DataElementIdentifier>(d => d.Id == signDocumentDataElement1.Id)))
🧹 Nitpick comments (5)
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs (3)
1-1: Remove unused using
System.Reflectionisn’t used in this file. Drop it to keep test files lean.-using System.Reflection;
462-467: Verify auth override happens before reading dataYou verify that
OverrideAuthenticationMethod(...)is called, but not that it occurs beforeGetBinaryData(...). Enforce call order withMockSequenceso we don’t regress and read restricted data without the override.Apply this diff within the test to replace the existing
GetBinaryDatasetup and add an ordered expectation:- cachedInstanceAccessor - .Setup(x => x.GetBinaryData(signeeStateDataElement)) - .ReturnsAsync(new ReadOnlyMemory<byte>(serializedData)); + var sequence = new MockSequence(); + cachedInstanceAccessor + .InSequence(sequence) + .Setup(m => + m.OverrideAuthenticationMethod( + It.Is<DataType>(dt => dt.Id == signatureConfiguration.SigneeStatesDataTypeId), + StorageAuthenticationMethod.ServiceOwner() + ) + ); + cachedInstanceAccessor + .InSequence(sequence) + .Setup(x => x.GetBinaryData(signeeStateDataElement)) + .ReturnsAsync(new ReadOnlyMemory<byte>(serializedData));Keep the existing
Verify(...)to assert it happened exactly once.Also applies to: 498-505
508-534: Rename test to reflect behavior and avoid duplication with Line 347 testThis test name says “ThrowsApplicationConfigException” but the assertion expects an empty result with no throw; it also duplicates the scenario already covered in
GetSigneeContexts_WithNoSigneeStatesDataTypeId_ReturnsEmptyList(Lines 347–375). Rename or remove one to reduce redundancy.- public async Task GetSigneeContexts_WithMissingSigneeStatesDataTypeId_ThrowsApplicationConfigException() + public async Task GetSigneeContexts_WithMissingSigneeStatesDataTypeId_ReturnsEmptyList()If you keep both, differentiate them (e.g., one validates log message or verifies no auth override call occurs).
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs (2)
38-46: Minor: align ApplicationMetadata AppId with instance AppIdNot functionally required here, but matching the AppId avoids confusion when debugging.
Apply this diff:
- new ApplicationMetadata("ttd/app") + new ApplicationMetadata("ttd/app1")
251-258: Verification is brittle to future metadata changesVerifying OverrideAuthenticationMethod once is fine now, but will fail if multiple restricted data types are passed later. Consider tying expected calls to the number of restricted data types.
Example:
- cachedInstanceMutator.Verify( + var expectedCalls = 1; // number of restricted data types under test + cachedInstanceMutator.Verify( m => m.OverrideAuthenticationMethod( It.Is<DataType>(dt => dt.Id == signatureConfiguration.SignatureDataType), StorageAuthenticationMethod.ServiceOwner() ), - Times.Once + Times.Exactly(expectedCalls) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs(5 hunks)test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs(10 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cstest/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cstest/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs
test/Altinn.App.Core.Tests/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Files:
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cstest/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cstest/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs
🧠 Learnings (6)
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/**/*.cs : New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Applied to files:
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs : Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Applied to files:
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Integration.Tests/**/*.cs : Include integration tests for platform service interactions (using Docker Testcontainers)
Applied to files:
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs
🧬 Code graph analysis (2)
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs (1)
src/Altinn.App.Core/Features/Signing/Services/SignDocumentManager.cs (1)
SignDocumentManager(21-341)
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs (2)
src/Altinn.App.Core/Features/Signing/Services/SigneeContextsManager.cs (1)
SigneeContextsManager(20-205)src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs (1)
DataType(85-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Static code analysis
🔇 Additional comments (13)
test/Altinn.App.Core.Tests/Features/Signing/SigneeContextsManagerTests.cs (7)
9-10: LGTM: required usingsAdding
Internal.App,Internal.Data, andCore.Modelsusings aligns with the new metadata + accessor extensions.Also applies to: 13-13
41-41: LGTM: app metadata is now a first‑class dependencyAdding
IAppMetadatamock is correct for driving data‑type based behavior.
45-45: LGTM: central constant for data type idUsing a single
SigneeStatesDataTypeIdconstant avoids string drift across tests.
56-66: LGTM: metadata setup covers restricted-read pathProviding
ActionRequiredToRead = "restricted-read"ensures the restricted override extension has something to act on.
73-73: LGTM: pass metadata into managerConstructor wiring matches the production signature.
126-127: LGTM: config now uses the shared data type idReplacing hardcoded
"signeeStates"withSigneeStatesDataTypeIdacross tests is consistent and future‑proof.Also applies to: 227-228, 294-295, 325-325, 385-385, 415-416
418-423: LGTM: data element uses the configured data typeEnsures the accessor extension finds the correct element.
test/Altinn.App.Core.Tests/Features/Signing/SignDocumentManagerTests.cs (6)
26-26: Good: app metadata mock added for restricted data handlingThis aligns tests with the new IAppMetadata dependency.
30-30: Nice: centralized SignatureDataTypeId constantReduces duplication and keeps tests consistent.
47-47: Constructor wiring looks correctInjecting IAppMetadata into SignDocumentManager in tests matches the production signature.
200-200: Good: uses the shared SignatureDataTypeIdKeeps the setup tight and intention‑revealing.
284-285: Good: consistent use of SignatureDataTypeIdKeeps tests coherent with the new config shape.
318-324: Good: data element uses configured SignatureDataTypeMatches production flow that filters by configured type.
Co-authored-by: Ivar Nesje <ivarne@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
391-398: Dispose MemoryAsStream to avoid leaks.Ensure the stream is disposed even if IDataClient does not dispose inputs.
Apply:
- var dataElement = await _dataClient.InsertBinaryData( + using var stream = new MemoryAsStream(bytes); + var dataElement = await _dataClient.InsertBinaryData( Instance.Id, change.DataType.Id, change.ContentType, (change as BinaryDataChange)?.FileName, - new MemoryAsStream(bytes), + stream, authenticationMethod: GetAuthenticationMethod(change.DataType) );
414-421: Dispose MemoryAsStream here as well.Same leak risk on updates.
Apply:
- var newDataElement = await _dataClient.UpdateBinaryData( + using var stream = new MemoryAsStream(bytes); + var newDataElement = await _dataClient.UpdateBinaryData( new InstanceIdentifier(Instance), contentType, filename, dataElementIdentifier.Guid, - new MemoryAsStream(bytes), + stream, authenticationMethod: GetAuthenticationMethod(dataElementIdentifier) );
🧹 Nitpick comments (5)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (2)
43-51: Clarify null/override semantics in the API contract.Document null guards and last‑writer‑wins behavior to set expectations for implementers/callers.
Apply:
/// <summary> /// <para>Set the authentication method used when reading and writing data of the given data type.</para> /// <para>This method allows multiple calls to the same <see cref="DataType"/> key, but only the last /// invocation will be used by the read/write process. If you need to obtain a list of currently registered /// overrides, see the <see cref="AuthenticationMethodOverrides"/> property.</para> /// </summary> + /// <exception cref="ArgumentNullException"> + /// Thrown if <paramref name="dataType"/> or <paramref name="method"/> is null. + /// </exception> /// <param name="dataType">The <see cref="DataType"/> this configuration applies to.</param> /// <param name="method">The <see cref="StorageAuthenticationMethod"/> to associate with the given data type.</param> void OverrideAuthenticationMethod(DataType dataType, StorageAuthenticationMethod method);
53-56: Document snapshot semantics and advise caching for hot paths.Property returns a fresh immutable snapshot on each access — InstanceDataUnitOfWork's getter calls _authenticationMethodOverrides.ToImmutableDictionary() (src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs). Update the XML docs to state this and advise callers to cache the result when used in tight loops; no call-sites were found in the repo beyond the property declarations.
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (3)
458-460: Prefer named boolean argument for clarity.Passing a bare false is ambiguous; use the parameter name (e.g., hardDelete: false) for readability.
648-671: Seal the comparer and use explicit ordinal hashing.Minor robustness/readability tweaks.
Apply:
-internal class DataTypeComparer : IEqualityComparer<DataType> +internal sealed class DataTypeComparer : IEqualityComparer<DataType> { @@ - public int GetHashCode(DataType obj) => obj.Id != null ? obj.Id.GetHashCode() : 0; + public int GetHashCode(DataType obj) => + obj.Id is not null ? StringComparer.Ordinal.GetHashCode(obj.Id) : 0; }
25-28: Snapshot allocation on each AuthenticationMethodOverrides accessAuthenticationMethodOverrides returns _authenticationMethodOverrides.ToImmutableDictionary(), which allocates a new snapshot on every get. If this property is read frequently, either cache/memoize the immutable snapshot (invalidate on writes) or add a read API like TryGetAuthenticationMethod(DataType, out StorageAuthenticationMethod) to avoid full copies; if reads are rare, document the allocation cost. Repo scan shows only the interface, LayoutEvaluatorStateInitializer and a test fake references—no obvious hot-path usages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs(1 hunks)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs(10 hunks)src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cs(1 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs
- src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
test/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cssrc/Altinn.App.Core/Features/IInstanceDataAccessor.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cssrc/Altinn.App.Core/Features/IInstanceDataAccessor.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Features/IInstanceDataAccessor.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
src/Altinn.App.Core/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Files:
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs
🧬 Code graph analysis (3)
test/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cs (1)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (3)
DataType(147-147)DataType(554-563)DataTypeComparer(651-670)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (3)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (5)
OverrideAuthenticationMethod(90-93)DataType(147-147)DataType(554-563)StorageAuthenticationMethod(565-570)StorageAuthenticationMethod(572-573)src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs (2)
OverrideAuthenticationMethod(94-95)DataType(92-92)test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs (2)
OverrideAuthenticationMethod(98-101)DataType(87-96)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (3)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (3)
DataType(41-41)DataType(68-83)OverrideAuthenticationMethod(51-51)src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs (2)
DataType(92-92)OverrideAuthenticationMethod(94-95)test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs (2)
DataType(87-96)OverrideAuthenticationMethod(98-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: Static code analysis
🔇 Additional comments (4)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (3)
55-60: LGTM on defaults.Using a per‑process default (CurrentUser) and a per‑UoW override map is sensible.
565-574: LGTM on lookup flow.Overloads keep call sites tidy and hide comparer details.
89-93: Add null guards and short‑circuit equal check in OverrideAuthenticationMethodFail fast on misconfigured metadata; keep last‑writer‑wins behavior.
Location: src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
public void OverrideAuthenticationMethod(DataType dataType, StorageAuthenticationMethod method) { - _authenticationMethodOverrides[dataType] = method; + ArgumentNullException.ThrowIfNull(dataType); + ArgumentNullException.ThrowIfNull(method); + if (_authenticationMethodOverrides.TryGetValue(dataType, out var existing) && Equals(existing, method)) + { + return; + } + _authenticationMethodOverrides[dataType] = method; }Also ensure all IInstanceDataMutator implementers implement this method (no NotImplementedException):
#!/bin/bash set -euo pipefail rg -n --hidden -S -g '!**/bin/**' -g '!**/obj/**' '\b:\s*IInstanceDataMutator\b' -C3 || true rg -n --hidden -S -g '!**/bin/**' -g '!**/obj/**' 'OverrideAuthenticationMethod\s*\(.*\)\s*=>\s*throw\s+new\s+NotImplementedException' -C3 || true rg -n --hidden -S -g '!**/bin/**' -g '!**/obj/**' 'OverrideAuthenticationMethod\s*\(.*\)\s*\{[^}]*throw\s+new\s+NotImplementedException' -C3 || truetest/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cs (1)
8-28: Add a null-Id unit test; internals and xUnit availability verified
- Add a test asserting DataType.Id == null — expect GetHashCode(obj) == 0 and equality to compare by Id (null == null → equal; null vs non-null → not equal).
- Internals are already exposed (src/Altinn.App.Core/Altinn.App.Core.csproj contains InternalsVisibleTo) and DataTypeComparer is internal at src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs; test project enables ImplicitUsings and references xUnit so [Fact] is available.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
391-399: Dispose MemoryAsStream to avoid leaks.Wrap the stream in a using to ensure disposal after the call completes.
- var dataElement = await _dataClient.InsertBinaryData( + DataElement dataElement; + using (var stream = new MemoryAsStream(bytes)) + { + dataElement = await _dataClient.InsertBinaryData( Instance.Id, change.DataType.Id, change.ContentType, (change as BinaryDataChange)?.FileName, - new MemoryAsStream(bytes), + stream, authenticationMethod: GetAuthenticationMethod(change.DataType) - ); + ); + }
414-421: Dispose MemoryAsStream here as well.Same leak risk on update path.
- var newDataElement = await _dataClient.UpdateBinaryData( + DataElement newDataElement; + using (var stream = new MemoryAsStream(bytes)) + { + newDataElement = await _dataClient.UpdateBinaryData( new InstanceIdentifier(Instance), contentType, filename, dataElementIdentifier.Guid, - new MemoryAsStream(bytes), + stream, authenticationMethod: GetAuthenticationMethod(dataElementIdentifier) - ); + ); + }
🧹 Nitpick comments (2)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
572-574: Guard against invalid DataType in resolver (debug-level).Add an assertion to catch unexpected null Ids early.
-private StorageAuthenticationMethod GetAuthenticationMethod(DataType dataType) => - _authenticationMethodOverrides.GetValueOrDefault(dataType, _defaultAuthenticationMethod); +private StorageAuthenticationMethod GetAuthenticationMethod(DataType dataType) +{ + Debug.Assert(dataType.Id is not null, "DataType.Id must be set"); + return _authenticationMethodOverrides.GetValueOrDefault(dataType, _defaultAuthenticationMethod); +}
651-670: Tighten comparer implementation.Seal the comparer and use StringComparer.Ordinal for stable hashing; assert non-null Ids.
-internal class DataTypeComparer : IEqualityComparer<DataType> +internal sealed class DataTypeComparer : IEqualityComparer<DataType> { @@ - public int GetHashCode(DataType obj) => obj.Id != null ? obj.Id.GetHashCode() : 0; + public int GetHashCode(DataType obj) + { + Debug.Assert(obj.Id is not null, "DataType.Id must not be null"); + return System.StringComparer.Ordinal.GetHashCode(obj.Id ?? string.Empty); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cs(1 hunks)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs(10 hunks)src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cs(1 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Altinn.App.Core/Features/IInstanceDataAccessor.cs
- test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
- test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs
- src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
test/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
🧠 Learnings (1)
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
🧬 Code graph analysis (2)
test/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cs (1)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (3)
DataType(147-147)DataType(554-563)DataTypeComparer(651-670)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (4)
src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs (2)
DataType(92-92)OverrideAuthenticationMethod(94-95)test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs (2)
DataType(87-96)OverrideAuthenticationMethod(98-99)src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (3)
DataType(41-41)DataType(68-83)OverrideAuthenticationMethod(51-51)src/Altinn.App.Core/Models/DataElementIdentifier.cs (6)
DataElementIdentifier(29-33)DataElementIdentifier(39-43)DataElementIdentifier(48-53)Equals(92-95)Equals(100-103)GetHashCode(108-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Analyze (csharp)
- GitHub Check: Static code analysis
🔇 Additional comments (4)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (3)
55-60: Good: concurrent overrides keyed by DataType with custom comparer.Approach is sound and thread-safe.
126-129: LGTM: auth method plumbed into GetDataBytes.
458-460: LGTM: auth method included in DeleteData.test/Altinn.App.Core.Tests/Internal/Data/DataTypeComparerTest.cs (1)
6-29: Test covers Id-based key semantics; looks good.Validates overwrite on same Id and distinct entries on different Ids.
|
github-project-automation
Bot
moved this from 👷 In progress
to ✅ Done
in Team Altinn Studio
…1472) --------- Co-authored-by: Bjørn Tore Gjerde <bjorn.tore.gjerde@vivende.no>
|
✅ Automatic backport successful! A backport PR has been automatically created for the The release branch The cherry-pick was clean with no conflicts. Please review the backport PR when it appears. |
…1472) --------- Co-authored-by: Bjørn Tore Gjerde <bjorn.tore.gjerde@vivende.no>
4 participants
Description
This PR implements
AuthenticationMethodsupport forIInstanceDataAccessor, and uses this ability to automatically support restricted data in theSigningService.Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Bug Fixes
Refactor
Behavior Change
Tests