Umbraco 9 - LoginProvider Duplicate Key

[cleversolutions]

Which exact Umbraco version are you using? For example: 8.13.1 - don't just write v8

9 rc1

Bug summary

When login from external provider, the second login you add throws an exception Cannot insert duplicate key row in object 'dbo.umbracoExternalLogin' with unique index 'IX_umbracoExternalLogin_LoginProvider'. I believe this index should be non-unique.

Specifics

I've added an oidc external login provider to v9-rc1 so creatively called "Umbraco.oidc". It works great when the first user signs up; however it pops an exception when he second user signs up or connects their account to the external provider.

I'm pretty sure LoginProvider column in umbracoExternalLogin table should not have a unique index on it.

This is the exception

SqlException: Cannot insert duplicate key row in object 'dbo.umbracoExternalLogin' with unique index 'IX_umbracoExternalLogin_LoginProvider'. The duplicate key value is (Umbraco.oidc). The statement has been terminated.
System.Data.SqlClient.SqlConnection.OnError(SqlException exception, bool breakConnection, Action<Action> wrapCloseInAction)
System.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, bool breakConnection, Action<Action> wrapCloseInAction)
System.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, bool callerHasConnectionLock, bool asyncClose)
System.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, out bool dataReady)
System.Data.SqlClient.TdsParser.Run(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj)
System.Data.SqlClient.SqlBulkCopy.RunParser(BulkCopySimpleResultSet bulkCopyHandler)
System.Data.SqlClient.SqlBulkCopy.CopyBatchesAsyncContinuedOnSuccess(BulkCopySimpleResultSet internalResults, string updateBulkCommandText, CancellationToken cts, TaskCompletionSource<object> source)
System.Data.SqlClient.SqlBulkCopy.CopyBatchesAsyncContinued(BulkCopySimpleResultSet internalResults, string updateBulkCommandText, CancellationToken cts, TaskCompletionSource<object> source)
System.Data.SqlClient.SqlBulkCopy.CopyBatchesAsync(BulkCopySimpleResultSet internalResults, string updateBulkCommandText, CancellationToken cts, TaskCompletionSource<object> source)
System.Data.SqlClient.SqlBulkCopy.WriteToServerInternalRestContinuedAsync(BulkCopySimpleResultSet internalResults, CancellationToken cts, TaskCompletionSource<object> source)
System.Data.SqlClient.SqlBulkCopy.WriteToServerInternalRestAsync(CancellationToken cts, TaskCompletionSource<object> source)
System.Data.SqlClient.SqlBulkCopy.WriteToServerInternalAsync(CancellationToken ctoken)
System.Data.SqlClient.SqlBulkCopy.WriteRowSourceToServerAsync(int columnCount, CancellationToken ctoken)
System.Data.SqlClient.SqlBulkCopy.WriteToServer(DataTable table, DataRowState rowState)
System.Data.SqlClient.SqlBulkCopy.WriteToServer(DataTable table)
NPoco.SqlBulkCopyHelper.BulkInsert<T>(IDatabase db, IEnumerable<T> list, SqlBulkCopyOptions sqlBulkCopyOptions)
NPoco.SqlBulkCopyHelper.BulkInsert<T>(IDatabase db, IEnumerable<T> list)
NPoco.DatabaseTypes.SqlServerDatabaseType.InsertBulk<T>(IDatabase db, IEnumerable<T> pocos)
NPoco.Database.InsertBulk<T>(IEnumerable<T> pocos)
Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement.ExternalLoginRepository.Save(int userId, IEnumerable<IExternalLogin> logins)
Umbraco.Cms.Core.Services.Implement.ExternalLoginService.Save(int userId, IEnumerable<IExternalLogin> logins)
Umbraco.Cms.Core.Security.BackOfficeUserStore.UpdateAsync(BackOfficeIdentityUser user, CancellationToken cancellationToken)
Microsoft.AspNetCore.Identity.UserManager<TUser>.UpdateUserAsync(TUser user)
Microsoft.AspNetCore.Identity.UserManager<TUser>.AddLoginAsync(TUser user, UserLoginInfo login)
Umbraco.Cms.Web.BackOffice.Security.BackOfficeSignInManager.LinkUser(BackOfficeIdentityUser autoLinkUser, ExternalLoginInfo loginInfo)
Umbraco.Cms.Web.BackOffice.Security.BackOfficeSignInManager.AutoLinkAndSignInExternalAccount(ExternalLoginInfo loginInfo, ExternalSignInAutoLinkOptions autoLinkOptions)
Umbraco.Cms.Web.BackOffice.Security.BackOfficeSignInManager.ExternalLoginSignInAsync(ExternalLoginInfo loginInfo, bool isPersistent, bool bypassTwoFactor)
Umbraco.Cms.Web.BackOffice.Controllers.BackOfficeController.ExternalSignInAsync(ExternalLoginInfo loginInfo, Func<IActionResult> response)
Umbraco.Cms.Web.BackOffice.Controllers.BackOfficeController.RenderDefaultOrProcessExternalLoginAsync(AuthenticateResult authenticateResult, Func<IActionResult> defaultResponse, Func<IActionResult> externalSignInResponse)
Umbraco.Cms.Web.BackOffice.Controllers.BackOfficeController.Default()
Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor+TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, object controller, object[] arguments)
System.Threading.Tasks.ValueTask<TResult>.get_Result()
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask<IActionResult> actionResultValueTask)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, object state, bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|24_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, object state, bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
Umbraco.Cms.Web.Website.Middleware.PublicAccessMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+<>c__DisplayClass6_1+<<UseMiddlewareInterface>b__1>d.MoveNext()
Umbraco.Cms.Web.BackOffice.Middleware.BackOfficeExternalLoginProviderErrorMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+<>c__DisplayClass6_1+<<UseMiddlewareInterface>b__1>d.MoveNext()
Microsoft.AspNetCore.Builder.Extensions.MapMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
SixLabors.ImageSharp.Web.Middleware.ImageSharpMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
StackExchange.Profiling.MiniProfilerMiddleware.Invoke(HttpContext context) in C:\projects\dotnet\src\MiniProfiler.AspNetCore\MiniProfilerMiddleware.cs
Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+<>c__DisplayClass6_1+<<UseMiddlewareInterface>b__1>d.MoveNext()
Umbraco.Cms.Web.Common.Middleware.PreviewAuthenticationMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+<>c__DisplayClass6_1+<<UseMiddlewareInterface>b__1>d.MoveNext()
Umbraco.Cms.Web.Common.Middleware.UmbracoRequestLoggingMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+<>c__DisplayClass6_1+<<UseMiddlewareInterface>b__1>d.MoveNext()
Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

