Signing: Simple signing integration test #1468
Conversation
📝 WalkthroughWalkthroughIntroduces a new signing workflow integration test with snapshots, adds a signing-specific test app configuration (metadata, authorization policy, BPMN), and updates the test fixture to support JSON data downloads and verification in instance data retrieval. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
martinothamar
added
ignore-for-release
backport-ignore
![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 (9)
test/Altinn.App.Integration.Tests/_testapps/_shared/Tracing.cs (1)
121-125: Optional: fallback for missing StatusCode in logs.FileStreamResult and some results don’t implement IStatusCodeActionResult, yielding blank status codes in snapshots. If you want consistent codes, fall back to HttpContext.Response.StatusCode (will require snapshot updates).
- var statusCode = (result as IStatusCodeActionResult)?.StatusCode; + var statusCode = (result as IStatusCodeActionResult)?.StatusCode + ?? context.HttpContext.Response.StatusCode;Will updating this require snapshot updates across tests? If yes, we can batch-regenerate them in a follow-up.
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/authorization/policy.xml (1)
4-66: Rule 1 description vs actions mismatch.Description says “instantiate,” but the Action target includes both instantiate and read. If “read” isn’t required here (covered by Rule 2), drop it to reduce over-permitting; otherwise, update the description.
<xacml:AnyOf> <xacml:AllOf> <xacml:Match MatchId="urn:oasis:names:tc:xacml:3.0:function:string-equal-ignore-case"> <xacml:AttributeValue DataType="http://www.w3.org/2001/XMLSchema#string">instantiate</xacml:AttributeValue> <xacml:AttributeDesignator AttributeId="urn:oasis:names:tc:xacml:1.0:action:action-id" Category="urn:oasis:names:tc:xacml:3.0:attribute-category:action" DataType="http://www.w3.org/2001/XMLSchema#string" MustBePresent="false" /> </xacml:Match> </xacml:AllOf> - <xacml:AllOf> - <xacml:Match MatchId="urn:oasis:names:tc:xacml:3.0:function:string-equal-ignore-case"> - <xacml:AttributeValue DataType="http://www.w3.org/2001/XMLSchema#string">read</xacml:AttributeValue> - <xacml:AttributeDesignator AttributeId="urn:oasis:names:tc:xacml:1.0:action:action-id" Category="urn:oasis:names:tc:xacml:3.0:attribute-category:action" DataType="http://www.w3.org/2001/XMLSchema#string" MustBePresent="false" /> - </xacml:Match> - </xacml:AllOf> </xacml:AnyOf>test/Altinn.App.Integration.Tests/Signing/Simple.cs (1)
5-5: Typo in namespace (“Singning”).Fix to “Signing” for consistency and discoverability.
-namespace Altinn.App.Integration.Tests.Singning; +namespace Altinn.App.Integration.Tests.Signing;test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_4_Download-Data[1].verified.txt (1)
82-82: Add trailing newline for consistency.A final newline helps avoid unnecessary diffs across platforms/editors.
test/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.cs (3)
92-97: Make content-type check case-insensitive.Be robust against metadata casing variations.
Apply:
- var hasJsonContentType = dataType.AllowedContentTypes.Any(ac => ac == "application/json"); + var hasJsonContentType = dataType.AllowedContentTypes + .Any(ac => string.Equals(ac, "application/json", StringComparison.OrdinalIgnoreCase));
112-126: Reduce duplication in JSON/form reading.Both branches read Argon.JToken then wrap into different records. Consolidate to one path for readability and fewer allocations.
- if (hasJsonContentType) - { - var dataResponse = new ApiResponse(_fixture, response); - var readResponse = await dataResponse.Read<Argon.JToken>(); // Argon is being used by VerifyTests for JSON - var formData = new InstanceDataDownload.Json(i, dataGuid, dataType.Id, readResponse); - data.Add(formData); - } - else - { - // This is binary data (attachment) - var dataResponse = new ApiResponse(_fixture, response); - var readResponse = await dataResponse.Read<byte[]>(); - var binaryData = new InstanceDataDownload.Binary(i, dataGuid, dataType.Id, readResponse); - data.Add(binaryData); - } + if (hasJsonContentType) + { + var dataResponse = new ApiResponse(_fixture, response); + var readResponse = await dataResponse.Read<Argon.JToken>(); + data.Add(new InstanceDataDownload.Json(i, dataGuid, dataType.Id, readResponse)); + } + else + { + var dataResponse = new ApiResponse(_fixture, response); + var readResponse = await dataResponse.Read<byte[]>(); + data.Add(new InstanceDataDownload.Binary(i, dataGuid, dataType.Id, readResponse)); + }Optionally, hoist a local helper:
async Task<ReadApiResponse<Argon.JToken>> ReadJson(HttpResponseMessage r) => await new ApiResponse(_fixture, r).Read<Argon.JToken>();and reuse in both the AppLogic and hasJson branches.
378-385: Fix typos in comments.Minor spelling nits: “deterministic”.
- // PDF output is not deterministic and ASP.NET core is not determnistic + // PDF output is not deterministic and ASP.NET Core is not deterministic - binary.Data.IncludeBodyInSnapshot = false; // Avoid non-determnistic PDF snapshots + binary.Data.IncludeBodyInSnapshot = false; // Avoid non-deterministic PDF snapshotstest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_0_Instance.verified.txt (2)
113-113: Inconsistent size scrubbing — prefer {Scrubbed} to avoid flaky diffs.Later snapshots scrub Size, but here it’s a concrete value (176). Align to scrubbed form for stability across framework changes and content variations.
Proposed diff:
- Size: 176, + Size: {Scrubbed},
51-53: Mask Content-Length in HTTP snapshots
Content-Length values vary with serialization and whitespace changes; extend the HTTP snapshot scrubber to replace these headers with a placeholder for more resilient tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
test/Altinn.App.Integration.Tests/Signing/Simple.cs(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_0_Instance.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_2_Download-Instance.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_3_Download-Data[0].verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_4_Download-Data[1].verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_5_Download-PDF.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_6_Logs.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.cs(4 hunks)test/Altinn.App.Integration.Tests/_testapps/_shared/Tracing.cs(1 hunks)test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/applicationmetadata.json(1 hunks)test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/authorization/policy.xml(1 hunks)test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/process/process.bpmn(1 hunks)
🧰 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.Integration.Tests/_testapps/_shared/Tracing.cstest/Altinn.App.Integration.Tests/Signing/Simple.cstest/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Integration.Tests/_testapps/_shared/Tracing.cstest/Altinn.App.Integration.Tests/Signing/Simple.cstest/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.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.Integration.Tests/_testapps/_shared/Tracing.cstest/Altinn.App.Integration.Tests/Signing/Simple.cstest/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.cs
test/Altinn.App.Integration.Tests/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Include integration tests for platform service interactions (using Docker Testcontainers)
Files:
test/Altinn.App.Integration.Tests/_testapps/_shared/Tracing.cstest/Altinn.App.Integration.Tests/Signing/Simple.cstest/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.cs
test/**/AppFixture.*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the AppFixture pattern as a central orchestrator for integration tests; add new API operation coverage via AppFixture.{Feature}.cs
Files:
test/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.cs
🧠 Learnings (9)
📓 Common learnings
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.
📚 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.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_0_Instance.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_5_Download-PDF.verified.txttest/Altinn.App.Integration.Tests/_testapps/_shared/Tracing.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/applicationmetadata.jsontest/Altinn.App.Integration.Tests/Signing/Simple.cstest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_3_Download-Data[0].verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_4_Download-Data[1].verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_2_Download-Instance.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_6_Logs.verified.txt
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_0_Instance.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_5_Download-PDF.verified.txttest/Altinn.App.Integration.Tests/Signing/Simple.cstest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_3_Download-Data[0].verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_4_Download-Data[1].verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_2_Download-Instance.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_6_Logs.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 test/Altinn.App.Integration.Tests/**/*.cs : Include integration tests for platform service interactions (using Docker Testcontainers)
Applied to files:
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_0_Instance.verified.txttest/Altinn.App.Integration.Tests/Signing/Simple.cstest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_3_Download-Data[0].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 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.Integration.Tests/Signing/Simple.cstest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_3_Download-Data[0].verified.txt
📚 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.Integration.Tests/Signing/Simple.cstest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_3_Download-Data[0].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 test/**/AppFixture.*.cs : Follow the AppFixture pattern as a central orchestrator for integration tests; add new API operation coverage via AppFixture.{Feature}.cs
Applied to files:
test/Altinn.App.Integration.Tests/Signing/Simple.cs
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
test/Altinn.App.Integration.Tests/Signing/Simple.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.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_3_Download-Data[0].verified.txt
🧬 Code graph analysis (2)
test/Altinn.App.Integration.Tests/Signing/Simple.cs (8)
test/Altinn.App.Integration.Tests/_fixture/AppFixtureClassFixture.cs (1)
AppFixtureClassFixture(27-88)test/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.cs (10)
Task(27-37)Task(39-52)Task(54-137)Task(139-162)Task(164-197)Task(199-209)Task(211-243)Task(245-281)Task(283-312)Task(326-401)test/Altinn.App.Integration.Tests/_fixture/TestApps.cs (1)
TestApps(3-6)test/Altinn.App.Integration.Tests/_fixture/ScopedVerifier.cs (1)
UseTestCase(21-21)test/Altinn.App.Tests.Common/Auth/TestAuthentication.cs (1)
GetSystemUserToken(559-577)src/Altinn.App.Api/Models/InstansiationInstance.cs (1)
InstansiationInstance(10-42)src/Altinn.App.Api/Models/ProcessNext.cs (1)
ProcessNext(8-21)test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs (1)
GetSnapshotAppLogs(227-295)
test/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.cs (1)
test/Altinn.App.Integration.Tests/_fixture/AppFixture.ApiResponse.cs (3)
ApiResponse(40-105)ReadApiResponse(12-38)Dispose(104-104)
⏰ 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: Static code analysis
- 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)
🔇 Additional comments (17)
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/authorization/policy.xml (1)
1-319: Policy looks coherent for the signing-simple scenario.Combining algorithm, placeholders, role/systemuser predicates, and obligations align with the BPMN and tests.
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/process/process.bpmn (1)
7-22: Signing task config LGTM.taskType=signing, actions (sign/reject), dataTypesToSign=model, and signatureDataType=signatureInformation match the test flow.
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_3_Download-Data[0].verified.txt (1)
1-66: Snapshot matches JSON download expectations.Accept/Content-Type are JSON; payload shape aligns with prefill.
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txt (1)
1-74: ProcessNext snapshot looks good.Correct method, endpoint, and successful AppProcessState response.
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_5_Download-PDF.verified.txt (1)
1-61: PDF download snapshot OK.application/pdf and Content-Disposition filename are as expected.
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_6_Logs.verified.txt (1)
1-43: Logs snapshot accepted with known init warning.INIT ERROR about missing services override is expected in this scenario; rest of traces show the intended flow.
test/Altinn.App.Integration.Tests/Signing/Simple.cs (1)
21-74: Test flow and disposals LGTM.Good use of fixture, snapshot scrubbers, and verification across create, process/next, downloads, and logs.
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_4_Download-Data[1].verified.txt (1)
1-81: Snapshot aligns with new JSON download path.Headers, Accept: application/json, and JSON body shape match the Json variant. Looks good.
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/applicationmetadata.json (3)
59-71: Good: JSON data type declared for signature information.application/json with maxCount 1 and app-owned contributors is appropriate for server-produced signing metadata.
Please confirm that policy.xml for this scenario grants read access for SystemUser to download signatureInformation while restricting writes to app:owned.
73-81: Double-check instantiation constraints.partyTypesAllowed all false + disallowUserInstantiation false can be confusing. If instantiation is only via SystemUser/service-owner, this is fine—just confirm it’s intentional for this scenario.
39-48: Verify classRef matches actual model class
EnsureAltinn.App.Models.model.modelexists in the test app and corresponds to the intended form model for Task_1 to avoid 415/500 on load/patch.test/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.cs (2)
359-367: LGTM: Snapshot handling for Json variant.Mirrors the Form case and uses the same scrubbers/parameters shape, keeping snapshots consistent.
414-419: LGTM: New Json data wrapper.Sealed record with proper disposal matches existing Binary/Form patterns.
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_0_Instance.verified.txt (1)
71-126: Overall snapshot content LGTM.Headers, process state (signing task), and SystemUser metadata look coherent. Synthetic identifiers align with our test data policy.
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_2_Download-Instance.verified.txt (3)
64-69: LGTM: SelfLinks hosts are consistent here.Apps and Platform URLs look correct and consistent with local env domains.
84-102: Verify model data element remains locked post-signing as intended.Locked: true for the model element after process end is plausible; confirm this matches the signing flow’s expected retention/lock policy and won’t break downstream edit scenarios.
77-83: Archival flags align with Ended process — just confirming intent.Instance is marked IsArchived: true with Archived timestamp, and LastChanged reflects completion. If this is expected immediately after signing in this scenario, no action needed.
Also applies to: 156-160
| Authenticated.SystemUser su => ( | ||
| $"{su.SystemUserOrgNr}/{su.SystemUserId}", | ||
| $"{su.SystemUserOrgNr}/{su.SystemUserId[0]}", | ||
| su.AuthenticationLevel, | ||
| su.AuthenticationMethod | ||
| ), |
There was a problem hiding this comment.
SystemUserId is truncated to a single character (likely unintended).
Indexing with [0] logs only the first char and risks out-of-range on empty ids. It also conflicts with the snapshots that show the full SystemUserId.
Apply:
- Authenticated.SystemUser su => (
- $"{su.SystemUserOrgNr}/{su.SystemUserId[0]}",
- su.AuthenticationLevel,
- su.AuthenticationMethod
- ),
+ Authenticated.SystemUser su => (
+ $"{su.SystemUserOrgNr}/{su.SystemUserId}",
+ su.AuthenticationLevel,
+ su.AuthenticationMethod
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Authenticated.SystemUser su => ( | |
| $"{su.SystemUserOrgNr}/{su.SystemUserId}", | |
| $"{su.SystemUserOrgNr}/{su.SystemUserId[0]}", | |
| su.AuthenticationLevel, | |
| su.AuthenticationMethod | |
| ), | |
| Authenticated.SystemUser su => ( | |
| $"{su.SystemUserOrgNr}/{su.SystemUserId}", | |
| su.AuthenticationLevel, | |
| su.AuthenticationMethod | |
| ), |
🤖 Prompt for AI Agents
In test/Altinn.App.Integration.Tests/_testapps/_shared/Tracing.cs around lines
98 to 102, the code currently uses su.SystemUserId[0] which truncates the id to
its first character and can throw if the id is empty; replace the indexed access
with the full id and guard against null by using the full string (e.g.
su.SystemUserId ?? string.Empty) so the logged tuple contains the complete
SystemUserId and will not throw on empty/null values.
| SelfLinks: { | ||
| Apps: https://app:5005/ttd/basic/instances/500700/<instanceGuid>/data/<dataElementId[0]>, | ||
| Platform: https://platform./storage/api/v1/instances/500700/<instanceGuid>/data/<dataElementId[0]> | ||
| }, |
There was a problem hiding this comment.
Broken Platform SelfLink host ("https://platform./...") — likely scrubber/formatting bug; fix snapshot (and source if applicable).
Apps link uses app:5005 and Platform link earlier uses platform.local.altinn.cloud, but Data[0].SelfLinks.Platform drops the host. This will mask a real defect or make the test assert an invalid URL.
Apply this diff in the snapshot (and ensure the producer code/scrubber emits consistent hosts):
- Platform: https://platform./storage/api/v1/instances/500700/<instanceGuid>/data/<dataElementId[0]>
+ Platform: https://platform.local.altinn.cloud/storage/api/v1/instances/500700/<instanceGuid>/data/<dataElementId[0]>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SelfLinks: { | |
| Apps: https://app:5005/ttd/basic/instances/500700/<instanceGuid>/data/<dataElementId[0]>, | |
| Platform: https://platform./storage/api/v1/instances/500700/<instanceGuid>/data/<dataElementId[0]> | |
| }, | |
| SelfLinks: { | |
| Apps: https://app:5005/ttd/basic/instances/500700/<instanceGuid>/data/<dataElementId[0]>, | |
| Platform: https://platform.local.altinn.cloud/storage/api/v1/instances/500700/<instanceGuid>/data/<dataElementId[0]> | |
| }, |
🤖 Prompt for AI Agents
In
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_0_Instance.verified.txt
around lines 109 to 112, the Platform SelfLinks host was scrubbed to
"https://platform./" which is invalid; update the snapshot to use the correct
host (match the other links, e.g. "https://platform.local.altinn.cloud/...") and
run the test to confirm; additionally, check and fix the scrubber/producer that
generates SelfLinks so it preserves or consistently replaces the platform host
(adjust the scrubber replacement rule or source generation to emit the full host
instead of "platform.").
martinothamar
force-pushed
the
chore/signing-integration-test
branch
from
98b9c41 to
8d26cf8
Compare
|
Found one issue in localtest, implementation seems to be a little out of sync: Altinn/app-localtest#178 But there is still the issue of But I guess the fundamental issue in |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_5_Logs.verified.txt (2)
15-18: SystemUser signing fails due to missing party mapping; suggest ActionOnBehalfOf in test.The error comes from missing SSN/OrgNr on the signee context when using a system user. In the test, pass
ProcessNext.ActionOnBehalfOf = "950474084"for the SystemUser run so the validator can resolve the party.
1-1: BOM present and no trailing newline.Consider removing BOM and adding a trailing newline to reduce diff churn in snapshots.
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_6_Logs.verified.txt (1)
1-1: BOM present and no trailing newline.Tiny snapshot hygiene nit; optional to fix.
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txt (2)
11-12: Snapshot encodes a known 500; gate this variant to avoid future flakes.Once localtest/auth mapping is fixed, this will turn 200 and break the snapshot. Consider gating the SystemUser run behind an env var until the dependency lands.
If you want, I can propose a small conditional-skip snippet.
1-1: BOM present and no trailing newline.Snapshot nit; optional.
test/Altinn.App.Integration.Tests/Signing/Simple.cs (3)
5-5: Fix namespace typo.Use “Signing” instead of “Singning”.
Apply this diff:
-namespace Altinn.App.Integration.Tests.Singning; +namespace Altinn.App.Integration.Tests.Signing;
63-67: Include ActionOnBehalfOf for SystemUser to enable party resolution during signing.Without this, the signing validator can’t map the system user to an org (leading to the 500 you snapshot). Pass org number when
auth == SystemUser.Apply this diff:
- using var processNextResponse = await fixture.Instances.ProcessNext( - token, - instantiationData, - new ProcessNext { Action = "sign" } - ); + var processNext = auth == Auth.SystemUser + ? new ProcessNext { Action = "sign", ActionOnBehalfOf = "950474084" } + : new ProcessNext { Action = "sign" }; + using var processNextResponse = await fixture.Instances.ProcessNext( + token, + instantiationData, + processNext + );
21-29: Optionally gate the SystemUser variant to avoid brittle snapshots until localtest is updated.Early-return when an env flag isn’t set; keeps CI green post‑fix.
Example (add near the start of the test method):
public async Task Full([CombinatorialValues(Auth.User, Auth.SystemUser)] Auth auth) { + if (auth == Auth.SystemUser && + Environment.GetEnvironmentVariable("TEST_INCLUDE_SYSTEMUSER_SIGNING") != "1") + { + _output.WriteLine("Skipping SystemUser signing until localtest fix is available."); + return; + }test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_2_Download-Instance.verified.txt (1)
1-1: BOM present and no trailing newline.Optional tidy-up for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
test/Altinn.App.Integration.Tests/Signing/Simple.cs(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_0_Instance.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_2_Download-Instance.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_3_Download-Data[0].verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_4_Download-Data[1].verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_5_Logs.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_0_Instance.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_1_ProcessNext.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_2_Download-Instance.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_3_Download-Data[0].verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_4_Download-Data[1].verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_5_Download-PDF.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_6_Logs.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.cs(4 hunks)test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/applicationmetadata.json(1 hunks)test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/authorization/policy.xml(1 hunks)test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/process/process.bpmn(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_3_Download-Data[0].verified.txt
- test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_5_Download-PDF.verified.txt
- test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_1_ProcessNext.verified.txt
- test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_0_Instance.verified.txt
- test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_4_Download-Data[1].verified.txt
- test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_2_Download-Instance.verified.txt
- test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_0_Instance.verified.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_4_Download-Data[1].verified.txt
- test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_3_Download-Data[0].verified.txt
- test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/authorization/policy.xml
- test/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.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.Integration.Tests/Signing/Simple.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Integration.Tests/Signing/Simple.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.Integration.Tests/Signing/Simple.cs
test/Altinn.App.Integration.Tests/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Include integration tests for platform service interactions (using Docker Testcontainers)
Files:
test/Altinn.App.Integration.Tests/Signing/Simple.cs
🧠 Learnings (9)
📓 Common learnings
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.
📚 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.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_6_Logs.verified.txttest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/applicationmetadata.jsontest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_5_Logs.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_2_Download-Instance.verified.txttest/Altinn.App.Integration.Tests/Signing/Simple.cs
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=User_6_Logs.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_5_Logs.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txttest/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_2_Download-Instance.verified.txttest/Altinn.App.Integration.Tests/Signing/Simple.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.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txttest/Altinn.App.Integration.Tests/Signing/Simple.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:
test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_1_ProcessNext.verified.txttest/Altinn.App.Integration.Tests/Signing/Simple.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.Integration.Tests/Signing/Simple.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.Integration.Tests/Signing/Simple.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/**/AppFixture.*.cs : Follow the AppFixture pattern as a central orchestrator for integration tests; add new API operation coverage via AppFixture.{Feature}.cs
Applied to files:
test/Altinn.App.Integration.Tests/Signing/Simple.cs
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
test/Altinn.App.Integration.Tests/Signing/Simple.cs
🧬 Code graph analysis (1)
test/Altinn.App.Integration.Tests/Signing/Simple.cs (7)
test/Altinn.App.Integration.Tests/_fixture/AppFixtureClassFixture.cs (1)
AppFixtureClassFixture(27-88)test/Altinn.App.Integration.Tests/_fixture/Operations/AppFixture.Instances.cs (10)
Task(27-37)Task(39-52)Task(54-137)Task(139-162)Task(164-197)Task(199-209)Task(211-243)Task(245-281)Task(283-312)Task(326-401)test/Altinn.App.Integration.Tests/_fixture/TestApps.cs (1)
TestApps(3-6)test/Altinn.App.Integration.Tests/_fixture/ScopedVerifier.cs (1)
UseTestCase(21-21)test/Altinn.App.Tests.Common/Auth/TestAuthentication.cs (1)
GetSystemUserToken(559-577)src/Altinn.App.Api/Models/InstansiationInstance.cs (1)
InstansiationInstance(10-42)src/Altinn.App.Api/Models/ProcessNext.cs (1)
ProcessNext(8-21)
🔇 Additional comments (4)
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/process/process.bpmn (1)
15-21: Signing task config looks correct and consistent with metadata.
dataTypesToSign=model,signatureDataType=signatureInformation, andrunDefaultValidator=truealign with the scenario and the applicationmetadata. No issues spotted.test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/signing-simple/config/applicationmetadata.json (2)
39-56: Model data type matches process Task_1; classRef is plausible for Basic app.
taskId: "Task_1"corresponds to the BPMN, andautoCreate: trueshould produce the form data needed before signing.
59-71: Good restriction on signature payload writers.
signatureInformationwithallowedContributers: ["app:owned"]prevents end-user tampering. Solid.test/Altinn.App.Integration.Tests/Signing/_snapshots/SimpleTests.Full_auth=SystemUser_2_Download-Instance.verified.txt (1)
71-81: Instance state and task metadata look correct for signing task.
AltinnTaskType: signingwithElementId: Task_1lines up with BPMN and metadata. Good snapshot coverage.
danielskovli
changed the title
2 participants
Description
Simple signing integration test
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit