-
Notifications
You must be signed in to change notification settings - Fork 218
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
Fix for #415 #428
Fix for #415 #428
Conversation
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.
LGTM.
Thanks @durandt
context.ProtocolMessage.DomainHint = domainHint; | ||
|
||
// delete the domainHint from the Properties when we are done otherwise | ||
// it will take up extra space in the cookie. | ||
context.Properties.Parameters.Remove(OpenIdConnectParameterNames.DomainHint); |
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.
Thanks for the contribution @durandt, this is great. We already have a test verifying the doman_hint is set, but not sure if we want to check one or the other as well, or this test is enough...personally, I'm okay with this current test and the work done here. cc: @jmprieur
src/Microsoft.Identity.Web/WebAppExtensions/WebAppAuthenticationBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
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.
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.
👍
…onBuilderExtensions.cs
thanks again @durandt |
Well, that wasn't much job 😊 Thank you @jennyf19 , that was fast! |
domain_hint ingored when login_hint is set.
Break up code so that domain_hint and login_hint are treated
independently.