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

umbracoExternalLoginToken table is outdated with OpenIdConnect for members #12749

Closed
jbreuer opened this issue Jul 29, 2022 · 6 comments
Closed

Comments

@jbreuer
Copy link
Contributor

jbreuer commented Jul 29, 2022

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

9.5.1

Bug summary

When an external login provider with OpenIdConnect is configured for members their tokens are saved in the umbracoExternalLoginToken table. However the tokens are only stored here if they don't exist yet. If you try to login again the tokens in the umbracoExternalLoginToken are not updated.

This problem probably also happens for backend users, but I could not test that.

Specifics

If you try to login with a member that has already been created at some point in MemberUserStore.cs the UpdateAsync method is called. If you retrieved new tokens from the external login provider the isTokensPropertyDirty is true and _externalLoginService.Save is called. After this I expected the new tokens to be stored in the umbracoExternalLoginToken table, but that never happend.

Steps to reproduce

Setup an Umbraco project and follow these instructions to connect to an external login provider with auto linking:
https://our.umbraco.com/documentation/reference/security/external-login-providers/
https://our.umbraco.com/documentation/reference/security/auto-linking/

This is an example of Startup.cs:

namespace UmbracoLogin
{
    using System;
    using System.Linq;
    using System.Security.Claims;
    using System.Threading.Tasks;
    using Microsoft.AspNetCore.Builder;
    using Microsoft.AspNetCore.Hosting;
    using Microsoft.Extensions.Configuration;
    using Microsoft.Extensions.Hosting;
    using Umbraco.Cms.Core.DependencyInjection;
    using Umbraco.Extensions;
    using Microsoft.Extensions.DependencyInjection;
    using Umbraco.Cms.Core.Security;

    public class Startup
    {
        private readonly IWebHostEnvironment _env;
        private readonly IConfiguration _config;

        /// <summary>
        /// Initializes a new instance of the <see cref="Startup" /> class.
        /// </summary>
        /// <param name="webHostEnvironment">The web hosting environment.</param>
        /// <param name="config">The configuration.</param>
        /// <remarks>
        /// Only a few services are possible to be injected here https://github.com/dotnet/aspnetcore/issues/9337.
        /// </remarks>
        public Startup(IWebHostEnvironment webHostEnvironment, IConfiguration config)
        {
            _env = webHostEnvironment ?? throw new ArgumentNullException(nameof(webHostEnvironment));
            _config = config ?? throw new ArgumentNullException(nameof(config));
        }

        /// <summary>
        /// Configures the services.
        /// </summary>
        /// <param name="services">The services.</param>
        /// <remarks>
        /// This method gets called by the runtime. Use this method to add services to the container.
        /// For more information on how to configure your application, visit https://go.microsoft.com/fwlink/?LinkID=398940.
        /// </remarks>
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddUmbraco(_env, _config)
                .AddBackOffice()
                .AddWebsite()
                .AddComposers()
                .AddOpenIdConnectAuthentication()
                .Build();
        }

        /// <summary>
        /// Configures the application.
        /// </summary>
        /// <param name="app">The application builder.</param>
        /// <param name="env">The web hosting environment.</param>
        public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
        {
            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }

            app.UseUmbraco()
                .WithMiddleware(u =>
                {
                    u.UseBackOffice();
                    u.UseWebsite();
                })
                .WithEndpoints(u =>
                {
                    u.UseInstallerEndpoints();
                    u.UseBackOfficeEndpoints();
                    u.UseWebsiteEndpoints();
                });
        }
    }

    public static class StaticStartup
    {   
        public static IUmbracoBuilder AddOpenIdConnectAuthentication(this IUmbracoBuilder builder)
        {
            builder.Services.ConfigureOptions<OpenIdConnectMemberExternalLoginProviderOptions>();

            builder.AddMemberExternalLogins(logins =>
            {
                logins.AddMemberLogin(
                    memberAuthenticationBuilder =>
                    {
                        memberAuthenticationBuilder.AddOpenIdConnect(
                            // The scheme must be set with this method to work for the umbraco members
                            memberAuthenticationBuilder.SchemeForMembers(OpenIdConnectMemberExternalLoginProviderOptions.SchemeName),
                            options =>
                            {
                                options.ResponseMode = "query";
                                options.ResponseType = "code";
                                options.Scope.Add("openid");
                                options.Scope.Add("offline_access");
                                options.Scope.Add("urn:opc:idm:t.user.me");
                                options.RequireHttpsMetadata = true;
                                options.MetadataAddress = "";
                                options.ClientId = "";
                                options.ClientSecret = "";
                                options.SaveTokens = true;
                                options.TokenValidationParameters.SaveSigninToken = true;
                                options.Events.OnTokenValidated = async context =>
                                {
                                    var claims = context?.Principal?.Claims.ToList();
                                    var email = claims?.SingleOrDefault(x => x.Type == ClaimTypes.NameIdentifier);
                                    if (email != null)
                                    {
                                        claims.Add(new Claim(ClaimTypes.Email, email.Value));
                                    }
                                    
                                    var name = claims?.SingleOrDefault(x => x.Type == "user_displayname");
                                    if (name != null)
                                    {
                                        claims.Add(new Claim(ClaimTypes.Name, name.Value));
                                    }

                                    if (context != null)
                                    {
                                        var authenticationType = context.Principal?.Identity?.AuthenticationType;
                                        context.Principal = new ClaimsPrincipal(new ClaimsIdentity(claims, authenticationType));
                                    }

                                    await Task.FromResult(0);
                                }; 
                                options.Events.OnRedirectToIdentityProviderForSignOut = async notification =>
                                {
                                    var memberManager = notification.HttpContext.RequestServices.GetService<IMemberManager>();

                                    if (memberManager != null)
                                    {
                                        var currentMember = await memberManager.GetCurrentMemberAsync();
                                        var currentMemberIdentityUser = (MemberIdentityUser)currentMember;
                                        var loginTokens = currentMemberIdentityUser.LoginTokens;
                                        
                                        // This gets and outdated token. Don't set it for now because we need to correct token.
                                        // notification.ProtocolMessage.IdTokenHint = loginTokens.ToList()[2].Value;
                                        
                                    }
                                    await Task.FromResult(0);
                                };
                            });
                    });
            });
            return builder;
        }
    }
}

