Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(mobile): use a valid OAuth callback URL #10832

Merged
merged 6 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions docs/docs/administration/oauth.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
This page contains details about using OAuth in Immich.

:::tip
Unable to set `app.immich:/` as a valid redirect URI? See [Mobile Redirect URI](#mobile-redirect-uri) for an alternative solution.
Unable to set `app.immich:///oauth-callback` as a valid redirect URI? See [Mobile Redirect URI](#mobile-redirect-uri) for an alternative solution.
:::

## Overview
Expand All @@ -30,15 +30,15 @@ Before enabling OAuth in Immich, a new client application needs to be configured

The **Sign-in redirect URIs** should include:

- `app.immich:/` - for logging in with OAuth from the [Mobile App](/docs/features/mobile-app.mdx)
- `app.immich:///oauth-callback` - for logging in with OAuth from the [Mobile App](/docs/features/mobile-app.mdx)
- `http://DOMAIN:PORT/auth/login` - for logging in with OAuth from the Web Client
- `http://DOMAIN:PORT/user-settings` - for manually linking OAuth in the Web Client

Redirect URIs should contain all the domains you will be using to access Immich. Some examples include:

Mobile

- `app.immich:/` (You **MUST** include this for iOS and Android mobile apps to work properly)
- `app.immich:///oauth-callback` (You **MUST** include this for iOS and Android mobile apps to work properly)

Localhost

Expand Down Expand Up @@ -96,16 +96,16 @@ When Auto Launch is enabled, the login page will automatically redirect the user

## Mobile Redirect URI

The redirect URI for the mobile app is `app.immich:/`, which is a [Custom Scheme](https://developer.apple.com/documentation/xcode/defining-a-custom-url-scheme-for-your-app). If this custom scheme is an invalid redirect URI for your OAuth Provider, you can work around this by doing the following:
The redirect URI for the mobile app is `app.immich:///oauth-callback`, which is a [Custom Scheme](https://developer.apple.com/documentation/xcode/defining-a-custom-url-scheme-for-your-app). If this custom scheme is an invalid redirect URI for your OAuth Provider, you can work around this by doing the following:

1. Configure an http(s) endpoint to forwards requests to `app.immich:/`
1. Configure an http(s) endpoint to forwards requests to `app.immich:///oauth-callback`
2. Whitelist the new endpoint as a valid redirect URI with your provider.
3. Specify the new endpoint as the `Mobile Redirect URI Override`, in the OAuth settings.

With these steps in place, you should be able to use OAuth from the [Mobile App](/docs/features/mobile-app.mdx) without a custom scheme redirect URI.

:::info
Immich has a route (`/api/oauth/mobile-redirect`) that is already configured to forward requests to `app.immich:/`, and can be used for step 1.
Immich has a route (`/api/oauth/mobile-redirect`) that is already configured to forward requests to `app.immich:///oauth-callback`, and can be used for step 1.
:::

## Example Configuration
Expand Down
4 changes: 2 additions & 2 deletions mobile/android/app/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
<action android:name="android.intent.action.VIEW" />
<category android:name="android.intent.category.DEFAULT" />
<category android:name="android.intent.category.BROWSABLE" />
<data android:scheme="app.immich" />
<data android:scheme="app.immich" android:pathPrefix="/oauth-callback"/>
</intent-filter>
</activity>
<!-- Don't delete the meta-data below.
Expand All @@ -94,4 +94,4 @@
<data android:scheme="geo" />
</intent>
</queries>
</manifest>
</manifest>
2 changes: 1 addition & 1 deletion mobile/lib/pages/login/login.page.dart
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class LoginPage extends HookConsumerWidget {
);

return Scaffold(
body: const LoginForm(),
body: LoginForm(),
bottomNavigationBar: SafeArea(
child: Padding(
padding: const EdgeInsets.only(bottom: 16.0),
Expand Down
42 changes: 27 additions & 15 deletions mobile/lib/services/oauth.service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import 'package:logging/logging.dart';
import 'package:openapi/api.dart';
import 'package:flutter_web_auth/flutter_web_auth.dart';

// Redirect URL = app.immich://
// Redirect URL = app.immich:///oauth-callback

class OAuthService {
final ApiService _apiService;
Expand All @@ -16,28 +16,40 @@ class OAuthService {
) async {
// Resolve API server endpoint from user provided serverUrl
await _apiService.resolveAndSetEndpoint(serverUrl);
final redirectUri = '$callbackUrlScheme:///oauth-callback';
log.info(
"Starting OAuth flow with redirect URI: $redirectUri",
);

final dto = await _apiService.oAuthApi.startOAuth(
OAuthConfigDto(redirectUri: '$callbackUrlScheme:/'),
OAuthConfigDto(redirectUri: redirectUri),
);
return dto?.url;

final authUrl = dto?.url;
log.info('Received Authorization URL: $authUrl');

return authUrl;
}

Future<LoginResponseDto?> oAuthLogin(String oauthUrl) async {
try {
var result = await FlutterWebAuth.authenticate(
url: oauthUrl,
callbackUrlScheme: callbackUrlScheme,
);
String result = await FlutterWebAuth.authenticate(
url: oauthUrl,
callbackUrlScheme: callbackUrlScheme,
);

log.info('Received OAuth callback: $result');

return await _apiService.oAuthApi.finishOAuth(
OAuthCallbackDto(
url: result,
),
if (result.startsWith('app.immich:/oauth-callback')) {
result = result.replaceAll(
'app.immich:/oauth-callback',
'app.immich:///oauth-callback',
);
} catch (e, stack) {
log.severe("OAuth login failed", e, stack);
return null;
}

return await _apiService.oAuthApi.finishOAuth(
OAuthCallbackDto(
url: result,
),
);
}
}
44 changes: 30 additions & 14 deletions mobile/lib/widgets/forms/login/login_form.dart
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,15 @@ import 'package:immich_mobile/widgets/forms/login/login_button.dart';
import 'package:immich_mobile/widgets/forms/login/o_auth_login_button.dart';
import 'package:immich_mobile/widgets/forms/login/password_input.dart';
import 'package:immich_mobile/widgets/forms/login/server_endpoint_input.dart';
import 'package:logging/logging.dart';
import 'package:openapi/api.dart';
import 'package:package_info_plus/package_info_plus.dart';
import 'package:permission_handler/permission_handler.dart';

class LoginForm extends HookConsumerWidget {
const LoginForm({super.key});
LoginForm({super.key});

final log = Logger('LoginForm');

@override
Widget build(BuildContext context, WidgetRef ref) {
Expand Down Expand Up @@ -229,7 +232,9 @@ class LoginForm extends HookConsumerWidget {
.getOAuthServerUrl(sanitizeUrl(serverEndpointController.text));

isLoading.value = true;
} catch (e) {
} catch (error, stack) {
log.severe('Error getting OAuth server Url: $error', stack);

ImmichToast.show(
context: context,
msg: "login_form_failed_get_oauth_server_config".tr(),
Expand All @@ -241,10 +246,19 @@ class LoginForm extends HookConsumerWidget {
}

if (oAuthServerUrl != null) {
var loginResponseDto = await oAuthService.oAuthLogin(oAuthServerUrl);
try {
final loginResponseDto =
await oAuthService.oAuthLogin(oAuthServerUrl);

if (loginResponseDto == null) {
return;
}

log.info(
"Finished OAuth login with response: ${loginResponseDto.userEmail}",
);

if (loginResponseDto != null) {
var isSuccess = await ref
final isSuccess = await ref
.watch(authenticationProvider.notifier)
.setSuccessLoginInfo(
accessToken: loginResponseDto.accessToken,
Expand All @@ -258,17 +272,19 @@ class LoginForm extends HookConsumerWidget {
ref.watch(backupProvider.notifier).resumeBackup();
}
context.replaceRoute(const TabControllerRoute());
} else {
ImmichToast.show(
context: context,
msg: "login_form_failed_login".tr(),
toastType: ToastType.error,
gravity: ToastGravity.TOP,
);
}
}
} catch (error, stack) {
log.severe('Error logging in with OAuth: $error', stack);

isLoading.value = false;
ImmichToast.show(
context: context,
msg: error.toString(),
toastType: ToastType.error,
gravity: ToastGravity.TOP,
);
} finally {
isLoading.value = false;
}
} else {
ImmichToast.show(
context: context,
Expand Down
2 changes: 1 addition & 1 deletion server/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export const resourcePaths = {
},
};

export const MOBILE_REDIRECT = 'app.immich:/';
export const MOBILE_REDIRECT = 'app.immich:///oauth-callback';
export const LOGIN_URL = '/auth/login?autoLaunch=0';

export enum AuthType {
Expand Down
42 changes: 21 additions & 21 deletions server/src/services/auth.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -423,11 +423,13 @@ describe('AuthService', () => {

describe('getMobileRedirect', () => {
it('should pass along the query params', () => {
expect(sut.getMobileRedirect('http://immich.app?code=123&state=456')).toEqual('app.immich:/?code=123&state=456');
expect(sut.getMobileRedirect('http://immich.app?code=123&state=456')).toEqual(
'app.immich:///oauth-callback?code=123&state=456',
);
});

it('should work if called without query params', () => {
expect(sut.getMobileRedirect('http://immich.app')).toEqual('app.immich:/?');
expect(sut.getMobileRedirect('http://immich.app')).toEqual('app.immich:///oauth-callback?');
});
});

Expand Down Expand Up @@ -488,25 +490,23 @@ describe('AuthService', () => {
expect(userMock.create).toHaveBeenCalledTimes(1);
});

it('should use the mobile redirect override', async () => {
systemMock.get.mockResolvedValue(systemConfigStub.oauthWithMobileOverride);
userMock.getByOAuthId.mockResolvedValue(userStub.user1);
sessionMock.create.mockResolvedValue(sessionStub.valid);

await sut.callback({ url: `app.immich:/?code=abc123` }, loginDetails);

expect(callbackMock).toHaveBeenCalledWith('http://mobile-redirect', { state: 'state' }, { state: 'state' });
});

it('should use the mobile redirect override for ios urls with multiple slashes', async () => {
systemMock.get.mockResolvedValue(systemConfigStub.oauthWithMobileOverride);
userMock.getByOAuthId.mockResolvedValue(userStub.user1);
sessionMock.create.mockResolvedValue(sessionStub.valid);

await sut.callback({ url: `app.immich:///?code=abc123` }, loginDetails);

expect(callbackMock).toHaveBeenCalledWith('http://mobile-redirect', { state: 'state' }, { state: 'state' });
});
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',
]) {
it(`should use the mobile redirect override for a url of ${url}`, async () => {
systemMock.get.mockResolvedValue(systemConfigStub.oauthWithMobileOverride);
userMock.getByOAuthId.mockResolvedValue(userStub.user1);
sessionMock.create.mockResolvedValue(sessionStub.valid);

await sut.callback({ url }, loginDetails);
expect(callbackMock).toHaveBeenCalledWith('http://mobile-redirect', { state: 'state' }, { state: 'state' });
});
}

it('should use the default quota', async () => {
systemMock.get.mockResolvedValue(systemConfigStub.oauthWithStorageQuota);
Expand Down
2 changes: 1 addition & 1 deletion server/src/services/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ export class AuthService {
}

private normalize(config: SystemConfig, redirectUri: string) {
const isMobile = redirectUri.startsWith(MOBILE_REDIRECT);
const isMobile = redirectUri.startsWith('app.immich:/');
const { mobileRedirectUri, mobileOverrideEnabled } = config.oauth;
if (isMobile && mobileOverrideEnabled && mobileRedirectUri) {
return mobileRedirectUri;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,9 @@

<SettingSwitch
title={$t('admin.oauth_mobile_redirect_uri_override').toUpperCase()}
subtitle={$t('admin.oauth_mobile_redirect_uri_override_description')}
subtitle={$t('admin.oauth_mobile_redirect_uri_override_description', {
values: { callback: 'app.immich:///oauth-callback' },
})}
disabled={disabled || !config.oauth.enabled}
on:click={() => handleToggleOverride()}
bind:checked={config.oauth.mobileOverrideEnabled}
Expand Down
2 changes: 1 addition & 1 deletion web/src/lib/i18n/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@
"oauth_issuer_url": "Issuer URL",
"oauth_mobile_redirect_uri": "Mobile redirect URI",
"oauth_mobile_redirect_uri_override": "Mobile redirect URI override",
"oauth_mobile_redirect_uri_override_description": "Enable when 'app.immich:/' is an invalid redirect URI.",
"oauth_mobile_redirect_uri_override_description": "Enable when OAuth provider does not allow a mobile URI, like '{callback}'",
"oauth_profile_signing_algorithm": "Profile signing algorithm",
"oauth_profile_signing_algorithm_description": "Algorithm used to sign the user profile.",
"oauth_scope": "Scope",
Expand Down
Loading