Allow overriding the maximum length when parsing refresh tokens#350
Allow overriding the maximum length when parsing refresh tokens#350
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a way to override the maximum allowed refresh token length at runtime (to support providers like ADFS that can emit refresh tokens > 4KB) while keeping the existing RefreshToken.MaxLength constant unchanged.
Changes:
- Added
RefreshToken.SetMaxLength(int)to allow adjusting the validation limit used byParse/TryParse. - Added unit tests covering
RefreshTokenparsing behavior and the new max-length override behavior. - Updated the public API verification snapshot to include the new method.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| access-token-management/src/AccessTokenManagement/RefreshToken.cs | Adds SetMaxLength to mutate the max-length validation behavior for refresh tokens. |
| access-token-management/test/AccessTokenManagement.Tests/Types/RefreshTokenTests.cs | Adds baseline parsing/validation tests for RefreshToken. |
| access-token-management/test/AccessTokenManagement.Tests/Types/RefreshTokenSetMaxLengthTests.cs | Adds tests for SetMaxLength (increase/decrease/invalid values) and resets static state after each test. |
| access-token-management/test/AccessTokenManagement.Tests/PublicApiVerificationTests.VerifyPublicApi.verified.txt | Updates API snapshot to include RefreshToken.SetMaxLength(int). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static void SetMaxLength(int maxLength) | ||
| { | ||
| ArgumentOutOfRangeException.ThrowIfNegativeOrZero(maxLength); | ||
|
|
||
| Validators[0] = ValidationRules.MaxLength(maxLength); | ||
| } |
There was a problem hiding this comment.
SetMaxLength updates Validators[0], which couples the API to the current ordering/shape of the Validators array. If additional validation rules are added later (or the order changes), this method could silently stop updating the max-length rule. Consider refactoring so the max-length check is implemented via a dedicated validator method that reads a static int (e.g., Volatile.Read/Write) or by rebuilding the validators array in a way that doesn't rely on a hard-coded index.
| /// <summary> | ||
| /// Both the <see cref="RefreshTokenTests"/> and <see cref="RefreshTokenSetMaxLengthTests"/> classes share a collection to prevent parallel execution, | ||
| /// since <see cref="RefreshToken.SetMaxLength"/> mutates static state. | ||
| /// </summary> | ||
| [Collection(nameof(RefreshTokenTests))] | ||
| public class RefreshTokenTests |
There was a problem hiding this comment.
The XML doc says these tests are placed in a collection to “prevent parallel execution”, but [Collection(...)] only serializes tests within the same collection; the collection can still run in parallel with other collections. If the goal is to avoid static-state interference, add a [CollectionDefinition(..., DisableParallelization = true)] for this collection (or otherwise disable parallelization for refresh-token tests) so no other test collection can run concurrently while RefreshToken.SetMaxLength mutates global state.
| /// <summary> | ||
| /// Tests for <see cref="RefreshToken.SetMaxLength"/>. These are isolated into a separate | ||
| /// non-parallel collection because <see cref="RefreshToken.SetMaxLength"/> mutates static state | ||
| /// that would affect other tests running concurrently. | ||
| /// </summary> | ||
| [Collection(nameof(RefreshTokenTests))] | ||
| public class RefreshTokenSetMaxLengthTests : IDisposable |
There was a problem hiding this comment.
Same as above: [Collection(nameof(RefreshTokenTests))] does not make the collection itself non-parallel vs other collections. If these tests rely on global static state being isolated, introduce a CollectionDefinition with DisableParallelization = true (or an equivalent mechanism) instead of only documenting it.
What issue does this PR address?
As reported in https://github.com/orgs/DuendeSoftware/discussions/514, ADFS can issue refresh tokens exceeding the current maximum length of 4K.
Since there is no existing documentation around the maximum length of refresh tokens in ADFS, this PR allows one to set their own new maximum length using
RefreshToken.SetMaxLength(8 * 1024);for example, rather than arbitrarily selecting a new default which may again be too limiting in the future.The existing
public const int MaxLength = 4 * 1024;was kept, so this PR does not introduce a breaking change.