This is an example of OpenIdConnectMemberExternalLoginProviderOptions.cs:

namespace UmbracoLogin
{
    using System.Collections.Generic;
    using Microsoft.Extensions.Options;
    using Umbraco.Cms.Web.Common.Security;
    using Umbraco.Cms.Core;

    public class OpenIdConnectMemberExternalLoginProviderOptions : IConfigureNamedOptions<MemberExternalLoginProviderOptions>
    {
        public const string SchemeName = "OpenIdConnect";
        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(
                // Must be true for auto-linking to be enabled
                autoLinkExternalAccount: true,

                // Optionally specify the default culture to create
                // the user as. If null it will use the default
                // culture defined in the web.config, or it can
                // be dynamically assigned in the OnAutoLinking
                // callback.
                defaultCulture: null,

                // Optionally specify the default "IsApprove" status. Must be true for auto-linking.
                defaultIsApproved:true,

                // Optionally specify the member type alias. Default is "Member"
                defaultMemberTypeAlias:"Member",

                // Optionally specify the member groups names to add the auto-linking user to.
                defaultMemberGroups: new List<string> { "Group1" }
            )
            {
                // Optional callback
                OnAutoLinking = (autoLinkUser, loginInfo) =>
                {
                    // 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
                },
                OnExternalLogin = (user, loginInfo) =>
                {   
                    // You can customize the user before it's saved whenever they have
                    // logged in with the external provider.
                    // i.e. Sync the user's name based on the Claims returned
                    // in the externalLogin info

                    return true; //returns a boolean indicating if sign in should continue or not.
                }
            };
        }
    }
}

After you have this setup you can use the Macro's from here to login: https://our.umbraco.com/documentation/Tutorials/Members-Registration-And-Logins/

