Fix user settings and calculations in impersonation mode

apps/api/src/app/activities/activities.controller.ts

@@ -1,4 +1,3 @@
-import { UserService } from '@ghostfolio/api/app/user/user.service';
 import { HasPermission } from '@ghostfolio/api/decorators/has-permission.decorator';
 import { HasPermissionGuard } from '@ghostfolio/api/guards/has-permission.guard';
 import { isActivityInFuture } from '@ghostfolio/api/helper/activity.helper';
@@ -8,16 +7,19 @@ import { TransformDataSourceInResponseInterceptor } from '@ghostfolio/api/interc
 import { ApiService } from '@ghostfolio/api/services/api/api.service';
 import { DataProviderService } from '@ghostfolio/api/services/data-provider/data-provider.service';
 import { ImpersonationService } from '@ghostfolio/api/services/impersonation/impersonation.service';
+import { PrismaService } from '@ghostfolio/api/services/prisma/prisma.service';
 import { DataGatheringService } from '@ghostfolio/api/services/queues/data-gathering/data-gathering.service';
 import { getIntervalFromDateRange } from '@ghostfolio/common/calculation-helper';
 import {
   DATA_GATHERING_QUEUE_PRIORITY_HIGH,
+  DEFAULT_CURRENCY,
   HEADER_KEY_IMPERSONATION
 } from '@ghostfolio/common/config';
 import { CreateOrderDto, UpdateOrderDto } from '@ghostfolio/common/dtos';
 import {
   ActivitiesResponse,
-  ActivityResponse
+  ActivityResponse,
+  UserSettings
 } from '@ghostfolio/common/interfaces';
 import { permissions } from '@ghostfolio/common/permissions';
 import type { RequestWithUser } from '@ghostfolio/common/types';
@@ -55,8 +57,8 @@ export class ActivitiesController {
     private readonly dataProviderService: DataProviderService,
     private readonly dataGatheringService: DataGatheringService,
     private readonly impersonationService: ImpersonationService,
-    @Inject(REQUEST) private readonly request: RequestWithUser,
+    private readonly prismaService: PrismaService,
-    private readonly userService: UserService
+    @Inject(REQUEST) private readonly request: RequestWithUser
   ) {}
 
   @Delete()
@@ -173,7 +175,7 @@ export class ActivitiesController {
       await this.impersonationService.validateImpersonationId(impersonationId);
     const userId = impersonationUserId || this.request.user.id;
 
-    const { settings } = await this.userService.user({ id: userId });
+    const userCurrency = await this.getUserCurrency(impersonationUserId);
 
     const { activities, count } = await this.activitiesService.getActivities({
       endDate,
@@ -183,10 +185,10 @@ export class ActivitiesController {
       sortDirection,
       startDate,
       take,
+      userCurrency,
       userId,
       includeDrafts: true,
       types: activityTypes,
-      userCurrency: settings.settings.baseCurrency,
       withExcludedAccountsAndActivities: true
     });
 
@@ -205,12 +207,12 @@ export class ActivitiesController {
       await this.impersonationService.validateImpersonationId(impersonationId);
     const userId = impersonationUserId || this.request.user.id;
 
-    const { settings } = await this.userService.user({ id: userId });
+    const userCurrency = await this.getUserCurrency(impersonationUserId);
 
     const { activities } = await this.activitiesService.getActivities({
+      userCurrency,
       userId,
       includeDrafts: true,
-      userCurrency: settings.settings.baseCurrency,
       withExcludedAccountsAndActivities: true
     });
 
@@ -382,4 +384,18 @@ export class ActivitiesController {
       }
     });
   }
+
+  private async getUserCurrency(impersonationUserId: string) {
+    if (!impersonationUserId) {
+      return this.request.user.settings.settings.baseCurrency;
+    }
+
+    const settings = await this.prismaService.settings.findUnique({
+      where: { userId: impersonationUserId }
+    });
+
+    return (
+      (settings?.settings as UserSettings)?.baseCurrency ?? DEFAULT_CURRENCY
+    );
+  }
 }

