fix: review notes

This commit is contained in:
bo0tzz
2026-04-16 21:39:48 +02:00
parent 5731c261eb
commit b42fdcfca9
11 changed files with 72 additions and 154 deletions
@@ -9,7 +9,6 @@ import {
OAuthBackchannelLogoutDto,
OAuthCallbackDto,
OAuthConfigDto,
OAuthLinkDto,
} from 'src/dtos/auth.dto';
import { UserAdminResponseDto } from 'src/dtos/user.dto';
import { ApiTag, AuthType, ImmichCookie } from 'src/enum';
@@ -87,22 +86,6 @@ export class OAuthController {
});
}
@Post('link')
@Authenticated()
@HttpCode(HttpStatus.OK)
@Endpoint({
summary: 'Link OAuth account',
description: 'Link an OAuth account to the authenticated user.',
history: new HistoryBuilder().added('v1').beta('v1').stable('v2'),
})
linkOAuthAccount(
@Req() request: Request,
@Auth() auth: AuthDto,
@Body() dto: OAuthLinkDto,
): Promise<UserAdminResponseDto> {
return this.service.link(auth, dto, request.headers);
}
@Post('unlink')
@Authenticated()
@HttpCode(HttpStatus.OK)
+1 -13
View File
@@ -23,6 +23,7 @@ const LoginCredentialSchema = z
.object({
email: toEmail.describe('User email').meta({ example: 'testuser@email.com' }),
password: z.string().describe('User password').meta({ example: 'password' }),
linkToken: z.string().optional().describe('OAuth link token to consume on successful login'),
})
.meta({ id: 'LoginCredentialDto' });
@@ -110,18 +111,6 @@ 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'),
@@ -161,7 +150,6 @@ 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) {}
@@ -13,7 +13,6 @@ export class OAuthLinkTokenRepository {
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')
+33 -2
View File
@@ -88,6 +88,37 @@ describe(AuthService.name, () => {
expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1);
});
it('should link an OAuth account when linkToken is provided', async () => {
const user = UserFactory.create({ password: 'immich_password' });
const session = SessionFactory.create();
mocks.user.getByEmail.mockResolvedValue(user);
mocks.session.create.mockResolvedValue(session);
mocks.oauthLinkToken.consumeToken.mockResolvedValue({
id: 'token-id',
oauthSub: 'oauth-sub-123',
userEmail: user.email,
token: Buffer.from('hashed'),
expiresAt: new Date(Date.now() + 600_000),
createdAt: new Date(),
});
mocks.user.update.mockResolvedValue(user);
await sut.login({ email, password: 'password', linkToken: 'plain-token' }, loginDetails);
expect(mocks.oauthLinkToken.consumeToken).toHaveBeenCalledTimes(1);
expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: 'oauth-sub-123' });
});
it('should reject login with invalid linkToken', async () => {
const user = UserFactory.create({ password: 'immich_password' });
mocks.user.getByEmail.mockResolvedValue(user);
mocks.oauthLinkToken.consumeToken.mockResolvedValue(null as any);
await expect(sut.login({ email, password: 'password', linkToken: 'bad-token' }, loginDetails)).rejects.toThrow(
'Invalid or expired link token',
);
});
});
describe('changePassword', () => {
@@ -718,7 +749,7 @@ describe(AuthService.name, () => {
{},
loginDetails,
),
).rejects.toThrow('oauth_account_link_required');
).rejects.toThrow(ForbiddenException);
expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1);
expect(mocks.user.update).not.toHaveBeenCalled();
@@ -762,7 +793,7 @@ describe(AuthService.name, () => {
{},
loginDetails,
),
).rejects.toThrow('oauth_account_link_required');
).rejects.toThrow(ForbiddenException);
expect(mocks.user.update).not.toHaveBeenCalled();
expect(mocks.user.create).not.toHaveBeenCalled();
+17 -2
View File
@@ -75,6 +75,21 @@ export class AuthService extends BaseService {
throw new UnauthorizedException('Incorrect email or password');
}
if (dto.linkToken) {
const hashedToken = this.cryptoRepository.hashSha256(dto.linkToken);
const record = await this.oauthLinkTokenRepository.consumeToken(hashedToken);
if (!record) {
throw new BadRequestException('Invalid or expired link token');
}
const duplicate = await this.userRepository.getByOAuthId(record.oauthSub);
if (duplicate && duplicate.id !== user.id) {
throw new BadRequestException('This OAuth account has already been linked to another user.');
}
await this.userRepository.update(user.id, { oauthId: record.oauthSub });
}
return this.createLoginResponse(user, details);
}
@@ -330,9 +345,9 @@ export class AuthService extends BaseService {
token: hashedToken,
oauthSub: profile.sub,
userEmail: emailUser.email,
expiresAt: new Date(Date.now() + 10 * 60 * 1000),
expiresAt: DateTime.now().plus({ minutes: 10 }).toJSDate(),
});
throw new BadRequestException({
throw new ForbiddenException({
message: 'oauth_account_link_required',
userEmail: emailUser.email,
linkToken: plainToken,