feat: add oauth2 code verifier

* fix: ensure oauth state param matches before finishing oauth flow

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>

* chore: upgrade openid-client to v6

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>

* feat: use PKCE for oauth2 on supported clients

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>

* feat: use state and PKCE in mobile app

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>

* fix: remove obsolete oauth repository init

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>

* fix: rewrite callback url if mobile redirect url is enabled

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>

* fix: propagate oidc client error cause when oauth callback fails

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>

* fix: adapt auth service tests to required state and PKCE params

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>

* fix: update sdk types

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>

* fix: adapt oauth e2e test to work with PKCE

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>

* fix: allow insecure (http) oauth clients

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>

---------

Signed-off-by: Tin Pecirep <tin.pecirep@gmail.com>
Co-authored-by: Jason Rasmussen <jason@rasm.me>
This commit is contained in:
Tin Pecirep 2025-04-23 16:05:00 +02:00 committed by Zack Pollard
parent 13d6bd67b1
commit b7a0cf2470
18 changed files with 469 additions and 192 deletions
server/src/services

View file

@ -55,7 +55,7 @@ describe(AuthService.name, () => {
beforeEach(() => {
({ sut, mocks } = newTestService(AuthService));
mocks.oauth.authorize.mockResolvedValue('access-token');
mocks.oauth.authorize.mockResolvedValue({ url: 'http://test', state: 'state', codeVerifier: 'codeVerifier' });
mocks.oauth.getProfile.mockResolvedValue({ sub, email });
mocks.oauth.getLogoutEndpoint.mockResolvedValue('http://end-session-endpoint');
});
@ -64,16 +64,6 @@ describe(AuthService.name, () => {
expect(sut).toBeDefined();
});
describe('onBootstrap', () => {
it('should init the repo', () => {
mocks.oauth.init.mockResolvedValue();
sut.onBootstrap();
expect(mocks.oauth.init).toHaveBeenCalled();
});
});
describe('login', () => {
it('should throw an error if password login is disabled', async () => {
mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.disabled);
@ -519,16 +509,22 @@ describe(AuthService.name, () => {
describe('callback', () => {
it('should throw an error if OAuth is not enabled', async () => {
await expect(sut.callback({ url: '' }, loginDetails)).rejects.toBeInstanceOf(BadRequestException);
await expect(
sut.callback({ url: '', state: 'xyz789', codeVerifier: 'foo' }, {}, loginDetails),
).rejects.toBeInstanceOf(BadRequestException);
});
it('should not allow auto registering', async () => {
mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.oauthEnabled);
mocks.user.getByEmail.mockResolvedValue(void 0);
await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).rejects.toBeInstanceOf(
BadRequestException,
);
await expect(
sut.callback(
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' },
{},
loginDetails,
),
).rejects.toBeInstanceOf(BadRequestException);
expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1);
});
@ -541,9 +537,13 @@ describe(AuthService.name, () => {
mocks.user.update.mockResolvedValue(user);
mocks.session.create.mockResolvedValue(factory.session());
await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).resolves.toEqual(
oauthResponse(user),
);
await expect(
sut.callback(
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foobar' },
{},
loginDetails,
),
).resolves.toEqual(oauthResponse(user));
expect(mocks.user.getByEmail).toHaveBeenCalledTimes(1);
expect(mocks.user.update).toHaveBeenCalledWith(user.id, { oauthId: sub });
@ -557,9 +557,13 @@ describe(AuthService.name, () => {
mocks.user.getAdmin.mockResolvedValue(user);
mocks.user.create.mockResolvedValue(user);
await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).rejects.toThrow(
BadRequestException,
);
await expect(
sut.callback(
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foobar' },
{},
loginDetails,
),
).rejects.toThrow(BadRequestException);
expect(mocks.user.update).not.toHaveBeenCalled();
expect(mocks.user.create).not.toHaveBeenCalled();
@ -574,9 +578,13 @@ describe(AuthService.name, () => {
mocks.user.create.mockResolvedValue(user);
mocks.session.create.mockResolvedValue(factory.session());
await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).resolves.toEqual(
oauthResponse(user),
);
await expect(
sut.callback(
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foobar' },
{},
loginDetails,
),
).resolves.toEqual(oauthResponse(user));
expect(mocks.user.getByEmail).toHaveBeenCalledTimes(2); // second call is for domain check before create
expect(mocks.user.create).toHaveBeenCalledTimes(1);
@ -592,18 +600,19 @@ describe(AuthService.name, () => {
mocks.session.create.mockResolvedValue(factory.session());
mocks.oauth.getProfile.mockResolvedValue({ sub, email: undefined });
await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).rejects.toBeInstanceOf(
BadRequestException,
);
await expect(
sut.callback(
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foobar' },
{},
loginDetails,
),
).rejects.toBeInstanceOf(BadRequestException);
expect(mocks.user.getByEmail).not.toHaveBeenCalled();
expect(mocks.user.create).not.toHaveBeenCalled();
});
for (const url of [
'app.immich:/',
'app.immich://',
'app.immich:///',
'app.immich:/oauth-callback?code=abc123',
'app.immich://oauth-callback?code=abc123',
'app.immich:///oauth-callback?code=abc123',
@ -615,9 +624,14 @@ describe(AuthService.name, () => {
mocks.user.getByOAuthId.mockResolvedValue(user);
mocks.session.create.mockResolvedValue(factory.session());
await sut.callback({ url }, loginDetails);
await sut.callback({ url, state: 'xyz789', codeVerifier: 'foo' }, {}, loginDetails);
expect(mocks.oauth.getProfile).toHaveBeenCalledWith(expect.objectContaining({}), url, 'http://mobile-redirect');
expect(mocks.oauth.getProfile).toHaveBeenCalledWith(
expect.objectContaining({}),
'http://mobile-redirect?code=abc123',
'xyz789',
'foo',
);
});
}
@ -630,9 +644,13 @@ describe(AuthService.name, () => {
mocks.user.create.mockResolvedValue(user);
mocks.session.create.mockResolvedValue(factory.session());
await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).resolves.toEqual(
oauthResponse(user),
);
await expect(
sut.callback(
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' },
{},
loginDetails,
),
).resolves.toEqual(oauthResponse(user));
expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ quotaSizeInBytes: 1_073_741_824 }));
});
@ -647,9 +665,13 @@ describe(AuthService.name, () => {
mocks.user.create.mockResolvedValue(user);
mocks.session.create.mockResolvedValue(factory.session());
await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).resolves.toEqual(
oauthResponse(user),
);
await expect(
sut.callback(
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' },
{},
loginDetails,
),
).resolves.toEqual(oauthResponse(user));
expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ quotaSizeInBytes: 1_073_741_824 }));
});
@ -664,9 +686,13 @@ describe(AuthService.name, () => {
mocks.user.create.mockResolvedValue(user);
mocks.session.create.mockResolvedValue(factory.session());
await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).resolves.toEqual(
oauthResponse(user),
);
await expect(
sut.callback(
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' },
{},
loginDetails,
),
).resolves.toEqual(oauthResponse(user));
expect(mocks.user.create).toHaveBeenCalledWith(expect.objectContaining({ quotaSizeInBytes: 1_073_741_824 }));
});
@ -681,9 +707,13 @@ describe(AuthService.name, () => {
mocks.user.create.mockResolvedValue(user);
mocks.session.create.mockResolvedValue(factory.session());
await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).resolves.toEqual(
oauthResponse(user),
);
await expect(
sut.callback(
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' },
{},
loginDetails,
),
).resolves.toEqual(oauthResponse(user));
expect(mocks.user.create).toHaveBeenCalledWith({
email: user.email,
@ -705,9 +735,13 @@ describe(AuthService.name, () => {
mocks.user.create.mockResolvedValue(user);
mocks.session.create.mockResolvedValue(factory.session());
await expect(sut.callback({ url: 'http://immich/auth/login?code=abc123' }, loginDetails)).resolves.toEqual(
oauthResponse(user),
);
await expect(
sut.callback(
{ url: 'http://immich/auth/login?code=abc123', state: 'xyz789', codeVerifier: 'foo' },
{},
loginDetails,
),
).resolves.toEqual(oauthResponse(user));
expect(mocks.user.create).toHaveBeenCalledWith({
email: user.email,
@ -779,7 +813,11 @@ describe(AuthService.name, () => {
mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled);
mocks.user.update.mockResolvedValue(user);
await sut.link(auth, { url: 'http://immich/user-settings?code=abc123' });
await sut.link(
auth,
{ url: 'http://immich/user-settings?code=abc123', state: 'xyz789', codeVerifier: 'foo' },
{},
);
expect(mocks.user.update).toHaveBeenCalledWith(auth.user.id, { oauthId: sub });
});
@ -792,9 +830,9 @@ describe(AuthService.name, () => {
mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.enabled);
mocks.user.getByOAuthId.mockResolvedValue({ id: 'other-user' } as UserAdmin);
await expect(sut.link(auth, { url: 'http://immich/user-settings?code=abc123' })).rejects.toBeInstanceOf(
BadRequestException,
);
await expect(
sut.link(auth, { url: 'http://immich/user-settings?code=abc123', state: 'xyz789', codeVerifier: 'foo' }, {}),
).rejects.toBeInstanceOf(BadRequestException);
expect(mocks.user.update).not.toHaveBeenCalled();
});