umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.54k stars 2.71k forks source link

Unauthorized SSO user logging into backoffice gets redirected to login without error #16590

Open rmnaderdev opened 5 months ago

rmnaderdev commented 5 months ago

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

13.2.2

Bug summary

Background: In my current project, we are using BO SSO using OpenIdConnect with AzureAD. I want to be able to prevent user login (and account creation) if the user's account is not already in the system (based on email address). If the user is not in the system, a user account should not be added to the Umbraco system.

The bug: The bug I am reporting is that if a user is not allowed into the BO after using the SSO login, they are just sent back to the Umbraco login screen with no further error message or anything. In the OnTicketReceived hook, I am calling context.Fail("Invalid user") if the user's email is not found in the system. In the OnExternalLogin property in the OpenIdConnect options, I am also returning false if the user's email is not found in the system.

Specifics

Using Microsoft.AspNetCore.Authentication.OpenIdConnect @ v8.0.0

BackOfficeExternalLoginProviderOptions file:

public class ProviderBackOfficeExternalLoginProviderOptions : IConfigureNamedOptions<BackOfficeExternalLoginProviderOptions>
{
    private IUserService UserService { get; }

    public const string SchemeName = OpenIdConnectDefaults.AuthenticationScheme;

    public ProviderBackOfficeExternalLoginProviderOptions(IUserService userService)
    {
        UserService = userService;
    }

    public void Configure(string? name, BackOfficeExternalLoginProviderOptions options)
    {
        if (name != Umbraco.Cms.Core.Constants.Security.BackOfficeExternalAuthenticationTypePrefix + SchemeName)
        {
            return;
        }

        Configure(options);
    }

    public void Configure(BackOfficeExternalLoginProviderOptions options)
    {
        // Customize the login button
        options.ButtonStyle = "btn-microsoft";
        options.Icon = "fa fa-cloud";

        // The following options are only relevant if you
        // want to configure auto-linking on the authentication.
        options.AutoLinkOptions = new ExternalSignInAutoLinkOptions(

            // Set to true to enable auto-linking
            autoLinkExternalAccount: true,

            // [OPTIONAL]
            // Default: "Editor"
            // Specify User Group.
            defaultUserGroups: new[] { Umbraco.Cms.Core.Constants.Security.EditorGroupAlias },

            // [OPTIONAL]
            // Default: The culture specified in appsettings.json.
            // Specify the default culture to create the User as.
            // It can be dynamically assigned in the OnAutoLinking callback.
            defaultCulture: null,

            // [OPTIONAL]
            // Enable the ability to link/unlink manually from within
            // the Umbraco backoffice.
            // Set this to false if you don't want the user to unlink 
            // from this external login provider.
            allowManualLinking: false
        )
        {
            // [OPTIONAL] Callback
            OnAutoLinking = (autoLinkUser, loginInfo) =>
            {
                // Customize the user before it's linked.
                // Modify the User's groups based on the Claims returned
                // in the external ogin info.

                // You can customize the user before it's linked.
                // i.e. Modify the user's groups based on the Claims returned
                // in the externalLogin info
                var claims = loginInfo.Principal.Claims.ToList();

                var userUpn = claims.FirstOrDefault(p => p.Type.Equals(ClaimTypes.Upn))?.Value ?? string.Empty;

                var umbracoUser = UserService.GetByEmail(userUpn);
                if (umbracoUser != null && !userUpn.Contains("#EXT#"))
                {
                    autoLinkUser.Name = claims.FirstOrDefault(p => p.Type.Equals(ClaimTypes.GivenName))?.Value;
                    autoLinkUser.IsApproved = true;
                }
                else
                {
                    //fail safe
                    autoLinkUser.UserName = null;  //hack to prevent the user from being created in the backoffice
                    autoLinkUser.IsApproved = false;
                }

            },
            OnExternalLogin = (autoLinkUser, loginInfo) =>
            {
                // Customize the User before it is saved whenever they have
                // logged in with the external provider.
                // Sync the Users name based on the Claims returned
                // in the external login info
                var claims = loginInfo.Principal.Claims.ToList();
                var userUpn = claims
                    .FirstOrDefault(p => p.Type.Equals(ClaimTypes.Upn))
                    ?.Value.Replace("#EXT#", string.Empty) ?? string.Empty;

                var umbracoUser = UserService.GetByEmail(userUpn);
                if (umbracoUser == null || !autoLinkUser.IsApproved)
                {
                    return false;
                }
                // Returns a boolean indicating if sign-in should continue or not.
                return true;
            }
        };

        // [OPTIONAL]
        // Disable the ability for users to login with a username/password.
        // If set to true, it will disable username/password login
        // even if there are other external login providers installed.
        options.DenyLocalLogin = true;

        // [OPTIONAL]
        // Choose to automatically redirect to the external login provider
        // effectively removing the login button.
        options.AutoRedirectLoginToExternalProvider = false;
    }
}

ExternalProviderBackOfficeExtension:

