feat: dialogtoken validator return claims #1931
Conversation
📝 WalkthroughWalkthroughThe changes update the Dialogporten WebApiClient by modifying the dialog management and token validation components. The API now documents a new HTTP 410 status for removed dialogs and adds optional non-sensitive properties to several dialog content classes. In token validation, the interfaces and classes have been enhanced with a new Claims property and updated annotations to reflect non-null claims when valid. Additionally, the token decoding logic now uses a new JsonTokenParts structure, and corresponding tests assert the presence of claims for valid tokens. Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs (3)
51-65: Handle missing or malformed properties gracefully.
Usingbody.GetProperty(...)may throw if the properties are missing or malformed in the token. For robustness, consider usingTryGetProperty(...)and validating any conversions (e.g., the expected numeric fields).- new(DialogTokenClaimTypes.C, body.GetProperty(DialogTokenClaimTypes.C).GetString() ?? string.Empty), + if (body.TryGetProperty(DialogTokenClaimTypes.C, out var cProp) && cProp.ValueKind == JsonValueKind.String) + { + claims.Add(new Claim(DialogTokenClaimTypes.C, cProp.GetString() ?? string.Empty)); + } + else + { + // Optionally, add validation error or default claim + }
186-195: Expiration checks are straightforward, but consider additional grace handling.
This is correct for basic expiration checks. Some systems might require clock skew allowance. If needed, you could incorporate a small margin for differences in server time.
277-289: Claim type constants are self-documenting.
These short property names map well to your token’s structure. No concerns here, though more descriptive naming might assist unfamiliar developers.tests/Digdir.Library.Dialogporten.WebApiClient.Unit.Tests/DialogTokenValidatorTests.cs (2)
36-37: Enhance claims validation assertions.While the new assertion verifies that claims are present, consider these improvements:
- Use
Assert.NotNull(result.Claims)for more specific assertion- Add assertions to verify the expected claims from the token (e.g., 'jti', 'c', 'l', etc.)
Assert.True(result.IsValid); -Assert.True(result.Claims != null); +Assert.NotNull(result.Claims); +Assert.Contains(result.Claims.Claims, c => c.Type == "jti" && c.Value == "38cfdcb9-388b-47b8-a1bf-9f15b2819894"); +Assert.Contains(result.Claims.Claims, c => c.Type == "c" && c.Value == "urn:altinn:person:identifier-no:14886498226"); +Assert.Contains(result.Claims.Claims, c => c.Type == "l" && c.Value == "3");
52-226: Add Claims assertions to invalid token scenarios.For completeness, consider adding assertions to verify that
Claimsis null when the token is invalid. This aligns with the[MemberNotNullWhen(true, nameof(Claims))]attribute contract.Add the following assertion to each test method that expects invalid tokens:
Assert.False(result.IsValid); Assert.True(result.Errors.ContainsKey("token")); Assert.Contains("<error_message>", result.Errors["token"]); +Assert.Null(result.Claims);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Digdir.Library.Dialogporten.WebApiClient/Features/V1/RefitterInterface.cs(6 hunks)src/Digdir.Library.Dialogporten.WebApiClient/IDialogTokenValidator.cs(2 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Services/DefaultValidationResult.cs(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs(6 hunks)tests/Digdir.Library.Dialogporten.WebApiClient.Unit.Tests/DialogTokenValidatorTests.cs(1 hunks)
🔇 Additional comments (15)
src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs (7)
5-6: Imports look appropriate.
No issues found with addingSystem.GlobalizationandSystem.Security.Claimsreferences.
46-49: Early exit on invalid state.
Returning immediately if the validation result is already marked invalid helps avoid unnecessary processing. This is a good guard clause.
69-92: Decoding token into JSON is well-structured.
TheTryDecodeTokenapproach and usage ofJsonTokenPartsneatly separate concerns for decoding and parsing. Good job returning early if any step fails.
93-100: JSON parsing and exception handling look fine.
Catching theJsonExceptionand returning false is a clean way to handle malformed JSON.
157-159: Extended signature-verification contract.
AdjustingVerifySignatureto acceptJsonTokenPartsfor header/body access is consistent with the new structure. Implementation remains straightforward.
221-232: Kid property retrieval with[NotNullWhen(true)].
Leverages modern C# nullability features well. The property-based approach is clear, though you might consider adding explicit error context if thekidis missing or unrecognized.
259-275: Ref struct usage for JSON parsing is efficient.
The immutableJsonTokenPartsstructure cleanly bundles the header, body, and signature. This is a good design for performance and clarity.src/Digdir.Library.Dialogporten.WebApiClient/IDialogTokenValidator.cs (2)
1-3: New imports align with claims-based logic.
References toSystem.Diagnostics.CodeAnalysisandSystem.Security.Claimsare consistent with the updated token validation approach.
14-15:IsValidandClaimsrelationship is well defined.
Using[MemberNotNullWhen(true, nameof(Claims))]clarifies thatClaimsis guaranteed to be non-null when the validation has succeeded. This is a nice usage of compiler annotations.src/Digdir.Library.Dialogporten.WebApiClient/Services/DefaultValidationResult.cs (2)
1-2: Imports align with the new claims property.
Pulling inSystem.Security.ClaimsandSystem.Diagnostics.CodeAnalysisfor the annotation is consistent.
9-10: Implementation ofIsValidandClaims.
The addition of[MemberNotNullWhen(true, nameof(Claims))]and aClaimsPrincipal?property ensures consistency with the contract inIValidationResult, simplifying usage in the validator logic.src/Digdir.Library.Dialogporten.WebApiClient/Features/V1/RefitterInterface.cs (4)
541-543: LGTM! Documentation for HTTP 410 status code is clear and consistent.The added documentation for HTTP 410 status code properly indicates that the entity with the given key(s) is removed, which aligns with REST API best practices for handling deleted resources.
1729-1751: LGTM! Well-designed security enhancement for handling sensitive content.The addition of
NonSensitiveTitleandNonSensitiveSummaryproperties provides a robust mechanism for displaying appropriate content based on the user's eIDAS authentication level. This is a good security practice that helps prevent accidental exposure of sensitive information.
2600-2622: LGTM! Consistent implementation of non-sensitive content properties.The implementation of
NonSensitiveTitleandNonSensitiveSummaryin the search content model maintains consistency with the update content model, ensuring a uniform approach to handling sensitive content across different operations.
2910-2932: LGTM! Completes the consistent implementation of non-sensitive content properties.The implementation of
NonSensitiveTitleandNonSensitiveSummaryin the get content model maintains consistency with other content models, ensuring a uniform approach to handling sensitive content across all dialog operations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs (5)
95-106: Add logging for JSON parsing failures.Silent exception handling might mask specific issues during JSON parsing. Consider adding logging to help with debugging.
+ private readonly ILogger<DialogTokenValidator> _logger; + + public DialogTokenValidator( + IEdDsaSecurityKeysCache publicKeysCache, + IClock clock, + ILogger<DialogTokenValidator> logger) + { + _publicKeysCache = publicKeysCache; + _clock = clock; + _logger = logger; + } private static bool TryParseJson(ReadOnlySpan<byte> span, out JsonElement jsonElement) { jsonElement = default; var reader = new Utf8JsonReader(span); try { jsonElement = JsonElement.ParseValue(ref reader); return true; } - catch (JsonException) { } + catch (JsonException ex) + { + _logger.LogDebug(ex, "Failed to parse JSON token part"); + } return false; }
158-185: Improve error handling and logging in signature verification.Consider these improvements:
- Move the large error message to a constant
- Add logging for signature verification failures
+ private const string NO_PUBLIC_KEYS_ERROR = + "No public keys available. Most likely due to an error when fetching the " + + "public keys from the dialogporten well-known endpoint. Please check the " + + "logs for more information. Alternatively, set " + + "DialogportenSettings.ThrowOnPublicKeyFetchInit=true to ensure public " + + "keys are fetched before starting up the application."; private bool VerifySignature( JwksTokenParts<char> tokenParts, JsonTokenParts decodedTokenParts) { var publicKeys = _publicKeysCache.PublicKeys; if (publicKeys.Count == 0) { - throw new InvalidOperationException( - "No public keys available. Most likely due to an error when fetching the " + - "public keys from the dialogporten well-known endpoint. Please check the " + - "logs for more information. Alternatively, set " + - "DialogportenSettings.ThrowOnPublicKeyFetchInit=true to ensure public " + - "keys are fetched before starting up the application."); + throw new InvalidOperationException(NO_PUBLIC_KEYS_ERROR); } // ... existing code ... - return TryGetPublicKey(publicKeys, decodedTokenParts.Header, out var publicKey) - && SignatureAlgorithm.Ed25519.Verify(publicKey, signedPart, decodedTokenParts.Signature); + if (!TryGetPublicKey(publicKeys, decodedTokenParts.Header, out var publicKey)) + { + _logger.LogWarning("Failed to find matching public key for token"); + return false; + } + + var isValid = SignatureAlgorithm.Ed25519.Verify( + publicKey, signedPart, decodedTokenParts.Signature); + + if (!isValid) + { + _logger.LogWarning("Token signature verification failed"); + } + + return isValid;
187-202: Use constant from DialogTokenClaimTypes for consistency.Replace the magic string "exp" with the constant from
DialogTokenClaimTypesfor better maintainability.private bool VerifyExpiration(JsonTokenParts decodedTokenParts) { - const string expiresPropertyName = "exp"; - if (!decodedTokenParts.Body.TryGetProperty(expiresPropertyName, out var expiresElement)) + if (!decodedTokenParts.Body.TryGetProperty(DialogTokenClaimTypes.Expire, out var expiresElement)) { return false; }
222-239: Improve public key lookup implementation.Consider these improvements:
- Move the magic string "kid" to a constant
- Use LINQ for more concise key lookup
+ private const string KID_PROPERTY_NAME = "kid"; + private static bool TryGetPublicKey( ReadOnlyCollection<PublicKeyPair> keyPairs, JsonElement header, [NotNullWhen(true)] out PublicKey? publicKey) { - const string kidPropertyName = "kid"; publicKey = null; - if (!header.TryGetProperty(kidPropertyName, out var tokenKid)) + if (!header.TryGetProperty(KID_PROPERTY_NAME, out var tokenKid)) { return false; } - foreach (var (kid, key) in keyPairs) - { - if (!kid.AsSpan().SequenceEqual(tokenKid.GetString())) continue; - publicKey = key; - return true; - } + var tokenKidString = tokenKid.GetString(); + var keyPair = keyPairs.FirstOrDefault( + pair => pair.Kid.AsSpan().SequenceEqual(tokenKidString)); + + publicKey = keyPair?.Key; + return publicKey != null;
278-290: Add XML documentation for better IntelliSense support.Consider adding XML documentation to describe each claim type's purpose and format.
internal static class DialogTokenClaimTypes { + /// <summary> + /// The 'c' claim represents the client identifier. + /// </summary> public const string C = "c"; + /// <summary> + /// The 'l' claim represents the security level. + /// </summary> public const string Level = "l"; // Add similar documentation for other claims
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- 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
- GitHub Check: build / build-and-test
🔇 Additional comments (2)
src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs (2)
108-156: Implementation looks good!The token parts handling implementation is robust with proper boundary checks, clear error handling, and efficient buffer usage.
241-275: Well-designed data structures!The implementation of both
JwksTokenParts<T>andJsonTokenPartsstructs is efficient and well-designed:
- Use of readonly ref structs for performance
- Clear property definitions
- Proper constructor implementation
| var body = decodedTokenParts.Body; | ||
| validationResult.Claims = new ClaimsPrincipal( | ||
| new ClaimsIdentity( | ||
| new List<Claim> | ||
| { | ||
| new(DialogTokenClaimTypes.C, body.GetProperty(DialogTokenClaimTypes.C).GetString() ?? string.Empty), | ||
| new(DialogTokenClaimTypes.Level, body.GetProperty(DialogTokenClaimTypes.Level).GetInt64().ToString(CultureInfo.InvariantCulture)), | ||
| new(DialogTokenClaimTypes.Party, body.GetProperty(DialogTokenClaimTypes.Party).GetString() ?? string.Empty), | ||
| new(DialogTokenClaimTypes.Scope, body.GetProperty(DialogTokenClaimTypes.Scope).GetString() ?? string.Empty), | ||
| new(DialogTokenClaimTypes.DialogId, body.GetProperty(DialogTokenClaimTypes.DialogId).GetString() ?? string.Empty), | ||
| new(DialogTokenClaimTypes.A, body.GetProperty(DialogTokenClaimTypes.A).GetString() ?? string.Empty), | ||
| new(DialogTokenClaimTypes.Issuer, body.GetProperty(DialogTokenClaimTypes.Issuer).GetString() ?? string.Empty), | ||
| new(DialogTokenClaimTypes.Expire, body.GetProperty(DialogTokenClaimTypes.Expire).GetInt64().ToString(CultureInfo.InvariantCulture)), | ||
| new(DialogTokenClaimTypes.IssuedAt, body.GetProperty(DialogTokenClaimTypes.IssuedAt).GetInt64().ToString(CultureInfo.InvariantCulture)), | ||
| new(DialogTokenClaimTypes.NotVisibleBefore, body.GetProperty(DialogTokenClaimTypes.NotVisibleBefore).GetInt64().ToString(CultureInfo.InvariantCulture)) | ||
| }, "DialogToken")); |
There was a problem hiding this comment.
Add null checks and validation for required claims.
The current implementation has potential issues:
- Missing null checks for required JSON properties could lead to runtime exceptions
- Using empty string fallback for missing claims might mask validation issues
- No validation of claim values
Consider this safer implementation:
var body = decodedTokenParts.Body;
+ // Validate required properties exist
+ var requiredProperties = new[]
+ {
+ DialogTokenClaimTypes.C,
+ DialogTokenClaimTypes.Level,
+ DialogTokenClaimTypes.Party,
+ // Add other required properties
+ };
+
+ foreach (var prop in requiredProperties)
+ {
+ if (!body.TryGetProperty(prop, out var _))
+ {
+ validationResult.AddError("token", $"Missing required claim: {prop}");
+ return validationResult;
+ }
+ }
+
validationResult.Claims = new ClaimsPrincipal(
new ClaimsIdentity(
new List<Claim>
{
- new(DialogTokenClaimTypes.C, body.GetProperty(DialogTokenClaimTypes.C).GetString() ?? string.Empty),
+ new(DialogTokenClaimTypes.C, body.GetProperty(DialogTokenClaimTypes.C).GetString()
+ ?? throw new JsonException($"Required claim {DialogTokenClaimTypes.C} is null")),
// Apply similar changes to other required claims
|
Dupe of #1933 ? |
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)