Validate signature hashes #1474
Conversation
bjorntore
requested review from
HauklandJ,
cammiida,
danielskovli,
ivarne and
martinothamar
bjorntore
added
the
backport-ignore
📝 WalkthroughWalkthroughAdds cancellation-aware HTTP helpers and a streaming GET, implements streaming binary retrieval in storage, introduces SignatureHashValidator with SHA‑256 checks and new InvalidSignatureHash code, introduces PlatformHttpResponseSnapshotException and related tests and mocks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
bjorntore
added
the
feature
|
Ser jeg glemte å ta i bruk GetStreamingAsync.... WIP |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs
Fixed
Show fixed
Hide fixed
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs
Dismissed
Show dismissed
Hide dismissed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
.claude/settings.local.json (1)
1-11: Local Claude settings checked in — confirm intent; add trailing newline.If this is meant for local dev only, consider ignoring it via .gitignore or documenting why it’s versioned. Also add a trailing newline to appease some linters.
src/Altinn.App.Core/Extensions/HttpClientExtension.cs (1)
106-135: Expose CancellationToken on GetStreamingAsync.Streaming calls often need cancellation; currently hard-wired to CancellationToken.None.
Apply this diff:
- public static async Task<HttpResponseMessage> GetStreamingAsync( + public static async Task<HttpResponseMessage> GetStreamingAsync( this HttpClient httpClient, string authorizationToken, string requestUri, - string? platformAccessToken = null + string? platformAccessToken = null, + CancellationToken cancellationToken = default ) { using HttpRequestMessage request = new(HttpMethod.Get, requestUri); @@ - return await httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, CancellationToken.None); + return await httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken); }src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (4)
93-97: Avoid FormatException from Guid.Parse — use TryParse and fail gracefully.A malformed id will currently throw. Prefer a validation issue.
Apply this diff:
- Guid.Parse(dataElementSignature.DataElementId), + Guid.TryParse(dataElementSignature.DataElementId, out var dataGuid) + ? dataGuid + : throw new ApplicationConfigException($"Invalid data element id: {dataElementSignature.DataElementId}"),
42-45: Narrow exception catch in ShouldRunForTask.Catching Exception hides unexpected faults. Consider catching ApplicationConfigException (or the specific type thrown by GetAltinnTaskExtension) instead.
142-143: Tiny cleanup: redundant null-conditional.dataType is already null-checked above; drop the ?.
Apply this diff:
- return !string.IsNullOrEmpty(dataType?.ActionRequiredToRead); + return !string.IsNullOrEmpty(dataType.ActionRequiredToRead);
152-155: Typo in XML doc: "best best practice".Minor doc fix.
Apply this diff:
- /// Formats a SHA digest with common best best practice:<br/> + /// Formats a SHA digest with common best practice:<br/>test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (9)
32-38: Prefer NullLogger in tests over mocking ILoggerSlightly simpler and avoids unnecessary Moq setup.
- new Mock<ILogger<SignatureHashValidator>>().Object + NullLogger<SignatureHashValidator>.InstanceAlso add this using at the top:
+using Microsoft.Extensions.Logging.Abstractions;
46-55: Avoid magic SHA-256 constants; compute from the arranged dataComputing the expected hash in-test makes intent clear and prevents drift if encoding changes.
- const string testData = "test data"; - const string expectedHash = "916f0027a575074ce72a331777c3478d6513f786a591bd892da1a577bf2335f9"; + const string testData = "test data"; + string expectedHash = Sha256Hex(testData);Add this helper to the test class:
private static string Sha256Hex(string s) { using var sha = System.Security.Cryptography.SHA256.Create(); return Convert.ToHexString(sha.ComputeHash(Encoding.UTF8.GetBytes(s))).ToLowerInvariant(); }And add at file top if missing:
using System.Security.Cryptography; // optional; not needed if using fully qualified above
65-73: Use a valid (but different) SHA-256 instead of a non-hex placeholderBetter represents real-world data and keeps the test robust if validation tightens.
- const string testData = "test data"; - const string storedHash = "different-hash"; + const string testData = "test data"; + string storedHash = Sha256Hex("mismatching content");
181-215: LGTM: validates across multiple signee contextsAccurate call-count check for GetBinaryData. Consider also a test with multiple signatures in a single context to cover that loop.
303-314: Add test for null AltinnTaskExtensionCovers the case where GetAltinnTaskExtension returns null (not just throws).
[Fact] public void ShouldRunForTask_WithNullExtension_ReturnsFalse() { _processReaderMock.Setup(x => x.GetAltinnTaskExtension("task")).Returns((AltinnTaskExtension?)null); Assert.False(_validator.ShouldRunForTask("task")); }
341-365: Use obviously fake identifiers to avoid realistic PII patternsSwap the SSN and name to clear dummy values.
- SocialSecurityNumber = "12345678901", - FullName = "Test Person", + SocialSecurityNumber = "00000000000", + FullName = "Test User",Apply in both places: Line 348 and Line 230.
383-386: Reduce brittle matching in Moq setupMatch by type rather than exact instance to decouple from the test fixture’s private fields.
- It.Is<IInstanceDataAccessor>(d => d == _dataAccessorMock.Object), - It.Is<AltinnSignatureConfiguration>(c => c == signingConfiguration), + It.IsAny<IInstanceDataAccessor>(), + It.IsAny<AltinnSignatureConfiguration>(),
57-60: Structured logging in SUT for the invalid-hash branchSwitch from string interpolation to structured logging placeholders to preserve fields in logs.
In SignatureHashValidator.Validate:
- logger.LogError( - $"Found an invalid signature for data element {dataElementSignature.DataElementId} on instance {instance.Id}. Expected hash {dataElementSignature.Sha256Hash}, calculated hash {sha256Hash}." - ); + logger.LogError( + "Found an invalid signature for data element {DataElementId} on instance {InstanceId}. Expected hash {ExpectedHash}, calculated hash {CalculatedHash}.", + dataElementSignature.DataElementId, + instance.Id, + dataElementSignature.Sha256Hash, + sha256Hash + );
57-60: Consider aggregating all invalid signatures instead of early-returnReturning all issues improves UX and reduces back-and-forth for users. If kept as-is, document that only the first failure is reported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.claude/settings.local.json(1 hunks)src/Altinn.App.Core/Extensions/HttpClientExtension.cs(1 hunks)src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs(1 hunks)src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs(1 hunks)src/Altinn.App.Core/Models/Validation/ValidationIssueCodes.cs(1 hunks)test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs(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:
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cstest/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Extensions/HttpClientExtension.cssrc/Altinn.App.Core/Models/Validation/ValidationIssueCodes.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cstest/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Extensions/HttpClientExtension.cssrc/Altinn.App.Core/Models/Validation/ValidationIssueCodes.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/SignatureHashValidator.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Extensions/HttpClientExtension.cssrc/Altinn.App.Core/Models/Validation/ValidationIssueCodes.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/SignatureHashValidator.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/SignatureHashValidatorTests.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/SignatureHashValidatorTests.cs
🧠 Learnings (2)
📚 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:
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.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/Validators/Default/SignatureHashValidatorTests.cs
🧬 Code graph analysis (4)
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (5)
src/Altinn.App.Core/Models/Validation/ValidationIssue.cs (1)
ValidationIssue(9-105)test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (1)
SigneeContext(341-365)src/Altinn.App.Core/Models/InstanceIdentifier.cs (1)
InstanceOwnerPartyId(111-118)src/Altinn.App.Core/Models/Validation/ValidationIssueCodes.cs (2)
ValidationIssueCodes(6-79)DataElementCodes(27-78)test/Altinn.App.Integration.Tests/_fixture/NuGetPackaging.cs (1)
ToHexString(169-169)
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (4)
src/Altinn.App.Core/Internal/Sign/SignatureContext.cs (1)
Signee(85-109)src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (1)
SignatureHashValidator(19-162)src/Altinn.App.Core/Models/Validation/ValidationIssue.cs (1)
ValidationIssue(9-105)src/Altinn.App.Core/Models/Validation/ValidationIssueCodes.cs (2)
ValidationIssueCodes(6-79)DataElementCodes(27-78)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (1)
SignatureHashValidator(19-162)
src/Altinn.App.Core/Extensions/HttpClientExtension.cs (3)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (6)
Task(61-77)Task(80-125)Task(128-169)Task(172-205)Task(208-247)Task(250-277)src/Altinn.App.Core/Constants/AuthorizationSchemes.cs (1)
AuthorizationSchemes(6-12)src/Altinn.App.Core/Constants/General.cs (1)
General(6-42)
⏰ 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 (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
src/Altinn.App.Core/Models/Validation/ValidationIssueCodes.cs (1)
74-77: New code added: DataElementCodes.InvalidSignatureHash — ensure i18n text exists.Looks good. Verify a corresponding localized text exists (frontend expects lookup by this code).
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
233-234: Multiple IValidator registrations are safe — no single-IValidator resolution found.Verified: DI registers multiple IValidator implementations (src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs); validators are collected via IValidatorFactory.GetAll and used as IValidator[]/IEnumerable (src/Altinn.App.Core/Internal/Validation/IValidatorFactory.cs, src/Altinn.App.Core/Internal/Validation/ValidationService.cs). No constructor or GetService/GetRequiredService resolving a lone IValidator in src/; only a test-only singleton exists in test/Altinn.App.Integration.Tests/_testapps/_shared/Tracing.cs.
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (8)
98-127: LGTM: verifies ServiceOwner auth is used for restricted readGood assertion on non-null StorageAuthenticationMethod and correct routing params.
129-158: LGTM: verifies no ServiceOwner auth for non-restricted readClear, targeted Verify with null auth.
160-179: LGTM: precise exception assertion for missing data typeStrong contract check with exact message.
217-256: LGTM: skips when SignDocument is nullGood negative-path coverage and Verify(Times.Never).
271-279: LGTM: ShouldRunForTask returns true for signing tasks
282-301: LGTM: ShouldRunForTask returns false for non-signing and null task types
316-328: LGTM: HasRelevantChanges throws as NoIncrementalValidation is true
21-41: Test infra verified — no action required.
- SignatureHashValidatorTests is public (test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs).
- InternalsVisibleTo grants Altinn.App.Core.Tests (test/Altinn.App.Tests.Common/Altinn.App.Tests.Common.csproj); src/Altinn.App.Core/Altinn.App.Core.csproj also exposes test assemblies. ImplicitUsings is enabled for test projects.
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs
Show resolved
Hide resolved
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
I see that this matches the corresponding code in Storage: https://github.com/Altinn/altinn-storage/blob/6259f1a741cb0c6fc6b625575af867bc455bcf2b/src/Storage/Services/DataService.cs#L110
Maybe make a comment/note saying how this is coupled together?
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs
Dismissed
Show dismissed
Hide dismissed
…from GetBinaryDataStream. Throw if element is not found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (2)
114-127: Avoid reference equality for StorageAuthenticationMethod in Moq matcherStorageAuthenticationMethod.ServiceOwner() returns a new instance; using == compares references and can be brittle or fail if equality isn’t overridden. Match on intent (e.g., underlying request/type) instead of instance.
Example approaches:
- Match properties: It.Is<StorageAuthenticationMethod?>(a => a is not null /* and a.Request is ServiceOwner */)
- Or expose/compare a discriminant on StorageAuthenticationMethod (e.g., MethodKind == ServiceOwner) and assert that.
Please adjust similarly in the non-restricted test below.
315-316: Use Assert.Equal for clarityPrefer Assert.Equal(expected, actual) over Assert.True(a == b) for message comparison.
- Assert.True(thrownException.Message == mockedException.Message); + Assert.Equal(mockedException.Message, thrownException.Message);src/Altinn.App.Core/Internal/Data/IDataClient.cs (1)
110-112: Clarify semantics and disposal requirements; avoid hard-coding timeout
- Document 404 behavior for GetBinaryData (returns null despite non-nullable signature) to prevent surprises.
- In GetBinaryDataStream docs, explicitly state: “Caller must dispose the returned Stream to release the underlying HTTP response.”
- Avoid hard-coding “Defaults to 100 seconds”; reference “default operation timeout” to prevent drift from implementation constants.
Also applies to: 130-153
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (3)
219-220: Unify error handling: prefer snapshot exception to avoid retaining HttpResponseMessageNon-streaming paths still throw PlatformHttpException, which keeps a reference to the response (risking leaks/late ObjectDisposed). For consistency and safety, use PlatformHttpResponseSnapshotException.CreateAndDisposeHttpResponse(...) as in the streaming method.
Example (apply pattern):
- throw await PlatformHttpException.CreateAsync(response); + throw await PlatformHttpResponseSnapshotException.CreateAndDisposeHttpResponse( + response, + cancellationToken: timeoutCts.Token + );Also applies to: 314-315, 347-348, 390-391, 475-476, 518-519, 616-617, 663-664, 698-699, 741-742, 784-785
28-29: Seal the classPer guidelines, seal classes by default unless inheritance is required. DataClient appears DI-constructed and not intended for inheritance.
As per coding guidelines
183-221: Document the null-on-404 behavior or plan deprecationGetBinaryData returns null for 404 while the streaming variant throws. Consider:
- Add XML doc here mirroring the null return on 404, and
- Plan to make return type Stream? (breaking change) in next major, or steer consumers to streaming method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(29 hunks)src/Altinn.App.Core/Internal/Data/IDataClient.cs(2 hunks)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs(1 hunks)test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Api.Tests/Mocks/DataClientMock.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/Features/Validators/Default/SignatureHashValidatorTests.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.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/SignatureHashValidatorTests.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/SignatureHashValidatorTests.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/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
🧠 Learnings (1)
📚 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:
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs
🧬 Code graph analysis (3)
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (4)
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (2)
SignatureHashValidator(19-178)ShouldRunForTask(37-41)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (16)
Task(64-83)Task(86-134)Task(137-181)Task(184-220)Task(223-270)Task(273-315)Task(318-348)Task(351-391)Task(429-439)Task(442-476)Task(479-519)Task(522-574)Task(577-617)Task(620-664)Task(667-699)Task(702-742)src/Altinn.App.Core/Models/Validation/ValidationIssue.cs (1)
ValidationIssue(9-105)src/Altinn.App.Core/Models/Validation/ValidationIssueCodes.cs (2)
ValidationIssueCodes(6-79)DataElementCodes(27-78)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (1)
src/Altinn.App.Core/Features/AuthenticationMethod.cs (5)
StorageAuthenticationMethod(85-85)StorageAuthenticationMethod(88-88)StorageAuthenticationMethod(91-92)StorageAuthenticationMethod(95-96)StorageAuthenticationMethod(100-103)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
src/Altinn.App.Core/Helpers/ResponseWrapperStream.cs (3)
ResponseWrapperStream(6-165)ResponseWrapperStream(16-23)Dispose(29-37)src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (2)
PlatformHttpResponseSnapshotException(18-300)PlatformHttpResponseSnapshotException(157-175)
⏰ 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). (3)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Static code analysis
🔇 Additional comments (3)
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (1)
43-61: Good coverage and setupTests exercise valid/invalid hash, auth selection (restricted vs non-restricted), multi-signee, and control methods (TaskId/NoIncrementalValidation/ShouldRunForTask/HasRelevantChanges). Nice use of MemoryStream to emulate content.
Also applies to: 183-218
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
222-270: Streaming path looks correct; wrapper ensures response disposalUsing GetStreamingAsync + ResponseWrapperStream with linked CTS is solid. The try/catch disposing the response on failures is good.
55-61: HttpClientExtension methods don’t support CancellationToken
ThePostAsync,PutAsync,GetAsyncandDeleteAsyncextensions in HttpClientExtension.cs only accept(HttpClient, string authorizationToken, string requestUri[, HttpContent/ platformAccessToken])and have no CancellationToken parameter. You can’t passtimeoutCts.Tokento them—either add overloads (or useHttpClient.SendAsync) that accept a CancellationToken, or reconsider relying on infiniteHttpClient.Timeout.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
131-140: CA2000 “undisposed FileStream” warning is a false positive here.The mock returns the FileStream to the caller (SUT disposes it). If code scanning keeps flagging this, suppress locally.
Apply one of these:
- var fs = new FileStream( + // Suppressed: stream ownership is transferred to the caller (SUT disposes). +#pragma warning disable CA2000 // Dispose objects before losing scope + var fs = new FileStream( path, FileMode.Open, FileAccess.Read, - FileShare.Read, + FileShare.Read, // consider FileShare.ReadWrite | FileShare.Delete if tests write concurrently bufferSize: 64 * 1024, options: FileOptions.Asynchronous | FileOptions.SequentialScan ); +#pragma warning restore CA2000test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (1)
371-402: Analyzer note: reference equality in SetupMocks is intentional—make it explicit.The predicate d == _dataAccessorMock.Object triggered a previous analyzer note about System.Object reference equality. Make intent explicit to quiet the rule.
- It.Is<IInstanceDataAccessor>(d => d == _dataAccessorMock.Object), + It.Is<IInstanceDataAccessor>(d => ReferenceEquals(d, _dataAccessorMock.Object)),
🧹 Nitpick comments (6)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
126-131: Align 404/error semantics with production DataClient.GetBinaryDataStream.Production throws a PlatformHttpResponseSnapshotException on non-success; this mock throws FileNotFoundException. Consider mirroring production to avoid surprise in tests that assert on exception type/status in the streaming path. If you want to keep the mock light, at least document the divergence here.
Would you like a small helper that throws a NotFound snapshot exception for mocks?
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (2)
114-126: Fragile equality on authentication method in Moq predicate.auth == StorageAuthenticationMethod.ServiceOwner() may rely on reference equality. Prefer a predicate that asserts “non-null” (or a stable discriminant) to avoid flakiness.
Example:
- It.Is<StorageAuthenticationMethod?>(auth => auth == StorageAuthenticationMethod.ServiceOwner()), + It.Is<StorageAuthenticationMethod?>(auth => auth is not null),If StorageAuthenticationMethod exposes a discriminant, match on that instead.
46-56: Nice, focused tests; consider computing the expected hash to reduce brittleness.Hard-coding the SHA-256 string is fine; alternatively compute it inline to avoid accidental mismatch if test data changes.
static string Sha256Hex(string s) { using var sha = SHA256.Create(); return Convert.ToHexString(sha.ComputeHash(Encoding.UTF8.GetBytes(s))).ToLowerInvariant(); }Then use Sha256Hex("test data") for expectedHash.
Also applies to: 62-75, 160-179, 181-215
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (3)
223-270: Inconsistent 404 behavior between buffered and streaming APIs.GetBinaryData returns null on 404 (with #nullable disable), while GetBinaryDataStream throws PlatformHttpResponseSnapshotException for any non-success. This divergence complicates callers.
Pick one convention for both (prefer exception + typed status via snapshot for clarity), adjust interface nullability accordingly, and update mocks/tests. Based on coding guidelines (NRTs enabled), avoiding null Stream is cleaner.
Also applies to: 195-218
39-41: Expose timeouts via configuration, not hardcoded constants.Consider IOptions with defaults (100s standard, 30m streaming) to allow environment tuning and tests to shorten timeouts.
128-135: Unify exception strategy (snapshot vs PlatformHttpException).Streaming uses PlatformHttpResponseSnapshotException (safe, disposes response); other paths throw PlatformHttpException, which retains HttpResponseMessage. Consider migrating to snapshot for consistency and to avoid accidental use of disposed responses (as discussed in PR).
Do you want a follow-up PR to switch remaining call sites?
Also applies to: 181-182, 220-221, 314-315, 347-348, 390-391, 475-476, 518-519, 573-574, 616-617, 663-664, 698-699, 741-742, 784-785
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(29 hunks)src/Altinn.App.Core/Internal/Data/IDataClient.cs(2 hunks)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs(1 hunks)test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Altinn.App.Core/Internal/Data/IDataClient.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.Api.Tests/Mocks/DataClientMock.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.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.Api.Tests/Mocks/DataClientMock.cstest/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.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/Infrastructure/Clients/Storage/DataClient.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/SignatureHashValidatorTests.cs
🧠 Learnings (1)
📚 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:
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs
🧬 Code graph analysis (3)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
test/Altinn.App.Api.Tests/Data/TestData.cs (3)
org(69-90)TestData(9-221)GetDataBlobPath(116-120)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
src/Altinn.App.Core/Helpers/ResponseWrapperStream.cs (3)
ResponseWrapperStream(6-165)ResponseWrapperStream(16-23)Dispose(29-37)src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (2)
PlatformHttpResponseSnapshotException(18-300)PlatformHttpResponseSnapshotException(157-175)
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (3)
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (2)
SignatureHashValidator(19-178)ShouldRunForTask(37-41)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (16)
Task(65-84)Task(87-135)Task(138-182)Task(185-221)Task(224-270)Task(273-315)Task(318-348)Task(351-391)Task(429-439)Task(442-476)Task(479-519)Task(522-574)Task(577-617)Task(620-664)Task(667-699)Task(702-742)src/Altinn.App.Core/Models/Validation/ValidationIssueCodes.cs (2)
ValidationIssueCodes(6-79)DataElementCodes(27-78)
⏰ 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: Analyze (csharp)
- 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: Static code analysis
🔇 Additional comments (4)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
121-123: Early-cancel path: good.Returning Task.FromCanceled when the token is already canceled is correct and prevents opening the file unnecessarily.
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (2)
145-157: Non-restricted read: assert null auth only.This is precise and stable. No change requested.
199-215: Good coverage for multiple signee contexts.Verifies per-context streaming fetch and ensures exactly two calls.
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
237-270: Streaming disposal pattern looks solid.Wrapping the content stream with ResponseWrapperStream and disposing the HttpResponseMessage on error paths avoids leaks and TDZ-like uses of disposed responses.
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Core/Extensions/HttpClientExtension.cs(8 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(26 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/Extensions/HttpClientExtension.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Extensions/HttpClientExtension.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.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/Extensions/HttpClientExtension.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
🧬 Code graph analysis (2)
src/Altinn.App.Core/Extensions/HttpClientExtension.cs (3)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (16)
Task(65-84)Task(87-140)Task(143-192)Task(195-231)Task(234-280)Task(283-325)Task(328-358)Task(361-401)Task(439-449)Task(452-486)Task(489-534)Task(537-594)Task(597-642)Task(645-694)Task(697-734)Task(737-782)src/Altinn.App.Core/Constants/AuthorizationSchemes.cs (1)
AuthorizationSchemes(6-12)src/Altinn.App.Core/Constants/General.cs (1)
General(6-42)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
src/Altinn.App.Core/Helpers/ResponseWrapperStream.cs (3)
ResponseWrapperStream(6-165)ResponseWrapperStream(16-23)Dispose(29-37)src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (2)
PlatformHttpResponseSnapshotException(18-300)PlatformHttpResponseSnapshotException(157-175)
⏰ 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 (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
src/Altinn.App.Core/Extensions/HttpClientExtension.cs (5)
18-43: LGTM: CancellationToken support added to PostAsync.The addition of the
cancellationTokenparameter and forwarding it toSendAsyncenables proper timeout and cancellation control for POST operations.
53-78: LGTM: CancellationToken support added to PutAsync.Consistent with PostAsync, the cancellation token is properly forwarded to
SendAsync.
87-110: LGTM: GetAsync enhanced with explicit completion option and cancellation token.The explicit use of
HttpCompletionOption.ResponseContentReadon line 109 documents the buffering behavior, and the cancellation token is properly forwarded.
112-145: LGTM: GetStreamingAsync correctly implements streaming semantics.The new method properly uses
HttpCompletionOption.ResponseHeadersReadto enable streaming, and the XML documentation (lines 122-123) appropriately warns consumers about timeout configuration for large files.
154-177: LGTM: CancellationToken support added to DeleteAsync.Consistent with other methods, the cancellation token is properly forwarded.
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (3)
39-40: LGTM: Timeout strategy correctly implements per-operation cancellation.Setting
HttpClient.Timeout = Timeout.InfiniteTimeSpan(line 60) combined with per-operationCancellationTokenSourceinstances (using the timeout constants on lines 39-40) provides fine-grained timeout control while allowing callers to override via their own cancellation tokens. This addresses the concerns raised in previous reviews.Also applies to: 60-60
233-280: LGTM: GetBinaryDataStream implements robust streaming with proper resource management.The new streaming method correctly:
- Uses the extended
_streamingHttpOperationTimeout(30 minutes) for large file operations (line 245)- Leverages
GetStreamingAsyncwithHttpCompletionOption.ResponseHeadersReadfor true streaming (lines 256-260)- Wraps the response and stream in
ResponseWrapperStream(line 267), ensuring the response is disposed when the stream is disposed- Handles
ReadAsStreamAsyncfailures by disposing the response in the catch block (lines 269-273)- On non-success responses, uses
PlatformHttpResponseSnapshotException.CreateAndDisposeHttpResponse(lines 276-279) which safely snapshots and disposes the response
78-84: LGTM: Per-operation timeout pattern applied consistently throughout.All methods correctly implement the pattern of:
- Creating a linked
CancellationTokenSourcewith the incomingcancellationToken- Setting the appropriate timeout (
_httpOperationTimeoutfor most operations)- Passing
timeoutCts.Tokento all async operations:GetAccessToken, HTTP calls, and content readsThis ensures that all operations respect both caller-provided cancellation tokens and per-operation timeouts, preventing indefinite hangs while maintaining flexibility.
Also applies to: 97-127, 156-191, 205-231, 294-325, 338-358, 370-401, 463-486, 500-534, 548-594, 608-642, 655-694, 704-734, 744-782, 792-825
.../Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (1)
311-314: Prefer Assert.Equal over string == in tests.This reads clearer and avoids “reference equality” lint noise.
- Assert.True(thrownException.Message == mockedException.Message); + Assert.Equal(mockedException.Message, thrownException.Message);src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (1)
126-142: Include DataElementId and compare hash case-insensitively.Avoid case pitfalls and return the offending element id.
- if (sha256Hash != dataElementSignature.Sha256Hash) + if (!string.Equals(sha256Hash, dataElementSignature.Sha256Hash, StringComparison.OrdinalIgnoreCase)) { logger.LogError( "Found an invalid signature for data element {DataElementId} on instance {InstanceId}. Expected hash {ExpectedHash}, calculated hash {CalculatedHash}", dataElementSignature.DataElementId, instance.Id, dataElementSignature.Sha256Hash, sha256Hash ); - return new ValidationIssue + return new ValidationIssue { Code = ValidationIssueCodes.DataElementCodes.InvalidSignatureHash, Severity = ValidationIssueSeverity.Error, + DataElementId = dataElementSignature.DataElementId, Description = ValidationIssueCodes.DataElementCodes.InvalidSignatureHash, }; }
🧹 Nitpick comments (5)
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (3)
80-84: Also assert DataElementId on error for stronger contract.Add an assertion to ensure the validator populates DataElementId.
Assert.Single(result); Assert.Equal(ValidationIssueCodes.DataElementCodes.InvalidSignatureHash, result[0].Code); Assert.Equal(ValidationIssueSeverity.Error, result[0].Severity); Assert.Equal(ValidationIssueCodes.DataElementCodes.InvalidSignatureHash, result[0].Description); + Assert.Equal("550e8400-e29b-41d4-a716-446655440001", result[0].DataElementId);
342-366: Helper name is misleading; consider renaming.Method sets any provided hash (valid or not). Rename to CreateSigneeContextWithHash; update call sites.
368-404: Add a test to verify RequestAborted is propagated to IDataClient.Ensure cancellation flows from HttpContext to GetBinaryDataStream.
Add this test (append to the class):
[Fact] public async Task Validate_PropagatesRequestAbortedToken_ToDataClient() { // Arrange const string testData = "test data"; const string expectedHash = "916f0027a575074ce72a331777c3478d6513f786a591bd892da1a577bf2335f9"; var signingConfiguration = new AltinnSignatureConfiguration { SignatureDataType = "signature" }; var applicationMetadata = new ApplicationMetadata("testorg/testapp") { DataTypes = [ new DataType { Id = "form", ActionRequiredToRead = null } ], }; var signeeContext = CreateSigneeContextWithValidHash(expectedHash); var context = new DefaultHttpContext(); var cts = new CancellationTokenSource(); context.RequestAborted = cts.Token; var httpContextAccessor = new Mock<IHttpContextAccessor>(); httpContextAccessor.Setup(x => x.HttpContext).Returns(context); var validator = new SignatureHashValidator( _signingServiceMock.Object, _processReaderMock.Object, _dataClientMock.Object, _appMetadataMock.Object, httpContextAccessor.Object, new Mock<ILogger<SignatureHashValidator>>().Object ); // Reuse existing setup helpers SetupMocks(signingConfiguration, applicationMetadata, [signeeContext], testData); // Act await validator.Validate(_dataAccessorMock.Object, "signing-task", "en"); // Assert _dataClientMock.Verify(x => x.GetBinaryDataStream( It.IsAny<string>(), It.IsAny<string>(), It.IsAny<int>(), It.IsAny<Guid>(), It.IsAny<Guid>(), It.IsAny<StorageAuthenticationMethod?>(), It.Is<CancellationToken>(t => t == context.RequestAborted) ), Times.AtLeastOnce); }src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (2)
124-126: Propagate cancellation and make hashing resilient to non-zero stream position.Pass the request token into ComputeHashAsync and reset Position when possible.
- string sha256Hash = await GenerateSha256Hash(dataStream); + string sha256Hash = await GenerateSha256Hash(dataStream, cancellationToken); @@ - private static async Task<string> GenerateSha256Hash(Stream stream) + private static async Task<string> GenerateSha256Hash(Stream stream, CancellationToken cancellationToken) { using var sha256 = SHA256.Create(); - byte[] digest = await sha256.ComputeHashAsync(stream); + if (stream.CanSeek) + { + try { stream.Position = 0; } catch { /* ignore */ } + } + byte[] digest = await sha256.ComputeHashAsync(stream, cancellationToken); return FormatShaDigest(digest); }Also applies to: 167-172
91-95: Optional: collect all mismatches before returning.Today you short-circuit on first mismatch. Consider aggregating all invalid signatures to reduce user roundtrips.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs(1 hunks)test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs(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:
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cstest/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cstest/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.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/SignatureHashValidator.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/SignatureHashValidator.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/SignatureHashValidatorTests.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/SignatureHashValidatorTests.cs
🧬 Code graph analysis (2)
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (4)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (15)
Task(26-37)Task(50-58)Task(74-85)Task(98-107)Task(120-128)Task(142-150)Task(163-171)Task(183-190)Task(214-223)Task(236-245)Task(283-291)Task(304-313)Task(323-328)Task(337-342)Task(351-356)src/Altinn.App.Core/Models/Validation/ValidationIssue.cs (1)
ValidationIssue(9-105)src/Altinn.App.Core/Models/InstanceIdentifier.cs (1)
InstanceOwnerPartyId(111-118)src/Altinn.App.Core/Models/Validation/ValidationIssueCodes.cs (2)
ValidationIssueCodes(6-79)DataElementCodes(27-78)
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (3)
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (6)
SignatureHashValidator(20-185)Task(48-53)Task(55-101)Task(103-145)Task(167-172)ShouldRunForTask(39-43)src/Altinn.App.Core/Internal/Data/IDataClient.cs (12)
Task(26-37)Task(50-58)Task(74-85)Task(98-107)Task(120-128)Task(142-150)Task(163-171)Task(183-190)Task(214-223)Task(236-245)Task(283-291)Task(304-313)src/Altinn.App.Core/Models/Validation/ValidationIssueCodes.cs (2)
ValidationIssueCodes(6-79)DataElementCodes(27-78)
⏰ 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: 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)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (1)
45-62: Good coverage of the valid-hash happy path.Solid setup/mocks, verifies no issues returned.
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (2)
20-28: Design and defaults look good.internal sealed, DI, and await using on the stream are all on point.
112-123: Behavior on 404 from storage: confirm desired semantics.If GetBinaryDataStream throws (e.g., 404), should validator surface a ValidationIssue (e.g., treat as invalid/missing) or bubble the exception to block task exit? Align with platform guidance and update tests accordingly.
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs
Outdated
Show resolved
Hide resolved
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Altinn.App.Core/Extensions/CancellationTokenExtensions.cs (1)
5-22: LGTM! Correct implementation with clear disposal guidance.The method correctly creates a linked
CancellationTokenSourcethat combines the input token with a timeout. The documentation clearly states the disposal requirement, and the example demonstrates proper usage withusing var.The implementation is efficient and follows .NET best practices for this pattern.
Optional: Consider documenting edge case behavior.
You could enhance the XML documentation to mention behavior for special timeout values:
/// <summary> /// Creates a linked cancellation token source that will be canceled after the specified timeout or when the original token is canceled. /// </summary> /// <param name="cancellationToken">The cancellation token to link with the timeout.</param> -/// <param name="timeout">The timeout duration.</param> +/// <param name="timeout">The timeout duration. Use <see cref="Timeout.InfiniteTimeSpan"/> to disable the timeout.</param> /// <returns>A CancellationTokenSource that must be disposed to clean up resources.</returns> +/// <exception cref="ArgumentOutOfRangeException">Thrown when timeout is negative (except <see cref="Timeout.InfiniteTimeSpan"/>) or too large.</exception> /// <example>src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
39-40: Consider ivarne's suggestion to use IHttpClientFactory for cleaner timeout management.The current approach of combining tokens in every method works but creates repetitive code throughout the class. As ivarne noted in past comments, using
IHttpClientFactorywith a custom timeout argument could reduce this noise and improve maintainability.Based on past review comments.
Also applies to: 60-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Core/Extensions/CancellationTokenExtensions.cs(1 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(26 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/Extensions/CancellationTokenExtensions.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Extensions/CancellationTokenExtensions.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.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/Extensions/CancellationTokenExtensions.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
🧬 Code graph analysis (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
src/Altinn.App.Core/Helpers/ResponseWrapperStream.cs (3)
ResponseWrapperStream(6-165)ResponseWrapperStream(16-23)Dispose(29-37)src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (2)
PlatformHttpResponseSnapshotException(18-300)PlatformHttpResponseSnapshotException(157-175)
⏰ 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: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
src/Altinn.App.Core/Extensions/CancellationTokenExtensions.cs (1)
3-4: LGTM! Class declaration follows coding guidelines.The class correctly uses
internalaccessibility as required by the coding guidelines andstaticfor an extension class.src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
229-269: LGTM on GetBinaryDataStream streaming implementation.The method correctly handles disposal semantics:
- Success path wraps the stream in ResponseWrapperStream to ensure response disposal when the stream is disposed
- Exception during stream read properly disposes the response before rethrowing
- Non-success responses are snapshotted and disposed via PlatformHttpResponseSnapshotException
The streaming timeout covers request establishment and header receipt, while allowing the caller to control the pace of reading the stream content.
78-120: Consistent per-operation timeout implementation across all methods.The pattern of creating a linked CancellationTokenSource with
WithTimeout, then passingcts.Tokento all authentication, HTTP operations, and content reads is applied consistently throughout the class. This successfully addresses the previous blocker about infinite HttpClient.Timeout requiring per-operation cancellation tokens.Also applies to: 154-178, 202-217, 283-297, 326-342, 357-374, 449-460, 485-498, 527-552, 581-598, 622-644, 665-682, 699-725, 746-767
…flicts. Might consider using cts for cancellation later.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (6)
222-257: Streaming path: wrapper + snapshot exception are solid.
- ResponseWrapperStream correctly ties response lifetime to stream disposal.
- Snapshot exception ensures details without leaking disposed responses.
LGTM. Consider adding XML remarks on the method that callers must dispose the returned stream (use await using).
206-220: 404 semantics and nullability: unify behavior and avoid #nullable suppression.GetBinaryData returns null on 404 despite Task return type, forcing a #nullable disable block and making consumer behavior inconsistent with GetBinaryDataStream (which throws on 404).
Options:
- Preferred: change signature to Task<Stream?> and document null on 404.
- Or: throw (PlatformHttpException) on 404 to align with streaming.
If back‑compat constrains you now, add XML docs clarifying the nullable behavior and plan a breaking change later.
Would you like a follow‑up PR to introduce TryGetBinaryData(..., out Stream?) and obsolete this method in favor of consistent semantics?
447-451: Use structured logging templates instead of string interpolation.Interpolate-to-string loses structured fields and allocates unnecessarily. Prefer message templates.
- _logger.LogError( - $"Deleting data element {dataGuid} for instance {instanceIdentifier} failed with status code {response.StatusCode}" - ); + _logger.LogError( + "Deleting data element {DataGuid} for instance {InstanceIdentifier} failed with status code {StatusCode}", + dataGuid, instanceIdentifier, response.StatusCode);- _logger.LogError( - $"Storing attachment for instance {instanceId} failed with status code {StatusCode} - content {Content}", - response.StatusCode, await response.Content.ReadAsStringAsync(cancellationToken)); + _logger.LogError( + "Storing attachment for instance {InstanceId} failed with status code {StatusCode} - content {Content}", + instanceId, response.StatusCode, await response.Content.ReadAsStringAsync(cancellationToken));As per coding guidelines.
Also applies to: 539-543
506-511: URL building: keep relative paths consistently (BaseAddress is already set).Some methods build absolute URLs with _platformSettings.ApiStorageEndpoint while others use relative paths. Prefer relative paths everywhere; it reduces duplication and misconfiguration risk.
Example:
- string apiUrl = $"{_platformSettings.ApiStorageEndpoint}instances/{instanceId}/data?dataType={dataType}"; + string apiUrl = $"instances/{instanceId}/data?dataType={dataType}";Apply similarly in other absolute URL spots.
Also applies to: 567-573, 616-647
28-29: Optionally seal the class.Unless inheritance is intended, mark DataClient as sealed.
As per coding guidelines.
63-73: Use a named HttpClient ("StorageStreaming") for the streaming client to enable centralized configuration and explicit naming.The parameterless
CreateClient()call on line 64 works but is less explicit and harder to configure centrally. Adopting the named client pattern improves clarity and testability.To implement this suggestion:
- Change line 64 from
httpClientFactory.CreateClient()tohttpClientFactory.CreateClient("StorageStreaming")- Register and configure the named client in DI (e.g., in
ServiceCollectionExtensions.cs):services.AddHttpClient("StorageStreaming") .ConfigureHttpClient(client => { client.BaseAddress = new Uri(platformSettings.ApiStorageEndpoint); client.DefaultRequestHeaders.Add(General.SubscriptionKeyHeaderName, platformSettings.SubscriptionKey); client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/xml")); client.Timeout = TimeSpan.FromMinutes(30); });- Update test mocks in
DataClientTests.csto handle named client requests.test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs (2)
1166-1170: Align factory usage with a named client for streaming.Tests set up CreateClient(string.Empty). If production moves to a named client (e.g., "StorageStreaming"), mock and fixture should match:
- mocks.HttpClientFactoryMock.Setup(x => x.CreateClient(string.Empty)).Returns(streamingClient); + mocks.HttpClientFactoryMock.Setup(x => x.CreateClient("StorageStreaming")).Returns(streamingClient); ... - DataClient = new DataClient(httpClient, serviceProvider), + DataClient = new DataClient(httpClient, serviceProvider), BaseHttpClient = httpClient, StreamingHttpClient = streamingClient,And dispose remains as-is.
If you keep relying on a parameterless CreateClient() extension, add a matching mock setup for that extension path; otherwise, switch to the named client approach above.
Also applies to: 1194-1201, 1213-1219
414-450: Rename for accuracy: method returns null, not an empty stream.Test name suggests “empty stream” but asserts null. Rename to reflect behavior:
- public async Task GetBinaryData_returns_empty_stream_when_storage_returns_notfound(...) + public async Task GetBinaryData_returns_null_when_storage_returns_notfound(...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(24 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs(5 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/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.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/Infrastructure/Clients/Storage/DataClient.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/Infrastructure/Clients/Storage/DataClientTests.cs
🧬 Code graph analysis (2)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (4)
test/Altinn.App.Api.Tests/CustomWebApplicationFactory.cs (3)
HttpClient(92-105)HttpClient(107-120)HttpClient(126-177)src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs (15)
Activity(9-14)Activity(16-22)Activity(24-30)Activity(32-38)Activity(40-46)Activity(48-54)Activity(56-62)Activity(64-69)Activity(71-77)Activity(79-84)Activity(86-91)Activity(93-99)Activity(101-107)Activity(109-115)Activity(117-123)src/Altinn.App.Core/Helpers/ResponseWrapperStream.cs (3)
ResponseWrapperStream(6-165)ResponseWrapperStream(16-23)Dispose(29-37)src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (2)
PlatformHttpResponseSnapshotException(18-300)PlatformHttpResponseSnapshotException(157-175)
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs (3)
test/Altinn.App.Core.Tests/Internal/Auth/AuthenticationTokenResolverTest.cs (1)
Fixture(146-203)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (18)
Task(76-92)Task(95-140)Task(143-184)Task(187-220)Task(223-257)Task(260-299)Task(302-329)Task(332-369)Task(407-417)Task(420-451)Task(454-491)Task(494-543)Task(546-583)Task(586-627)Task(630-659)Task(662-705)DataClient(28-746)DataClient(47-73)src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (2)
PlatformHttpResponseSnapshotException(18-300)PlatformHttpResponseSnapshotException(157-175)
⏰ 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: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs (1)
483-525: Streaming tests look good and dispose correctly.
- Using await using on the returned stream prevents leaks.
- Error cases assert PlatformHttpResponseSnapshotException and status codes.
LGTM.
Also applies to: 526-597
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
55-72: Consider ivarne's suggestion: use IHttpClientFactory with per-call timeout instead of dedicated streaming client.As ivarne noted in the past review, the current approach of maintaining a separate
_streamingClientwith a fixed 30-minute timeout adds noise and lacks flexibility. Based on past review comments.Consider refactoring to:
- Remove the dedicated
_streamingClientfield- Inject
IHttpClientFactoryand callCreateClient()in methods that need custom timeouts- Pass timeout as a parameter to allow callers to override when needed
This would reduce the class's surface area and provide better flexibility for different streaming scenarios (some might need more or less than 30 minutes).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(18 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/Infrastructure/Clients/Storage/DataClient.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.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/Infrastructure/Clients/Storage/DataClient.cs
🧬 Code graph analysis (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (3)
src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs (15)
Activity(9-14)Activity(16-22)Activity(24-30)Activity(32-38)Activity(40-46)Activity(48-54)Activity(56-62)Activity(64-69)Activity(71-77)Activity(79-84)Activity(86-91)Activity(93-99)Activity(101-107)Activity(109-115)Activity(117-123)src/Altinn.App.Core/Helpers/ResponseWrapperStream.cs (3)
ResponseWrapperStream(6-165)ResponseWrapperStream(16-23)Dispose(29-37)src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (2)
PlatformHttpResponseSnapshotException(18-300)PlatformHttpResponseSnapshotException(157-175)
🪛 GitHub Actions: Verify dotnet format
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
[error] 122-122: CSharpier formatting check failed. File was not formatted. Expected: Around Line 122; Actual: Around Line 122. Run 'dotnet csharpier check .' (and 'dotnet csharpier write' to fix) to format the file.
⏰ 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: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
247-256: Disposal handling is correct.The try-catch ensures the response is disposed if
ReadAsStreamAsyncthrows, andResponseWrapperStreamtakes ownership on success. This properly addresses the past review concerns about response disposal.
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (2)
124-124: Consider case-insensitive hash comparison for robustness.While both hashes should be lowercase hex, using case-insensitive comparison (
string.Equals(sha256Hash, dataElementSignature.Sha256Hash, StringComparison.OrdinalIgnoreCase)) would be more defensive against format inconsistencies.Apply this diff if you prefer defensive comparison:
- if (sha256Hash != dataElementSignature.Sha256Hash) + if (!string.Equals(sha256Hash, dataElementSignature.Sha256Hash, StringComparison.OrdinalIgnoreCase))
134-139: Include DataElementId in ValidationIssue for better error reporting.Users will benefit from knowing which specific data element has an invalid signature, especially when multiple elements are signed.
Apply this diff:
return new ValidationIssue { Code = ValidationIssueCodes.DataElementCodes.InvalidSignatureHash, Severity = ValidationIssueSeverity.Error, CustomTextKey = "backend.validation_errors.invalid_signature_hash", + DataElementId = dataElementSignature.DataElementId, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs(1 hunks)src/Altinn.App.Core/Internal/Texts/TranslationService.cs(1 hunks)test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs(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:
src/Altinn.App.Core/Internal/Texts/TranslationService.cssrc/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cstest/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Internal/Texts/TranslationService.cssrc/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cstest/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.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/Texts/TranslationService.cssrc/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.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/SignatureHashValidator.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/SignatureHashValidatorTests.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/SignatureHashValidatorTests.cs
🧠 Learnings (1)
📚 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:
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs
🧬 Code graph analysis (3)
src/Altinn.App.Core/Internal/Texts/TranslationService.cs (1)
src/Altinn.App.Core/Internal/Language/LanguageConst.cs (1)
LanguageConst(3-8)
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (4)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (15)
Task(26-37)Task(50-58)Task(74-85)Task(98-107)Task(120-128)Task(140-146)Task(159-167)Task(179-186)Task(210-219)Task(232-241)Task(279-287)Task(300-309)Task(319-324)Task(333-338)Task(347-352)src/Altinn.App.Core/Models/Validation/ValidationIssue.cs (1)
ValidationIssue(9-105)src/Altinn.App.Core/Models/InstanceIdentifier.cs (1)
InstanceOwnerPartyId(111-118)src/Altinn.App.Core/Models/Validation/ValidationIssueCodes.cs (2)
ValidationIssueCodes(6-79)DataElementCodes(27-78)
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (5)
src/Altinn.App.Core/Internal/Sign/SignatureContext.cs (1)
Signee(85-109)src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (6)
SignatureHashValidator(20-183)Task(48-53)Task(55-101)Task(103-143)Task(165-170)ShouldRunForTask(39-43)src/Altinn.App.Core/Internal/Data/IDataClient.cs (12)
Task(26-37)Task(50-58)Task(74-85)Task(98-107)Task(120-128)Task(140-146)Task(159-167)Task(179-186)Task(210-219)Task(232-241)Task(279-287)Task(300-309)src/Altinn.App.Core/Models/Validation/ValidationIssue.cs (1)
ValidationIssue(9-105)src/Altinn.App.Core/Models/Validation/ValidationIssueCodes.cs (2)
ValidationIssueCodes(6-79)DataElementCodes(27-78)
⏰ 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: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Analyze (csharp)
- GitHub Check: Static code analysis
🔇 Additional comments (8)
src/Altinn.App.Core/Internal/Texts/TranslationService.cs (1)
202-212: LGTM! Clean addition of localized fallback text.The new backend fallback resource follows the established pattern and provides appropriate translations for signature hash validation errors across all supported languages.
test/Altinn.App.Core.Tests/Features/Validators/Default/SignatureHashValidatorTests.cs (1)
22-388: Excellent test coverage!The test suite comprehensively covers all critical paths including valid/invalid hashes, authentication modes, error handling, and behavioral contracts. The test structure is clean, well-organized, and follows xUnit best practices.
Based on learnings
src/Altinn.App.Core/Features/Validation/Default/SignatureHashValidator.cs (6)
20-43: LGTM! Clean implementation following coding guidelines.The class properly uses
internal sealedmodifiers and implements task-specific validation logic correctly by checking the task type inShouldRunForTask.As per coding guidelines
45-53: LGTM! Correct NoIncrementalValidation pattern.The validator correctly signals that incremental validation is not supported and throws
UnreachableExceptioninHasRelevantChangesto catch programming errors.
55-101: LGTM! Well-structured validation flow.The method properly propagates cancellation tokens, loads required configuration and metadata, and uses fail-fast semantics by returning on the first invalid signature. Structured logging on success is a nice touch.
112-120: Excellent! Proper async disposal prevents resource leaks.The
await usingpattern ensures the stream and underlying HTTP response are disposed correctly, even when handling large data elements with the streaming API.
145-163: LGTM! Solid error handling and configuration validation.The method correctly identifies restricted-read data types and throws a clear exception when the data type is misconfigured, helping catch configuration errors early.
165-182: LGTM! Correct SHA-256 hash generation and formatting.The implementation properly disposes the SHA256 instance and formats the digest consistently with the storage service format (lowercase hex). The documentation note about staying in sync with altinn-storage is valuable.
|
4 participants
Add a standard validator that validates that the signature hashes are still valid when leaving a signature task.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Localization