Now you can login into the external login provider and you will be redirected back to the website where your username will be shown. In the umbracoExternalLoginToken table are now your tokens. Than logout on the website and the external login provider (do that manually for now because we can't set the IdTokenHint yet). If you login again with the same user you will get new tokens from the external login provider, but those are not stored in umbracoExternalLoginToken table. If you try to get the LoginTokens from MemberIdentityUser those are also the old ones.

You can also find more info in this topic: https://our.umbraco.com/forum/using-umbraco-and-getting-started/108415-persist-and-refresh-access-token-after-external-microsoft-b2c-login#comment-337421

Expected result / actual result

I want the logout to also happen on the external login provider. For that I need to set the IdTokenHint property on OnRedirectToIdentityProviderForSignOut. Currently I can't do that because the id_token I get back from the umbracoExternalLoginToken table is outdated.


This item has been added to our backlog AB#21593

@github-actions
Copy link

Hi there @jbreuer!

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'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
  • If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
  • We'll replicate the issue to ensure that the problem is as described.
  • We'll decide whether the behavior is an issue or if the behavior is intended.

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 🤖 🙂

@jbreuer
Copy link
Contributor Author

jbreuer commented Aug 9, 2022

Has anyone been able to reproduce this? I could give a live demo if anyone is interested.

@lassefredslund lassefredslund added the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Aug 10, 2022
@nul800sebastiaan nul800sebastiaan added this to the sprint195 milestone Aug 10, 2022
@nul800sebastiaan
Copy link
Member

This has been added to sprint planning for next sprint, we'll do our best to have a look in the next two weeks.

@jbreuer
Copy link
Contributor Author

jbreuer commented Aug 15, 2022

I've run into another issue. Not sure if it's the same bug, but since it's probably related I'll add the info here.

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

10.1.0

Bug summary

I was trying to do the same setup as from the original bug report, but on a new 10.1.0 project with SQLite. When I try to login I'm redirect to the Identity Provider. After I login there I'm redirected back to Umbraco, but that gives the following error:

SQLite Error 19: 'FOREIGN KEY constraint failed'.

image

Specifics

This is the full error from the logs:

Microsoft.Data.Sqlite.SqliteException (0x80004005): SQLite Error 19: 'FOREIGN KEY constraint failed'.
   at Microsoft.Data.Sqlite.SqliteException.ThrowExceptionForRC(Int32 rc, sqlite3 db)
   at Microsoft.Data.Sqlite.SqliteDataReader.NextResult()
   at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader(CommandBehavior behavior)
   at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader()
   at Microsoft.Data.Sqlite.SqliteCommand.ExecuteNonQuery()
   at Umbraco.Cms.Persistence.Sqlite.Services.SqlitePreferDeferredTransactionsConnection.CommandWrapper.ExecuteNonQuery()
   at StackExchange.Profiling.Data.ProfiledDbCommand.ExecuteNonQuery() in C:\projects\dotnet\src\MiniProfiler.Shared\Data\ProfiledDbCommand.cs:line 281
   at Umbraco.Cms.Infrastructure.Persistence.FaultHandling.FaultHandlingDbCommand.<ExecuteNonQuery>b__32_0()
   at Umbraco.Cms.Infrastructure.Persistence.FaultHandling.FaultHandlingDbCommand.<>c__DisplayClass38_0`1.<Execute>b__0()
   at Umbraco.Cms.Infrastructure.Persistence.FaultHandling.RetryPolicy.ExecuteAction[TResult](Func`1 func)
   at Umbraco.Cms.Infrastructure.Persistence.FaultHandling.FaultHandlingDbCommand.Execute[T](Func`1 f)
   at Umbraco.Cms.Infrastructure.Persistence.FaultHandling.FaultHandlingDbCommand.ExecuteNonQuery()
   at NPoco.Database.<>c__DisplayClass296_0.<ExecuteNonQueryHelper>b__0()
   at NPoco.Database.ExecutionHook[T](Func`1 action)
   at NPoco.Database.ExecuteNonQueryHelper(DbCommand cmd)
   at NPoco.Database.InsertAsyncImp[T](PocoData pocoData, String tableName, String primaryKeyName, Boolean autoIncrement, T poco, Boolean sync)
   at NPoco.AsyncHelper.RunSync[T](Task`1 task)
   at NPoco.Database.Insert[T](String tableName, String primaryKeyName, Boolean autoIncrement, T poco)
   at NPoco.Database.Insert[T](T poco)
   at NPoco.DatabaseType.InsertBulk[T](IDatabase db, IEnumerable`1 pocos, InsertBulkOptions options)
   at NPoco.Database.InsertBulk[T](IEnumerable`1 pocos, InsertBulkOptions options)

Timestamp 2022-08-15T07:59:30.3014175+00:00
@MessageTemplate Exception ({InstanceId}).
InstanceId 7865c0d9
SourceContext Umbraco.Cms.Infrastructure.Persistence.UmbracoDatabase
ActionId 4351ce4e-1744-44b1-9368-d4ab995b85e4
ActionName Umbraco.Cms.Web.Website.Controllers.UmbExternalLoginController.ExternalLoginCallback (Umbraco.Web.Website)
RequestId 800000c3-0002-fa00-b63f-84710c7967bb
RequestPath /login/
ProcessId 9964
ProcessName iisexpress
ThreadId 66
ApplicationId 0521d4839bc6f50f6ff21829f41b9aeff4fb79a6
MachineName 4QG5RQ2
Log4NetLevel ERROR
HttpRequestId 4803ef19-5686-4c97-8e34-aa5fd4f36718
HttpRequestNumber 4
HttpSessionId ea74524a-ef0f-e05a-ebc4-35f00479889d

Steps to reproduce

Follow the steps to reproduce from the original bug on a 10.1.0 site with SQLite. Then try to login to the external provider with a new member.

Expected result / actual result

After I login into the external provider I should also be logged into Umbraco. My updated tokens should be stored in the umbracoExternalLoginToken table.

jbreuer referenced this issue Aug 17, 2022
Might want to experiment with only removing/updating if the value is different
@jbreuer
Copy link
Contributor Author

jbreuer commented Aug 17, 2022

As mentioned by @nikolajlauridsen the SQLite error might not be related: e2f5c93#commitcomment-81412921

That's why I created a separate for it: #12853

@jbreuer
Copy link
Contributor Author

jbreuer commented Aug 22, 2022

Closing this issue since it's fixed in this PR: #12856

@jbreuer jbreuer closed this as completed Aug 22, 2022
@nul800sebastiaan nul800sebastiaan removed the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants