Skip to content

Support loading OIDC options from configuration#42679

Merged
captainsafia merged 5 commits intomainfrom
cs/oidc-opts-config
Jul 16, 2022
Merged

Support loading OIDC options from configuration#42679
captainsafia merged 5 commits intomainfrom
cs/oidc-opts-config

Conversation

@captainsafia
Copy link
Copy Markdown
Contributor

@captainsafia captainsafia commented Jul 12, 2022 •
edited
Loading

Part of #42170 and fixes #42000.

  • Support configuring more options from config for OpenIdConnectOptions and JwtBearerOptions
  • Support multiple issuers being set via array from config for JwtBearerOptions
  • Support single audience being set via string from config for JwtBearerOptions
  • Update writing of config properties in user-jwts to use ValidIssuer and ValidAudiences names to match TokenValidationParameters
  • Support multiple signing keys in JWT bearer-based authentication

This does not add support for setting nested properties under JwtBearerOptions.TokenValidationParameters since I don't think that level of customization is necessary at the moment.

@captainsafia captainsafia requested a review from HaoK July 12, 2022 02:44
@captainsafia captainsafia added area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jul 12, 2022
@captainsafia captainsafia marked this pull request as draft July 12, 2022 18:53
@captainsafia captainsafia force-pushed the cs/oidc-opts-config branch 2 times, most recently from 49f6dc4 to 1374217 Compare July 13, 2022 01:00
Kahbazi
Kahbazi reviewed Jul 13, 2022
View reviewed changes
@captainsafia captainsafia force-pushed the cs/oidc-opts-config branch from 1374217 to 5e86e1b Compare July 13, 2022 14:58
@captainsafia captainsafia requested a review from a team July 13, 2022 15:07
@captainsafia captainsafia marked this pull request as ready for review July 13, 2022 15:07
Tratcher
Tratcher reviewed Jul 13, 2022
View reviewed changes
Kahbazi
Kahbazi reviewed Jul 14, 2022
View reviewed changes
src/Security/Authentication/OpenIdConnect/src/OpenIdConnectConfigureOptions.cs Outdated
var scopes = configSection.GetSection(nameof(options.Scope)).GetChildren().Select(scope => scope.Value).OfType<string>();
foreach (var scope in scopes)
{
options.Scope.Add(scope);
Copy link
Copy Markdown
Member

@Kahbazi Kahbazi Jul 14, 2022

Choose a reason for hiding this comment

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

I'm not sure if it's needed, but there's no way to remove the default scopes right now.

aspnetcore/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectOptions.cs

Lines 43 to 44 in 5e86e1b

Scope.Add("openid");
Scope.Add("profile");

Copy link
Copy Markdown
Contributor Author

@captainsafia captainsafia Jul 14, 2022

Choose a reason for hiding this comment

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

The openid scope is required in OIDC and I think scenarios where the user would not want a profile scope too are rare to justify supporting modification here.

Copy link
Copy Markdown
Member

@Tratcher Tratcher Jul 14, 2022

Choose a reason for hiding this comment

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

should Scope be cleared before adding new values? (if there are any)

@captainsafia captainsafia force-pushed the cs/oidc-opts-config branch from 5e86e1b to 4117466 Compare July 14, 2022 16:18
Tratcher
Tratcher reviewed Jul 14, 2022
View reviewed changes
src/Security/Authentication/OpenIdConnect/src/OpenIdConnectConfigureOptions.cs Outdated
cookieBuilder.MaxAge = cookieBuilder.MaxAge is TimeSpan maxAge ? StringHelpers.ParseValueOrDefault(maxAge, cookieConfigSection[nameof(cookieBuilder.MaxAge)], _invariantTimeSpanParse) : cookieBuilder.MaxAge;
cookieBuilder.Name = cookieConfigSection[nameof(CookieBuilder.Name)] ?? cookieBuilder.Name;
cookieBuilder.Path = cookieConfigSection[nameof(CookieBuilder.Path)];
cookieBuilder.SameSite = cookieConfigSection[nameof(CookieBuilder.SameSite)]?.ToLowerInvariant() switch
Copy link
Copy Markdown
Member

@Tratcher Tratcher Jul 14, 2022

Choose a reason for hiding this comment

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

Is there a reason you didn't use Enum.Parse?

Copy link
Copy Markdown
Contributor Author

@captainsafia captainsafia Jul 15, 2022

Choose a reason for hiding this comment

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

It doesn't support case-insensitive parsing as far as I can tell.

Copy link
Copy Markdown
Member

@Tratcher Tratcher Jul 15, 2022

Choose a reason for hiding this comment

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

https://docs.microsoft.com/en-us/dotnet/api/system.enum.parse?view=net-6.0#system-enum-parse(system-type-system-string-system-boolean)

Tratcher
Tratcher reviewed Jul 15, 2022
View reviewed changes
Tratcher
Tratcher approved these changes Jul 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

...after addressing the Enum.Parse comments.

@captainsafia captainsafia enabled auto-merge (squash) July 15, 2022 22:41
@captainsafia captainsafia merged commit 2e08d0c into main Jul 16, 2022
@captainsafia captainsafia deleted the cs/oidc-opts-config branch July 16, 2022 00:16
@ghost ghost added this to the 7.0-rc1 milestone Jul 16, 2022
Kahbazi
Kahbazi reviewed Jul 16, 2022
View reviewed changes
src/Security/Authentication/JwtBearer/src/JwtBearerConfigureOptions.cs

options.Authority = configSection[nameof(options.Authority)] ?? options.Authority;
options.BackchannelTimeout = StringHelpers.ParseValueOrDefault(configSection[nameof(options.BackchannelTimeout)], _invariantTimeSpanParse, options.BackchannelTimeout);
options.Challenge = configSection[nameof(options.Challenge)] ?? options.Challenge;
Copy link
Copy Markdown
Member

@Kahbazi Kahbazi Jul 16, 2022

Choose a reason for hiding this comment

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

@captainsafia @Tratcher Could it be useful that EventsType be set from config too?

options.EventsType = StringHelpers.ParseValueOrDefault(configSection[nameof(options.EventsType)], Type.GetType, null);

Copy link
Copy Markdown
Member

@Tratcher Tratcher Jul 16, 2022

Choose a reason for hiding this comment

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

Would you really switch out the events dynamically?

I think we've added more than we should have already and we're past the point of diminishing returns.

captainsafia pushed a commit that referenced this pull request Jul 18, 2022
Support loading OIDC options from configuration (#42679)
323fd08 
* Support loading OIDC options from configuration

* Address feedback from review

* Address review feedback

* Support fallbacks for all options and override lists

* Fix up Enum.Parse and default for Authority
wtgodbe pushed a commit that referenced this pull request Jul 18, 2022
Support loading OIDC options from configuration (#42679) (#42771)
c83c384 
* Support loading OIDC options from configuration

* Address feedback from review

* Address review feedback

* Support fallbacks for all options and override lists

* Fix up Enum.Parse and default for Authority
@Kahbazi Kahbazi mentioned this pull request Dec 14, 2023
Closed
1 task
@halter73 halter73 mentioned this pull request Jan 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tratcher Tratcher Tratcher approved these changes

@HaoK HaoK Awaiting requested review from HaoK

@Pilchie Pilchie Awaiting requested review from Pilchie

+1 more reviewer

@Kahbazi Kahbazi Kahbazi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels

Projects

None yet

Milestone

7.0-rc1

Development

Successfully merging this pull request may close these issues.

Support multiple issuers in JwtBearerConfigureOptions

3 participants

@captainsafia @Tratcher @Kahbazi