From 3163e107221d14ff803cc2beaa904fc13abf4589 Mon Sep 17 00:00:00 2001 From: cryptalith Date: Wed, 2 Sep 2026 18:21:02 -0600 Subject: [PATCH] PD-8720 mount the interstitial outlet before attaching it The already authorized OAuth path skips the authorization component, so none of the flags gating the card were set when the deferred showInterstitial() ran. The outlet lives inside that card, so the static:false ViewChild was never assigned and attachComponentPortal() threw, leaving the user with a blank page and no redirect to redirect_uri. Latent since a519d01c3 (PD-2371) gave the card an *ngIf; the setTimeout added back when the card was unconditional was no longer waiting for anything. showInterstitial() now mounts the card and runs a change detection pass before attaching, both in one synchronous block so the card is never painted empty, and falls back to the redirect if the outlet still cannot be resolved. The specs could not have caught this: the TestBed omitted PortalModule and every test either stubbed showInterstitial() out or hand-assigned an outlet. --- .../pages/authorize/authorize.component.html | 10 +- .../authorize/authorize.component.spec.ts | 129 ++++++++++++++---- .../pages/authorize/authorize.component.ts | 37 ++++- 3 files changed, 141 insertions(+), 35 deletions(-) diff --git a/src/app/authorize/pages/authorize/authorize.component.html b/src/app/authorize/pages/authorize/authorize.component.html index 40f26c5f5..df53f1715 100644 --- a/src/app/authorize/pages/authorize/authorize.component.html +++ b/src/app/authorize/pages/authorize/authorize.component.html @@ -2,10 +2,12 @@
- + interstitial