apps/api/src/app/activities/activities.module.ts

@@ -2,7 +2,6 @@ import { AccountBalanceService } from '@ghostfolio/api/app/account-balance/accou
 import { AccountService } from '@ghostfolio/api/app/account/account.service';
 import { CacheModule } from '@ghostfolio/api/app/cache/cache.module';
 import { RedisCacheModule } from '@ghostfolio/api/app/redis-cache/redis-cache.module';
-import { UserModule } from '@ghostfolio/api/app/user/user.module';
 import { RedactValuesInResponseModule } from '@ghostfolio/api/interceptors/redact-values-in-response/redact-values-in-response.module';
 import { TransformDataSourceInRequestModule } from '@ghostfolio/api/interceptors/transform-data-source-in-request/transform-data-source-in-request.module';
 import { TransformDataSourceInResponseModule } from '@ghostfolio/api/interceptors/transform-data-source-in-response/transform-data-source-in-response.module';
@@ -40,8 +39,7 @@ import { ActivitiesService } from './activities.service';
     SymbolProfileModule,
     TagModule,
     TransformDataSourceInRequestModule,
-    TransformDataSourceInResponseModule,
+    TransformDataSourceInResponseModule
-    UserModule
   ],
   providers: [AccountBalanceService, AccountService, ActivitiesService]
 })

apps/api/src/app/user/user.service.ts

@@ -59,7 +59,7 @@ import { PerformanceCalculationType } from '@ghostfolio/common/types/performance
 import { Injectable, Logger } from '@nestjs/common';
 import { EventEmitter2 } from '@nestjs/event-emitter';
 import { InjectThrottlerStorage, ThrottlerStorage } from '@nestjs/throttler';
-import { Prisma, Role, Settings, User } from '@prisma/client';
+import { Prisma, Role, User } from '@prisma/client';
 import { differenceInDays, subDays } from 'date-fns';
 import { isNil, without } from 'lodash';
 import { createHmac } from 'node:crypto';
@@ -128,7 +128,7 @@ export class UserService {
       accounts,
       activitiesCount,
       firstActivity,
-      impersonationUserSettings,
+      impersonationUser,
       tagsForUser
     ] = await Promise.all([
       this.prismaService.access.findMany({
@@ -157,16 +157,14 @@ export class UserService {
         where: { userId: impersonationUserId || user.id }
       }),
       impersonationUserId
-        ? this.prismaService.settings.findUnique({
+        ? this.user({ id: impersonationUserId })
-            where: { userId: impersonationUserId }
+        : Promise.resolve<UserWithSettings>(null),
-          })
-        : Promise.resolve<Settings>(null),
       this.tagService.getTagsForUser(impersonationUserId || user.id)
     ]);
 
     const resolvedUserSettings = resolveUserSettings({
       impersonationUserSettings: impersonationUserId
-        ? ((impersonationUserSettings?.settings ?? {}) as UserSettings)
+        ? ((impersonationUser?.settings?.settings ?? {}) as UserSettings)
         : undefined,
       userSettings: settings.settings as UserSettings
     });

apps/client/src/app/pages/portfolio/analysis/analysis-page.component.ts

@@ -88,7 +88,6 @@ export class GfAnalysisPageComponent implements OnInit {
   protected dividendsByGroup: InvestmentItem[];
   protected readonly dividendTimelineDataLabel = $localize`Dividend`;
   protected hasPermissionToReadAiPrompt: boolean;
-  protected hasPermissionToUpdateUserSettings: boolean;
   protected impersonationId: string | null;
   protected investments: InvestmentItem[];
   protected readonly investmentTimelineDataLabel = $localize`Invested Capital`;
@@ -175,11 +174,6 @@ export class GfAnalysisPageComponent implements OnInit {
             permissions.readAiPrompt
           );
 
-          this.hasPermissionToUpdateUserSettings = hasPermission(
-            this.user.permissions,
-            permissions.updateUserSettings
-          );
-
           this.update();
         }
 

apps/client/src/app/pages/portfolio/analysis/analysis-page.html

@@ -137,9 +137,7 @@
         [benchmarkDataItems]="benchmarkDataItems"
         [benchmarks]="benchmarks"
         [colorScheme]="user?.settings?.colorScheme"
-        [hasPermissionToUpdateUserSettings]="
+        [hasPermissionToUpdateUserSettings]="!impersonationId"
-          hasPermissionToUpdateUserSettings && !impersonationId
-        "
         [isLoading]="isLoadingBenchmarkComparator || isLoadingInvestmentChart"
         [locale]="user?.settings?.locale"
         [performanceDataItems]="performanceDataItemsInPercentage"

libs/common/src/lib/helper.spec.ts

@@ -441,6 +441,35 @@ describe('Helper', () => {
       });
     });
 