public static class ExternalProviderBackOfficeExtensions
{
    public static IUmbracoBuilder AddProviderBackofficeAuthentication(this IUmbracoBuilder builder)
    {
        var configuration = builder.Config;

        var microsoftTenantId = ...
        var microsoftClientId = ...
        var microsoftClientSecret = ...

        var callbackPath = configuration.GetValue<string>("ADAuthentication:BackofficeCallbackPath");

        // Register ProviderBackOfficeExternalLoginProviderOptions here rather than require it in startup
        builder.Services.ConfigureOptions<ProviderBackOfficeExternalLoginProviderOptions>();

        builder.AddBackOfficeExternalLogins(logins =>
        {
            logins.AddBackOfficeLogin(
                backOfficeAuthenticationBuilder =>
                {
                    backOfficeAuthenticationBuilder.AddOpenIdConnect(
                        // The scheme must be set with this method to work for the back office
                        backOfficeAuthenticationBuilder.SchemeForBackOffice(ProviderBackOfficeExternalLoginProviderOptions.SchemeName)!, "My Company Azure Active Directory",
                        options =>
                        {
                            // Callback path: Represents the URL to which the browser should be redirected to.
                            // The value here should match what you have configured in you external login provider.
                            // The value needs to be unique.
                            options.Authority = $"https://login.microsoftonline.com/{microsoftTenantId}";
                            options.ClientId = microsoftClientId; // Replace with your client id generated while creating OAuth client ID
                            options.ClientSecret = microsoftClientSecret; // Replace with your client secret generated while creating OAuth client ID
                            options.CallbackPath = callbackPath;
                            options.UsePkce = true;
                            // What to ask IdentityServer for
                            options.ResponseType = options.ResponseType = Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectResponseType.IdToken;
                            options.Scope.Add("openid");
                            options.Scope.Add("profile");
                            options.Scope.Add("email");
                            options.Scope.Add("roles");

                            options.GetClaimsFromUserInfoEndpoint = true;
                            options.RequireHttpsMetadata = true;
                            options.Events.OnTokenValidated = context =>
                            {
                                ClaimsPrincipal? principal = context.Principal;
                                if (principal is null)
                                {
                                    throw new InvalidOperationException("No claims found..");
                                }

                                // Rename some claims if needed

                                var authenticationType = principal.Identity?.AuthenticationType;
                                context.Principal = new ClaimsPrincipal(new ClaimsIdentity(claims, authenticationType));

                                return Task.CompletedTask;
                            };

                            options.Events.OnTicketReceived = async context =>
                            {
                                var userService = context.HttpContext.RequestServices
                                    .GetRequiredService<IUserService>();

                                // Get the user's email from the upn in the claims
                                var claims = context.Principal.Claims.ToList();

                                var userUpn = claims
                                    .FirstOrDefault(p => p.Type.Equals(ClaimTypes.Upn));

                                var umbracoUser = userService.GetByEmail(userUpn);

                                if (umbracoUser == null || !umbracoUser.IsApproved)
                                {
                                    // Prevent user login
                                    context.Fail("User does not exist");    // <-- WHEN THIS IS EXECUTED, THE USER IS NOT LOGGED IN BUT IS REDIRECTED TO THE LOGIN PAGE WITHOUT ANY ERROR MESSAGE
                                }

                                await Task.CompletedTask;
                            };
                        });
                });
        });
        return builder;
    }
}

Steps to reproduce

  1. Create a blank Umbraco 13 project. (Latest v13 version has this issue too).
  2. Add SSO using OpenIdConnect with the code under the specifics field. I am using AzureAD.
  3. Ensure there is logic in the OpenIdConnect Umbraco extensions (like I shared) to prevent the user from logging in.
  4. Try to login using SSO.
  5. The user will be redirected for SSO (like normal) then redirected back to Umbraco. The screen will flash and they will be back to the login screen with no errors or messages.

Expected result / actual result

When a user is redirected back from Azure SSO, I would expect the Umbraco login page to have some sort of message or indication that the login failed. Perhaps it would show the message passed to the OnTicketReceived Func. For example: context.Fail("MY MESSAGE HERE") would display "MY MESSAGE HERE" somewhere on the login screen.

I understand my use-case for user SSO is a bit different than the Doc examples. We want the user accounts to be auto-linked, but only based on our logic. If the SSO user's email is not found in the Umbraco users, deny login. Please let me know if I missed something.

github-actions[bot] commented 5 months ago

Hi there @PotatoDotJar!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

NguyenThuyLan commented 4 months ago

Thanks @rmnaderdev for reporting this issue 😊. As I can see there are 2 parts to this issue.

1. Umbraco login page does not show error (This is a bug)

I tried logging in case autoLinkExternalAccount = false, I can see that the error message has been added in the following code BackOfficeController - ExternalSignInAsync :

image

In the index.cshtml (HTML for Umbraco Login page) has the code to show this errors:

image

But the truth is that these errors were not displayed. The reason in my opinion is because the Login page was not re-rendered to update externalSignInErrors.

2. Disable creating new user when using auto-link (This is an feature):

Solution: to solve this problem, I think we can consider adding a new flag like CreateNewUser = true/false inside AutoLinkAndSignInExternalAccount() in BackOfficeSignInManager.cs

kjac commented 4 months ago

Hi @rmnaderdev,

Thanks for reporting this and for the detailed description 💪

A quick FYI: Your OnTicketReceived shouldn't need the context.Fail("User does not exist") in order to prevent users from being logged in; returning false in OnExternalLogin should be sufficient.

This issue really is a combination of a bug and a feature request, so we'll have to split it eventually.

The missing error message

This is indeed a bug, and I can reproduce this as well in the latest V13. It might also be an issue in V14 - it's tied to the new, AngularJS-less login screen, which was introduced in V13.

I'll have a chat with the team about fixing this.

Not creating auto-linked users

This is a new feature. I like the idea 👍

Time will tell if we can squeeze it into V13 or if it will be a V14+ thing.