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

Update Wilson7 branch #49524

Merged
merged 4 commits into from
Jul 20, 2023
Merged

Update Wilson7 branch #49524

merged 4 commits into from
Jul 20, 2023

Conversation

keegan-caruso
Copy link
Contributor

Update Wilson7 branch

Description

Default to using new handlers
Changes from API review

Update Wilson7 branch
d9bf9d3 
Default to using new handlers
Changes from API review
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jul 19, 2023
keegan-caruso
keegan-caruso commented Jul 19, 2023
View reviewed changes
src/Security/Authentication/WsFederation/src/WsFederationOptions.cs
@@ -208,5 +209,5 @@ public TokenValidationParameters TokenValidationParameters
/// <para>The default token handler for JsonWebTokens is a <see cref="JsonWebTokenHandler"/> which is faster than a <see cref="JwtSecurityTokenHandler"/>.</para>
/// <para>There is an ability to make use of a Last-Known-Good model for metadata that protects applications when metadata is published with errors.</para>
/// </remarks>
public bool UseTokenHandlers { get; set; }
public bool UseSecurityTokenHandlers { get; set; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: naming is slightly different than what was called out in API review because API review mistakenly used the name 'SecurityTokenValidators ' where for wsfed it is called 'SecurityTokenHandlers'

#49469

Copy link
Contributor

Choose a reason for hiding this comment

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

which are not the TokenHandler unfortunately.
I wonder if, for the sake of simplicity and uniformity, we shouldn't keep UseSecurityTokenValidators even for WsFed ... just a suggestion.
Although looking at the code .. probably not.

keegan-caruso
keegan-caruso commented Jul 19, 2023
View reviewed changes
src/Security/Authentication/test/JwtBearerTests_Handler.cs
}

[Fact]
public async Task ExpirationAndIssuedWhenMinOrMaxValue()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: behavior is slightly different than:

aspnetcore/src/Security/Authentication/test/JwtBearerTests.cs

Line 933 in d9bf9d3

public async Task ExpirationAndIssuedNullWhenMinOrMaxValue()

As JsonWebToken correctly handles datetimes represented by greater than int32 values

jennyf19
jennyf19 reviewed Jul 19, 2023
View reviewed changes
src/Security/Authentication/JwtBearer/src/JwtBearerOptions.cs Outdated
@@ -111,6 +113,7 @@ public new JwtBearerEvents Events
/// <summary>
/// Gets the ordered list of <see cref="ISecurityTokenValidator"/> used to validate access tokens.
/// </summary>
[Obsolete("SecurityTokenValidators is no longer used by default. Use TokenHandlers instead. To continue using SecurityTokenValidators, set UseSecurityTokenValidators to true.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to include an aka.ms link w/more details? @jmprieur

Copy link
Contributor

Choose a reason for hiding this comment

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

@keegan-caruso @jennyf19
What about: https://aka.ms/aspnetcore8/security-token-changes
(already links to the breaking change announcement)

jennyf19
jennyf19 approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

:shipit: Great work, @keegan-caruso

jmprieur
jmprieur approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @keegan-caruso

src/Security/Authentication/WsFederation/src/WsFederationOptions.cs
@@ -208,5 +209,5 @@ public TokenValidationParameters TokenValidationParameters
/// <para>The default token handler for JsonWebTokens is a <see cref="JsonWebTokenHandler"/> which is faster than a <see cref="JwtSecurityTokenHandler"/>.</para>
/// <para>There is an ability to make use of a Last-Known-Good model for metadata that protects applications when metadata is published with errors.</para>
/// </remarks>
public bool UseTokenHandlers { get; set; }
public bool UseSecurityTokenHandlers { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

which are not the TokenHandler unfortunately.
I wonder if, for the sake of simplicity and uniformity, we shouldn't keep UseSecurityTokenValidators even for WsFed ... just a suggestion.
Although looking at the code .. probably not.

@captainsafia
Copy link
Member

Merging this to the Wilson7 feature branch so we can move forward with merging Wilson7 to main.

@captainsafia captainsafia merged commit f22eba9 into dotnet:Wilson7 Jul 20, 2023
@keegan-caruso keegan-caruso deleted the keegan-caruso/update-wilson-7-branch branch July 20, 2023 02:31
wtgodbe pushed a commit that referenced this pull request Jul 21, 2023
@keegan-caruso @brentschmaltz @captainsafia
Update JwtBearer, WsFed, and OIDC handlers to use identity model 7 (#…
2922b05 
…49542)

* Option to use JsonWebTokenHandler in OpenIdConnectHandler

* fix sample

* split event tests

* update IdentityModel to 6.31.0

* Added JsonWebTokenHandler and TokenHandlers (#48857)

Co-authored-by: Brent Schmaltz <brentsch@hotmail.com>

* adjust for claims mapping
remove using System

* moved api's to unshipped

* Addressed PR comments

* Removed setter.

* Use 7.0.0-preview for identity model libraries

* fix bild break, useTokenHanlders default false.

* use var for identitymodel versions

* Move new apis to unshipped

* Increase key size to 256 bits or HMAC will fail.

* update version of identity model libraries (#49349)

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>

* Update Wilson7 branch (#49491)

* SymmetricSecurityKey needs 32 bytes

* Update source-build-externals dependencies

---------

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>

* Update Wilson7 branch (#49524)

* Update Wilson7 branch

Default to using new handlers
Changes from API review

* Handle obsolete members

* Handle obsolete members in tests

* Add aka.ms link

---------

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>

* Changes from API review

* Comments from review

---------

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>
Co-authored-by: Brent Schmaltz <brentsch@hotmail.com>
Co-authored-by: BrentSchmaltz <brentschmaltz@hotmail.com>
Co-authored-by: Safia Abdalla <safia@microsoft.com>
eerhardt pushed a commit that referenced this pull request Jul 21, 2023
@keegan-caruso @brentschmaltz @captainsafia
Update JwtBearer, WsFed, and OIDC handlers to use identity model 7 (#…
a56e968 
…49541)

* Option to use JsonWebTokenHandler in OpenIdConnectHandler

* fix sample

* split event tests

* update IdentityModel to 6.31.0

* Added JsonWebTokenHandler and TokenHandlers (#48857)

Co-authored-by: Brent Schmaltz <brentsch@hotmail.com>

* adjust for claims mapping
remove using System

* moved api's to unshipped

* Addressed PR comments

* Removed setter.

* Use 7.0.0-preview for identity model libraries

* fix bild break, useTokenHanlders default false.

* use var for identitymodel versions

* Move new apis to unshipped

* Increase key size to 256 bits or HMAC will fail.

* update version of identity model libraries (#49349)

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>

* Update Wilson7 branch (#49491)

* SymmetricSecurityKey needs 32 bytes

* Update source-build-externals dependencies

---------

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>

* Update Wilson7 branch (#49524)

* Update Wilson7 branch

Default to using new handlers
Changes from API review

* Handle obsolete members

* Handle obsolete members in tests

* Add aka.ms link

---------

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>

* Changes from API review

* Comments from review

---------

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>
Co-authored-by: Brent Schmaltz <brentsch@hotmail.com>
Co-authored-by: BrentSchmaltz <brentschmaltz@hotmail.com>
Co-authored-by: Safia Abdalla <safia@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennyf19 jennyf19 jennyf19 approved these changes

@jmprieur jmprieur jmprieur approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

Assignees
No one assigned
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@keegan-caruso @captainsafia @jmprieur @jennyf19