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() @@ -324,13 +356,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, @@ -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,18 +380,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() @@ -373,22 +391,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') @@ -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
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')