mirror of
https://github.com/immich-app/immich.git
synced 2026-05-18 03:10:24 +03:00
fix: require users to authenticate existing Immich account before OAuth linking
This commit is contained in:
@@ -9,6 +9,7 @@ import {
|
||||
OAuthBackchannelLogoutDto,
|
||||
OAuthCallbackDto,
|
||||
OAuthConfigDto,
|
||||
OAuthLinkDto,
|
||||
} from 'src/dtos/auth.dto';
|
||||
import { UserAdminResponseDto } from 'src/dtos/user.dto';
|
||||
import { ApiTag, AuthType, ImmichCookie } from 'src/enum';
|
||||
@@ -97,7 +98,7 @@ export class OAuthController {
|
||||
linkOAuthAccount(
|
||||
@Req() request: Request,
|
||||
@Auth() auth: AuthDto,
|
||||
@Body() dto: OAuthCallbackDto,
|
||||
@Body() dto: OAuthLinkDto,
|
||||
): Promise<UserAdminResponseDto> {
|
||||
return this.service.link(auth, dto, request.headers);
|
||||
}
|
||||
|
||||
@@ -110,6 +110,18 @@ const OAuthCallbackSchema = z
|
||||
})
|
||||
.meta({ id: 'OAuthCallbackDto' });
|
||||
|
||||
const OAuthLinkSchema = z
|
||||
.object({
|
||||
url: z.string().optional().describe('OAuth callback URL'),
|
||||
state: z.string().optional().describe('OAuth state parameter'),
|
||||
codeVerifier: z.string().optional().describe('OAuth code verifier (PKCE)'),
|
||||
linkToken: z.string().optional().describe('OAuth link token from prior callback'),
|
||||
})
|
||||
.refine((data) => data.url || data.linkToken, {
|
||||
message: 'Either url or linkToken is required',
|
||||
})
|
||||
.meta({ id: 'OAuthLinkDto' });
|
||||
|
||||
const OAuthConfigSchema = z
|
||||
.object({
|
||||
redirectUri: z.string().describe('OAuth redirect URI'),
|
||||
@@ -149,6 +161,7 @@ export class SessionUnlockDto extends createZodDto(SessionUnlockSchema) {}
|
||||
export class PinCodeChangeDto extends createZodDto(PinCodeChangeSchema) {}
|
||||
export class ValidateAccessTokenResponseDto extends createZodDto(ValidateAccessTokenResponseSchema) {}
|
||||
export class OAuthCallbackDto extends createZodDto(OAuthCallbackSchema) {}
|
||||
export class OAuthLinkDto extends createZodDto(OAuthLinkSchema) {}
|
||||
export class OAuthConfigDto extends createZodDto(OAuthConfigSchema) {}
|
||||
export class OAuthAuthorizeResponseDto extends createZodDto(OAuthAuthorizeResponseSchema) {}
|
||||
export class OAuthBackchannelLogoutDto extends createZodDto(OAuthBackchannelLogoutSchema) {}
|
||||
|
||||
@@ -25,6 +25,7 @@ import { MemoryRepository } from 'src/repositories/memory.repository';
|
||||
import { MetadataRepository } from 'src/repositories/metadata.repository';
|
||||
import { MoveRepository } from 'src/repositories/move.repository';
|
||||
import { NotificationRepository } from 'src/repositories/notification.repository';
|
||||
import { OAuthLinkTokenRepository } from 'src/repositories/oauth-link-token.repository';
|
||||
import { OAuthRepository } from 'src/repositories/oauth.repository';
|
||||
import { OcrRepository } from 'src/repositories/ocr.repository';
|
||||
import { PartnerRepository } from 'src/repositories/partner.repository';
|
||||
@@ -78,6 +79,7 @@ export const repositories = [
|
||||
MetadataRepository,
|
||||
MoveRepository,
|
||||
NotificationRepository,
|
||||
OAuthLinkTokenRepository,
|
||||
OAuthRepository,
|
||||
OcrRepository,
|
||||
PartnerRepository,
|
||||
|
||||
@@ -0,0 +1,37 @@
|
||||
import { Injectable } from '@nestjs/common';
|
||||
import { Insertable, Kysely } from 'kysely';
|
||||
import { DateTime } from 'luxon';
|
||||
import { InjectKysely } from 'nestjs-kysely';
|
||||
import { DB } from 'src/schema';
|
||||
import { OAuthLinkTokenTable } from 'src/schema/tables/oauth-link-token.table';
|
||||
|
||||
@Injectable()
|
||||
export class OAuthLinkTokenRepository {
|
||||
constructor(@InjectKysely() private db: Kysely<DB>) {}
|
||||
|
||||
create(dto: Insertable<OAuthLinkTokenTable>) {
|
||||
return this.db.insertInto('oauth_link_token').values(dto).returningAll().executeTakeFirstOrThrow();
|
||||
}
|
||||
|
||||
// Atomic consume: delete and return in one query (single-use guarantee)
|
||||
consumeToken(token: Buffer) {
|
||||
return this.db
|
||||
.deleteFrom('oauth_link_token')
|
||||
.where('token', '=', token)
|
||||
.where('expiresAt', '>', DateTime.now().toJSDate())
|
||||
.returningAll()
|
||||
.executeTakeFirst();
|
||||
}
|
||||
|
||||
async deleteByEmail(userEmail: string) {
|
||||
await this.db.deleteFrom('oauth_link_token').where('userEmail', '=', userEmail).execute();
|
||||
}
|
||||
|
||||
async cleanup() {
|
||||
const result = await this.db
|
||||
.deleteFrom('oauth_link_token')
|
||||
.where('expiresAt', '<=', DateTime.now().toJSDate())
|
||||
.execute();
|
||||
return Number(result[0]?.numDeletedRows ?? 0);
|
||||
}
|
||||
}
|
||||
@@ -50,6 +50,7 @@ import { MemoryTable } from 'src/schema/tables/memory.table';
|
||||
import { MoveTable } from 'src/schema/tables/move.table';
|
||||
import { NaturalEarthCountriesTable } from 'src/schema/tables/natural-earth-countries.table';
|
||||
import { NotificationTable } from 'src/schema/tables/notification.table';
|
||||
import { OAuthLinkTokenTable } from 'src/schema/tables/oauth-link-token.table';
|
||||
import { OcrSearchTable } from 'src/schema/tables/ocr-search.table';
|
||||
import { PartnerAuditTable } from 'src/schema/tables/partner-audit.table';
|
||||
import { PartnerTable } from 'src/schema/tables/partner.table';
|
||||
@@ -108,6 +109,7 @@ export class ImmichDatabase {
|
||||
MoveTable,
|
||||
NaturalEarthCountriesTable,
|
||||
NotificationTable,
|
||||
OAuthLinkTokenTable,
|
||||
OcrSearchTable,
|
||||
PartnerAuditTable,
|
||||
PartnerTable,
|
||||
@@ -210,6 +212,8 @@ export interface DB {
|
||||
|
||||
notification: NotificationTable;
|
||||
|
||||
oauth_link_token: OAuthLinkTokenTable;
|
||||
|
||||
move_history: MoveTable;
|
||||
|
||||
naturalearth_countries: NaturalEarthCountriesTable;
|
||||
|
||||
@@ -0,0 +1,15 @@
|
||||
import { Kysely, sql } from 'kysely';
|
||||
|
||||
export async function up(db: Kysely<any>): Promise<void> {
|
||||
await sql`CREATE TABLE "oauth_link_token" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "token" bytea NOT NULL, "oauthSub" character varying NOT NULL, "userEmail" character varying NOT NULL, "expiresAt" timestamp with time zone NOT NULL, "createdAt" timestamp with time zone NOT NULL DEFAULT now());`.execute(
|
||||
db,
|
||||
);
|
||||
await sql`ALTER TABLE "oauth_link_token" ADD CONSTRAINT "PK_oauth_link_token_id" PRIMARY KEY ("id");`.execute(db);
|
||||
await sql`CREATE INDEX "IDX_oauth_link_token_token" ON "oauth_link_token" ("token")`.execute(db);
|
||||
}
|
||||
|
||||
export async function down(db: Kysely<any>): Promise<void> {
|
||||
await sql`DROP INDEX "IDX_oauth_link_token_token";`.execute(db);
|
||||
await sql`ALTER TABLE "oauth_link_token" DROP CONSTRAINT "PK_oauth_link_token_id";`.execute(db);
|
||||
await sql`DROP TABLE "oauth_link_token";`.execute(db);
|
||||
}
|
||||
@@ -0,0 +1,22 @@
|
||||
import { Column, CreateDateColumn, Generated, PrimaryGeneratedColumn, Table, Timestamp } from '@immich/sql-tools';
|
||||
|
||||
@Table({ name: 'oauth_link_token' })
|
||||
export class OAuthLinkTokenTable {
|
||||
@PrimaryGeneratedColumn()
|
||||
id!: Generated<string>;
|
||||
|
||||
@Column({ type: 'bytea', index: true })
|
||||
token!: Buffer;
|
||||
|
||||
@Column()
|
||||
oauthSub!: string;
|
||||
|
||||
@Column()
|
||||
userEmail!: string;
|
||||
|
||||
@Column({ type: 'timestamp with time zone' })
|
||||
expiresAt!: Timestamp;
|
||||
|
||||
@CreateDateColumn()
|
||||
createdAt!: Generated<Timestamp>;
|
||||
}
|
||||
@@ -702,24 +702,28 @@ describe(AuthService.name, () => {
|
||||
expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('should link an existing user', async () => {
|
||||
it('should reject when existing user found by email and create a link token', async () => {
|
||||
const user = UserFactory.create();
|
||||
const profile = OAuthProfileFactory.create();
|
||||
|
||||
mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled);
|
||||
mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile });
|
||||
mocks.user.getByEmail.mockResolvedValue(user);
|
||||
mocks.user.update.mockResolvedValue(user);
|
||||
mocks.session.create.mockResolvedValue(SessionFactory.create());
|
||||
mocks.oauthLinkToken.deleteByEmail.mockResolvedValue();
|
||||
mocks.oauthLinkToken.create.mockResolvedValue({} as any);
|
||||
|
||||
await sut.callback(
|
||||
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foobar' },
|
||||
{},
|
||||
loginDetails,
|
||||
);
|
||||
await expect(
|
||||
sut.callback(
|
||||
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foobar' },
|
||||
{},
|
||||
loginDetails,
|
||||
),
|
||||
).rejects.toThrow('oauth_account_link_required');
|
||||
|
||||
expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1);
|
||||
expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: profile.sub });
|
||||
expect(mocks.user.update).not.toHaveBeenCalled();
|
||||
expect(mocks.oauthLinkToken.deleteByEmail).toHaveBeenCalledTimes(1);
|
||||
expect(mocks.oauthLinkToken.create).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('should normalize the email from the OAuth profile before linking', async () => {
|
||||
@@ -749,6 +753,8 @@ describe(AuthService.name, () => {
|
||||
mocks.oauth.getProfileAndOAuthSid.mockResolvedValue({ profile: OAuthProfileFactory.create() });
|
||||
mocks.user.getByEmail.mockResolvedValueOnce(user);
|
||||
mocks.user.getAdmin.mockResolvedValue(UserFactory.create({ isAdmin: true }));
|
||||
mocks.oauthLinkToken.deleteByEmail.mockResolvedValue();
|
||||
mocks.oauthLinkToken.create.mockResolvedValue({} as any);
|
||||
|
||||
await expect(
|
||||
sut.callback(
|
||||
@@ -756,7 +762,7 @@ describe(AuthService.name, () => {
|
||||
{},
|
||||
loginDetails,
|
||||
),
|
||||
).rejects.toThrow(BadRequestException);
|
||||
).rejects.toThrow('oauth_account_link_required');
|
||||
|
||||
expect(mocks.user.update).not.toHaveBeenCalled();
|
||||
expect(mocks.user.create).not.toHaveBeenCalled();
|
||||
|
||||
@@ -320,14 +320,23 @@ export class AuthService extends BaseService {
|
||||
this.logger.debug(`Logging in with OAuth: ${JSON.stringify(profile)}`);
|
||||
let user: UserAdmin | undefined = await this.userRepository.getByOAuthId(profile.sub);
|
||||
|
||||
// link by email
|
||||
if (!user && normalizedEmail) {
|
||||
const emailUser = await this.userRepository.getByEmail(normalizedEmail);
|
||||
if (emailUser) {
|
||||
if (emailUser.oauthId) {
|
||||
throw new BadRequestException('User already exists, but is linked to another account.');
|
||||
}
|
||||
user = await this.userRepository.update(emailUser.id, { oauthId: profile.sub });
|
||||
await this.oauthLinkTokenRepository.deleteByEmail(emailUser.email);
|
||||
const plainToken = this.cryptoRepository.randomBytesAsText(32);
|
||||
const hashedToken = this.cryptoRepository.hashSha256(plainToken);
|
||||
await this.oauthLinkTokenRepository.create({
|
||||
token: hashedToken,
|
||||
oauthSub: profile.sub,
|
||||
userEmail: emailUser.email,
|
||||
expiresAt: new Date(Date.now() + 10 * 60 * 1000),
|
||||
});
|
||||
throw new BadRequestException({
|
||||
message: 'oauth_account_link_required',
|
||||
userEmail: emailUser.email,
|
||||
linkToken: plainToken,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -32,6 +32,7 @@ import { MemoryRepository } from 'src/repositories/memory.repository';
|
||||
import { MetadataRepository } from 'src/repositories/metadata.repository';
|
||||
import { MoveRepository } from 'src/repositories/move.repository';
|
||||
import { NotificationRepository } from 'src/repositories/notification.repository';
|
||||
import { OAuthLinkTokenRepository } from 'src/repositories/oauth-link-token.repository';
|
||||
import { OAuthRepository } from 'src/repositories/oauth.repository';
|
||||
import { OcrRepository } from 'src/repositories/ocr.repository';
|
||||
import { PartnerRepository } from 'src/repositories/partner.repository';
|
||||
@@ -88,6 +89,7 @@ export const BASE_SERVICE_DEPENDENCIES = [
|
||||
MetadataRepository,
|
||||
MoveRepository,
|
||||
NotificationRepository,
|
||||
OAuthLinkTokenRepository,
|
||||
OAuthRepository,
|
||||
OcrRepository,
|
||||
PartnerRepository,
|
||||
@@ -146,6 +148,7 @@ export class BaseService {
|
||||
protected metadataRepository: MetadataRepository,
|
||||
protected moveRepository: MoveRepository,
|
||||
protected notificationRepository: NotificationRepository,
|
||||
protected oauthLinkTokenRepository: OAuthLinkTokenRepository,
|
||||
protected oauthRepository: OAuthRepository,
|
||||
protected ocrRepository: OcrRepository,
|
||||
protected partnerRepository: PartnerRepository,
|
||||
|
||||
@@ -24,6 +24,11 @@ export class SessionService extends BaseService {
|
||||
|
||||
this.logger.log(`Deleted ${sessions.length} expired session tokens`);
|
||||
|
||||
const expiredLinkTokens = await this.oauthLinkTokenRepository.cleanup();
|
||||
if (expiredLinkTokens > 0) {
|
||||
this.logger.log(`Deleted ${expiredLinkTokens} expired OAuth link tokens`);
|
||||
}
|
||||
|
||||
return JobStatus.Success;
|
||||
}
|
||||
|
||||
|
||||
@@ -43,6 +43,7 @@ import { MemoryRepository } from 'src/repositories/memory.repository';
|
||||
import { MetadataRepository } from 'src/repositories/metadata.repository';
|
||||
import { MoveRepository } from 'src/repositories/move.repository';
|
||||
import { NotificationRepository } from 'src/repositories/notification.repository';
|
||||
import { OAuthLinkTokenRepository } from 'src/repositories/oauth-link-token.repository';
|
||||
import { OAuthRepository } from 'src/repositories/oauth.repository';
|
||||
import { OcrRepository } from 'src/repositories/ocr.repository';
|
||||
import { PartnerRepository } from 'src/repositories/partner.repository';
|
||||
@@ -239,6 +240,7 @@ export type ServiceOverrides = {
|
||||
metadata: MetadataRepository;
|
||||
move: MoveRepository;
|
||||
notification: NotificationRepository;
|
||||
oauthLinkToken: OAuthLinkTokenRepository;
|
||||
ocr: OcrRepository;
|
||||
oauth: OAuthRepository;
|
||||
partner: PartnerRepository;
|
||||
@@ -321,6 +323,7 @@ export const getMocks = () => {
|
||||
move: automock(MoveRepository, { strict: false }),
|
||||
notification: automock(NotificationRepository),
|
||||
ocr: automock(OcrRepository, { strict: false }),
|
||||
oauthLinkToken: automock(OAuthLinkTokenRepository),
|
||||
oauth: automock(OAuthRepository, { args: [loggerMock] }),
|
||||
partner: automock(PartnerRepository, { strict: false }),
|
||||
person: automock(PersonRepository, { strict: false }),
|
||||
@@ -387,6 +390,7 @@ export const newTestService = <T extends BaseService>(
|
||||
overrides.metadata || (mocks.metadata as As<MetadataRepository>),
|
||||
overrides.move || (mocks.move as As<MoveRepository>),
|
||||
overrides.notification || (mocks.notification as As<NotificationRepository>),
|
||||
overrides.oauthLinkToken || (mocks.oauthLinkToken as As<OAuthLinkTokenRepository>),
|
||||
overrides.oauth || (mocks.oauth as As<OAuthRepository>),
|
||||
overrides.ocr || (mocks.ocr as As<OcrRepository>),
|
||||
overrides.partner || (mocks.partner as As<PartnerRepository>),
|
||||
|
||||
Reference in New Issue
Block a user