Skip to content

Commit

Permalink
PM-16220: Account does not exist during login race condition
Browse files Browse the repository at this point in the history
Wait for an account to become available from separate observable, instead of blindly accepting that the value is there using `firstValueFrom`, while it's sometimes not there immediately.
  • Loading branch information
mzieniukbw committed Dec 19, 2024
1 parent 51f6594 commit 9c7240d
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 12 deletions.
3 changes: 2 additions & 1 deletion libs/common/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"scripts": {
"clean": "rimraf dist",
"build": "npm run clean && tsc",
"build:watch": "npm run clean && tsc -watch"
"build:watch": "npm run clean && tsc -watch",
"test": "jest"
}
}
31 changes: 31 additions & 0 deletions libs/common/src/auth/services/account.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ describe("accountService", () => {
let sut: AccountServiceImplementation;
let accountsState: FakeGlobalState<Record<UserId, AccountInfo>>;
let activeAccountIdState: FakeGlobalState<UserId>;
let accountActivityState: FakeGlobalState<Record<UserId, Date>>;
const userId = Utils.newGuid() as UserId;
const userInfo = { email: "email", name: "name", emailVerified: true };

Expand All @@ -81,6 +82,7 @@ describe("accountService", () => {

accountsState = globalStateProvider.getFake(ACCOUNT_ACCOUNTS);
activeAccountIdState = globalStateProvider.getFake(ACCOUNT_ACTIVE_ACCOUNT_ID);
accountActivityState = globalStateProvider.getFake(ACCOUNT_ACTIVITY);
});

afterEach(() => {
Expand Down Expand Up @@ -256,6 +258,7 @@ describe("accountService", () => {
beforeEach(() => {
accountsState.stateSubject.next({ [userId]: userInfo });
activeAccountIdState.stateSubject.next(userId);
accountActivityState.stateSubject.next({ [userId]: new Date(1) });
});

it("should emit null if no account is provided", async () => {
Expand All @@ -269,6 +272,34 @@ describe("accountService", () => {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
expect(sut.switchAccount("unknown" as UserId)).rejects.toThrowError("Account does not exist");
});

it("should change active account when switched to the new account", async () => {
const newUserId = Utils.newGuid() as UserId;
accountsState.stateSubject.next({ [newUserId]: userInfo });

await sut.switchAccount(newUserId);

await expect(firstValueFrom(sut.activeAccount$)).resolves.toEqual({
id: newUserId,
...userInfo,
});
await expect(firstValueFrom(sut.accountActivity$)).resolves.toEqual({
[userId]: new Date(1),
[newUserId]: expect.toAlmostEqual(new Date(), 1000),
});
});

it("should not change active account when already switched to the same account", async () => {
await sut.switchAccount(userId);

await expect(firstValueFrom(sut.activeAccount$)).resolves.toEqual({
id: userId,
...userInfo,
});
await expect(firstValueFrom(sut.accountActivity$)).resolves.toEqual({
[userId]: new Date(1),
});
});
});

describe("account activity", () => {
Expand Down
32 changes: 21 additions & 11 deletions libs/common/src/auth/services/account.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import {
shareReplay,
combineLatest,
Observable,
filter,
timeout,
of,
} from "rxjs";

import {
Expand Down Expand Up @@ -149,21 +152,28 @@ export class AccountServiceImplementation implements InternalAccountService {
async switchAccount(userId: UserId | null): Promise<void> {
let updateActivity = false;
await this.activeAccountIdState.update(
(_, accounts) => {
if (userId == null) {
// indicates no account is active
return null;
}

if (accounts?.[userId] == null) {
throw new Error("Account does not exist");
}
(_, __) => {
updateActivity = true;
return userId;
},
{
combineLatestWith: this.accounts$,
shouldUpdate: (id) => {
combineLatestWith: this.accountsState.state$.pipe(
filter((accounts) => {
if (userId == null) {
// Don't worry about accounts when we are about to set active user to null
return true;
}

return accounts?.[userId] != null;
}),
// If we don't get the desired account with enough time, just return empty as that will result in the same error
timeout({ first: 1000, with: () => of({} as Record<UserId, AccountInfo>) }),

Check warning on line 170 in libs/common/src/auth/services/account.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/auth/services/account.service.ts#L170

Added line #L170 was not covered by tests
),
shouldUpdate: (id, accounts) => {
if (userId != null && accounts?.[userId] == null) {
throw new Error("Account does not exist");

Check warning on line 174 in libs/common/src/auth/services/account.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/auth/services/account.service.ts#L174

Added line #L174 was not covered by tests
}

// update only if userId changes
return id !== userId;
},
Expand Down

0 comments on commit 9c7240d

Please sign in to comment.