diff --git a/src/app/authorize/pages/authorize/authorize.component.spec.ts b/src/app/authorize/pages/authorize/authorize.component.spec.ts index df788deab..041d8c71e 100644 --- a/src/app/authorize/pages/authorize/authorize.component.spec.ts +++ b/src/app/authorize/pages/authorize/authorize.component.spec.ts @@ -165,6 +165,12 @@ describe('AuthorizeComponent', () => { expect(loginInterstitialsSpy.isUserFullyLoaded).toHaveBeenCalled() expect(loginInterstitialsSpy.checkLoginInterstitials).toHaveBeenCalled() + // The manager only consults the post-registration OAuth flag when it is + // told it is on the OAuth surface, so this prefix is load bearing (PD-12904) + expect(loginInterstitialsSpy.checkLoginInterstitials).toHaveBeenCalledWith( + jasmine.anything(), + { returnType: 'component', togglzPrefix: 'OAUTH' } + ) expect((component as any).interstitialComponent).toBe( DummyInterstitialComponent as any ) diff --git a/src/app/core/login-interstitials-manager/login-main-interstitials-manager.service.ts b/src/app/core/login-interstitials-manager/login-main-interstitials-manager.service.ts index 9ab3228fc..3d2091460 100644 --- a/src/app/core/login-interstitials-manager/login-main-interstitials-manager.service.ts +++ b/src/app/core/login-interstitials-manager/login-main-interstitials-manager.service.ts @@ -20,6 +20,7 @@ import { } from './abstractions/dialog-interface' import { ComponentType } from '@angular/cdk/overlay' import { PlatformInfoService } from 'src/app/cdk/platform-info' +import { OauthURLSessionManagerService } from '../oauth-urlsession-manager/oauth-urlsession-manager.service' @Injectable({ providedIn: 'root', @@ -36,6 +37,7 @@ export class LoginMainInterstitialsManagerService { constructor( private interstitialsService: InterstitialsService, private _platform: PlatformInfoService, + private _oauthUrlSession: OauthURLSessionManagerService, LoginDomainInterstitialManagerService: LoginDomainInterstitialManagerService, LoginAffiliationInterstitialManagerService: LoginAffiliationInterstitialManagerService, LoginBackupEmailInterstitialManagerService: LoginBackupEmailInterstitialManagerService @@ -86,7 +88,7 @@ export class LoginMainInterstitialsManagerService { return EMPTY } - if (this.userJustRegistered()) { + if (this.userJustRegistered(opts.togglzPrefix)) { if (runtimeEnvironment.debugger) { console.info( '[Interstitial Manager] Just registered, not checking interstitials' @@ -186,12 +188,24 @@ export class LoginMainInterstitialsManagerService { /** * A user who has just finished registering has already been through a long - * form and is being shown the verify your email banner, so no interstitial - * should interrupt that. Registration lands here with `justRegistered` on the - * URL; the OAuth branch goes to the authorize page instead and never reaches - * my-orcid carrying the parameter. + * form, so no interstitial should interrupt them — in either flow. + * + * Registration signals this two different ways, because the backend only + * appends the query parameter when there is no saved request target: + * - direct registration lands on my-orcid with `justRegistered` on the URL + * - registering inside an OAuth request goes to the authorize page instead, + * carrying the `oauthJustRegistered` localStorage flag + * + * The localStorage flag is only consulted on the OAuth surface. On my-orcid + * the query parameter already covers the case and that branch never sets the + * flag, so reading it there could only ever act on one left behind by an + * unrelated OAuth flow — suppressing an interstitial that should have shown. + * + * The flag is read non-destructively: `consumeJustRegistered()` is the + * one-time read owned by the RUM journey callers, and the authorize page + * mounts those only after this check, so consuming here would race them. */ - private userJustRegistered(): boolean { + private userJustRegistered(toggglzPrefix: 'OAUTH' | 'LOGIN'): boolean { let justRegistered = false this._platform .get() @@ -200,7 +214,10 @@ export class LoginMainInterstitialsManagerService { justRegistered = platform.queryParameters.hasOwnProperty('justRegistered') }) - return justRegistered + if (justRegistered) { + return true + } + return toggglzPrefix === 'OAUTH' && this._oauthUrlSession.isJustRegistered() } isAccountOwner(userRecord: UserRecord): boolean { diff --git a/src/app/core/login-interstitials-manager/test/login-main-interstitials-manager.service.spec.ts b/src/app/core/login-interstitials-manager/test/login-main-interstitials-manager.service.spec.ts index 270a5f4a1..175bd4b97 100644 --- a/src/app/core/login-interstitials-manager/test/login-main-interstitials-manager.service.spec.ts +++ b/src/app/core/login-interstitials-manager/test/login-main-interstitials-manager.service.spec.ts @@ -12,6 +12,7 @@ import { ShareEmailsDomainsComponentDialogOutput } from 'src/app/cdk/interstitia import { AffilationsComponentDialogOutput } from 'src/app/cdk/interstitials/affiliations-interstitial/interstitial-dialog-extend/affiliations-interstitial-dialog.component' import { BackupEmailComponentDialogOutput } from 'src/app/cdk/interstitials/backup-email/interstitial-dialog-extend/backup-email-dialog.component' import { PlatformInfoService } from 'src/app/cdk/platform-info' +import { OauthURLSessionManagerService } from '../../oauth-urlsession-manager/oauth-urlsession-manager.service' import { PlatformInfo } from 'src/app/cdk/platform-info/platform-info.type' // Mock runtime environment for debugging logs if needed @@ -28,6 +29,7 @@ describe('LoginMainInterstitialsManagerService', () => { let mockLoginAffiliationInterstitialManagerService: jasmine.SpyObj let mockLoginBackupEmailInterstitialManagerService: jasmine.SpyObj let mockPlatformInfoService: jasmine.SpyObj + let oauthUrlSession: OauthURLSessionManagerService // Example valid user const validUserRecord: UserRecord = { @@ -60,6 +62,7 @@ describe('LoginMainInterstitialsManagerService', () => { } as UserRecord beforeEach(() => { + localStorage.removeItem('oauthJustRegistered') // Create spy objects for each dependency mockInterstitialsService = jasmine.createSpyObj( 'InterstitialsService', @@ -106,6 +109,7 @@ describe('LoginMainInterstitialsManagerService', () => { 'getInterstitialTogglz', 'getInterstitialViewed', 'showInterstitialAsDialog', + 'showInterstitialAsComponent', ], { INTERSTITIAL_NAME: 'BACKUP_EMAIL_INTERSTITIAL', @@ -132,6 +136,10 @@ describe('LoginMainInterstitialsManagerService', () => { LoginMainInterstitialsManagerService, { provide: InterstitialsService, useValue: mockInterstitialsService }, { provide: PlatformInfoService, useValue: mockPlatformInfoService }, + // Real service on purpose: it has no constructor dependencies, so the + // OAuth just-registered flag is exercised through actual localStorage + // rather than a spy that could drift from the real read. + OauthURLSessionManagerService, { provide: LoginDomainInterstitialManagerService, useValue: mockLoginDomainInterstitialManagerService, @@ -147,10 +155,12 @@ describe('LoginMainInterstitialsManagerService', () => { ], }) + oauthUrlSession = TestBed.inject(OauthURLSessionManagerService) service = TestBed.inject(LoginMainInterstitialsManagerService) }) afterEach(() => { + localStorage.removeItem('oauthJustRegistered') // Reset calls so each test starts fresh jasmine.clock().uninstall() mockInterstitialsService.checkIfSessionAlreadyCheckedInterstitialsLogic.calls.reset() @@ -628,6 +638,147 @@ describe('LoginMainInterstitialsManagerService', () => { }) }) + it('shows no interstitial in the OAuth flow, where a localStorage flag replaces the query parameter', (done) => { + // Registering inside an OAuth request never reaches my-orcid, so the + // backend never appends `justRegistered`; register.component sets this + // flag instead and the user lands on the authorize page. PD-12904. + mockInterstitialsService.checkIfSessionAlreadyCheckedInterstitialsLogic.and.returnValue( + false + ) + oauthUrlSession.setJustRegistered(true) + // Would otherwise qualify + mockLoginBackupEmailInterstitialManagerService.userIsElegibleForInterstitial.and.returnValue( + of(true) + ) + + service + .checkLoginInterstitials(validUserRecord, { + returnType: 'component', + togglzPrefix: 'OAUTH', + }) + .subscribe({ + next: () => fail('Should not emit any value'), + complete: () => { + expect( + mockLoginBackupEmailInterstitialManagerService.userIsElegibleForInterstitial + ).not.toHaveBeenCalled() + expect( + mockLoginDomainInterstitialManagerService.userIsElegibleForInterstitial + ).not.toHaveBeenCalled() + expect( + mockLoginAffiliationInterstitialManagerService.userIsElegibleForInterstitial + ).not.toHaveBeenCalled() + expect( + mockInterstitialsService.markCurrentSessionToNoCheckInterstitialsLogic + ).toHaveBeenCalled() + done() + }, + }) + }) + + it('still suppresses after the flag was consumed destructively by another reader', (done) => { + // form-authorize and oauth-error consume this flag for RUM context. The + // authorize page renders those only after the interstitial check, but the + // gate must not depend on that ordering. + mockInterstitialsService.checkIfSessionAlreadyCheckedInterstitialsLogic.and.returnValue( + false + ) + oauthUrlSession.setJustRegistered(true) + expect(oauthUrlSession.consumeJustRegistered()).toBeTrue() + expect(localStorage.getItem('oauthJustRegistered')).toBeNull() + + mockLoginBackupEmailInterstitialManagerService.userIsElegibleForInterstitial.and.returnValue( + of(true) + ) + + service + .checkLoginInterstitials(validUserRecord, { + returnType: 'component', + togglzPrefix: 'OAUTH', + }) + .subscribe({ + next: () => fail('Should not emit any value'), + complete: () => { + expect( + mockLoginBackupEmailInterstitialManagerService.userIsElegibleForInterstitial + ).not.toHaveBeenCalled() + done() + }, + }) + }) + + it('does not suppress when the OAuth flag has expired', fakeAsync(() => { + mockInterstitialsService.checkIfSessionAlreadyCheckedInterstitialsLogic.and.returnValue( + false + ) + localStorage.setItem( + 'oauthJustRegistered', + JSON.stringify({ value: true, expiresAt: Date.now() - 1000 }) + ) + + mockLoginBackupEmailInterstitialManagerService.userIsElegibleForInterstitial.and.returnValue( + of(true) + ) + mockLoginBackupEmailInterstitialManagerService.getInterstitialTogglz.and.returnValue( + of(true) + ) + mockLoginBackupEmailInterstitialManagerService.getInterstitialViewed.and.returnValue( + of(false) + ) + mockLoginBackupEmailInterstitialManagerService.showInterstitialAsComponent.and.returnValue( + of({} as any) + ) + + service + .checkLoginInterstitials(validUserRecord, { + returnType: 'component', + togglzPrefix: 'OAUTH', + }) + .subscribe() + tick(1) + + expect( + mockLoginBackupEmailInterstitialManagerService.showInterstitialAsComponent + ).toHaveBeenCalled() + })) + + it('ignores a stale OAuth flag on the my-orcid surface', fakeAsync(() => { + // The my-orcid branch never sets this flag — it carries `justRegistered` + // on the URL instead. A flag seen here can only be left over from an + // unrelated OAuth flow, so it must not suppress anything. + mockInterstitialsService.checkIfSessionAlreadyCheckedInterstitialsLogic.and.returnValue( + false + ) + oauthUrlSession.setJustRegistered(true) + + mockLoginBackupEmailInterstitialManagerService.userIsElegibleForInterstitial.and.returnValue( + of(true) + ) + mockLoginBackupEmailInterstitialManagerService.getInterstitialTogglz.and.returnValue( + of(true) + ) + mockLoginBackupEmailInterstitialManagerService.getInterstitialViewed.and.returnValue( + of(false) + ) + mockLoginBackupEmailInterstitialManagerService.showInterstitialAsDialog.and.returnValue( + of({ + type: 'backup-email-interstitial', + } as BackupEmailComponentDialogOutput) + ) + + service + .checkLoginInterstitials(validUserRecord, { + returnType: 'dialog', + togglzPrefix: 'LOGIN', + }) + .subscribe() + tick(1) + + expect( + mockLoginBackupEmailInterstitialManagerService.showInterstitialAsDialog + ).toHaveBeenCalled() + })) + it('still shows the interstitial without the query parameter', fakeAsync(() => { mockInterstitialsService.checkIfSessionAlreadyCheckedInterstitialsLogic.and.returnValue( false diff --git a/src/app/core/oauth-urlsession-manager/oauth-urlsession-manager.service.spec.ts b/src/app/core/oauth-urlsession-manager/oauth-urlsession-manager.service.spec.ts index 42a967347..1930c4e8c 100644 --- a/src/app/core/oauth-urlsession-manager/oauth-urlsession-manager.service.spec.ts +++ b/src/app/core/oauth-urlsession-manager/oauth-urlsession-manager.service.spec.ts @@ -2,15 +2,104 @@ import { TestBed } from '@angular/core/testing' import { OauthURLSessionManagerService } from './oauth-urlsession-manager.service' +const JUST_REGISTERED_KEY = 'oauthJustRegistered' + describe('OauthURLSessionManagerService', () => { let service: OauthURLSessionManagerService beforeEach(() => { + localStorage.removeItem(JUST_REGISTERED_KEY) TestBed.configureTestingModule({}) service = TestBed.inject(OauthURLSessionManagerService) }) + afterEach(() => { + localStorage.removeItem(JUST_REGISTERED_KEY) + }) + it('should be created', () => { expect(service).toBeTruthy() }) + + describe('consumeJustRegistered', () => { + it('returns the flag and clears storage', () => { + service.setJustRegistered(true) + + expect(service.consumeJustRegistered()).toBeTrue() + expect(localStorage.getItem(JUST_REGISTERED_KEY)).toBeNull() + // One-time by contract: the stored flag is gone for the next caller + expect(service.consumeJustRegistered()).toBeFalse() + }) + + it('returns false and still clears an expired flag', () => { + localStorage.setItem( + JUST_REGISTERED_KEY, + JSON.stringify({ value: true, expiresAt: Date.now() - 1000 }) + ) + + expect(service.consumeJustRegistered()).toBeFalse() + expect(localStorage.getItem(JUST_REGISTERED_KEY)).toBeNull() + }) + }) + + describe('isJustRegistered', () => { + it('reads the stored flag without removing it', () => { + service.setJustRegistered(true) + + expect(service.isJustRegistered()).toBeTrue() + expect(localStorage.getItem(JUST_REGISTERED_KEY)).not.toBeNull() + // Repeatable, unlike consumeJustRegistered + expect(service.isJustRegistered()).toBeTrue() + }) + + it('stays true after consumeJustRegistered has cleared storage', () => { + service.setJustRegistered(true) + + expect(service.consumeJustRegistered()).toBeTrue() + expect(localStorage.getItem(JUST_REGISTERED_KEY)).toBeNull() + expect(service.isJustRegistered()).toBeTrue() + }) + + it('is false when nothing was ever stored', () => { + expect(service.isJustRegistered()).toBeFalse() + }) + + it('is false for an expired flag', () => { + localStorage.setItem( + JUST_REGISTERED_KEY, + JSON.stringify({ value: true, expiresAt: Date.now() - 1000 }) + ) + + expect(service.isJustRegistered()).toBeFalse() + }) + + it('is false for a malformed payload', () => { + localStorage.setItem(JUST_REGISTERED_KEY, 'not json') + + expect(service.isJustRegistered()).toBeFalse() + }) + + it('is false when the flag was stored as false', () => { + service.setJustRegistered(false) + + expect(service.isJustRegistered()).toBeFalse() + }) + + it('is false once an already-expired flag was consumed', () => { + localStorage.setItem( + JUST_REGISTERED_KEY, + JSON.stringify({ value: true, expiresAt: Date.now() - 1000 }) + ) + + expect(service.consumeJustRegistered()).toBeFalse() + expect(service.isJustRegistered()).toBeFalse() + }) + + it('is not resurrected by clear()', () => { + service.setJustRegistered(true) + service.clear() + + expect(service.isJustRegistered()).toBeFalse() + }) + }) }) diff --git a/src/app/core/oauth-urlsession-manager/oauth-urlsession-manager.service.ts b/src/app/core/oauth-urlsession-manager/oauth-urlsession-manager.service.ts index eddb2f401..2b1b95272 100644 --- a/src/app/core/oauth-urlsession-manager/oauth-urlsession-manager.service.ts +++ b/src/app/core/oauth-urlsession-manager/oauth-urlsession-manager.service.ts @@ -29,6 +29,14 @@ interface StoredBooleanFlag { providedIn: 'root', }) export class OauthURLSessionManagerService { + /** + * Latches once `consumeJustRegistered()` has taken the stored flag away, so + * later non-destructive readers still see it. A registration always crosses a + * full page navigation, so this can only ever mean "this page load followed a + * registration". + */ + private justRegisteredThisPageLoad = false + /** * Persist the OAuth URL with a 30‑minute TTL. */ @@ -88,7 +96,8 @@ export class OauthURLSessionManagerService { } /** - * Remove the redirect entry from storage. + * Remove the redirect entry from storage. Only storage is cleared — + * `justRegisteredThisPageLoad` is page-load scoped on purpose. */ clear(): void { localStorage.removeItem(LOCALSTORAGE_KEY) @@ -99,6 +108,23 @@ export class OauthURLSessionManagerService { * Read and clear one-time post-registration flag. */ consumeJustRegistered(): boolean { + const value = this.readJustRegisteredFlag() + this.justRegisteredThisPageLoad = this.justRegisteredThisPageLoad || value + localStorage.removeItem(LOCALSTORAGE_JUST_REGISTERED_KEY) + return value + } + + /** + * Non-destructive read of the same flag, for callers that only need to know + * whether this page load followed a registration. Stays true for the rest of + * the page load even after `consumeJustRegistered()` has cleared storage, so + * readers never have to race each other over a one-time flag. + */ + isJustRegistered(): boolean { + return this.justRegisteredThisPageLoad || this.readJustRegisteredFlag() + } + + private readJustRegisteredFlag(): boolean { const raw = localStorage.getItem(LOCALSTORAGE_JUST_REGISTERED_KEY) if (!raw) { return false @@ -110,8 +136,6 @@ export class OauthURLSessionManagerService { } } catch { // Ignore malformed payload and return false. - } finally { - localStorage.removeItem(LOCALSTORAGE_JUST_REGISTERED_KEY) } return false }