Steps to reproduce

This is hard to reproduce, you would need an OAuth and OIDC server. Likely easier if you just enable Google Authentication OAuth 2. In my case I'm connecting to our own OIDC/OAuth2 server using an extension to the IUmbracoBuilder as below

using Microsoft.AspNetCore.Authentication.Cookies;
using Microsoft.AspNetCore.Authentication.OpenIdConnect;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.IdentityModel.Protocols.OpenIdConnect;
using Umbraco.Cms.Core.DependencyInjection;
using Umbraco.Cms.Web.BackOffice.Security;
using Umbraco.Extensions;
using System.Threading.Tasks;

public static class AuthenticationServerExtensions
{
    public static IUmbracoBuilder AddAuthenticationServer(this IUmbracoBuilder builder)
    {
        var authServerOptions = new AuthenticationServerOptions();
        builder.Config.GetSection(AuthenticationServerOptions.AuthenticationServer).Bind(authServerOptions);

        // create the autoLinkOptions with the OnAutoLinking action set
        var autoLinkOptions = new ExternalSignInAutoLinkOptions(
            autoLinkExternalAccount: true,
            defaultUserGroups: new string[] {"editor"}
        );
        var autoLinkAction = new AutoLinkAction();
        autoLinkOptions.OnAutoLinking = autoLinkAction.Action;

        builder.AddBackOfficeExternalLogins(backOfficeLoginOPtions =>
        {
            backOfficeLoginOPtions.AddBackOfficeLogin(new BackOfficeExternalLoginProviderOptions(
                "btn-success",
                "fa-cloud",
                autoLinkOptions,
                denyLocalLogin: authServerOptions.DenyLocalLogin,
                autoRedirectLoginToExternalProvider: authServerOptions.AutoRedirectLoginToExternalProvider
            ), backOfficeAuthOptions =>
            {
                backOfficeAuthOptions.AddOpenIdConnect("Umbraco.oidc", "City Account", openIdOptions =>
                {
                    // use cookies
                    openIdOptions.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme;

                    // pass configured options along
                    openIdOptions.Authority = authServerOptions.Authority;
                    openIdOptions.ClientId = authServerOptions.ClientId;
                    openIdOptions.ClientSecret = authServerOptions.ClientSecret;

                    // Use the authorization code flow
                    openIdOptions.ResponseType = OpenIdConnectResponseType.Code;
                    openIdOptions.AuthenticationMethod = OpenIdConnectRedirectBehavior.RedirectGet;

                    // map claims
                    openIdOptions.TokenValidationParameters.NameClaimType = "name";
                    openIdOptions.TokenValidationParameters.RoleClaimType = "role";
                    
                    openIdOptions.RequireHttpsMetadata = true;
                    openIdOptions.GetClaimsFromUserInfoEndpoint = true;
                    openIdOptions.SaveTokens = true;

                    // add scopes
                    openIdOptions.Scope.Add("openid");
                    openIdOptions.Scope.Add("email");
                    openIdOptions.Scope.Add("roles");

                    // options.SecurityTokenValidator = new JwtSecurityTokenHandler
                    // {
                    //     // Disable the built-in JWT claims mapping feature.
                    //     InboundClaimTypeMap = new Dictionary<string, string>()
                    // }; 

                    openIdOptions.UsePkce = true;

                    openIdOptions.Events.OnAuthorizationCodeReceived = (AuthorizationCodeReceivedContext context) => {
                        return Task.CompletedTask;
                    };

                    openIdOptions.Events.OnTokenValidated = (TokenValidatedContext) => {
                        return Task.CompletedTask;
                    };

                    openIdOptions.Events.OnUserInformationReceived = (UserInformationReceivedContext ctx) => {
                        return Task.CompletedTask;
                    };
                });
            });
        });
        return builder;
    }
}

