-
Notifications
You must be signed in to change notification settings - Fork 2.9k
External login: wait for app-entry-points before the login provider decision #23167
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
leekelleher
merged 3 commits into
release/17.5.0
from
v17/bugfix/external-login-entrypoint-race
Jun 19, 2026
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
102e4aa
External login: wait for app-entry-points before the login provider d…
iOvergaard ea78147
test(backoffice): guard the loaded-gate timing for permission loading
iOvergaard c566dd0
fix(backoffice): harden loaded signal + narrow acceptance test glob (…
iOvergaard File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
106 changes: 106 additions & 0 deletions
106
...raco.Web.UI.Client/src/libs/extension-api/initializers/extension-initializer-base.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| import type { ManifestBase } from '../types/index.js'; | ||
| import { UmbExtensionRegistry } from '../registry/extension.registry.js'; | ||
| import { loadManifestPlainJs } from '../functions/load-manifest-plain-js.function.js'; | ||
| import { UmbExtensionInitializerBase } from './extension-initializer-base.js'; | ||
| import { UmbObserver } from '../../observable-api/observer.js'; | ||
| import { expect, fixture } from '@open-wc/testing'; | ||
| import { UmbControllerHostElementMixin } from '@umbraco-cms/backoffice/controller-api'; | ||
| import type { UmbControllerHostElement } from '@umbraco-cms/backoffice/controller-api'; | ||
| import { customElement, html } from '@umbraco-cms/backoffice/external/lit'; | ||
|
|
||
| @customElement('umb-test-initializer-base-host') | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| class UmbTestInitializerBaseHostElement extends UmbControllerHostElementMixin(HTMLElement) {} | ||
|
|
||
| async function wait(ms: number) { | ||
| await new Promise((r) => setTimeout(r, ms)); | ||
| } | ||
|
|
||
| // Factory for a concrete initializer over the 'test' manifest type. The base constructor's | ||
| // `observe` callback fires synchronously during `super()` — before any subclass field would | ||
| // initialise — so the record of instantiated aliases is a closed-over array created up front | ||
| // rather than instance state. | ||
| function createTestInitializer(host: UmbControllerHostElement, registry: UmbExtensionRegistry<ManifestBase>) { | ||
| const instantiated: string[] = []; | ||
| class UmbTestInitializer extends UmbExtensionInitializerBase<'test'> { | ||
| constructor() { | ||
| super(host, registry as never, 'test'); | ||
| } | ||
| async instantiateExtension(manifest: ManifestBase & { js?: unknown }): Promise<void> { | ||
| if (manifest.js) { | ||
| await loadManifestPlainJs(manifest.js as never); | ||
| } | ||
| instantiated.push(manifest.alias); | ||
| } | ||
| unloadExtension(manifest: ManifestBase): void { | ||
| const index = instantiated.indexOf(manifest.alias); | ||
| if (index !== -1) instantiated.splice(index, 1); | ||
| } | ||
| } | ||
| return { initializer: new UmbTestInitializer(), instantiated }; | ||
| } | ||
|
|
||
| describe('UmbExtensionInitializerBase — loaded signal', () => { | ||
| let hostElement: UmbControllerHostElement; | ||
|
|
||
| beforeEach(async () => { | ||
| hostElement = await fixture(html`<umb-test-initializer-base-host></umb-test-initializer-base-host>`); | ||
| }); | ||
|
|
||
| // Regression for the v17.4+ external-login race (introduced in #22522). | ||
| // | ||
| // A default Umbraco install registers ZERO app-entry-point extensions. The boot sequence | ||
| // awaits the app-entry-point initializer's `loaded` before deciding which login provider | ||
| // to use. If `loaded` never resolves when there are no matching extensions, that await | ||
| // hangs forever — which is precisely why the await was removed, leaving externally | ||
| // registered auth providers un-awaited and the login flow racing on slow connections. | ||
| // | ||
| // So: an initializer for a type with zero matching extensions MUST still resolve `loaded`. | ||
| it('resolves `loaded` even when no extensions of the type are registered', async () => { | ||
| const extensionRegistry = new UmbExtensionRegistry<ManifestBase>(); | ||
| const { initializer } = createTestInitializer(hostElement, extensionRegistry); | ||
|
|
||
| const outcome = await Promise.race([ | ||
| new UmbObserver(initializer.loaded).asPromise().then(() => 'resolved'), | ||
| wait(1000).then(() => 'timeout'), | ||
| ]); | ||
|
|
||
| expect(outcome, '`loaded` must resolve for an initializer with zero matching extensions').to.equal('resolved'); | ||
| }); | ||
|
|
||
| // Regression for the late-loading race that the external-login bug is built on. | ||
| // | ||
| // This simulates an extension that registers AFTER the initial load and whose | ||
| // instantiation is slow (the app-entry-point case: its onInit registers an auth provider | ||
| // after an async module load). A consumer that awaits `loaded` must not be told "loaded" | ||
| // until that late, slow extension has actually finished instantiating — otherwise it makes | ||
| // its decision (e.g. which login provider to redirect to) against a stale registry. | ||
| it('does not report `loaded` until a late-registered, slow extension has finished instantiating', async () => { | ||
| const extensionRegistry = new UmbExtensionRegistry<ManifestBase>(); | ||
|
|
||
| // Initial, fast extension — load settles to `true`. | ||
| extensionRegistry.register({ type: 'test', name: 'a', alias: 'Umb.Test.A' } as never); | ||
| const { initializer, instantiated } = createTestInitializer(hostElement, extensionRegistry); | ||
| await new UmbObserver(initializer.loaded).asPromise(); | ||
| expect(instantiated, 'initial extension instantiated').to.eql(['Umb.Test.A']); | ||
|
|
||
| // A late, slow extension registers (mirrors an app-entry-point's onInit registering an | ||
| // auth provider after an async delay). | ||
| extensionRegistry.register({ | ||
| type: 'test', | ||
| name: 'b-late', | ||
| alias: 'Umb.Test.B.Late', | ||
| js: () => new Promise((r) => setTimeout(() => r({}), 100)), | ||
| } as never); | ||
|
|
||
| // Awaiting `loaded` now must wait for the late extension to finish instantiating. | ||
| const lateExtInstantiatedWhenLoaded = await new UmbObserver(initializer.loaded) | ||
| .asPromise() | ||
| .then(() => instantiated.includes('Umb.Test.B.Late')); | ||
|
|
||
| expect( | ||
| lateExtInstantiatedWhenLoaded, | ||
| '`loaded` resolved before the late, slow extension finished instantiating', | ||
| ).to.be.true; | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
30 changes: 30 additions & 0 deletions
30
.../AuthProviderLateRegistration/AdditionalSetup/App_Plugins/LateAuthProvider/entry-point.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| // Test fixture: an appEntryPoint that registers an external auth provider LATE — i.e. after | ||
| // an async delay inside onInit — mirroring a real provider (e.g. Umbraco ID) whose onInit | ||
| // registers its authProvider after fetching/initialising on a slow connection. | ||
| // | ||
| // The backoffice boot must wait for app-entry-points to settle before deciding which login | ||
| // provider to use. If it doesn't, the login screen renders before this provider is registered | ||
| // and the late provider never appears (the v17.4+ regression). The delay makes that race | ||
| // deterministic. | ||
|
|
||
| const LATE_REGISTRATION_DELAY_MS = 1500; | ||
|
|
||
| export const onInit = async (_host, extensionRegistry) => { | ||
| await new Promise((resolve) => setTimeout(resolve, LATE_REGISTRATION_DELAY_MS)); | ||
|
|
||
| extensionRegistry.register({ | ||
| type: 'authProvider', | ||
| alias: 'Test.LateAuthProvider', | ||
| name: 'Late External Login', | ||
| forProviderName: 'Umbraco.LateTest', | ||
| meta: { | ||
| label: 'Late External Login', | ||
| defaultView: { | ||
| icon: 'icon-cloud', | ||
| }, | ||
| behavior: { | ||
| autoRedirect: false, | ||
| }, | ||
| }, | ||
| }); | ||
| }; |
12 changes: 12 additions & 0 deletions
12
...roviderLateRegistration/AdditionalSetup/App_Plugins/LateAuthProvider/umbraco-package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| { | ||
| "name": "Late Auth Provider (test)", | ||
| "allowPublicAccess": true, | ||
| "extensions": [ | ||
| { | ||
| "type": "appEntryPoint", | ||
| "alias": "Test.LateAuthProvider.EntryPoint", | ||
| "name": "Late Auth Provider Entry Point", | ||
| "js": "/App_Plugins/LateAuthProvider/entry-point.js" | ||
| } | ||
| ] | ||
| } |
30 changes: 30 additions & 0 deletions
30
.../Umbraco.Tests.AcceptanceTest/tests/AuthProviderLateRegistration/LateAuthProvider.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import {test} from '@umbraco/acceptance-test-helpers'; | ||
| import {expect} from '@playwright/test'; | ||
|
|
||
| // Regression guard for the v17.4+ external-login race (introduced in #22522). | ||
| // | ||
| // AdditionalSetup/App_Plugins/LateAuthProvider deploys an appEntryPoint whose onInit, after a | ||
| // 1.5s delay, registers an external auth provider ("Late External Login"). This mirrors a real | ||
| // provider (e.g. Umbraco ID) that registers its authProvider during an async onInit on a slow | ||
| // connection. | ||
| // | ||
| // The backoffice boot must wait for app-entry-points to settle before rendering the login | ||
| // screen. If it does not, the login decision is made before the provider is registered, the | ||
| // late provider never appears, and the user is dropped on the local login instead. So: the | ||
| // late-registered provider MUST be offered on the login screen. | ||
| // | ||
| // This test is intentionally brittle (it depends on boot timing) but must remain working — it | ||
| // is the only end-to-end guard for the boot-gate behaviour. | ||
| test('a late-registered external auth provider is offered on the login screen', async ({umbracoUi}) => { | ||
| test.slow(); | ||
|
|
||
| // Act - navigate to the backoffice unauthenticated (the login screen). | ||
| await umbracoUi.goToBackOffice(); | ||
|
|
||
| // Assert - the provider registered late by the appEntryPoint is still offered. On the | ||
| // buggy boot the login screen renders before this provider exists, so it never appears. | ||
| const lateProviderButton = umbracoUi.page | ||
| .locator('umb-auth-provider-default') | ||
| .getByText('Sign in with Late External Login'); | ||
| await expect(lateProviderButton).toBeVisible({timeout: 15000}); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.