Skip to content

Invalidate external login session on removal of provider (13)#19273

Merged
kjac merged 10 commits intov13/devfrom
v13/bugfix/invalidate-external-login-tokens-on-provider-change
May 9, 2025
Merged

Invalidate external login session on removal of provider (13)#19273
kjac merged 10 commits intov13/devfrom
v13/bugfix/invalidate-external-login-tokens-on-provider-change

Conversation

@AndyButland
Copy link
Contributor

@AndyButland AndyButland commented May 8, 2025

Prerequisites

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

Description

This request has come up through testing on Umbraco Cloud relating to the external login provider feature they provide, that we'd ideally resolve in the CMS.

The raised issue is that if an external login provider is removed, sessions already created for user that logged in when the provider was available, are still valid and they continue logged into the backoffice.

So I've tackled this by:

  • Tracking the Umbraco key value table the external login providers installed (similar to how we track the NuCache serializer and the URL providers in 15+),
  • If on start-up it's detected as different we now:
    • Invalidate the security stamps on all users that have an external login record with that providers.
    • Delete all the external logins associated with the removed providers.

Just invalidating the security stamp won't immediately log the user out. Rather there's an interval set by ASPNetCore.Identity before the security stamp is validated, and if incorrect, the user is logged out.

You can configure this to a lower value in code.

Testing

First step is to set up an external login provider. I've used Google, following the instructions in the documentation.

Then start the web application. Verify the names of the registered external login providers are set in the key value table:

select * from umbracoKeyValue where [key] = 'Umbraco.Cms.Web.BackOffice.Security.BackOfficeExternalLoginProviders'

Sign in using that external login provider to create an Umbraco user. Verify you have expected values in the following tables:

select id, securityStampToken from umbracoUser where userLogin = '<email>'
select * from umbracoExternalLogin

Configure a lower value for the security stamp validation interval by adding this to Program.cs (so that testing is quicker):

builder.Services.Configure<SecurityStampValidatorOptions>(o => o.ValidationInterval = TimeSpan.FromMinutes(2));

Remove the external login provider by commenting it out of your start-up code, e.g. in Program.cs:

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

Restart the web application.

Verify the value in the key value table is now empty, the user's security stamp has been invalided (set to a string of zeros) and the external login record for the user has been removed.

select id, securityStampToken from umbracoUser where userLogin = '<email>'
select * from umbracoExternalLogin

Wait for the security stamp validation interval to pass you should find you are logged out (should be automatically, but certainly after a page refresh).

Remove external logins for providers no longer registered.
Expire sessions associated with removed external login providers.
Copilot AI review requested due to automatic review settings May 8, 2025 08:22
Copy link
Contributor

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 implements a feature to invalidate backoffice sessions when an external login provider is removed, ensuring that tokens issued with a now-defunct provider are no longer valid. Key changes include:

  • Adding new tests to validate the GUID-to-user ID conversion.
  • Implementing methods and services to invalidate sessions and delete external login records for removed providers.
  • Integrating the invalidation logic into the application startup through a notification handler.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/…/UserRepositoryTests.cs and IntExtensionsTests.cs Added tests to verify conversion between GUIDs and integer user IDs.
src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs Updated to use a default security stamp validation interval from SecuritySettings.
src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs and BackOfficeExternalLoginProviders.cs Introduced a method to invalidate sessions if external login providers change and enhanced dependency injection.
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs Added logic to invalidate sessions for removed providers by converting external login GUIDs to integer IDs.
Other files (Repositories, Services, Configuration) Added corresponding methods and interfaces for deleting removed provider records, and wired up notification handlers to trigger the invalidation at startup.

@AndyButland AndyButland added area/backend status/needs-docs Requires new or updated documentation labels May 8, 2025
@kjac kjac self-assigned this May 8, 2025
@AndyButland AndyButland removed the status/needs-docs Requires new or updated documentation label May 8, 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.

Good stuff @AndyButland 👍

The PR works as advertised. I've left a few comments for consideration.

The only other thing I can think of is: Are external login providers for members affected by all this as well? If that's the case, we should probably queue a separate task to handle members.

@AndyButland
Copy link
Contributor Author

Are external login providers for members affected by all this as well? If that's the case, we should probably queue a separate task to handle members.

The Cloud feature that triggered this request only supports backoffice logins, so I think this is sufficient. If it comes up in another context we can look further for members.

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.

Looks great 👍 I'll enable auto-merge now

@kjac kjac enabled auto-merge (squash) May 9, 2025 10:10
@kjac kjac merged commit 3d44a6f into v13/dev May 9, 2025
18 of 19 checks passed
@kjac kjac deleted the v13/bugfix/invalidate-external-login-tokens-on-provider-change branch May 9, 2025 10:22
@AndyButland AndyButland changed the title Invalidate external login session on removal of provider Invalidate external login session on removal of provider (13) May 9, 2025
@chrisbrasington
Copy link

I may have an issue that brought down our test environment entirely due to a change at v13.9.0.

An error occurred while starting the application.

SqlException: The DELETE statement conflicted with the REFERENCE constraint "FK_umbracoExternalLoginToken_umbracoExternalLogin_id". The conflict occurred in database "c4yevjkcppp", table "dbo.umbracoExternalLoginToken", column 'externalLoginId'.
The statement has been terminated.

I was a bit surprised to bring down both the front and back-end at this minor version. I would be ok correcting this in the database directly, but is it related to this PR?

For context, I use okta SSO at the backoffice and keycloak SSO on the front-end.

@AndyButland
Copy link
Contributor Author

Yes, we've had this raised as #19499, which has #19512 proposed as a fix that will be released as a 13.9.1 patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants