chore: Return claimsprinciple on dialogtoken validate #1933
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThis pull request introduces a new utility class that provides extension methods for validating JSON and extracting a Changes
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
|
Fargekritt
approved these changes
Feb 21, 2025
![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 (6)
src/Digdir.Library.Dialogporten.WebApiClient/Common/ByteSpanExtensions.cs (3)
10-21: Consider logging JSON parsing failures.The method silently swallows all JSON exceptions. Consider logging the specific error for debugging purposes.
public static bool IsValidJson(this ReadOnlySpan<byte> span) { var reader = new Utf8JsonReader(span); try { while (reader.Read()) { } return true; } - catch (JsonException) { } + catch (JsonException ex) + { + // TODO: Add logging here + return false; + } - return false; }
48-62: Handle array of primitive values in claims parsing.The current implementation only handles arrays and objects by converting them to raw JSON strings. Consider handling arrays of primitive values as separate claims.
var claimValue = reader.TokenType switch { JsonTokenType.String => reader.GetString(), JsonTokenType.Number => reader.TryGetInt64(out var longValue) ? longValue.ToString(CultureInfo.InvariantCulture) : reader.GetDouble().ToString(CultureInfo.InvariantCulture), JsonTokenType.True => "true", JsonTokenType.False => "false", - JsonTokenType.StartArray => ParseObject(ref reader), + JsonTokenType.StartArray => HandleArray(ref reader, propertyName, claims), JsonTokenType.StartObject => ParseObject(ref reader), _ => null }; - if (claimValue is null) continue; - claims.Add(new Claim(propertyName, claimValue)); + if (claimValue is not null) + { + claims.Add(new Claim(propertyName, claimValue)); + }Additional method to handle arrays:
private static string? HandleArray(ref Utf8JsonReader reader, string propertyName, List<Claim> claims) { using var doc = JsonDocument.ParseValue(ref reader); var array = doc.RootElement; if (array.ValueKind != JsonValueKind.Array) return null; // If array contains primitives, add each as a separate claim if (array.EnumerateArray().All(e => e.ValueKind is JsonValueKind.String or JsonValueKind.Number or JsonValueKind.True or JsonValueKind.False)) { foreach (var element in array.EnumerateArray()) { claims.Add(new Claim(propertyName, element.ToString())); } return null; } // For complex arrays, return as JSON string return array.GetRawText(); }
23-35: Improve error handling specificity in TryGetClaimsPrincipal.The method catches all JsonExceptions without distinguishing between different error cases. Consider handling specific exceptions for better error reporting.
public static bool TryGetClaimsPrincipal(this ReadOnlySpan<byte> body, [NotNullWhen(true)] out ClaimsPrincipal? claimsPrincipal) { claimsPrincipal = null; try { claimsPrincipal = body.GetClaimsPrincipal(); return true; } - catch (JsonException) { /* swallow exception by design */ } + catch (JsonException ex) when (ex is JsonReaderException or JsonParsingException) + { + // TODO: Add logging for invalid JSON format + return false; + } + catch (JsonException) + { + // TODO: Add logging for other JSON-related errors + return false; + } return false; }src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs (1)
50-68: Add XML documentation for the modified TryDecodeToken method.The method has been updated to handle claims, but lacks documentation explaining the new functionality and parameters.
+ /// <summary> + /// Attempts to decode a token into its constituent parts and extract claims. + /// </summary> + /// <param name="token">The token to decode.</param> + /// <param name="tokenDecodeBuffer">Buffer for decoding operations.</param> + /// <param name="tokenParts">The decoded token parts in char format.</param> + /// <param name="decodedTokenParts">The decoded token parts in byte format.</param> + /// <param name="claimsPrincipal">When successful, contains the extracted claims principal.</param> + /// <returns>True if decoding and claims extraction succeeded; otherwise, false.</returns> private static bool TryDecodeToken( ReadOnlySpan<char> token, Span<byte> tokenDecodeBuffer, out JwksTokenParts<char> tokenParts, out JwksTokenParts<byte> decodedTokenParts, [NotNullWhen(true)] out ClaimsPrincipal? claimsPrincipal)tests/Digdir.Library.Dialogporten.WebApiClient.Unit.Tests/DialogTokenValidatorTests.cs (2)
227-252: LGTM! Consider improving test maintainability.The test thoroughly verifies all claims from the token. However, the test could be more maintainable by extracting the expected values into constants or a test data object.
Consider refactoring the test to use a test data object:
+ private static readonly TokenTestData ValidTokenData = new( + Jti: "38cfdcb9-388b-47b8-a1bf-9f15b2819894", + ConsumerId: "urn:altinn:person:identifier-no:14886498226", + Level: "3", + PartyId: "urn:altinn:person:identifier-no:14886498226", + Scope: "urn:altinn:resource:dagl-correspondence", + Id: "0194fe82-9280-77a5-a7cd-5ff0e6a6fa07", + Action: "read", + Issuer: "https://platform.tt02.altinn.no/dialogporten/api/v1", + IssuedAt: "1739523367", + NotBefore: "1739523367", + ExpiresAt: "1739523967" + ); + + private record TokenTestData( + string Jti, string ConsumerId, string Level, string PartyId, + string Scope, string Id, string Action, string Issuer, + string IssuedAt, string NotBefore, string ExpiresAt); [Fact] public void ShouldContainClaims_GivenValidTokenWithClaims() { // Arrange var sut = GetSut( DateTimeOffset.Parse(ValidTimeStampString, CultureInfo.InvariantCulture), ValidPublicKeyPairs); // Act var result = sut.Validate(DialogToken); // Assert Assert.True(result.IsValid); Assert.NotNull(result.ClaimsPrincipal); - Assert.Equal("38cfdcb9-388b-47b8-a1bf-9f15b2819894", result.ClaimsPrincipal.FindFirst("jti")!.Value); - Assert.Equal("urn:altinn:person:identifier-no:14886498226", result.ClaimsPrincipal.FindFirst("c")!.Value); - Assert.Equal("3", result.ClaimsPrincipal.FindFirst("l")!.Value); - Assert.Equal("urn:altinn:person:identifier-no:14886498226", result.ClaimsPrincipal.FindFirst("p")!.Value); - Assert.Equal("urn:altinn:resource:dagl-correspondence", result.ClaimsPrincipal.FindFirst("s")!.Value); - Assert.Equal("0194fe82-9280-77a5-a7cd-5ff0e6a6fa07", result.ClaimsPrincipal.FindFirst("i")!.Value); - Assert.Equal("read", result.ClaimsPrincipal.FindFirst("a")!.Value); - Assert.Equal("https://platform.tt02.altinn.no/dialogporten/api/v1", result.ClaimsPrincipal.FindFirst("iss")!.Value); - Assert.Equal("1739523367", result.ClaimsPrincipal.FindFirst("iat")!.Value); - Assert.Equal("1739523367", result.ClaimsPrincipal.FindFirst("nbf")!.Value); - Assert.Equal("1739523967", result.ClaimsPrincipal.FindFirst("exp")!.Value); + Assert.Equal(ValidTokenData.Jti, result.ClaimsPrincipal.FindFirst("jti")!.Value); + Assert.Equal(ValidTokenData.ConsumerId, result.ClaimsPrincipal.FindFirst("c")!.Value); + Assert.Equal(ValidTokenData.Level, result.ClaimsPrincipal.FindFirst("l")!.Value); + Assert.Equal(ValidTokenData.PartyId, result.ClaimsPrincipal.FindFirst("p")!.Value); + Assert.Equal(ValidTokenData.Scope, result.ClaimsPrincipal.FindFirst("s")!.Value); + Assert.Equal(ValidTokenData.Id, result.ClaimsPrincipal.FindFirst("i")!.Value); + Assert.Equal(ValidTokenData.Action, result.ClaimsPrincipal.FindFirst("a")!.Value); + Assert.Equal(ValidTokenData.Issuer, result.ClaimsPrincipal.FindFirst("iss")!.Value); + Assert.Equal(ValidTokenData.IssuedAt, result.ClaimsPrincipal.FindFirst("iat")!.Value); + Assert.Equal(ValidTokenData.NotBefore, result.ClaimsPrincipal.FindFirst("nbf")!.Value); + Assert.Equal(ValidTokenData.ExpiresAt, result.ClaimsPrincipal.FindFirst("exp")!.Value); }
254-276: Consider reusing existing test helper method.The test duplicates the invalid JSON body setup from
ShouldReturnError_GivenMalformedJsonBody. Consider extracting the common setup into a helper method.+ private static string GetTokenWithInvalidBody() + { + var invalidBody = """ + { + "jti": "38cfdcb9-388b-47b8-a1bf-9f15b2819894", + "c": "urn:altinn:person:identifier-no:14886498226", + """u8; + var tokenParts = DialogToken.Split('.'); + tokenParts[1] = Base64Url.EncodeToString(invalidBody); + return string.Join(".", tokenParts); + } [Fact] public void ClaimsShouldBeNull_GivenInvalidTokenFormat() { - var invalidBody = """ - { - "jti": "38cfdcb9-388b-47b8-a1bf-9f15b2819894", - "c": "urn:altinn:person:identifier-no:14886498226", - """u8; // Arrange var sut = GetSut( DateTimeOffset.Parse(ValidTimeStampString, CultureInfo.InvariantCulture), ValidPublicKeyPairs); - var tokenParts = DialogToken.Split('.'); - tokenParts[1] = Base64Url.EncodeToString(invalidBody); - var token = string.Join(".", tokenParts); + var token = GetTokenWithInvalidBody(); // Act var result = sut.Validate(token); // Assert Assert.False(result.IsValid); Assert.Null(result.ClaimsPrincipal); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Digdir.Library.Dialogporten.WebApiClient/Common/ByteSpanExtensions.cs(1 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(3 hunks)tests/Digdir.Library.Dialogporten.WebApiClient.Unit.Tests/DialogTokenValidatorTests.cs(1 hunks)
🔇 Additional comments (3)
src/Digdir.Library.Dialogporten.WebApiClient/IDialogTokenValidator.cs (1)
13-16: LGTM! Well-structured interface changes.The addition of the
ClaimsPrincipalproperty with proper nullability attributes enhances type safety and provides a clear contract for implementations.src/Digdir.Library.Dialogporten.WebApiClient/Services/DefaultValidationResult.cs (1)
9-11: LGTM! Clean implementation of IValidationResult.The implementation properly handles nullability and encapsulation with the internal setter.
src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenValidator.cs (1)
30-36:❓ Verification inconclusive
Consider validating minimum required claims.
The code accepts any valid JSON as claims without verifying the presence of essential claims. Consider validating that required claims are present.
🏁 Script executed:
#!/bin/bash # Search for any existing claim validation patterns in the codebase ast-grep --pattern 'if (!TryGetPropertyValue($body, $name, out var $value))'Length of output: 78
Subject: Validate Presence of Essential Claims in Token
The current implementation in
DialogTokenValidator.cs(lines 30-36) decodes the token and assigns the resultingClaimsPrincipalwithout checking whether required claims are present. Our search for a claims validation pattern (using the providedast-greppattern) did not yield any evidence of such validation elsewhere in the codebase, which raises concerns that tokens with incomplete or insufficient claims might be considered valid.Action Items:
- Review the token validation logic: Ensure that after decoding, the
ClaimsPrincipalis checked for the presence of critical claims (e.g., issuer, audience, subject, etc.).- Centralize or document validations: If claim validations are handled in another part of the codebase, make sure this module explicitly references that logic or provide clear documentation on the validation flow.
- Enhance security: Implement checks to guarantee that tokens missing any essential claims are rejected to prevent potential security issues.
MagnusSandgren
deleted the
chore/return-claimsprinciple-on-dialogtoken-validate
branch
4 tasks
oskogstad
pushed a commit
that referenced
this pull request
Feb 22, 2025
🤖 I have created a release *beep* *boop* --- ## [1.55.2](v1.55.1...v1.55.2) (2025-02-22) ### Miscellaneous Chores * Add docblocks to parties DTO ([#1934](#1934)) ([c9e10db](c9e10db)) * Return claimsprinciple on dialogtoken validate ([#1933](#1933)) ([2814365](2814365)) * Update swagger summaries, add scopes, global description ([#1929](#1929)) ([d91eacb](d91eacb)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This was referenced Feb 26, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.