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.

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

Domain hint only specified when login hint is not null or blank #415

Closed
1 of 8 tasks
glasody opened this issue Aug 7, 2020 · 5 comments · Fixed by #428
Closed
1 of 8 tasks

Domain hint only specified when login hint is not null or blank #415

glasody opened this issue Aug 7, 2020 · 5 comments · Fixed by #428
Labels
bug Something isn't working fixed good first issue Good for newcomers P1
Milestone

Comments

@glasody
Copy link

glasody commented Aug 7, 2020

Which version of Microsoft Identity Web are you using?
0.2.1-preview

Where is the issue?

  • Web app

    • Sign-in users
    • Sign-in users and call web APIs
  • Web API

    • Protected web APIs (validating tokens)
    • Protected web APIs (validating scopes)
    • Protected web APIs call downstream web APIs
  • Token cache serialization

    • In-memory caches
    • Session caches
    • Distributed caches
  • Other (please describe)

Repro
Create new net core app
Add identity web auth
Define AzureAd properties in config file with domain property defined but no login hint

Runt the Web app and Web API

Expected behavior
Domain hint to be added to the authorization request even if login hint isn't specified

Actual behavior
domain hint doesn't get added to the authorization call

Possible solution
Move domain hint definition outside the if clause


Hi,

I was working with this package to get a web app running with Azure AD, but found that defining the domain did not add it into my authentication request thus not directly opening the federation login page.

I found that this was because domain_hint is only being added when login_hint is not blank or null.
Is this a bug? I was under the assumption that domain_hint could be provided without a login_hint.

var login = context.Properties.GetParameter<string>(OpenIdConnectParameterNames.LoginHint);
if (!string.IsNullOrWhiteSpace(login))
{
context.ProtocolMessage.LoginHint = login;
context.ProtocolMessage.DomainHint = context.Properties.GetParameter<string>(
OpenIdConnectParameterNames.DomainHint);
// delete the login_hint and domainHint from the Properties when we are done otherwise
// it will take up extra space in the cookie.
context.Properties.Parameters.Remove(OpenIdConnectParameterNames.LoginHint);
context.Properties.Parameters.Remove(OpenIdConnectParameterNames.DomainHint);
}

@jmprieur jmprieur added bug Something isn't working P1 labels Aug 7, 2020
@pmaytak
Copy link
Contributor

pmaytak commented Aug 8, 2020

Hi @glasody. Thanks for reporting. We also appreciate outside contributions, if you'd like to submit a PR.

@pmaytak pmaytak added the good first issue Good for newcomers label Aug 8, 2020
@durandt
Copy link
Contributor

durandt commented Aug 10, 2020

I stumbled upon the same issue and it was a great time-saver that @glasody found it before me.
I pushed a PR-suggestion.

Regards,

@jennyf19 jennyf19 added this to the 0.3.0-preview milestone Aug 10, 2020
@jennyf19 jennyf19 linked a pull request Aug 10, 2020 that will close this issue
jennyf19 pushed a commit that referenced this issue Aug 10, 2020
domain_hint ingored when login_hint is set.
Break up code so that domain_hint and login_hint are treated
independently.

Co-authored-by: Thibault Durand <thibault.durand@liu.se>
@jennyf19
Copy link
Collaborator

Included in 0.3.0-preview release.

@durandt
Copy link
Contributor

durandt commented Aug 26, 2020

Thank you for the heads-up. Works as expected 👍

@jmprieur
Copy link
Collaborator

Thanks for the update, @durandt !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed good first issue Good for newcomers P1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants