When using OpenIddict.Sandbox.AspNet.Server with ADFS, the /Account/ExternalLoginCallback shows the login url instead of the provider name

gentledepp commented 1 month ago

Personal contribution

[ ] I'm not interested in submitting a pull request and understand that this provider may not be fixed in the near future without my contribution.

Version

5.6.0

Provider name

AddActiveDirectoryFederationServices

Describe the bug

 options.UseWebProviders()
     .AddGitHub(options =>
     {
         ...
     })
     .AddActiveDirectoryFederationServices(options =>
     {
         options
             .SetClientId("some client id")
             .SetClientSecret("some secret ;-)")
             .SetIssuer("https://login.microsoftonline.com/50cb0450-52d6-459d-b3c4-4465012aa1a2/v2.0")
             .SetRedirectUri("callback/login/microsoft")
             .SetProviderDisplayName("Microsoft Entry ID")
             .SetProviderName("Microsoft");
     })

in Openiddict.Sandbox.AspNet.Server creates the following issues:

The login form does not show the ProviderDisplayName, but the ProviderName:
When login was successful but an account must be created, the form shows the login url instead of the ProviderDisplayName:

Note: I do not know where to get the ProviderDisplayName from, but at least one could use one of the two claims:

E.g.:


    //
    // GET: /Account/ExternalLoginCallback
    [AllowAnonymous]
    public async Task<ActionResult> ExternalLoginCallback(string returnUrl)
    {
        var loginInfo = await AuthenticationManager.GetExternalLoginInfoAsync();
        if (loginInfo == null)
        {
            return RedirectToAction("Login");
        }

+        var oi_prvd_name = loginInfo.ExternalIdentity.Claims.FirstOrDefault(c => c.Type == "oi_prvd_name")?.Value;

        // Connecter cet utilisateur à ce fournisseur de connexion externe si l'utilisateur possède déjà une connexion
        var result = await SignInManager.ExternalSignInAsync(loginInfo, isPersistent: false);
        switch (result)
        {
            case SignInStatus.Success:
                return RedirectToLocal(returnUrl);
            case SignInStatus.LockedOut:
                return View("Lockout");
            case SignInStatus.RequiresVerification:
                return RedirectToAction("SendCode", new { ReturnUrl = returnUrl, RememberMe = false });
            case SignInStatus.Failure:
            default:
                // Si l'utilisateur n'a pas de compte, invitez alors celui-ci à créer un compte
                ViewBag.ReturnUrl = returnUrl;
+                ViewBag.LoginProvider = oi_prvd_name ?? loginInfo.Login.LoginProvider;
                return View("ExternalLoginConfirmation", new ExternalLoginConfirmationViewModel { Email = loginInfo.Email });
        }
    }

Would return:

Can I improve this somehow?

To reproduce

Check out the branch https://github.com/gentledepp/openiddict-core/tree/issue/adfs Replace

AddActiveDirectoryFederationServices(options =>
                        {
                            options
                                .SetClientId("some client id") // application (client) id
                                .SetClientSecret("some secret") // generated secret from azure portal

with

.AddActiveDirectoryFederationServices(options =>
{
    options
        .SetClientId("e622a0e5-f3e8-4998-b4e1-35f45e9b18cd") // application (client) id
        .SetClientSecret("_8x8Q~fc7GxwbieK04mL2tQuzrlqMU_yPTd5rawD") // generated secret from azure portal

... since github did not let me push secrets

Run the sample and login with the test user username gadget@icltestportal.onmicrosoft.com password: q328j+ye6;k"~*

Exceptions (if any)

1. In Asp.Net, show the ProviderDisplayName (if provided) in the external login section
2. In the "Account/ExternalLoginCallback" view, show the ProviderDisplayName instead of the long URL which may confuse or frighten users.

gentledepp commented 1 month ago

So I switched tp "AddMicrosoft" as suggested,

 .AddMicrosoft(options =>
 {
     options
         .SetClientId("e622a0e5-f3e8-4998-b4e1-35f45e9b18cd") // application (client) id
         .SetClientSecret("_8x8Q~fc7GxwbieK04mL2tQuzrlqMU_yPTd5rawD") // generated secret from azure portal
         .SetRedirectUri("/callback/login/microsoft");
 })
 ;

On AspNetCore, this works flawlessly. On AspNet (OpenIddict.Sandbox.AspNet.Server) though, I again get some weird URL displayed:

To fix this, I would have to do something like this:

Would that be correct?

It seems, though, that for Asp.Net there is

either an issue with the WebProviders not always correctly displaying the caption
or the sample has a bug 🤷‍♂️👀

kevinchalet commented 1 month ago

It seems, though, that for Asp.Net there is

It's actually by design: unlike ASP.NET Core Identity, ASP.NET Identity uses the Claim.Issuer property of the name claim. Since both IdentityModel and OpenIddict use the issuer URI as the claims issuer, that's why you get a URI in the GUI.

That said, I agree it's not really a great UX so I went ahead and introduced a new claims issuer option that will ship in OpenIddict 6.0 RC2: https://github.com/openiddict/openiddict-core/pull/2209.

openiddict / openiddict-core