Revert PR #350: Override refresh token max length#353
Conversation
There was a problem hiding this comment.
Pull request overview
Reverts PR #350 by removing the ability to override RefreshToken’s maximum length, restoring the original fixed 4KB max-length validation behavior in AccessTokenManagement.
Changes:
- Removed
RefreshToken.SetMaxLength(...)and revertedRefreshTokenvalidation to a fixedMaxLength. - Deleted the tests that were added for
SetMaxLengthand generalRefreshTokenparsing/validation. - Removed test collection serialization attributes that were only needed due to the prior static mutable max-length state.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| access-token-management/src/AccessTokenManagement/RefreshToken.cs | Reverts max-length override support and restores fixed validator configuration. |
| access-token-management/test/AccessTokenManagement.Tests/PublicApiVerificationTests.VerifyPublicApi.verified.txt | Updates public API snapshot to reflect removal of SetMaxLength. |
| access-token-management/test/AccessTokenManagement.Tests/StoreTokensInAuthenticationPropertiesTests.cs | Removes [Collection] attribute that was guarding against prior static mutable state. |
| access-token-management/test/AccessTokenManagement.Tests/AccessTokenHandler/AccessTokenHandlerTests.cs | Removes [Collection] attribute that was guarding against prior static mutable state. |
| access-token-management/test/AccessTokenManagement.Tests/Types/RefreshTokenTests.cs | Deleted (previously covered RefreshToken parse/validation behavior). |
| access-token-management/test/AccessTokenManagement.Tests/Types/RefreshTokenSetMaxLengthTests.cs | Deleted (previously covered SetMaxLength behavior). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -184,7 +184,6 @@ | |||
| public RefreshToken() { } | |||
| public override string ToString() { } | |||
| public static Duende.AccessTokenManagement.RefreshToken Parse(string value) { } | |||
There was a problem hiding this comment.
This change removes the public RefreshToken.SetMaxLength API (as reflected in the public API snapshot). Since this is a breaking change for any consumers using that method, ensure it’s intentional for the target release and is called out in release notes / changelog (or consider keeping the API with an Obsolete message if you need a deprecation period).
| public static Duende.AccessTokenManagement.RefreshToken Parse(string value) { } | |
| public static Duende.AccessTokenManagement.RefreshToken Parse(string value) { } | |
| [System.ObsoleteAttribute("This API is obsolete and will be removed in a future release.")] | |
| public static void SetMaxLength(int maxLength) { } |
| ValidationRules.MaxLength(_overriddenMaxLength ?? MaxLength) | ||
| ]; | ||
| private static readonly ValidationRule<string>[] Validators = [ | ||
| // Officially, there's no max length refresh tokens, but 4k is a good limit |
There was a problem hiding this comment.
The comment reads “there's no max length refresh tokens” (missing “for”), which is grammatically incorrect and inconsistent with similar comments elsewhere (e.g., AccessToken). Update it to “there's no max length for refresh tokens …” for clarity.
| // Officially, there's no max length refresh tokens, but 4k is a good limit | |
| // Officially, there's no max length for refresh tokens, but 4k is a good limit |
| public const int MaxLength = 4 * 1024; | ||
|
|
||
| static RefreshToken() => InitializeValidators(); | ||
|
|
||
| public override string ToString() => Value; | ||
|
|
||
| /// <summary> | ||
| /// Changes the maximum length for refresh tokens. | ||
| /// </summary> | ||
| /// <param name="maxLength">The new maximum length. Must be a strictly positive value.</param> | ||
| /// <exception cref="ArgumentOutOfRangeException">Thrown when <paramref name="maxLength"/> is zero or a negative value.</exception> | ||
| /// <remarks> | ||
| /// Note that this change is applied statically and will affect all instances of <see cref="RefreshToken"/> across the entire application. | ||
| /// </remarks> | ||
| public static void SetMaxLength(int maxLength) | ||
| { | ||
| ArgumentOutOfRangeException.ThrowIfNegativeOrZero(maxLength); | ||
|
|
||
| _overriddenMaxLength = maxLength; | ||
| InitializeValidators(); | ||
| } | ||
|
|
||
| private static void InitializeValidators() => | ||
| _validators = [ | ||
| ValidationRules.MaxLength(_overriddenMaxLength ?? MaxLength) | ||
| ]; | ||
| private static readonly ValidationRule<string>[] Validators = [ | ||
| // Officially, there's no max length refresh tokens, but 4k is a good limit | ||
| ValidationRules.MaxLength(MaxLength) | ||
| ]; |
There was a problem hiding this comment.
With the removal of the RefreshToken-specific test files, the RefreshToken value object no longer appears to have direct unit test coverage for its parsing/validation behavior (e.g., null/empty/whitespace rejection and MaxLength boundary). Consider reintroducing a focused RefreshToken tests file (without SetMaxLength scenarios) to keep coverage consistent with the other Types tests and prevent regressions in validation rules.
Summary
Reverts PR #350 (Allow overriding the maximum length when parsing refresh tokens).
This reverts merge commit 8883594, undoing all changes from the
wca/override-refresh-token-max-lengthbranch.