diff --git a/libs/common/src/platform/abstractions/config/config-api.service.abstraction.ts b/libs/common/src/platform/abstractions/config/config-api.service.abstraction.ts index 0460e8c715f..3c191f59ccc 100644 --- a/libs/common/src/platform/abstractions/config/config-api.service.abstraction.ts +++ b/libs/common/src/platform/abstractions/config/config-api.service.abstraction.ts @@ -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; + abstract get(userId: UserId | undefined): Promise; } diff --git a/libs/common/src/platform/abstractions/environment.service.ts b/libs/common/src/platform/abstractions/environment.service.ts index 86a0fbea242..b8931656848 100644 --- a/libs/common/src/platform/abstractions/environment.service.ts +++ b/libs/common/src/platform/abstractions/environment.service.ts @@ -95,13 +95,6 @@ export interface Environment { */ export abstract class EnvironmentService { abstract environment$: Observable; - - /** - * The environment stored in global state, when a user signs in the state stored here will become - * their user environment. - */ - abstract globalEnvironment$: Observable; - abstract cloudWebVaultUrl$: Observable; /** @@ -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; + abstract setCloudRegion(userId: UserId, region: Region): Promise; /** * Get the environment from state. Useful if you need to get the environment for another user. */ - abstract getEnvironment$(userId: UserId): Observable; + abstract getEnvironment$(userId: UserId): Observable; /** * @deprecated Use {@link getEnvironment$} instead. diff --git a/libs/common/src/platform/services/config/config-api.service.ts b/libs/common/src/platform/services/config/config-api.service.ts index b7ecb9c8712..f283410acea 100644 --- a/libs/common/src/platform/services/config/config-api.service.ts +++ b/libs/common/src/platform/services/config/config-api.service.ts @@ -10,7 +10,7 @@ export class ConfigApiService implements ConfigApiServiceAbstraction { private tokenService: TokenService, ) {} - async get(userId: UserId | null): Promise { + async get(userId: UserId | undefined): Promise { // 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 = diff --git a/libs/common/src/platform/services/config/config.service.spec.ts b/libs/common/src/platform/services/config/config.service.spec.ts index e8a1872c4c1..ea3b56a32f1 100644 --- a/libs/common/src/platform/services/config/config.service.spec.ts +++ b/libs/common/src/platform/services/config/config.service.spec.ts @@ -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(); - 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({ diff --git a/libs/common/src/platform/services/config/default-config.service.ts b/libs/common/src/platform/services/config/default-config.service.ts index 4297b65cb60..33f86d30885 100644 --- a/libs/common/src/platform/services/config/default-config.service.ts +++ b/libs/common/src/platform/services/config/default-config.service.ts @@ -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 { - 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(); + private failedFetchFallbackSubject = new Subject(); - serverConfig$: Observable; + serverConfig$: Observable; serverSettings$: Observable; @@ -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 { 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 { + private globalConfigFor$(apiUrl: string): Observable { 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 { + private userConfigFor$(userId: UserId): Observable { return this.stateProvider.getUser(userId, USER_SERVER_CONFIG).state$; } } diff --git a/libs/common/src/platform/services/default-environment.service.ts b/libs/common/src/platform/services/default-environment.service.ts index 4a1af68505a..df55693ba0b 100644 --- a/libs/common/src/platform/services/default-environment.service.ts +++ b/libs/common/src/platform/services/default-environment.service.ts @@ -133,7 +133,6 @@ export class DefaultEnvironmentService implements EnvironmentService { ); environment$: Observable; - globalEnvironment$: Observable; cloudWebVaultUrl$: Observable; 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 { + getEnvironment$(userId: UserId): Observable { return this.stateProvider.getUser(userId, USER_ENVIRONMENT_KEY).state$.pipe( map((state) => { return this.buildEnvironment(state?.region, state?.urls);