PD-8720 mount the interstitial outlet before attaching it

src/app/authorize/pages/authorize/authorize.component.html

@@ -2,10 +2,12 @@
   <div class="container">
     <div class="row space-around">
       <div class="col m6 s4 l6">
-        <!-- showInterstital keeps this card mounted after showInterstitial()
-             clears showAuthorizationComponent. The portal outlet below lives
-             inside the card, so without it the interstitial is attached and
-             then destroyed in the same tick, leaving a blank page. -->
+        <!-- The portal outlet below lives inside this card, so showInterstitial()
+             sets showInterstital to mount it before attaching, and this keeps the
+             card mounted afterwards when showAuthorizationComponent is cleared.
+             Without it the interstitial has nowhere to attach on the already
+             authorized path, and is destroyed in the same tick on the other one —
+             both leaving a blank page. -->
         <mat-card
           *ngIf="
             showAuthorizationComponent ||

src/app/authorize/pages/authorize/authorize.component.spec.ts

@@ -7,6 +7,8 @@ import {
 import { HttpClientTestingModule } from '@angular/common/http/testing'
 import { RouterTestingModule } from '@angular/router/testing'
 import { CUSTOM_ELEMENTS_SCHEMA, Component } from '@angular/core'
+import { PortalModule } from '@angular/cdk/portal'
+import { MatCardModule } from '@angular/material/card'
 import { of, Subject, NEVER, throwError } from 'rxjs'
 
 import { WINDOW } from '../../../cdk/window'
@@ -22,10 +24,21 @@ import { InterstitialObservabilityService } from 'src/app/core/login-interstitia
 
 import { AuthorizeComponent } from './authorize.component'
 
-// Dummy interstitial component used for typing purposes in tests
-@Component({ template: '', standalone: true })
+// Dummy interstitial component used for typing purposes in tests. It renders a
+// marker so tests can tell "attached to the outlet" from "the flag was set".
+@Component({
+  template: '<p class="interstitial-body">interstitial</p>',
+  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<void>()
+
+  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()
@@ -324,13 +356,7 @@ describe('AuthorizeComponent', () => {
     ;(component as any).interstitialComponent =
       DummyInterstitialComponent as any
 
-    const finish$ = new Subject<void>()
-    ;(component as any).outlet = {
-      attachComponentPortal: () => ({
-        instance: { finish: finish$.asObservable() },
-        changeDetectorRef: { detectChanges: () => {} },
-      }),
-    }
+    mountAuthorizationCard()
 
     const finishSpy = spyOn<any>(
       component as any,
@@ -340,7 +366,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
@@ -354,41 +380,28 @@ 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<void>().asObservable() },
-        changeDetectorRef: { detectChanges: () => {} },
-      }),
-    }
 
     component.handleRedirect('/x')
     fixture.detectChanges()
 
     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<void>()
-    ;(component as any).outlet = {
-      attachComponentPortal: () => ({
-        instance: { finish: finish$.asObservable() },
-        changeDetectorRef: { detectChanges: () => {} },
-      }),
-    }
-
     spyOn<any>(component as any, 'finishRedirect').and.returnValue(NEVER)
 
     component.handleRedirect('/x')
@@ -397,6 +410,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 <main> 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<any>(
+      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()
 

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')