diff --git a/src/app/services/authorization/authorization.spec.ts b/src/app/services/authorization/authorization.spec.ts index ab9be71..67be71f 100644 --- a/src/app/services/authorization/authorization.spec.ts +++ b/src/app/services/authorization/authorization.spec.ts @@ -3,6 +3,8 @@ // Attribution term under AGPL-3.0 section 7(b): see LICENSE-ADDENDUM. import { TestBed } from '@angular/core/testing'; +import { of, throwError } from 'rxjs'; +import { vi } from 'vitest'; import { Authorization } from './authorization'; @@ -17,4 +19,68 @@ describe('Authorization', () => { it('should be created', () => { expect(service).toBeTruthy(); }); + + describe('userChanged', () => { + /** + * A per-user cache (ProjectsService, FlowsService) subscribes to this to know when to drop + * what it is holding. If it fired on every persisted state, or missed a real transition, that + * cache would either thrash on every session check or leak one user's data into another's. + */ + it('fires with the new username on login', () => { + service.authCall.login = vi.fn(() => of({ username: 'alice', email: null, role: 'USER' })) as any; + const seen: (string | null)[] = []; + service.userChanged.subscribe((username) => seen.push(username)); + + service.login('alice', 'secret').subscribe(); + + expect(seen).toEqual(['alice']); + }); + + it('fires with null on logout', () => { + service.authCall.login = vi.fn(() => of({ username: 'alice', email: null, role: 'USER' })) as any; + service.authCall.logout = vi.fn(() => of(void 0)) as any; + service.login('alice', 'secret').subscribe(); + const seen: (string | null)[] = []; + service.userChanged.subscribe((username) => seen.push(username)); + + service.logout().subscribe(); + + expect(seen).toEqual([null]); + }); + + it('fires again when a different account logs in over the current one', () => { + service.authCall.login = vi.fn() + .mockReturnValueOnce(of({ username: 'alice', email: null, role: 'USER' })) + .mockReturnValueOnce(of({ username: 'bob', email: null, role: 'USER' })) as any; + service.login('alice', 'secret').subscribe(); + const seen: (string | null)[] = []; + service.userChanged.subscribe((username) => seen.push(username)); + + service.login('bob', 'secret').subscribe(); + + expect(seen).toEqual(['bob']); + }); + + it('does not fire when a session check confirms the same user is still signed in', () => { + service.authCall.login = vi.fn(() => of({ username: 'alice', email: null, role: 'USER' })) as any; + service.authCall.currentUser = vi.fn(() => of({ username: 'alice', email: null, role: 'USER' })) as any; + service.login('alice', 'secret').subscribe(); + const seen: (string | null)[] = []; + service.userChanged.subscribe((username) => seen.push(username)); + + service.validateSession().subscribe(); + + expect(seen).toEqual([]); + }); + + it('does not fire for a failed login attempt', () => { + service.authCall.login = vi.fn(() => throwError(() => new Error('bad credentials'))) as any; + const seen: (string | null)[] = []; + service.userChanged.subscribe((username) => seen.push(username)); + + service.login('alice', 'wrong').subscribe({ error: () => {} }); + + expect(seen).toEqual([]); + }); + }); }); diff --git a/src/app/services/authorization/authorization.ts b/src/app/services/authorization/authorization.ts index 5df3b9f..859e10b 100644 --- a/src/app/services/authorization/authorization.ts +++ b/src/app/services/authorization/authorization.ts @@ -10,7 +10,7 @@ import { } from '@models/user'; import { AuthorizationCallServiceBase } from './authorization-call.base'; import { environment } from '@environment'; -import { Observable, catchError, finalize, map, of, shareReplay, take, tap, throwError } from 'rxjs'; +import { Observable, Subject, catchError, finalize, map, of, shareReplay, take, tap, throwError } from 'rxjs'; @Injectable({ providedIn: 'root', @@ -25,6 +25,17 @@ export class Authorization { loggedInUser = this.user.asReadonly(); + private readonly userChanged$ = new Subject(); + /** + * Emits the new username (or null) whenever the signed-in identity actually changes - login, + * logout, or one account replacing another - but never for the initial hydration from storage on + * app boot, since that is a page load continuing a session, not a change. A per-user client cache + * (`ProjectsService`, `FlowsService`, ...) subscribes to invalidate itself: those services are + * `providedIn: 'root'` and outlive any one login, so without this a second person signing in on + * the same tab without a hard reload would still see whoever was logged in before them. + */ + readonly userChanged: Observable = this.userChanged$.asObservable(); + constructor() { this.restoreUserFromStorage(); } @@ -114,8 +125,14 @@ export class Authorization { private persistUserState(user: User | null) { const normalizedUser = this.normalizeUser(user); + const previousUsername = this.user()?.username ?? null; this.user.set(normalizedUser); + const nextUsername = normalizedUser?.username ?? null; + if (previousUsername !== nextUsername) { + this.userChanged$.next(nextUsername); + } + try { if (typeof localStorage === 'undefined') return; if (normalizedUser) { diff --git a/src/app/services/flows/flows.spec.ts b/src/app/services/flows/flows.spec.ts index 725c194..fa69bd9 100644 --- a/src/app/services/flows/flows.spec.ts +++ b/src/app/services/flows/flows.spec.ts @@ -4,6 +4,7 @@ import { TestBed } from '@angular/core/testing'; import { Flow, FlowData } from '@models/flow'; +import { Authorization } from '@services/authorization/authorization'; import { firstValueFrom, of, throwError } from 'rxjs'; import { vi } from 'vitest'; import { FlowsCallServiceBase } from './flows-call.base'; @@ -97,6 +98,25 @@ describe('FlowsService', () => { }); }); + it('drops the cached list when a different user signs in, so the next read refetches', async () => { + // Regression: this service is providedIn 'root' and outlives any one login. Without this, a + // second person signing in on the same tab kept seeing whoever's flows were loaded before. + callServiceSpy.retrieveAllFlows + .mockReturnValueOnce(of([makeFlow('alice-1')])) + .mockReturnValueOnce(of([makeFlow('bob-1')])); + await service.getAllFlows(); + expect(service.flows().map((flow) => flow.id)).toEqual(['alice-1']); + + const authorization = TestBed.inject(Authorization); + authorization.authCall.login = vi.fn(() => of({ username: 'bob', email: null, role: 'USER' })) as any; + authorization.login('bob', 'secret').subscribe(); + + expect(service.flows()).toEqual([]); + + await service.getAllFlows(); + expect(service.flows().map((flow) => flow.id)).toEqual(['bob-1']); + }); + describe('refresh', () => { it('should update flows signal', async () => { const flows = [makeFlow('r1')]; diff --git a/src/app/services/flows/flows.ts b/src/app/services/flows/flows.ts index f9de3a4..4fbb598 100644 --- a/src/app/services/flows/flows.ts +++ b/src/app/services/flows/flows.ts @@ -2,9 +2,10 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // Attribution term under AGPL-3.0 section 7(b): see LICENSE-ADDENDUM. -import { Injectable, signal } from '@angular/core'; +import { inject, Injectable, signal } from '@angular/core'; import { environment } from '@environment'; import { Flow, FlowData } from '@models/flow'; +import { Authorization } from '@services/authorization/authorization'; import { FlowsCallServiceBase } from './flows-call.base'; import { catchError, firstValueFrom, Observable, of, switchMap, tap, throwError } from 'rxjs'; @@ -21,6 +22,17 @@ export class FlowsService { private _flows = signal([]); readonly flows = this._flows.asReadonly(); + constructor() { + // See Authorization.userChanged / ProjectsService: same cache-outlives-login leak, same fix. + inject(Authorization).userChanged.subscribe(() => this.reset()); + } + + reset() { + this._flows.set([]); + this.toInit = true; + this.loadingPromise = null; + } + hasLoadedFlows() { return this._flows().length > 0 || !this.toInit; } diff --git a/src/app/services/projects/projects.spec.ts b/src/app/services/projects/projects.spec.ts index 09833f5..20f4ac7 100644 --- a/src/app/services/projects/projects.spec.ts +++ b/src/app/services/projects/projects.spec.ts @@ -4,6 +4,7 @@ import { TestBed } from '@angular/core/testing'; import { Project } from '@models/project'; +import { Authorization } from '@services/authorization/authorization'; import { of, throwError } from 'rxjs'; import { vi } from 'vitest'; @@ -73,6 +74,25 @@ describe('ProjectsService', () => { expect(service.hasLoadedProjects()).toBe(false); }); + it('drops the cached list when a different user signs in, so the next read refetches', async () => { + // Regression: this service is providedIn 'root' and outlives any one login. Without this, a + // second person signing in on the same tab kept seeing whoever's projects were loaded before. + callServiceSpy.retrieveAllProjects + .mockReturnValueOnce(of([makeProject('p1', "Alice's project")])) + .mockReturnValueOnce(of([makeProject('p2', "Bob's project")])); + await service.getAllProjects(); + expect(service.projects().map((project) => project.id)).toEqual(['p1']); + + const authorization = TestBed.inject(Authorization); + authorization.authCall.login = vi.fn(() => of({ username: 'bob', email: null, role: 'USER' })) as any; + authorization.login('bob', 'secret').subscribe(); + + expect(service.projects()).toEqual([]); + + await service.getAllProjects(); + expect(service.projects().map((project) => project.id)).toEqual(['p2']); + }); + it('indexes projects by id for O(1) lookup from a flow', async () => { callServiceSpy.retrieveAllProjects.mockReturnValue(of([makeProject('p1', 'Recruiting')])); await service.getAllProjects(); diff --git a/src/app/services/projects/projects.ts b/src/app/services/projects/projects.ts index 4415005..a641dc8 100644 --- a/src/app/services/projects/projects.ts +++ b/src/app/services/projects/projects.ts @@ -2,9 +2,10 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // Attribution term under AGPL-3.0 section 7(b): see LICENSE-ADDENDUM. -import { computed, Injectable, signal } from '@angular/core'; +import { computed, inject, Injectable, signal } from '@angular/core'; import { environment } from '@environment'; import { Project, ProjectContext, ProjectDraft } from '@models/project'; +import { Authorization } from '@services/authorization/authorization'; import { catchError, firstValueFrom, Observable, of, tap, throwError } from 'rxjs'; import { ProjectsCallServiceBase } from './projects-call.base'; @@ -24,6 +25,19 @@ export class ProjectsService { /** Lets a flow resolve its project in O(1) without threading inputs through the tree. */ readonly projectById = computed(() => new Map(this._projects().map((project) => [project.id, project]))); + constructor() { + // See Authorization.userChanged: without this, a second person signing in on the same tab + // would still see whoever loaded the page before them, since this service outlives any one login. + inject(Authorization).userChanged.subscribe(() => this.reset()); + } + + /** Drops the cached list so the next read has to ask the server again, as who is asking has changed. */ + reset() { + this._projects.set([]); + this.toInit = true; + this.loadingPromise = null; + } + hasLoadedProjects() { return this._projects().length > 0 || !this.toInit; } diff --git a/src/app/shared/flows-list/flow-item/flow-item.spec.ts b/src/app/shared/flows-list/flow-item/flow-item.spec.ts index d0c5698..5f978f0 100644 --- a/src/app/shared/flows-list/flow-item/flow-item.spec.ts +++ b/src/app/shared/flows-list/flow-item/flow-item.spec.ts @@ -51,7 +51,8 @@ describe('FlowItem', () => { { provide: Authorization, useValue: { - loggedInUser: vi.fn().mockReturnValue({ username: 'author' }) + loggedInUser: vi.fn().mockReturnValue({ username: 'author' }), + userChanged: of(null) } }, {