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

Add support for reading default scheme from config #41987

Merged
merged 5 commits into from
Jun 13, 2022
Merged

Add support for reading default scheme from config #41987

merged 5 commits into from
Jun 13, 2022

Conversation

captainsafia
Copy link
Member

Part of #41956.

  • Set AuthenticationOptions.DefaultScheme from Authentication:DefaultScheme in config
  • Don't set default scheme when registering authentication services in WebApplicationBuilder.Authentication
  • Add GetAuthenticationConfiguration API to IAuthetnciationConfigurationProvider

@captainsafia captainsafia requested review from HaoK and a team and removed request for halter73, Tratcher and BrennanConroy June 1, 2022 21:48
@Pilchie Pilchie added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jun 2, 2022
@captainsafia captainsafia mentioned this pull request Jun 3, 2022
Closed
davidfowl
davidfowl reviewed Jun 10, 2022
View reviewed changes
src/Security/Authentication/Core/src/AuthenticationServiceCollectionExtensions.cs
@@ -28,6 +29,8 @@ public static AuthenticationBuilder AddAuthentication(this IServiceCollection se
services.AddWebEncoders();
services.TryAddSingleton<ISystemClock, SystemClock>();
services.TryAddSingleton<IAuthenticationConfigurationProvider, DefaultAuthenticationConfigurationProvider>();
services.TryAddEnumerable(ServiceDescriptor.Singleton<IConfigureOptions<AuthenticationOptions>, AuthenticationConfigureOptions>());
Copy link
Member

Choose a reason for hiding this comment

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

We should also discuss configuration changes.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah at a minimum we'd have to move away from IOptions and use IOptionsMonitor for AuthenticationOptions to be able to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

#42170

HaoK
HaoK reviewed Jun 13, 2022
View reviewed changes
src/Azure/AzureAD/Authentication.AzureAD.UI/test/AzureADAuthenticationBuilderExtensionsTests.cs
@@ -510,4 +486,12 @@ public void AddAzureAD_SkipsOptionsValidationForNonAzureOpenIdConnect()

Assert.NotNull(openIdConnectOptions.Get("other"));
}

private IServiceCollection GenerateServicesForTest()
Copy link
Member

Choose a reason for hiding this comment

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

really nitty (don't feel obligated to do this): GenerateTestServices? But I'm not a huge fan of method names as sentences without spaces.

HaoK
HaoK reviewed Jun 13, 2022
View reviewed changes
src/Security/Authentication/test/TestExtensions.cs
@@ -77,4 +79,12 @@ public static Task DescribeAsync(this HttpResponse res, IEnumerable<Authenticati
return res.Body.WriteAsync(xmlBytes, 0, xmlBytes.Length);
}

public static IServiceCollection ConfigureAuthTestServices(this IServiceCollection services)
Copy link
Member

Choose a reason for hiding this comment

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

nit: could drop Auth, ConfigureTestServices(), since this doesn't actually configure any auth services anyways.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe want to go with the same name between this one and GenerateServicesForTest for consistency too

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do this in another PR to avoid having to rekick the build again. 😓

@captainsafia captainsafia merged commit e07cdd8 into dotnet:main Jun 13, 2022
@ghost ghost added this to the 7.0-preview6 milestone Jun 13, 2022
@DamianEdwards
Copy link
Member

😀 🐼

@ghost
Copy link

ghost commented Jun 13, 2022

Hi @DamianEdwards. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@rafikiassumani-msft rafikiassumani-msft mentioned this pull request Jun 14, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HaoK HaoK HaoK approved these changes

@martincostello martincostello martincostello left review comments

@Pilchie Pilchie Awaiting requested review from Pilchie

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@DamianEdwards DamianEdwards Awaiting requested review from DamianEdwards

@davidfowl davidfowl Awaiting requested review from davidfowl

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

Successfully merging this pull request may close these issues.
7 participants
@captainsafia @DamianEdwards @halter73 @davidfowl @martincostello @HaoK @Pilchie