Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression testing: Token type, replay, and lifetime validation. #2823

Closed
wants to merge 1 commit into from

Conversation

iNinja
Copy link
Contributor

@iNinja iNinja commented Sep 11, 2024

Regression testing: Token type, replay, and lifetime validation.

  • Updated theory, added new test cases.

Part of #2711

@iNinja iNinja requested a review from a team as a code owner September 11, 2024 15:22
{
private bool _onAddReturnValue;
private bool _onFindReturnValue;
public TestTokenReplayCache(bool onAddReturnValue, bool onFindReturnValue)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public TestTokenReplayCache(bool onAddReturnValue, bool onFindReturnValue)
public TestTokenReplayCache(bool onAddReturnValue, bool onFindReturnValue)

_onAddReturnValue = onAddReturnValue;
_onFindReturnValue = onFindReturnValue;
}
public bool TryAdd(string securityToken, DateTime expirationTime)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public bool TryAdd(string securityToken, DateTime expirationTime)
public bool TryAdd(string securityToken, DateTime expirationTime)

Copy link
Contributor

@keegan-caruso keegan-caruso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few spacing issues, otherwise LGTM

@@ -13,6 +13,8 @@
using Microsoft.IdentityModel.Tokens;
using Xunit;

#nullable enable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be consistent with spacing around #nullable.
Other files do not have a blank line.

@@ -102,6 +109,11 @@ public static TheoryData<JsonWebTokenHandlerValidationParametersTheoryData> Json
ValidationParameters = CreateValidationParameters(
Default.Issuer, [Default.Audience], Default.AsymmetricSigningKey),
},
new JsonWebTokenHandlerValidationParametersTheoryData("Invalid_ValidationParametersNull")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a new line between creation of theoryData.

@@ -23,6 +25,7 @@ public async Task ValidateTokenAsync(JsonWebTokenHandlerValidationParametersTheo
var context = TestUtilities.WriteHeader($"{this}.ValidateTokenAsync", theoryData);

JsonWebTokenHandler jsonWebTokenHandler = new JsonWebTokenHandler();
jsonWebTokenHandler.SetDefaultTimesOnTokenCreation = theoryData.SetDefaultValuesOnTokenCreation;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we not create the jwt when creating the theoryData?

@brentschmaltz
Copy link
Member

If the goal of this test is to check that the path with TokenValidationParameters is the same as ValidationParameters, the number of tests will be large.

I would suggest having separate files for each validation step.
In this way we will be able to focus on one validation step at time.
This separation will result in fewer merge conflicts when multiple engineers will work on validation.

Copy link
Member

@brentschmaltz brentschmaltz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split into PR's focusing on one validation step.

@iNinja
Copy link
Contributor Author

iNinja commented Sep 13, 2024

Closing the PR, will re-raise individual ones as I add more tests.

@iNinja iNinja closed this Sep 13, 2024
@iNinja iNinja deleted the iinglese/add-more-regression-tests branch November 15, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants