Skip to content

login: Support web-based auth methods #600

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

Merged
merged 14 commits into from
Apr 2, 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
7 changes: 7 additions & 0 deletions android/app/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@
<action android:name="android.intent.action.MAIN"/>
<category android:name="android.intent.category.LAUNCHER"/>
</intent-filter>
<meta-data android:name="flutter_deeplinking_enabled" android:value="true" />
<intent-filter>
Comment on lines +28 to +29
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, the changes here and in Info.plist LGTM.

(The intent filter and CFBundleUrlTypes match what we have in zulip-mobile; the other bits are what's prescribed in the docs under https://docs.flutter.dev/ui/navigation/deep-linking .)

So what remains to do on this PR is those small comments at #600 (review) on the new test, and then the bigger point from my end-to-end testing after that.

<action android:name="android.intent.action.VIEW" />
<category android:name="android.intent.category.DEFAULT" />
<category android:name="android.intent.category.BROWSABLE" />
<data android:scheme="zulip" android:host="login" />
</intent-filter>
</activity>
<!-- Don't delete the meta-data below.
This is used by the Flutter tool to generate GeneratedPluginRegistrant.java -->
Expand Down
19 changes: 19 additions & 0 deletions assets/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@
"@actionSheetOptionUnstarMessage": {
"description": "Label for unstar button on action sheet."
},
"errorWebAuthOperationalErrorTitle": "Something went wrong",
"@errorWebAuthOperationalErrorTitle": {
"description": "Error title when third-party authentication has an operational error (not necessarily caused by invalid credentials)."
},
"errorWebAuthOperationalError": "An unexpected error occurred.",
"@errorWebAuthOperationalError": {
"description": "Error message when third-party authentication has an operational error (not necessarily caused by invalid credentials)."
},
"errorAccountLoggedInTitle": "Account already logged in",
"@errorAccountLoggedInTitle": {
"description": "Error title on attempting to log into an account that's already logged in."
Expand Down Expand Up @@ -281,6 +289,17 @@
"@loginFormSubmitLabel": {
"description": "Button text to submit login credentials."
},
"loginMethodDivider": "OR",
"@loginMethodDivider": {
"description": "Text on the divider between the username/password form and the third-party login options. Uppercase (for languages with letter case)."
},
"signInWithFoo": "Sign in with {method}",
"@signInWithFoo": {
"description": "Button to use {method} to sign in to the app.",
"placeholders": {
"method": {"type": "String", "example": "Google"}
}
},
"loginAddAnAccountPageTitle": "Add an account",
"@loginAddAnAccountPageTitle": {
"description": "Page title for screen to add a Zulip account."
Expand Down
13 changes: 13 additions & 0 deletions ios/Runner/Info.plist
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,21 @@
<string>$(FLUTTER_BUILD_NAME)</string>
<key>CFBundleSignature</key>
<string>????</string>
<key>CFBundleURLTypes</key>
<array>
<dict>
<key>CFBundleURLName</key>
<string>com.zulip.flutter</string>
<key>CFBundleURLSchemes</key>
<array>
<string>zulip</string>
</array>
</dict>
</array>
<key>CFBundleVersion</key>
<string>$(FLUTTER_BUILD_NUMBER)</string>
<key>FlutterDeepLinkingEnabled</key>
<true/>
<key>ITSAppUsesNonExemptEncryption</key>
<false/>
<key>LSRequiresIPhoneOS</key>
Expand Down
87 changes: 87 additions & 0 deletions lib/api/model/web_auth.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import 'dart:math';

import 'package:convert/convert.dart';
import 'package:flutter/foundation.dart';

/// The authentication information contained in the zulip:// redirect URL.
class WebAuthPayload {
final Uri realm;
final String email;
final int? userId; // TODO(server-5) new in FL 108
final String otpEncryptedApiKey;

WebAuthPayload._({
required this.realm,
required this.email,
required this.userId,
required this.otpEncryptedApiKey,
});

factory WebAuthPayload.parse(Uri url) {
if (
url case Uri(
scheme: 'zulip',
host: 'login',
queryParameters: {
'realm': String realmStr,
'email': String email,
// 'user_id' handled below
'otp_encrypted_api_key': String otpEncryptedApiKey,
},
)
) {
final Uri? realm = Uri.tryParse(realmStr);
if (realm == null) throw const FormatException();

// TODO(server-5) require in queryParameters (new in FL 108)
final userIdStr = url.queryParameters['user_id'];
int? userId;
if (userIdStr != null) {
userId = int.tryParse(userIdStr, radix: 10);
if (userId == null) throw const FormatException();
}

if (!RegExp(r'^[0-9a-fA-F]{64}$').hasMatch(otpEncryptedApiKey)) {
throw const FormatException();
}

return WebAuthPayload._(
otpEncryptedApiKey: otpEncryptedApiKey,
email: email,
userId: userId,
realm: realm,
);
} else {
// TODO(dart): simplify after https://github.com/dart-lang/language/issues/2537
throw const FormatException();
}
}

String decodeApiKey(String otp) {
final otpBytes = hex.decode(otp);
final otpEncryptedApiKeyBytes = hex.decode(otpEncryptedApiKey);
if (otpBytes.length != otpEncryptedApiKeyBytes.length) {
throw const FormatException();
}
return String.fromCharCodes(Iterable.generate(otpBytes.length,
(i) => otpBytes[i] ^ otpEncryptedApiKeyBytes[i]));
}
}

String generateOtp() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give this a unit test, too. For a smoke test: it returns a value (rather than throw), and the value is a hex string of the right length.

And then given that a previous draft of this function elsewhere actually had a bug where the randomness was off — it had rand.nextInt(255) instead of 256 — let's have a test that would have caught that, so necessarily a probabilistic test.

We can follow the lead of our BackoffMachine test and say the probability of a false failure just needs to be under 1e-9. So…

  • generate N = 216 of these
  • take the set of bytes that ever appear (after decoding them all from hex)
  • check that all 256 possible byte values do appear somewhere.

That should be plenty quick to run, I hope. And each possible byte value gets N * 32 opportunities to show up, each with probability 1/256; so its probability of missing all of those is exp(- N * 32 / 256) < 2e-12, and there are 256 such possible byte values so the probability that any of them gets missed is < 1e-9.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then since we'll be generating all those sample results, may as well use those to subsume the smoke test: just check each of them has the right length (and we'll already be decoding them as hex so implicitly checking they're hex strings).

One possible failure mode of a buggy implementation could be to drop leading zero bytes, for example, and this check would catch that.

final rand = Random.secure();
final Uint8List bytes = Uint8List.fromList(
List.generate(32, (_) => rand.nextInt(256)));
return hex.encode(bytes);
}

/// For tests, create an OTP-encrypted API key.
@visibleForTesting
String debugEncodeApiKey(String apiKey, String otp) {
final apiKeyBytes = apiKey.codeUnits;
assert(apiKeyBytes.every((byte) => byte <= 0xff));
final otpBytes = hex.decode(otp);
assert(apiKeyBytes.length == otpBytes.length);
return hex.encode(List.generate(otpBytes.length,
(i) => apiKeyBytes[i] ^ otpBytes[i]));
}
28 changes: 28 additions & 0 deletions lib/model/binding.dart
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ abstract class ZulipBinding {
/// to a [GlobalStore].
Future<GlobalStore> loadGlobalStore();

/// Checks whether the platform can launch [url], via package:url_launcher.
///
/// This wraps [url_launcher.canLaunchUrl].
Future<bool> canLaunchUrl(Uri url);

/// Pass [url] to the underlying platform, via package:url_launcher.
///
/// This wraps [url_launcher.launchUrl].
Expand All @@ -83,6 +88,16 @@ abstract class ZulipBinding {
url_launcher.LaunchMode mode = url_launcher.LaunchMode.platformDefault,
});

/// Checks whether [closeInAppWebView] is supported, via package:url_launcher.
///
/// This wraps [url_launcher.supportsCloseForLaunchMode].
Future<bool> supportsCloseForLaunchMode(url_launcher.LaunchMode mode);

/// Closes the current in-app web view, via package:url_launcher.
///
/// This wraps [url_launcher.closeInAppWebView].
Future<void> closeInAppWebView();

/// Provides device and operating system information,
/// via package:device_info_plus.
///
Expand Down Expand Up @@ -159,6 +174,9 @@ class LiveZulipBinding extends ZulipBinding {
return LiveGlobalStore.load();
}

@override
Future<bool> canLaunchUrl(Uri url) => url_launcher.canLaunchUrl(url);

@override
Future<bool> launchUrl(
Uri url, {
Expand All @@ -167,6 +185,16 @@ class LiveZulipBinding extends ZulipBinding {
return url_launcher.launchUrl(url, mode: mode);
}

@override
Future<bool> supportsCloseForLaunchMode(url_launcher.LaunchMode mode) async {
return url_launcher.supportsCloseForLaunchMode(mode);
}

@override
Future<void> closeInAppWebView() async {
return url_launcher.closeInAppWebView();
}

@override
Future<BaseDeviceInfo> deviceInfo() async {
final deviceInfo = await device_info_plus.DeviceInfoPlugin().deviceInfo;
Expand Down
38 changes: 33 additions & 5 deletions lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import 'store.dart';
import 'subscription_list.dart';
import 'text.dart';

class ZulipApp extends StatelessWidget {
class ZulipApp extends StatefulWidget {
const ZulipApp({super.key, this.navigatorObservers});

/// Whether the app's widget tree is ready.
Expand Down Expand Up @@ -79,6 +79,34 @@ class ZulipApp extends StatelessWidget {
_ready.value = true;
}

@override
State<ZulipApp> createState() => _ZulipAppState();
}

class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
@override
Future<bool> didPushRouteInformation(routeInformation) async {
if (routeInformation case RouteInformation(
uri: Uri(scheme: 'zulip', host: 'login') && var url)
) {
await LoginPage.handleWebAuthUrl(url);
return true;
}
return super.didPushRouteInformation(routeInformation);
}

@override
void initState() {
super.initState();
WidgetsBinding.instance.addObserver(this);
}

@override
void dispose() {
WidgetsBinding.instance.removeObserver(this);
super.dispose();
}

@override
Widget build(BuildContext context) {
final theme = ThemeData(
Expand Down Expand Up @@ -123,12 +151,12 @@ class ZulipApp extends StatelessWidget {
supportedLocales: ZulipLocalizations.supportedLocales,
theme: theme,

navigatorKey: navigatorKey,
navigatorObservers: navigatorObservers ?? const [],
navigatorKey: ZulipApp.navigatorKey,
navigatorObservers: widget.navigatorObservers ?? const [],
builder: (BuildContext context, Widget? child) {
if (!ready.value) {
if (!ZulipApp.ready.value) {
SchedulerBinding.instance.addPostFrameCallback(
(_) => _declareReady());
(_) => widget._declareReady());
}
GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context);
return child!;
Expand Down
Loading