', + standalone: true, +}) class DummyInterstitialComponent { + // Set on construction so tests that attach through a real outlet can reach + // the instance the portal built, rather than a hand-made stand-in. + static lastInstance: DummyInterstitialComponent | null = null finish = new Subject() + + constructor() { + DummyInterstitialComponent.lastInstance = this + } } describe('AuthorizeComponent', () => { @@ -84,6 +97,12 @@ describe('AuthorizeComponent', () => { imports: [ HttpClientTestingModule, RouterTestingModule, + // The real AuthorizeModule imports both. Without them cdkPortalOutlet is + // an unknown attribute here, the outlet ViewChild can never resolve, and + // no test can tell a mounted outlet from a missing one — which is how + // PD-8720 shipped green. + PortalModule, + MatCardModule, DummyInterstitialComponent, ], declarations: [AuthorizeComponent], @@ -112,11 +131,24 @@ describe('AuthorizeComponent', () => { }) function createComponent() { + DummyInterstitialComponent.lastInstance = null fixture = TestBed.createComponent(AuthorizeComponent) component = fixture.componentInstance fixture.detectChanges() } + /** + * Renders the authorization card so the `#interstitialOutlet` ViewChild + * resolves against a real CdkPortalOutlet. Hand-assigning `component.outlet` + * instead is what hid PD-8720: it guarantees the one thing that was actually + * undefined in production. + */ + function mountAuthorizationCard() { + component.loading = false + component.showAuthorizationComponent = true + fixture.detectChanges() + } + it('should create', () => { createComponent() expect(component).toBeTruthy() @@ -318,13 +350,7 @@ describe('AuthorizeComponent', () => { ;(component as any).interstitialComponent = DummyInterstitialComponent as any - const finish$ = new Subject() - ;(component as any).outlet = { - attachComponentPortal: () => ({ - instance: { finish: finish$.asObservable() }, - changeDetectorRef: { detectChanges: () => {} }, - }), - } + mountAuthorizationCard() const finishSpy = spyOn( component as any, @@ -334,7 +360,7 @@ describe('AuthorizeComponent', () => { component.handleRedirect('/x') expect(component.showInterstital).toBeTrue() - finish$.next() + DummyInterstitialComponent.lastInstance.finish.next() expect(finishSpy).toHaveBeenCalled() // The dialog path closes on afterClosed(); here `finish` is the only signal @@ -348,18 +374,10 @@ describe('AuthorizeComponent', () => { // card's *ngIf does not also test showInterstital the interstitial is // destroyed in the same tick and the user is left on a blank page. createComponent() - component.loading = false - component.showAuthorizationComponent = true - fixture.detectChanges() + mountAuthorizationCard() expect(fixture.nativeElement.querySelector('mat-card')).toBeTruthy() ;(component as any).interstitialComponent = DummyInterstitialComponent as any - ;(component as any).outlet = { - attachComponentPortal: () => ({ - instance: { finish: new Subject().asObservable() }, - changeDetectorRef: { detectChanges: () => {} }, - }), - } component.handleRedirect('/x') fixture.detectChanges() @@ -367,22 +385,17 @@ describe('AuthorizeComponent', () => { expect(component.showAuthorizationComponent).toBeFalse() expect(component.showInterstital).toBeTrue() expect(fixture.nativeElement.querySelector('mat-card')).toBeTruthy() + expect( + fixture.nativeElement.querySelector('.interstitial-body') + ).toBeTruthy() }) it('handleRedirect: with interstitial -> shows interstitial instead of redirect', () => { createComponent() + mountAuthorizationCard() ;(component as any).interstitialComponent = DummyInterstitialComponent as any - // mock outlet to avoid CDK dependency - const finish$ = new Subject() - ;(component as any).outlet = { - attachComponentPortal: () => ({ - instance: { finish: finish$.asObservable() }, - changeDetectorRef: { detectChanges: () => {} }, - }), - } - spyOn(component as any, 'finishRedirect').and.returnValue(NEVER) component.handleRedirect('/x') @@ -391,6 +404,66 @@ describe('AuthorizeComponent', () => { expect(component.showAuthorizationComponent).toBeFalse() }) + it('ngOnInit: already authorized WITH interstitial -> renders it instead of a blank page', fakeAsync(() => { + // PD-8720. This path skips the authorization component, so nothing mounts + // the card that holds the outlet. showInterstitial() has to mount it + // itself; before it did, the ViewChild was undefined and attaching the + // portal threw, leaving no interstitial and no redirect. + userServiceSpy.getUserSession.and.returnValue( + of({ + loggedIn: true, + oauthSession: { redirectUrl: '/here?code=1', responseType: 'code' }, + } as any) + ) + recordServiceSpy.getRecord.and.returnValue(of({} as any)) + loginInterstitialsSpy.isUserFullyLoaded.and.returnValue(true) + loginInterstitialsSpy.checkLoginInterstitials.and.returnValue( + of(DummyInterstitialComponent as any) + ) + + createComponent() + + // Nothing has mounted the card at this point, which is the state the bug + // was reported in. + expect(component.showAuthorizationComponent).toBeFalse() + expect(fixture.nativeElement.querySelector('mat-card')).toBeFalsy() + + tick() + fixture.detectChanges() + + expect(component.outlet).toBeDefined() + expect(component.showInterstital).toBeTrue() + expect(fixture.nativeElement.querySelector('mat-card')).toBeTruthy() + expect( + fixture.nativeElement.querySelector('.interstitial-body') + ).toBeTruthy() + // The flow waits on the interstitial rather than redirecting past it + expect(windowMock.outOfRouterNavigation).not.toHaveBeenCalled() + })) + + it('showInterstitial: outlet that cannot be mounted redirects instead of blanking', () => { + createComponent() + // `loading` keeps
and the card it holds out of the DOM, so the + // outlet cannot resolve. The user is still owed the redirect_uri. + component.loading = true + fixture.detectChanges() + ;(component as any).interstitialComponent = + DummyInterstitialComponent as any + + const finishSpy = spyOn( + component as any, + 'finishRedirect' + ).and.returnValue(of(true)) + + expect(() => component.handleRedirect('/x')).not.toThrow() + + expect(component.outlet).toBeUndefined() + expect(finishSpy).toHaveBeenCalled() + expect(component.showInterstital).toBeFalse() + // The journey opened by shown() has to close, or it stays open forever + expect(interstitialObservabilitySpy.closed).toHaveBeenCalled() + }) + it('handleRedirect: without interstitial -> calls finishRedirect', () => { createComponent() diff --git a/src/app/authorize/pages/authorize/authorize.component.ts b/src/app/authorize/pages/authorize/authorize.component.ts index 91b5a2d90..d5e6c19a6 100644 --- a/src/app/authorize/pages/authorize/authorize.component.ts +++ b/src/app/authorize/pages/authorize/authorize.component.ts @@ -1,5 +1,11 @@ import { ComponentType } from '@angular/cdk/overlay' -import { Component, Inject, inject, ViewChild } from '@angular/core' +import { + ChangeDetectorRef, + Component, + Inject, + inject, + ViewChild, +} from '@angular/core' import { Observable, forkJoin, of } from 'rxjs' import { filter, @@ -62,7 +68,8 @@ export class AuthorizeComponent { private toglzService: TogglzService, private oauthUrlSessionManger: OauthURLSessionManagerService, private readonly featureLogger: FeatureLoggerService, - private readonly _observability: RumJourneyEventService + private readonly _observability: RumJourneyEventService, + private readonly changeDetectorRef: ChangeDetectorRef ) { this.log = this.featureLogger.scoped('Auth Component') } @@ -179,8 +186,30 @@ export class AuthorizeComponent { /** * Displays the interstitial + * + * The outlet lives inside the card, and the card only renders once one of the + * flags in its *ngIf is set. A `static: false` view query is refreshed by + * change detection, so mounting the card and running a pass are both + * preconditions for `outlet` to exist — the already authorized path reaches + * here with none of those flags set, which is what left `outlet` undefined. + * Doing both here keeps mount and attach in the same synchronous block, so + * the card is never painted empty (PD-2371). */ private showInterstitial(): void { + this.showInterstital = true + this.changeDetectorRef.detectChanges() + + if (!this.outlet) { + // Never strand the user on a blank page: the OAuth flow still owes them a + // redirect to redirect_uri, and the journey opened by `shown()` has to be + // closed or it stays open forever. + this.log.error('Interstitial outlet unavailable, redirecting instead') + this.showInterstital = false + this.interstitialObservability.closed() + this.finishRedirect().subscribe() + return + } + const portal = new ComponentPortal(this.interstitialComponent) const componentRef = this.outlet.attachComponentPortal(portal) @@ -197,7 +226,6 @@ export class AuthorizeComponent { componentRef.changeDetectorRef.detectChanges() this.showAuthorizationComponent = false - this.showInterstital = true } /** @@ -240,6 +268,9 @@ export class AuthorizeComponent { this.loading = false this.redirectUrl = this.oauthSession.redirectUrl trace.push('already authorized with interstitial → show interstitial') + // Deferred because `finalize` runs synchronously inside change + // detection when the source observables are, and `showInterstitial` + // runs a pass of its own. setTimeout(() => this.showInterstitial()) } else { trace.push('already authorized without interstitial → redirect now')