chore: enrich validator api #1958
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/Digdir.Library.Dialogporten.WebApiClient/IDialogTokenValidator.cs (1)
8-11: Consider adding doc comments for the new optional parameters.The
Validatemethod now has multiple optional parameters (dialogId, requiredActions, options). It would be clearer to future maintainers if each parameter's purpose and usage were documented.public interface IDialogTokenValidator { + /// <summary> + /// Validates a Dialog token with optional parameters for dialog context, required actions, and validation settings. + /// </summary> + /// <param name="token">The token to validate.</param> + /// <param name="dialogId">Optional, the dialog ID to check.</param> + /// <param name="requiredActions">Optional, the actions that must be included in the token.</param> + /// <param name="options">Optional, additional validation parameters (issuer check, lifetime check, etc.).</param> IValidationResult Validate( ReadOnlySpan<char> token, Guid? dialogId = null, string[]? requiredActions = null, DialogTokenValidationParameters? options = null); }src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs (1)
27-31: Consider using a built-in URI builder for combining segments.While
CombineUriSegmentshandles trimming and separators, usingUrior existing API might avoid possible edge cases with trailing slashes or special characters.src/Digdir.Library.Dialogporten.WebApiClient/Common/ClaimsPrincipleExtensions.cs (2)
10-11: Consider using more descriptive claim names or adding documentationThe claim names "i" for dialog ID and "a" for actions are not very descriptive. Consider:
- Using more descriptive constant names (e.g.,
DIALOG_ID_CLAIMinstead of justdialogIdClaimName)- Adding XML documentation to explain what these claim types represent
- Making the claim names configurable through the validator options
This would improve code readability and maintainability.
Also applies to: 18-19
80-93: Consider using built-in StringComparison instead of custom comparerWhile the
CaseInsensitiveCharComparerimplementation is correct, .NET already provides built-in mechanisms for case-insensitive character comparison. Consider usingstring.Equals(string1, string2, StringComparison.OrdinalIgnoreCase)instead of the custom comparer for better maintainability and potentially better performance.- var cleanedExpectedIssuer = expectedIssuer.AsSpan().TrimEnd('/'); - var cleanedIssuerClaim = iss.AsSpan().TrimEnd('/'); - return cleanedExpectedIssuer.SequenceEqual(cleanedIssuerClaim, CaseInsensitiveCharComparer.Instance); + var cleanedExpectedIssuer = expectedIssuer.TrimEnd('/'); + var cleanedIssuerClaim = iss.TrimEnd('/'); + return string.Equals(cleanedExpectedIssuer, cleanedIssuerClaim, StringComparison.OrdinalIgnoreCase);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Digdir.Library.Dialogporten.WebApiClient/Common/ClaimsPrincipleExtensions.cs(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/IDialogTokenValidator.cs(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs(2 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Services/EdDsaSecurityKeysCacheService.cs(1 hunks)src/Digdir.Tool.Dialogporten.Benchmarks/Digdir.Tool.Dialogporten.Benchmarks.csproj(1 hunks)src/Digdir.Tool.Dialogporten.Benchmarks/Program.cs(1 hunks)src/Digdir.Tool.Dialogporten.Benchmarks/TokenValidatorBenchmarks.cs(1 hunks)tests/Digdir.Library.Dialogporten.WebApiClient.Unit.Tests/DialogTokenValidatorTests.cs(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/Digdir.Tool.Dialogporten.Benchmarks/Digdir.Tool.Dialogporten.Benchmarks.csproj (1)
Learnt from: oskogstad
PR: Altinn/dialogporten#1941
File: src/Digdir.Library.Dialogporten.WebApiClient.WebApiSample/Digdir.Library.Dialogporten.WebApiClient.WebApiSample.csproj:1-3
Timestamp: 2025-02-23T19:09:42.565Z
Learning: In the Dialogporten solution, common build settings like `TargetFramework`, `UserSecretsId`, `Nullable`, and `ImplicitUsings` are defined in `Directory.Build.props` and shared across all projects, making them redundant in individual .csproj files.
🪛 Gitleaks (8.21.2)
src/Digdir.Tool.Dialogporten.Benchmarks/TokenValidatorBenchmarks.cs
18-18: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy apps / Deploy job sync-resource-policy-information-job to test
- GitHub Check: Dry run deploy apps / Deploy job sync-subject-resource-mappings-job to test
🔇 Additional comments (23)
src/Digdir.Tool.Dialogporten.Benchmarks/Program.cs (1)
3-3: Benchmark focus changed to token validation.The change from running
TokenGenerationBenchmarktoTokenValidatorBenchmarksaligns well with the PR objective of enriching the validator API.src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj (1)
37-37: Appropriate visibility for benchmarking.Adding
InternalsVisibleTofor the benchmarks project is the correct approach to allow access to internal members for performance testing without exposing them publicly.src/Digdir.Tool.Dialogporten.Benchmarks/Digdir.Tool.Dialogporten.Benchmarks.csproj (1)
15-15: Required project reference added for benchmarks.The project reference to WebApiClient is necessary for the validator benchmarks to work with the validation components.
src/Digdir.Library.Dialogporten.WebApiClient/Services/EdDsaSecurityKeysCacheService.cs (1)
24-28: Flexible constructor options enhance testability.The addition of both a parameterless constructor and one accepting predefined keys improves flexibility, particularly for testing and benchmarking scenarios where specific key sets need to be used.
src/Digdir.Library.Dialogporten.WebApiClient/IDialogTokenValidator.cs (2)
12-12: No functional changes at this line.
14-19: Guard against unintended shared state inDialogTokenValidationParameters.Default.Because
Defaultreturns a single shared instance, mutating any of its properties (e.g.,ValidateLifetime) in one place will affect all usage. If you intend for code to adjust these parameters at runtime for different contexts, consider returning a new instance on eachgetor making the properties read-only.src/Digdir.Tool.Dialogporten.Benchmarks/TokenValidatorBenchmarks.cs (4)
16-18: Verify if this embedded JWT contains sensitive or live data.The static analysis suggests this token might be sensitive. If it's only for testing or benchmarking, confirm that it doesn't expose real credentials or personally identifiable information. Otherwise, consider obfuscating or storing it in a secure location.
🧰 Tools
🪛 Gitleaks (8.21.2)
18-18: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
30-31: Benchmark approach is straightforward and effective.
No concerns for the one-liner benchmark method usage.
33-35: PublicKey import is well-structured.
No issues spotted in the helper method converting base64-URL strings to public keys.
36-41: Clean custom clock implementation.
This sealed class is well-defined for benchmarking. No further remarks.tests/Digdir.Library.Dialogporten.WebApiClient.Unit.Tests/DialogTokenValidatorTests.cs (8)
6-6: All these lines reference the new or updated test setup.
They appear consistent, and the usage ofGetSutwith theValidTimeStampandValidPublicKeyPairsis clear. No changes recommended.Also applies to: 16-16, 29-29, 43-43, 53-53, 68-68, 82-82, 100-100, 108-108
112-127: Good coverage for the 'Not Valid Before' scenario.
The new test (ShouldReturnError_WhenUsedBeforeNbf) adds essential coverage fornbf. No issues found.
128-132: Properly tests empty token handling.
TheShouldReturnError_GivenEmptyTokentest covers an important edge case. Looks good.
227-230: Valid token with claims test is well-structured.
The new lines ensure that the test environment uses a preset timestamp. No concerns.
273-286: Test for invalid issuer is appropriate.
This test confirms an invalid issuer triggers the correct error and helps ensure reliability of the code.
288-303: Dialog ID validity check test is thorough.
Ensures that any mismatch indialogIdis caught. Implementation looks correct.
304-318: Action requirement coverage is well-handled.
The test ensures enforcement for required actions in the token.
319-331: Global side-effects for clock skew.
DialogTokenValidationParameters.Default.ClockSkew = TimeSpan.Zero;modifies the default instance for all tests and possibly beyond. This can introduce subtle side-effects if other tests also useDialogTokenValidationParameters.Default.Would you like to isolate test configurations from the global default to prevent cross-test interference?
src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs (1)
33-39: Expanded validation checks look comprehensive.The new checks for lifetime (
nbf,exp), issuer, dialog ID, and actions are well-integrated. The flow is logically structured and the error messages are clear. Excellent improvement in flexibility and readability.Also applies to: 55-78
src/Digdir.Library.Dialogporten.WebApiClient/Common/ClaimsPrincipleExtensions.cs (4)
48-51: Excellent approach for issuer validationThe use of Span, trailing slash trimming, and case-insensitive comparison for issuer validation is efficient and handles common URL comparison edge cases correctly.
53-61: Good implementation of time validation with clock skewThe implementation of expiration time and not-before-time validation correctly accounts for clock skew in opposite directions. Adding clock skew to expiration time and subtracting it from not-before time properly increases the validity window to account for clock differences.
Also applies to: 63-71
73-78: Well-designed utility method with proper nullability annotationsThe
TryGetClaimValueutility method follows the pattern of .NET's TryParse methods, and the use of[NotNullWhen(true)]provides good compiler support for null checking.
30-38:❓ Verification inconclusive
Verify the actions verification logic
The current implementation requires all specified required actions to be present in the token's actions claim. However, it could be more efficient by using a HashSet for the required actions rather than relying on Distinct and Intersect operations.
🏁 Script executed:
#!/bin/bash # Confirm whether the actions verification logic is consistent with other parts of the codebase # Look for other instances of action validation to confirm consistent approach rg -A 5 -B 5 "requiredActions" --type csLength of output: 8801
Review the actions verification logic performance suggestion
The implementation in
src/Digdir.Library.Dialogporten.WebApiClient/Common/ClaimsPrincipleExtensions.cs(lines 30–38) correctly ensures that all required actions are present in the token’s actions claim by using LINQ’sDistinctandIntersectoperations. However, as a performance consideration, please review whether replacing these with a HashSet-based approach (usingStringComparer.OrdinalIgnoreCase) might simplify the logic and reduce overhead, especially if the list of required actions grows large. Note that similar logic is used elsewhere in the codebase and tests indicate consistent behavior. It’s advisable to benchmark the proposed change to ensure it meets both the efficiency and correctness requirements.
src/Digdir.Library.Dialogporten.WebApiClient/Common/ClaimsPrincipleExtensions.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Library.Dialogporten.WebApiClient/Common/ClaimsPrincipleExtensions.cs
Outdated
Show resolved
Hide resolved
|
Warning Rate limit exceeded@MagnusSandgren has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request enhances the Dialogporten Web API client library by introducing new extension methods for the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
src/Digdir.Library.Dialogporten.WebApiClient/Common/ClaimsPrincipleExtensions.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/Digdir.Library.Dialogporten.WebApiClient/IDialogTokenValidator.cs (1)
14-20: Consider adding XML documentation or summary comments.
TheDialogTokenValidationParametersclass is intuitive, but adding code comments for each property would clarify when and why to override defaults, improving maintainability.src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs (2)
15-15: Naming consistency.
Consider whetherTokenPropertyNameis needed in the class or if a shared constants file might aid reuse in other validators.
20-25: Dynamic issuer derivation.
Combiningsettings.BaseUriwith"api/v1"is convenient. Just confirm that trailing/leading slashes are properly handled and tested for various host URIs.src/Digdir.Library.Dialogporten.WebApiClient/Common/ClaimsPrincipalExtensions.cs (2)
1-7: Missing XML documentation for public API.Consider adding XML documentation comments to describe the purpose and usage of these extension methods, especially since they form part of the validation API.
namespace Altinn.ApiClients.Dialogporten.Common; +/// <summary> +/// Extension methods for ClaimsPrincipal to validate dialog token claims. +/// </summary> internal static class ClaimsPrincipalExtensions {
80-94: Consider reusing existing StringComparer.While the
CaseInsensitiveCharComparerimplementation is correct, you could potentially leverage existing .NET functionality for case-insensitive string comparison.public static bool VerifyIssuer(this ClaimsPrincipal claimsPrincipal, string expectedIssuer) { const string issuerClaimName = "iss"; if (!claimsPrincipal.TryGetClaimValue(issuerClaimName, out var iss)) { return false; } - var cleanedExpectedIssuer = expectedIssuer.AsSpan().TrimEnd('/'); - var cleanedIssuerClaim = iss.AsSpan().TrimEnd('/'); - return cleanedExpectedIssuer.SequenceEqual(cleanedIssuerClaim, CaseInsensitiveCharComparer.Instance); + return string.Equals( + expectedIssuer.TrimEnd('/'), + iss.TrimEnd('/'), + StringComparison.OrdinalIgnoreCase); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Digdir.Library.Dialogporten.WebApiClient/Common/ClaimsPrincipalExtensions.cs(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/IDialogTokenValidator.cs(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs(2 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Services/EdDsaSecurityKeysCacheService.cs(1 hunks)src/Digdir.Tool.Dialogporten.Benchmarks/Digdir.Tool.Dialogporten.Benchmarks.csproj(1 hunks)src/Digdir.Tool.Dialogporten.Benchmarks/Program.cs(1 hunks)src/Digdir.Tool.Dialogporten.Benchmarks/TokenValidatorBenchmarks.cs(1 hunks)tests/Digdir.Library.Dialogporten.WebApiClient.Unit.Tests/DialogTokenValidatorTests.cs(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/Digdir.Tool.Dialogporten.Benchmarks/Digdir.Tool.Dialogporten.Benchmarks.csproj (1)
Learnt from: oskogstad
PR: Altinn/dialogporten#1941
File: src/Digdir.Library.Dialogporten.WebApiClient.WebApiSample/Digdir.Library.Dialogporten.WebApiClient.WebApiSample.csproj:1-3
Timestamp: 2025-02-23T19:09:42.565Z
Learning: In the Dialogporten solution, common build settings like `TargetFramework`, `UserSecretsId`, `Nullable`, and `ImplicitUsings` are defined in `Directory.Build.props` and shared across all projects, making them redundant in individual .csproj files.
🪛 Gitleaks (8.21.2)
src/Digdir.Tool.Dialogporten.Benchmarks/TokenValidatorBenchmarks.cs
18-18: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy apps / Deploy web-api-so to test
- GitHub Check: build / build-and-test
🔇 Additional comments (25)
src/Digdir.Tool.Dialogporten.Benchmarks/Program.cs (1)
3-3: LGTM: Focus shifted to TokenValidatorBenchmarksThe change properly redirects the benchmark execution from the previous token generation focus to the token validation benchmarks, aligning with the PR's objective to enrich the validator API.
src/Digdir.Tool.Dialogporten.Benchmarks/Digdir.Tool.Dialogporten.Benchmarks.csproj (1)
15-15: LGTM: Required project reference addedThe added project reference to the WebApiClient is necessary to support the new TokenValidatorBenchmarks functionality. This correctly integrates the validator components being benchmarked.
src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj (1)
37-37: LGTM: InternalsVisibleTo declaration for benchmarksThis change appropriately exposes the internal members of the WebApiClient to the benchmarking tool, which is necessary for performance testing the token validation components.
src/Digdir.Library.Dialogporten.WebApiClient/Services/EdDsaSecurityKeysCacheService.cs (1)
24-28: LGTM: Enhanced constructor flexibility for DefaultEdDsaSecurityKeysCacheThe added constructors provide better flexibility for initializing the cache:
- The parameterless constructor supports default initialization
- The internal constructor with public keys parameter enables pre-populating the cache
This enhancement is particularly valuable for testing and benchmarking scenarios, making it easier to set up the cache with predefined keys.
src/Digdir.Library.Dialogporten.WebApiClient/IDialogTokenValidator.cs (1)
8-12: Extend usage documentation for new parameters.
Adding optional parameters for dialog ID, required actions, and validation options significantly enhances flexibility. However, ensure that all implementers and consumers of this interface are well-documented on how to use these parameters.src/Digdir.Tool.Dialogporten.Benchmarks/TokenValidatorBenchmarks.cs (3)
30-31: Benchmark approach looks correct.
Using[Benchmark]with a direct call to_sut.Validateis a clean way to measure performance.
33-35: Static helper method is cleanly implemented.
Importing the key withToPublicKeyis straightforward and readable. This is a good pattern for test or benchmark setup.
36-40: Clock abstraction facilitates time-based testing.
TheBenchmarkClockclass is a concise way to provide a reproducible timestamp, aiding test reliability.src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs (8)
18-18: Assign a default or validate null values for_expectedIssuer.
Ensuresettings.BaseUriis never null or empty before concatenation, to avoid unexpected runtime behavior.
27-31: Segment combination logic is well-structured.
Trimming slashes before concatenation ensures a valid combined URI. This helper method is concise and improves readability.
33-39: Optional parameters preserve backward compatibility.
The extended signature with default nulls is a good approach. Verify that all call sites are updated appropriately to avoid mismatched expectations.
55-59: Adding nbf check enhances token validity.
Usingoptions.ValidateLifetimeto control the not-before validation fine-tunes security. Ensure thorough testing of clock skew behavior.
60-64: Expiration validation is correct.
The sameClockSkewapproach forexpextends consistency to all lifetime checks. Confirm edge cases for tokens close to expiry.
65-69: Issuer validation looks robust.
Confirm that the_expectedIssuermatches expected domains and subdomains if partial matches are required.
70-74: Dialog ID check is straightforward.
Checking if the token claims matchdialogIdis clear and aligns with the new parameter usage.
75-78: Action list validation is a welcome addition.
This ensures tokens contain all required actions, further tightening security and clarifying usage.tests/Digdir.Library.Dialogporten.WebApiClient.Unit.Tests/DialogTokenValidatorTests.cs (4)
16-16: Good change: Using DateTimeOffset improves code clarity.Converting from a constant string to a static readonly DateTimeOffset provides better type safety and reduces repetitive parsing throughout the tests.
111-126: Test coverage improved for token validation failures.The test for validating the Not-Before (nbf) claim logic is well implemented, with clear error assertions.
273-317: Good test coverage for additional validation scenarios.These new tests cover important validation scenarios that were previously missing:
- Invalid issuer verification
- Invalid dialog ID verification
- Invalid actions verification
This ensures that the enhanced validation logic in the DialogTokenValidator is working correctly.
319-331: Good practice: Setting ClockSkew to zero for deterministic tests.Setting
DialogTokenValidationParameters.Default.ClockSkew = TimeSpan.Zerois good practice for tests as it eliminates time-based flakiness. The updated constructor parameters also properly support testing the new validation features.src/Digdir.Library.Dialogporten.WebApiClient/Common/ClaimsPrincipalExtensions.cs (5)
8-14: Well-implemented dialog ID verification.The method correctly parses and compares the dialog ID from claims.
16-38: Actions verification logic is robust.The implementation correctly handles edge cases:
- Empty required actions
- Missing action claim
- Case-insensitive comparison
- Duplicate actions in the requirements
Consider adding XML documentation to clarify the expected format of the actions claim.
40-51: Efficient issuer verification with performance optimizations.Good use of:
AsSpan()to avoid string allocationsTrimEnd('/')to normalize URL paths- Custom case-insensitive comparison
This approach provides both correctness and performance when comparing issuer URLs.
53-71: Time-based validation handles clock skew properly.Both
VerifyExpirationTimeandVerifyNotValidBeforemethods:
- Correctly parse Unix timestamps
- Apply appropriate clock skew in opposite directions
- Handle missing claims gracefully with sensible defaults
This is crucial for reliable token validation in distributed systems.
73-78: Well-designed helper method with nullable annotation.The
TryGetClaimValuemethod uses the[NotNullWhen(true)]attribute which enhances static analysis for null-reference checks, making it safer to use.
🤖 I have created a release *beep* *boop* --- ## [1.55.5](v1.55.4...v1.55.5) (2025-02-26) ### Bug Fixes * **webapi:** Clarify use of FCEs in content values ([#1959](#1959)) ([584a76c](584a76c)) ### Miscellaneous Chores * **deps:** update dotnet monorepo ([#1961](#1961)) ([e3f12d5](e3f12d5)) * **deps:** update grafana/grafana docker tag to v10.4.16 ([#1962](#1962)) ([4f93f67](4f93f67)) * **deps:** update otel/opentelemetry-collector-contrib docker tag to v0.119.0 ([#1963](#1963)) ([0fca9a5](0fca9a5)) * enrich validator api ([#1958](#1958)) ([739380a](739380a)) * **performance:** Fix serviceowner orgno ([#1965](#1965)) ([c2de39d](c2de39d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Validatoren kan etter denne pren:
I tillegg vil validatoren returnere en ClaimsPrinciple som representerer tokenet sammen med en liste av potensielle feil med tokenet. Tror dette skal være dekkende for 99,9% av alle use-case tjenesteeiere måtte ha for dialog token validering. Det er i hvert fall dekkende for adapterens token validering.