Skip to content

Fix issues with removal of user logins on change to external login provider configuration (13) #19511

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

Merged

Conversation

AndyButland
Copy link
Contributor

@AndyButland AndyButland commented Jun 9, 2025

Prerequisites

  • I have added steps to test this contribution in the description below

Resolves #19499

Description

The linked issue describes an error that occurs with the implementation provided in #19273, which is intended to ensure logins relating to backoffice users associated with external login providers are removed when the provider is removed from configuration. This was added to improve security with the Umbraco Cloud external login provider feature, but provides the same benefit to other setups where external login providers are used.

Investigating revealed two issues, both of which are resolved in this PR:

  • Some external login provider setups will store tokens as well as user logins. These need to be deleted first to avoid the foreign key constraint exception.
  • We shouldn't remove logins or tokens relating to login providers for members, that are stored in the same table.

Testing

To test this setup I've configured a Google backoffice and member login provider based on the documentation. For reference:

GoogleAuthenticationExtensions
public static class GoogleAuthenticationExtensions
{
    public static IUmbracoBuilder AddGoogleAuthentication(this IUmbracoBuilder builder)
    {
        builder.Services.ConfigureOptions<GoogleBackOfficeExternalLoginProviderOptions>();
        builder.Services.ConfigureOptions<GoogleMemberLoginProviderOptions>();

        builder.AddMemberExternalLogins(logins =>
        {
            logins.AddMemberLogin(
                membersAuthenticationBuilder =>
                {
                    membersAuthenticationBuilder.AddGoogle(
                        membersAuthenticationBuilder.SchemeForMembers(GoogleMemberLoginProviderOptions.SchemeName),
                        options =>
                        {
                            // Callback path: Represents the URL to which the browser should be redirected to.
                            // The default value is '/signin-google'.
                            // The value here should match what you have configured in you external login provider.
                            // The value needs to be unique.
                            options.CallbackPath = "/umbraco-google-signin";
                            options.ClientId = "<client id>";
                            options.ClientSecret = "<client secret>";
                        });


                });
        });

        builder.AddBackOfficeExternalLogins(logins =>
        {
            logins.AddBackOfficeLogin(
                backOfficeAuthenticationBuilder =>
                {
                    // The scheme must be set with this method to work for the back office
                    var schemeName =
                        backOfficeAuthenticationBuilder.SchemeForBackOffice(GoogleBackOfficeExternalLoginProviderOptions
                            .SchemeName);

                    ArgumentNullException.ThrowIfNull(schemeName);

                    backOfficeAuthenticationBuilder.AddGoogle(
                        schemeName,
                        options =>
                        {
                            // Callback path: Represents the URL to which the browser should be redirected to.
                            // The default value is '/signin-google'.
                            // The value here should match what you have configured in you external login provider.
                            // The value needs to be unique.
                            options.CallbackPath = "/umbraco-backoffice-google-signin";
                            options.ClientId = "<client id>";
                            options.ClientSecret = "<client secret>";
                        });
                });
        });
        return builder;
    }

}
GoogleMemberLoginProviderOptions
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core;
using Umbraco.Cms.Web.Common.Security;

public class GoogleMemberLoginProviderOptions : IConfigureNamedOptions<MemberExternalLoginProviderOptions>
{
    public const string SchemeName = "Google";

    public void Configure(string? name, MemberExternalLoginProviderOptions options)
    {
        if (name != Constants.Security.MemberExternalAuthenticationTypePrefix + SchemeName)
        {
            return;
        }

        Configure(options);
    }

    public void Configure(MemberExternalLoginProviderOptions options)
    {
        options.AutoLinkOptions = new MemberExternalSignInAutoLinkOptions(
            autoLinkExternalAccount: true,
            defaultCulture: null,
            defaultIsApproved: true,
            defaultMemberTypeAlias: Constants.Security.DefaultMemberTypeAlias)
        {
            OnAutoLinking = (autoLinkUser, loginInfo) =>
            {
            },
            OnExternalLogin = (user, loginInfo) =>
            {
                return true;
            }
        };
    }
}
GoogleBackOfficeExternalLoginProviderOptions
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core;
using Umbraco.Cms.Web.BackOffice.Security;

public class GoogleBackOfficeExternalLoginProviderOptions : IConfigureNamedOptions<BackOfficeExternalLoginProviderOptions>
{
    public const string SchemeName = "Google";

    public void Configure(string? name, BackOfficeExternalLoginProviderOptions options)
    {
        ArgumentNullException.ThrowIfNull(name);

        if (name != Constants.Security.BackOfficeExternalAuthenticationTypePrefix + SchemeName)
        {
            return;
        }

        Configure(options);
    }

    public void Configure(BackOfficeExternalLoginProviderOptions options)
    {
        options.Icon = "icon-google-fill";

        options.AutoLinkOptions = new ExternalSignInAutoLinkOptions(
            autoLinkExternalAccount: true,
            defaultUserGroups: new[] { Constants.Security.EditorGroupAlias },
            defaultCulture: null,
            allowManualLinking: true
        )
        {
            OnAutoLinking = (autoLinkUser, loginInfo) =>
            {
            },
            OnExternalLogin = (user, loginInfo) =>
            {
                return true;
            },
        };

        options.DenyLocalLogin = false;

        options.AutoRedirectLoginToExternalProvider = false;
    }
}

I've then logged in as both backoffice and members using Google accounts, and verified when I add and remove the Google authentication in Program.cs the expected database updates are made:

builder.CreateUmbracoBuilder()
    .AddBackOffice()
    .AddWebsite()
    .AddDeliveryApi()
    .AddComposers()
    //.AddGoogleAuthentication()
    .Build();

Note that the main feature has already been tested via the original PR, so focus here can just be on behaviour with members.

…rnal login providers.

Ensure to avoid removing logins for members.
@Copilot Copilot AI review requested due to automatic review settings June 9, 2025 07:10
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes issues related to the removal of external logins when an external login provider is removed from configuration, ensuring that logins and tokens for backoffice users are processed correctly while member logins remain unaffected.

  • Fixes deletion order to prevent foreign key constraint exceptions by deleting tokens before logins.
  • Adds a helper method, DeleteExternalLogins, to centralize deletion logic.
Comments suppressed due to low confidence (1)

src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs:120

  • [nitpick] The parameter name 'ids' is a bit ambiguous; consider renaming it to 'externalLoginIds' for improved clarity.
private void DeleteExternalLogins(List<int> ids)

@AndyButland AndyButland changed the title Fix issues with removal of user logins on change to external login provider configuration Fix issues with removal of user logins on change to external login provider configuration (13) Jun 9, 2025
Copy link
Contributor

@kjac kjac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as advertised 😄

@kjac kjac merged commit 28570b4 into release/13.9.1 Jun 10, 2025
17 checks passed
@kjac kjac deleted the v13/bugfix/fix-fk-exception-on-delete-external-user-token branch June 10, 2025 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants