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.

Sign up for GitHub

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

Jump to bottom

Validate Audience for SAML2TokenHandler with New Model #2863

Merged
merged 18 commits into from
Oct 14, 2024
Merged

Validate Audience for SAML2TokenHandler with New Model #2863

merged 18 commits into from
Oct 14, 2024

Conversation

FuPingFranco
Copy link
Contributor

@FuPingFranco FuPingFranco commented Oct 2, 2024 •
edited
Loading

Validate Audience for SAML2TokenHandler with New Model

Description

In order to build the new ValidateTokenAsync method for SAML2TokenHandler we will be taking an integration test approach to validate that both old and new paths function equally while constructing the new methods along the way. This PR validates only cases applicable to audience validation that exists in both JsonWebTokenHandler and Saml2SecurityTokenhandler, a separate PR will be done for Saml2SecurityTokenhandler specific tests.

Follow-up issue for additional testing: #2875

@FuPingFranco FuPingFranco changed the title [DRAFT] Validate Audience for SAML2TokenHandler with New Model Validate Audience for SAML2TokenHandler with New Model Oct 4, 2024
@FuPingFranco FuPingFranco marked this pull request as ready for review October 4, 2024 18:05
@FuPingFranco FuPingFranco requested a review from a team as a code owner October 4, 2024 18:05
@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 7, 2024

I haven't done a compare yet, but are these regression tests following the same model as the ones from @iNinja for JWT?

@FuPingFranco
Copy link
Contributor Author

FuPingFranco commented Oct 7, 2024 •
edited
Loading

I haven't done a compare yet, but are these regression tests following the same model as the ones from @iNinja for JWT?

For the most part except if we have a SAML specific case that show up when validating an audience but, so far it was only dealing with TokenValidationParameters.RequireAudience on the new path. We decided to remove it this morning.

Update: Simplified the tests in This PR to only account for the comparison between the new and old call graphs to work as expected.

@FuPingFranco @westin-m
Update test/Microsoft.IdentityModel.Tokens.Saml.Tests/Saml2SecurityTo…
e04c94c 
…kenHandlerTests.ValidateTokenAsyncTests.Audience.cs

Co-authored-by: Westin Musser <[email protected]>
@brentschmaltz brentschmaltz merged commit 3dab668 into dev Oct 14, 2024
6 checks passed
@FuPingFranco FuPingFranco mentioned this pull request Oct 18, 2024
Merged
@FuPingFranco FuPingFranco self-assigned this Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshLozensky JoshLozensky JoshLozensky left review comments

@jennyf19 jennyf19 jennyf19 left review comments

@iNinja iNinja iNinja left review comments

@westin-m westin-m westin-m approved these changes

@brentschmaltz brentschmaltz brentschmaltz approved these changes

Assignees

@FuPingFranco FuPingFranco

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@FuPingFranco @jennyf19 @iNinja @brentschmaltz @JoshLozensky @westin-m