bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2025-12-06 00:13:28 +00:00
Code Issues Releases Wiki Activity

fix(SignalR): Revert "[PM-23062] Fix extra signalr connections"

Browse Source 
This reverts commit ec7a2613cc.
This commit is contained in:
Todd Martin
2025-07-15 17:25:45 -04:00
committed by GitHub
parent 6cb6316afd
commit 97ec9a6339
6 changed files with 54 additions and 89 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
libs/common/src/platform/abstractions/config/config-api.service.abstraction.ts
View File

@@ -5,5 +5,5 @@ export abstract class ConfigApiServiceAbstraction {
/**
* Fetches the server configuration for the given user. If no user is provided, the configuration will not contain user-specific context.
*/
abstract get(userId: UserId | null): Promise<ServerConfigResponse>;
abstract get(userId: UserId | undefined): Promise<ServerConfigResponse>;
}

11
libs/common/src/platform/abstractions/environment.service.ts
View File

@@ -95,13 +95,6 @@ export interface Environment {
*/
export abstract class EnvironmentService {
abstract environment$: Observable<Environment>;
/**
* The environment stored in global state, when a user signs in the state stored here will become
* their user environment.
*/
abstract globalEnvironment$: Observable<Environment>;
abstract cloudWebVaultUrl$: Observable<string>;
/**
@@ -132,12 +125,12 @@ export abstract class EnvironmentService {
* @param userId - The user id to set the cloud web vault app URL for. If null or undefined the global environment is set.
* @param region - The region of the cloud web vault app.
*/
abstract setCloudRegion(userId: UserId | null, region: Region): Promise<void>;
abstract setCloudRegion(userId: UserId, region: Region): Promise<void>;
/**
* Get the environment from state. Useful if you need to get the environment for another user.
*/
abstract getEnvironment$(userId: UserId): Observable<Environment>;
abstract getEnvironment$(userId: UserId): Observable<Environment | undefined>;
/**
* @deprecated Use {@link getEnvironment$} instead.

2
libs/common/src/platform/services/config/config-api.service.ts
View File

@@ -10,7 +10,7 @@ export class ConfigApiService implements ConfigApiServiceAbstraction {
private tokenService: TokenService,
) {}
async get(userId: UserId | null): Promise<ServerConfigResponse> {
async get(userId: UserId | undefined): Promise<ServerConfigResponse> {
// Authentication adds extra context to config responses, if the user has an access token, we want to use it
// We don't particularly care about ensuring the token is valid and not expired, just that it exists
const authed: boolean =

32
libs/common/src/platform/services/config/config.service.spec.ts
View File

@@ -10,9 +10,9 @@ import {
FakeGlobalState,
FakeSingleUserState,
FakeStateProvider,
awaitAsync,
mockAccountServiceWith,
} from "../../../../spec";
import { Matrix } from "../../../../spec/matrix";
import { subscribeTo } from "../../../../spec/observable-tracker";
import { AuthService } from "../../../auth/abstractions/auth.service";
import { AuthenticationStatus } from "../../../auth/enums/authentication-status";
@@ -74,8 +74,7 @@ describe("ConfigService", () => {
});
beforeEach(() => {
Matrix.autoMockMethod(environmentService.getEnvironment$, () => environmentSubject);
environmentService.globalEnvironment$ = environmentSubject;
environmentService.environment$ = environmentSubject;
sut = new DefaultConfigService(
configApiService,
environmentService,
@@ -99,12 +98,9 @@ describe("ConfigService", () => {
: serverConfigFactory(activeApiUrl + userId, tooOld);
const globalStored =
configStateDescription === "missing"
? {
[activeApiUrl]: null,
}
? {}
: {
[activeApiUrl]: serverConfigFactory(activeApiUrl, tooOld),
[activeApiUrl + "0"]: serverConfigFactory(activeApiUrl + userId, tooOld),
};
beforeEach(() => {
@@ -112,6 +108,11 @@ describe("ConfigService", () => {
userState.nextState(userStored);
});
// sanity check
test("authed and unauthorized state are different", () => {
expect(globalStored[activeApiUrl]).not.toEqual(userStored);
});
describe("fail to fetch", () => {
beforeEach(() => {
configApiService.get.mockRejectedValue(new Error("Unable to fetch"));
@@ -177,7 +178,6 @@ describe("ConfigService", () => {
beforeEach(() => {
globalState.stateSubject.next(globalStored);
userState.nextState(userStored);
Matrix.autoMockMethod(environmentService.getEnvironment$, () => environmentSubject);
});
it("does not fetch from server", async () => {
await firstValueFrom(sut.serverConfig$);
@@ -189,13 +189,21 @@ describe("ConfigService", () => {
const actual = await firstValueFrom(sut.serverConfig$);
expect(actual).toEqual(activeUserId ? userStored : globalStored[activeApiUrl]);
});
it("does not complete after emit", async () => {
const emissions = [];
const subscription = sut.serverConfig$.subscribe((v) => emissions.push(v));
await awaitAsync();
expect(emissions.length).toBe(1);
expect(subscription.closed).toBe(false);
});
});
});
});
it("gets global config when there is an locked active user", async () => {
await accountService.switchAccount(userId);
environmentService.globalEnvironment$ = of(environmentFactory(activeApiUrl));
environmentService.environment$ = of(environmentFactory(activeApiUrl));
globalState.stateSubject.next({
[activeApiUrl]: serverConfigFactory(activeApiUrl + "global"),
@@ -228,8 +236,7 @@ describe("ConfigService", () => {
beforeEach(() => {
environmentSubject = new Subject<Environment>();
environmentService.globalEnvironment$ = environmentSubject;
Matrix.autoMockMethod(environmentService.getEnvironment$, () => environmentSubject);
environmentService.environment$ = environmentSubject;
sut = new DefaultConfigService(
configApiService,
environmentService,
@@ -320,8 +327,7 @@ describe("ConfigService", () => {
beforeEach(async () => {
const config = serverConfigFactory("existing-data", tooOld);
environmentService.globalEnvironment$ = environmentSubject;
Matrix.autoMockMethod(environmentService.getEnvironment$, () => environmentSubject);
environmentService.environment$ = environmentSubject;
globalState.stateSubject.next({ [apiUrl(0)]: config });
userState.stateSubject.next({

87
libs/common/src/platform/services/config/default-config.service.ts
View File

@@ -1,18 +1,17 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import {
combineLatest,
distinctUntilChanged,
firstValueFrom,
map,
mergeWith,
NEVER,
Observable,
of,
ReplaySubject,
share,
shareReplay,
Subject,
switchMap,
tap,
timer,
} from "rxjs";
import { SemVer } from "semver";
@@ -51,15 +50,11 @@ export const GLOBAL_SERVER_CONFIGURATIONS = KeyDefinition.record<ServerConfig, A
},
);
const environmentComparer = (previous: Environment, current: Environment) => {
return previous.getApiUrl() === current.getApiUrl();
};
// FIXME: currently we are limited to api requests for active users. Update to accept a UserId and APIUrl once ApiService supports it.
export class DefaultConfigService implements ConfigService {
private failedFetchFallbackSubject = new Subject<ServerConfig | null>();
private failedFetchFallbackSubject = new Subject<ServerConfig>();
serverConfig$: Observable<ServerConfig | null>;
serverConfig$: Observable<ServerConfig>;
serverSettings$: Observable<ServerSettings>;
@@ -72,51 +67,25 @@ export class DefaultConfigService implements ConfigService {
private stateProvider: StateProvider,
private authService: AuthService,
) {
const globalConfig$ = this.environmentService.globalEnvironment$.pipe(
distinctUntilChanged(environmentComparer),
switchMap((environment) =>
this.globalConfigFor$(environment.getApiUrl()).pipe(
map((config) => {
return [config, null as UserId | null, environment] as const;
}),
),
),
const userId$ = this.stateProvider.activeUserId$;
const authStatus$ = userId$.pipe(
switchMap((userId) => (userId == null ? of(null) : this.authService.authStatusFor$(userId))),
);
this.serverConfig$ = this.stateProvider.activeUserId$.pipe(
distinctUntilChanged(),
switchMap((userId) => {
if (userId == null) {
// Global
return globalConfig$;
this.serverConfig$ = combineLatest([
userId$,
this.environmentService.environment$,
authStatus$,
]).pipe(
switchMap(([userId, environment, authStatus]) => {
if (userId == null || authStatus !== AuthenticationStatus.Unlocked) {
return this.globalConfigFor$(environment.getApiUrl()).pipe(
map((config) => [config, null, environment] as const),
);
}
return this.authService.authStatusFor$(userId).pipe(
map((authStatus) => authStatus === AuthenticationStatus.Unlocked),
distinctUntilChanged(),
switchMap((isUnlocked) => {
if (!isUnlocked) {
return globalConfig$;
}
return combineLatest([
this.environmentService
.getEnvironment$(userId)
.pipe(distinctUntilChanged(environmentComparer)),
this.userConfigFor$(userId),
]).pipe(
switchMap(([environment, config]) => {
if (config == null) {
// If the user doesn't have any config yet, use the global config for that url as the fallback
return this.globalConfigFor$(environment.getApiUrl()).pipe(
map((globalConfig) => [globalConfig, userId, environment] as const),
);
}
return of([config, userId, environment] as const);
}),
);
}),
return this.userConfigFor$(userId).pipe(
map((config) => [config, userId, environment] as const),
);
}),
tap(async (rec) => {
@@ -137,7 +106,7 @@ export class DefaultConfigService implements ConfigService {
}),
// If fetch fails, we'll emit on this subject to fallback to the existing config
mergeWith(this.failedFetchFallbackSubject),
share({ connector: () => new ReplaySubject(1), resetOnRefCountZero: () => timer(1000) }),
shareReplay({ refCount: true, bufferSize: 1 }),
);
this.cloudRegion$ = this.serverConfig$.pipe(
@@ -186,8 +155,8 @@ export class DefaultConfigService implements ConfigService {
// Updates the on-disk configuration with a newly retrieved configuration
private async renewConfig(
existingConfig: ServerConfig | null,
userId: UserId | null,
existingConfig: ServerConfig,
userId: UserId,
environment: Environment,
): Promise<void> {
try {
@@ -195,7 +164,9 @@ export class DefaultConfigService implements ConfigService {
// somewhat quickly even though it may not be accurate, we won't cancel the HTTP request
// though so that hopefully it can have finished and hydrated a more accurate value.
const handle = setTimeout(() => {
this.logService.info("Environment did not respond in time, emitting previous config.");
this.logService.info(
"Self-host environment did not respond in time, emitting previous config.",
);
this.failedFetchFallbackSubject.next(existingConfig);
}, SLOW_EMISSION_GUARD);
const response = await this.configApiService.get(userId);
@@ -228,13 +199,13 @@ export class DefaultConfigService implements ConfigService {
}
}
private globalConfigFor$(apiUrl: string): Observable<ServerConfig | null> {
private globalConfigFor$(apiUrl: string): Observable<ServerConfig> {
return this.stateProvider
.getGlobal(GLOBAL_SERVER_CONFIGURATIONS)
.state$.pipe(map((configs) => configs?.[apiUrl] ?? null));
.state$.pipe(map((configs) => configs?.[apiUrl]));
}
private userConfigFor$(userId: UserId): Observable<ServerConfig | null> {
private userConfigFor$(userId: UserId): Observable<ServerConfig> {
return this.stateProvider.getUser(userId, USER_SERVER_CONFIG).state$;
}
}

9
libs/common/src/platform/services/default-environment.service.ts
View File

@@ -133,7 +133,6 @@ export class DefaultEnvironmentService implements EnvironmentService {
);
environment$: Observable<Environment>;
globalEnvironment$: Observable<Environment>;
cloudWebVaultUrl$: Observable<string>;
constructor(
@@ -149,10 +148,6 @@ export class DefaultEnvironmentService implements EnvironmentService {
distinctUntilChanged((oldUserId: UserId, newUserId: UserId) => oldUserId == newUserId),
);
this.globalEnvironment$ = this.stateProvider
.getGlobal(GLOBAL_ENVIRONMENT_KEY)
.state$.pipe(map((state) => this.buildEnvironment(state?.region, state?.urls)));
this.environment$ = account$.pipe(
switchMap((userId) => {
const t = userId
@@ -268,7 +263,7 @@ export class DefaultEnvironmentService implements EnvironmentService {
return new SelfHostedEnvironment(urls);
}
async setCloudRegion(userId: UserId | null, region: CloudRegion) {
async setCloudRegion(userId: UserId, region: CloudRegion) {
if (userId == null) {
await this.globalCloudRegionState.update(() => region);
} else {
@@ -276,7 +271,7 @@ export class DefaultEnvironmentService implements EnvironmentService {
}
}
getEnvironment$(userId: UserId): Observable<Environment> {
getEnvironment$(userId: UserId): Observable<Environment | undefined> {
return this.stateProvider.getUser(userId, USER_ENVIRONMENT_KEY).state$.pipe(
map((state) => {
return this.buildEnvironment(state?.region, state?.urls);