+    it('Filters stay with the authenticated user', () => {
+      // The filters are always written back to the authenticated user, so
+      // reading them from the impersonated user would overwrite them
+      const { 'filters.accounts': filtersAccounts } = resolveUserSettings({
+        impersonationUserSettings: {
+          'filters.accounts': ['3b3c2b5d-5a4f-4b0a-9d4f-9b1f5e6a7c8d']
+        },
+        userSettings: {
+          'filters.accounts': ['0a1b2c3d-4e5f-6a7b-8c9d-0e1f2a3b4c5d']
+        }
+      });
+
+      expect(filtersAccounts).toEqual(['0a1b2c3d-4e5f-6a7b-8c9d-0e1f2a3b4c5d']);
+    });
+
+    it('Presentation settings unset for the authenticated user do not leak', () => {
+      // An unset presentation setting must not fall back to the impersonated
+      // user, otherwise their appearance and language apply to the
+      // authenticated user
+      const { colorScheme, language, locale } = resolveUserSettings({
+        impersonationUserSettings,
+        userSettings: { baseCurrency: 'CHF' }
+      });
+
+      expect(colorScheme).toBeUndefined();
+      expect(language).toBeUndefined();
+      expect(locale).toBeUndefined();
+    });
+
     it('Unknown settings default to the impersonated user', () => {
       // A setting which is not classified as presentation must not leak from
       // the authenticated user into the impersonated portfolio

libs/common/src/lib/helper.ts

@@ -33,7 +33,7 @@ import {
   uk,
   zhCN
 } from 'date-fns/locale';
-import { get, isNil, isString, pick } from 'lodash';
+import { get, isNil, isString } from 'lodash';
 
 import {
   DEFAULT_CURRENCY,
@@ -63,9 +63,17 @@ export const DATE_FORMAT_YEARLY = 'yyyy';
 // Settings which describe the person looking at the screen rather than the
 // portfolio being looked at. They stay with the authenticated user while
 // impersonating. Every other setting follows the impersonated user.
+// The filters are included because they are always written back to the
+// authenticated user, so reading them from the impersonated user would
+// overwrite the filters of the authenticated user.
 const PRESENTATION_USER_SETTINGS_KEYS: (keyof UserSettings)[] = [
   'colorScheme',
   'dateRange',
+  'filters.accounts',
+  'filters.assetClasses',
+  'filters.dataSource',
+  'filters.symbol',
+  'filters.tags',
   'holdingsViewMode',
   'isExperimentalFeatures',
   'isRestrictedView',
@@ -699,6 +707,10 @@ export function resolveUserSettings({
 
   return {
     ...impersonationUserSettings,
-    ...pick(userSettings ?? {}, PRESENTATION_USER_SETTINGS_KEYS)
+    ...Object.fromEntries(
+      PRESENTATION_USER_SETTINGS_KEYS.map((key) => {
+        return [key, userSettings?.[key]];
+      })
+    )
   };
 }