Invoke in startup by adding AddAuthenticationServer() for example

services.AddUmbraco(_env, _config)
                .AddBackOffice()
                .AddWebsite()
                .AddComposers()
                .AddAuthenticationServer()
                .Build();

Link up one user. It should work great. Link up a second user and you get the exception.

Expected result / actual result

It shouldn't toss an exception.

[Shazwazza]

Yes I think this is a bug, the unique index needs to be across login provider and user id I think.

[cleversolutions]

That makes sense. I’ll submit a PR tomorrow.

[keithswright]

Hi Guys,

Thanks for fixing this issue. I just came across it today myself.
However, when I checkout the source on the v9/dev branch, I don't see the fix.

I think something went wrong with the merge:
Branch cleversolutions/temp-10656 contained the changes to fix this issue
A new branch cleversolutions/temp-10674 was branched off from cleversolutions/temp-10656 and a new commit (acda770 ) was added to remove the changes made on cleversolutions/temp-10656.
Because both branches have been merged now to v9/dev, including the commit that reverts the changes on cleversolutions/temp-10674, the fix has been reverted out.

My Apologies if I have missed something obvious and this is all nonsense.

[Shazwazza]

The PR fix was incorporated into this PR #10670 (not the v9/dev branch)

[keithswright]

Yes, and looking at that PR on github, it looks fine.

But the PR was merged into the v9/dev branch. And when I checkout the head of the V9/dev branch and look at the code, I see the code from some, but not all of the commits in the PR.

The changes made to add comments (0ca4dd1) are on v9/dev:

But the changes to the index (d4760f5, d045003) and the column length are not on V9/dev:

And I believe its because this commit reverts the changes to the index and column length: acda770

[cleversolutions]

🤦‍♂️ That is exactly what happened. If Linus were reading this he would call me a dumb git lol. @Shazwazza will #10721 re-add this code, or should I submit a new PR?
Thanks,
Evan

[Shazwazza]

yes, I fixed up the changes and the correct values should be there, but please review. For example:

https://github.com/umbraco/Umbraco-CMS/pull/10721/files#diff-3fdcbcc46f9fdf61a779626a8bba119f52dad113ce227e0614b196fe71731b55

and

https://github.com/umbraco/Umbraco-CMS/pull/10721/files#diff-3d0ac4f8fe380fb06bfadd3bb4936a06c9d6ecfa7a4ebee02468711c9ca4d4b9

[cleversolutions]

I see my changes are still in there. In fact while I was reviewing the PR it got merged 😄 @keithswright if you look at v9/dev now you will see the fixes are there. I think now is a great time for me to pull v9/dev and validate that our custom external login provider still works. H5YR @Shazwazza!

[Shazwazza]

My PR #10670 has just been merged to v9/dev so please test, feedback, etc...

There is a breaking change in how setting up an external provider is done now which you will see. External provider options now correctly use IOptions so you can configure their options separately, via config, and in all the ways that IOptions work.

[keithswright]

Thanks for your help guys.
Sorry to pollute this thread, but while I have you... This issue came up for me because I am trying to implement external login for members. It is a hacky solution at the moment, but it is working for us while we build other features. I was hoping the official external member login will be built in V9 by the time we go live (end of 2021). Do you guys think this might happen within this time frame? Because if not, I may have to put more effort towards building a more robust member external login ourselves.

[Shazwazza]

Unfortunately external login providers for members will probably not make it to 9.0.0 release since that feature isn't actually part of the CMS in v8. My own library called UmbracoIdentity filled this gap for v8 but that will be retired since this functionality will become directly available in v9.x at some stage.

That said, it should be more than possible to make this work on your own since Umbraco 9.0.0 comes with all of the identity requirements for external logins: IMemberSignInManager/MemberSignInManager, IMemberManager/MemberManager. Most of the concepts can be ported over from the aspnetcore examples for external logins (which are based on sign in manager and user manager, but are backed by EFCore providers... but the underlying provider will not matter).