-
Notifications
You must be signed in to change notification settings - Fork 388
Update to new config layout, AzureAd templates #862
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
Conversation
Is this ready to go? |
Not yet, we still need to update all the consumers of the config |
@HaoK any update? |
Still waiting to finish the items under the convension based config checkboxes... hopefully soonish |
Default schema for Kestrel endpoints (aspnet/KestrelHttpServer#1875) |
@danroth27 I've updated this PR and tweaked the AzureAD templates a bit, all the templates build/restore/run (except for IdentityService) |
@@ -12,6 +12,51 @@ | |||
<Disp Icon="Str" Expand="true" Disp="true" LocTbl="false" Path=" \ ;Managed Resources \ 0 \ 0" /> | |||
<Item ItemId=";Strings" ItemType="0" PsrId="211" Leaf="false"> | |||
<Disp Icon="Str" LocTbl="false" /> | |||
<Item ItemId=";AddProjToSlnPostActionFailed" ItemType="0" PsrId="211" Leaf="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these changes in this PR?
var properties = new AuthenticationProperties() { RedirectUri = "/" }; | ||
properties.Items[AzureAdB2COptions.PolicyAuthenticationProperty] = Options.ResetPasswordPolicyId; | ||
return Challenge(properties, OpenIdConnectDefaults.AuthenticationScheme); | ||
return Challenge(AzureAdB2CDefaults.ResetPasswordAuthenticationScheme); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much nicer!
{ | ||
public class AzureAdOptions | ||
{ | ||
public string ClientId { get; set; } | ||
public string ClientSecret { get; set; } | ||
public string AzureAdInstance { get; set; } | ||
public string Instance { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instance of what? How about BaseUri?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion, we should use whatever the official term for this is, my main intent was to drop the name of of the options from the property
{ | ||
public static class AzureAdB2CDefaults | ||
{ | ||
public const string AuthenticationScheme = "AzureAdB2C"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I'll remove
public string ClientId { get; set; } | ||
public string AzureAdB2CInstance { get; set; } | ||
public string Instance { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about BaseUri?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe InstanceUri?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this show up in any UI on the Azure side anywhere? What do they call it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just the base URI so that you could potentially point at a staging environment or some other variant.
// } | ||
// "DefaultSignInScheme": "Cookies", | ||
// "DefaultAuthenticateScheme": "Cookies", | ||
// "DefaultChallengeScheme": "OpenIdConnect", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the OpenIdConnect scheme is registered anymore given these changes. Should this be AzureAdB2CDefaults.SignUpSignInAuthenticationScheme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lcl changes need to be undone, looks good otherwise. I'd like to get an explicit ack that the IdentityService portion not working is expected at this point though (@danroth27)
@mlorbetske The identity service changes are being handled here: #870 |
Ah, didn't realize that was coordinated with this change. Thanks for the clarification! |
Reverted and rebased, should be good to merge, we can address any other minor tweaks in subsequent PRs |
Config updates addressing: aspnet/MetaPackages#117
@danroth27
New config's generated:
Individual:
B2C:
Multi-Org: