From 41befc0948542d06d79dfa2e56305f22e54456ae Mon Sep 17 00:00:00 2001 From: Jonathan Jogenfors Date: Wed, 11 Oct 2023 04:37:13 +0200 Subject: [PATCH] fix(server): don't publicly reveal user count (#4409) * fix: don't reveal user count publicly * fix: mobile and user controller * fix: update other frontend endpoints * fix: revert openapi change * chore: open api * fix: initialize * openapi --------- Co-authored-by: Alex Tran --- cli/src/api/open-api/api.ts | 15 +++++++++++++++ .../providers/server_info.provider.dart | 1 + mobile/openapi/doc/ServerConfigDto.md | Bin 524 -> 559 bytes mobile/openapi/doc/UserApi.md | Bin 19067 -> 20094 bytes .../openapi/lib/model/server_config_dto.dart | Bin 3750 -> 4054 bytes .../openapi/test/server_config_dto_test.dart | Bin 905 -> 1014 bytes server/immich-openapi-specs.json | 17 ++++++++++++++++- .../domain/repositories/user.repository.ts | 1 + .../src/domain/server-info/server-info.dto.ts | 1 + .../domain/server-info/server-info.service.ts | 3 +++ .../src/immich/controllers/user.controller.ts | 4 ++-- .../src/infra/repositories/user.repository.ts | 4 ++++ server/test/e2e/server-info.e2e-spec.ts | 1 + server/test/e2e/user.e2e-spec.ts | 6 +++--- .../test/repositories/user.repository.mock.ts | 1 + web/src/api/open-api/api.ts | 15 +++++++++++++++ web/src/lib/stores/server-config.store.ts | 1 + web/src/routes/+page.server.ts | 6 +++--- web/src/routes/auth/login/+page.server.ts | 4 ++-- web/src/routes/auth/register/+page.server.ts | 4 ++-- 20 files changed, 71 insertions(+), 13 deletions(-) diff --git a/cli/src/api/open-api/api.ts b/cli/src/api/open-api/api.ts index 824dd38835..2be639a0f1 100644 --- a/cli/src/api/open-api/api.ts +++ b/cli/src/api/open-api/api.ts @@ -2582,6 +2582,12 @@ export interface SearchResponseDto { * @interface ServerConfigDto */ export interface ServerConfigDto { + /** + * + * @type {boolean} + * @memberof ServerConfigDto + */ + 'isInitialized': boolean; /** * * @type {string} @@ -15081,6 +15087,15 @@ export const UserApiAxiosParamCreator = function (configuration?: Configuration) const localVarHeaderParameter = {} as any; const localVarQueryParameter = {} as any; + // authentication cookie required + + // authentication api_key required + await setApiKeyToObject(localVarHeaderParameter, "x-api-key", configuration) + + // authentication bearer required + // http bearer authentication required + await setBearerAuthToObject(localVarHeaderParameter, configuration) + if (admin !== undefined) { localVarQueryParameter['admin'] = admin; } diff --git a/mobile/lib/shared/providers/server_info.provider.dart b/mobile/lib/shared/providers/server_info.provider.dart index b4e05ffc93..ce431e7ab0 100644 --- a/mobile/lib/shared/providers/server_info.provider.dart +++ b/mobile/lib/shared/providers/server_info.provider.dart @@ -34,6 +34,7 @@ class ServerInfoNotifier extends StateNotifier { mapTileUrl: "https://tile.openstreetmap.org/{z}/{x}/{y}.png", oauthButtonText: "", trashDays: 30, + isInitialized: false, ), isVersionMismatch: false, versionMismatchErrorMessage: "", diff --git a/mobile/openapi/doc/ServerConfigDto.md b/mobile/openapi/doc/ServerConfigDto.md index 6b0029c992aaab3d12bf444fe8e5fb9234941332..cbe56d2f49d3b96ba1b9d04dbf1ce493f0376352 100644 GIT binary patch delta 43 ucmeBSSg+`ILNp;Pra0JbEvf%KLE#Z3$*|M delta 49 zcmex2hw=9m#tk+WlOMCIZ&tE7$SCQTuaH<;l96AOS(R9lnV+Xnlv-GtS(KWx*}=|E F008~p6HWjC diff --git a/mobile/openapi/lib/model/server_config_dto.dart b/mobile/openapi/lib/model/server_config_dto.dart index 3f4950ec500ba350118b71d18a72f4c5e816f00b..25bdeb6d55e08ccd951d3d314cd893865242e47d 100644 GIT binary patch delta 282 zcmZ1`drf{rKO=8uv1eXpNoHbBW>sp+; getAdmin(): Promise; + hasAdmin(): Promise; getByEmail(email: string, withPassword?: boolean): Promise; getByStorageLabel(storageLabel: string): Promise; getByOAuthId(oauthId: string): Promise; diff --git a/server/src/domain/server-info/server-info.dto.ts b/server/src/domain/server-info/server-info.dto.ts index 9bbda0f875..2b9ac95cc5 100644 --- a/server/src/domain/server-info/server-info.dto.ts +++ b/server/src/domain/server-info/server-info.dto.ts @@ -85,6 +85,7 @@ export class ServerConfigDto { mapTileUrl!: string; @ApiProperty({ type: 'integer' }) trashDays!: number; + isInitialized!: boolean; } export class ServerFeaturesDto implements FeatureFlags { diff --git a/server/src/domain/server-info/server-info.service.ts b/server/src/domain/server-info/server-info.service.ts index 69a925e86f..d68b484735 100644 --- a/server/src/domain/server-info/server-info.service.ts +++ b/server/src/domain/server-info/server-info.service.ts @@ -74,11 +74,14 @@ export class ServerInfoService { // TODO move to system config const loginPageMessage = process.env.PUBLIC_LOGIN_PAGE_MESSAGE || ''; + const isInitialized = await this.userRepository.hasAdmin(); + return { loginPageMessage, mapTileUrl: config.map.tileUrl, trashDays: config.trash.days, oauthButtonText: config.oauth.buttonText, + isInitialized, }; } diff --git a/server/src/immich/controllers/user.controller.ts b/server/src/immich/controllers/user.controller.ts index 925fcf2e2f..01bc676c5e 100644 --- a/server/src/immich/controllers/user.controller.ts +++ b/server/src/immich/controllers/user.controller.ts @@ -26,7 +26,7 @@ import { } from '@nestjs/common'; import { ApiBody, ApiConsumes, ApiTags } from '@nestjs/swagger'; import { Response as Res } from 'express'; -import { AdminRoute, AuthUser, Authenticated, PublicRoute } from '../app.guard'; +import { AdminRoute, AuthUser, Authenticated } from '../app.guard'; import { FileUploadInterceptor, Route } from '../app.interceptor'; import { UseValidation } from '../app.utils'; import { UUIDParamDto } from './dto/uuid-param.dto'; @@ -59,7 +59,7 @@ export class UserController { return this.service.create(createUserDto); } - @PublicRoute() + @AdminRoute() @Get('count') getUserCount(@Query() dto: CountDto): Promise { return this.service.getCount(dto); diff --git a/server/src/infra/repositories/user.repository.ts b/server/src/infra/repositories/user.repository.ts index 0fa1121289..559f16aa26 100644 --- a/server/src/infra/repositories/user.repository.ts +++ b/server/src/infra/repositories/user.repository.ts @@ -16,6 +16,10 @@ export class UserRepository implements IUserRepository { return this.userRepository.findOne({ where: { isAdmin: true } }); } + async hasAdmin(): Promise { + return this.userRepository.exist({ where: { isAdmin: true } }); + } + async getByEmail(email: string, withPassword?: boolean): Promise { let builder = this.userRepository.createQueryBuilder('user').where({ email }); diff --git a/server/test/e2e/server-info.e2e-spec.ts b/server/test/e2e/server-info.e2e-spec.ts index efdbbe5218..43cf471f40 100644 --- a/server/test/e2e/server-info.e2e-spec.ts +++ b/server/test/e2e/server-info.e2e-spec.ts @@ -102,6 +102,7 @@ describe(`${ServerInfoController.name} (e2e)`, () => { oauthButtonText: 'Login with OAuth', mapTileUrl: 'https://tile.openstreetmap.org/{z}/{x}/{y}.png', trashDays: 30, + isInitialized: true, }); }); }); diff --git a/server/test/e2e/user.e2e-spec.ts b/server/test/e2e/user.e2e-spec.ts index 9b976bc267..af0cbde745 100644 --- a/server/test/e2e/user.e2e-spec.ts +++ b/server/test/e2e/user.e2e-spec.ts @@ -311,10 +311,10 @@ describe(`${UserController.name}`, () => { }); describe('GET /user/count', () => { - it('should not require authentication', async () => { + it('should require authentication', async () => { const { status, body } = await request(server).get(`/user/count`); - expect(status).toBe(200); - expect(body).toEqual({ userCount: 1 }); + expect(status).toBe(401); + expect(body).toEqual(errorStub.unauthorized); }); it('should start with just the admin', async () => { diff --git a/server/test/repositories/user.repository.mock.ts b/server/test/repositories/user.repository.mock.ts index 09e2ef7bf1..30017e758e 100644 --- a/server/test/repositories/user.repository.mock.ts +++ b/server/test/repositories/user.repository.mock.ts @@ -14,5 +14,6 @@ export const newUserRepositoryMock = (): jest.Mocked => { delete: jest.fn(), getDeletedUsers: jest.fn(), restore: jest.fn(), + hasAdmin: jest.fn(), }; }; diff --git a/web/src/api/open-api/api.ts b/web/src/api/open-api/api.ts index 824dd38835..2be639a0f1 100644 --- a/web/src/api/open-api/api.ts +++ b/web/src/api/open-api/api.ts @@ -2582,6 +2582,12 @@ export interface SearchResponseDto { * @interface ServerConfigDto */ export interface ServerConfigDto { + /** + * + * @type {boolean} + * @memberof ServerConfigDto + */ + 'isInitialized': boolean; /** * * @type {string} @@ -15081,6 +15087,15 @@ export const UserApiAxiosParamCreator = function (configuration?: Configuration) const localVarHeaderParameter = {} as any; const localVarQueryParameter = {} as any; + // authentication cookie required + + // authentication api_key required + await setApiKeyToObject(localVarHeaderParameter, "x-api-key", configuration) + + // authentication bearer required + // http bearer authentication required + await setBearerAuthToObject(localVarHeaderParameter, configuration) + if (admin !== undefined) { localVarQueryParameter['admin'] = admin; } diff --git a/web/src/lib/stores/server-config.store.ts b/web/src/lib/stores/server-config.store.ts index 0cc9911e07..723ad49bd9 100644 --- a/web/src/lib/stores/server-config.store.ts +++ b/web/src/lib/stores/server-config.store.ts @@ -27,6 +27,7 @@ export const serverConfig = writable({ mapTileUrl: '', loginPageMessage: '', trashDays: 30, + isInitialized: false, }); export const loadConfig = async () => { diff --git a/web/src/routes/+page.server.ts b/web/src/routes/+page.server.ts index 51a6ef71d0..b469170bed 100644 --- a/web/src/routes/+page.server.ts +++ b/web/src/routes/+page.server.ts @@ -10,10 +10,10 @@ export const load = (async ({ parent, locals: { api } }) => { throw redirect(302, AppRoute.PHOTOS); } - const { data } = await api.userApi.getUserCount({ admin: true }); + const { data } = await api.serverInfoApi.getServerConfig(); - if (data.userCount > 0) { - // Redirect to login page if an admin is already registered. + if (data.isInitialized) { + // Redirect to login page if there exists an admin account (i.e. server is initialized) throw redirect(302, AppRoute.AUTH_LOGIN); } diff --git a/web/src/routes/auth/login/+page.server.ts b/web/src/routes/auth/login/+page.server.ts index f325c7cd2e..a294173b72 100644 --- a/web/src/routes/auth/login/+page.server.ts +++ b/web/src/routes/auth/login/+page.server.ts @@ -3,8 +3,8 @@ import { redirect } from '@sveltejs/kit'; import type { PageServerLoad } from './$types'; export const load = (async ({ locals: { api } }) => { - const { data } = await api.userApi.getUserCount({ admin: true }); - if (data.userCount === 0) { + const { data } = await api.serverInfoApi.getServerConfig(); + if (!data.isInitialized) { // Admin not registered throw redirect(302, AppRoute.AUTH_REGISTER); } diff --git a/web/src/routes/auth/register/+page.server.ts b/web/src/routes/auth/register/+page.server.ts index 85b4d9bc72..186ab2e3d8 100644 --- a/web/src/routes/auth/register/+page.server.ts +++ b/web/src/routes/auth/register/+page.server.ts @@ -3,8 +3,8 @@ import { redirect } from '@sveltejs/kit'; import type { PageServerLoad } from './$types'; export const load = (async ({ locals: { api } }) => { - const { data } = await api.userApi.getUserCount({ admin: true }); - if (data.userCount != 0) { + const { data } = await api.serverInfoApi.getServerConfig(); + if (data.isInitialized) { // Admin has been registered, redirect to login throw redirect(302, AppRoute.AUTH_LOGIN); }