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

login: Implement browser login #281

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions android/app/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@
<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>
<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
27 changes: 25 additions & 2 deletions lib/api/route/realm.dart
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,32 @@ Future<GetServerSettingsResult> getServerSettings({
}
}

@JsonSerializable(fieldRename: FieldRename.snake)
class ExternalAuthenticationMethod {
final String name;
final String displayName;
final String? displayIcon;
final String loginUrl;
final String signupUrl;

ExternalAuthenticationMethod({
required this.name,
required this.displayName,
this.displayIcon,
required this.loginUrl,
required this.signupUrl,
});

factory ExternalAuthenticationMethod.fromJson(Map<String, dynamic> json) =>
_$ExternalAuthenticationMethodFromJson(json);

Map<String, dynamic> toJson() => _$ExternalAuthenticationMethodToJson(this);
}

@JsonSerializable(fieldRename: FieldRename.snake)
class GetServerSettingsResult {
final Map<String, bool> authenticationMethods;
// final List<ExternalAuthenticationMethod> external_authentication_methods; // TODO handle
final List<ExternalAuthenticationMethod> externalAuthenticationMethods;
Comment on lines -34 to +56
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 put API bindings changes in a separate prep commit. That's a helpful way to split up the changes logically to make them easier to understand and review.


final int zulipFeatureLevel;
final String zulipVersion;
Expand All @@ -44,12 +66,13 @@ class GetServerSettingsResult {
final bool requireEmailFormatUsernames;
final Uri realmUri;
final String realmName;
final String realmIcon;
final Uri? realmIcon;
final String realmDescription;
final bool? realmWebPublicAccessEnabled; // TODO(server-5)

GetServerSettingsResult({
required this.authenticationMethods,
required this.externalAuthenticationMethods,
required this.zulipFeatureLevel,
required this.zulipVersion,
this.zulipMergeBase,
Expand Down
32 changes: 30 additions & 2 deletions lib/api/route/realm.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 24 additions & 4 deletions lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import 'package:flutter/material.dart';
import '../model/narrow.dart';
import 'about_zulip.dart';
import 'login.dart';
import 'login/browser_login.dart';
import 'message_list.dart';
import 'page.dart';
import 'recent_dm_conversations.dart';
Expand All @@ -25,10 +26,29 @@ class ZulipApp extends StatelessWidget {
// https://m3.material.io/theme-builder#/custom
colorScheme: ColorScheme.fromSeed(seedColor: kZulipBrandColor));
return GlobalStoreWidget(
child: MaterialApp(
title: 'Zulip',
theme: theme,
home: const ChooseAccountPage()));
child: BrowserLoginWidget(
child: Builder(
builder: (context) => MaterialApp(
title: 'Zulip',
theme: theme,
home: const ChooseAccountPage(),
navigatorKey: BrowserLoginWidget.of(context).navigatorKey,
// TODO: Migrate to `MaterialApp.router` & `Router`, so that we can receive
// a full Uri instead of just path+query components and also maybe
Comment on lines +36 to +37
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I was going to write that I believe using Router wouldn't change this, because if you trace through how the information flows through the engine and into the Flutter framework code, you find that the Dart side only ever sees path, query, and fragment.

… But it turns out that's because my engine tree was a few months old! This feature was added recently in flutter/flutter#100624 . Now Dart does see the full URL.

Still, I think we can do this orthogonally to any switch to Router. The key is to register a WidgetsBindingObserver and implement didPushRouteInformation:
https://api.flutter.dev/flutter/widgets/WidgetsBindingObserver/didPushRouteInformation.html

I think that can then replace the job that this onGenerateRoute is doing.

// remove the InheritedWidget + navigatorKey hack.
// See docs:
// https://api.flutter.dev/flutter/widgets/Router-class.html
onGenerateRoute: (settings) {
if (settings.name == null) return null;
final uri = Uri.parse(settings.name!);
if (uri.queryParameters.containsKey('otp_encrypted_api_key')) {
BrowserLoginWidget.of(context).loginFromExternalRoute(context, uri);
return null;
}
return null;
})),
),
);
}
}

Expand Down
90 changes: 83 additions & 7 deletions lib/widgets/login.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ import '../model/store.dart';
import 'app.dart';
import 'dialog.dart';
import 'input.dart';
import 'login/browser_login.dart';
import 'page.dart';
import 'store.dart';

class _LoginSequenceRoute extends MaterialWidgetRoute<void> {
_LoginSequenceRoute({
class LoginSequenceRoute extends MaterialWidgetRoute<void> {
LoginSequenceRoute({
required super.page,
});
}
Expand Down Expand Up @@ -102,7 +103,7 @@ class AddAccountPage extends StatefulWidget {
const AddAccountPage({super.key});

static Route<void> buildRoute() {
return _LoginSequenceRoute(page: const AddAccountPage());
return LoginSequenceRoute(page: const AddAccountPage());
}

@override
Expand Down Expand Up @@ -167,9 +168,8 @@ class _AddAccountPageState extends State<AddAccountPage> {
return;
}

// TODO(#36): support login methods beyond username/password
Navigator.push(context,
PasswordLoginPage.buildRoute(serverSettings: serverSettings));
AuthMethodsPage.buildRoute(serverSettings: serverSettings));
} finally {
setState(() {
_inProgress = false;
Expand Down Expand Up @@ -225,13 +225,89 @@ class _AddAccountPageState extends State<AddAccountPage> {
}
}

class AuthMethodsPage extends StatefulWidget {
const AuthMethodsPage({super.key, required this.serverSettings});

final GetServerSettingsResult serverSettings;

static Route<void> buildRoute({required GetServerSettingsResult serverSettings}) {
return LoginSequenceRoute(
page: AuthMethodsPage(serverSettings: serverSettings));
}

@override
State<AuthMethodsPage> createState() => _AuthMethodsPageState();
}

class _AuthMethodsPageState extends State<AuthMethodsPage> {
// TODO: Remove this list when all the methods are tested,
// or update to add a new method.
static const Set<String> _testedAuthMethods = {
'github',
'gitlab',
'google',
};

Future<void> _openBrowserLogin(ExternalAuthenticationMethod method) =>
BrowserLoginWidget.of(context).openLoginUrl(widget.serverSettings, method.loginUrl);

@override
Widget build(BuildContext context) {
// 'realmIcon' for chat.zulip.org, only contains the path component.
// So, resolve it to the 'realmUri' to get the full Uri with host.
final Uri? iconUrl = widget.serverSettings.realmIcon != null
? widget.serverSettings.realmUri.resolveUri(widget.serverSettings.realmIcon!)
: null;

return Scaffold(
appBar: AppBar(title: const Text('Log in')),
body: SafeArea(
child: ListView(
padding: const EdgeInsets.all(8),
children: [
Padding(
padding: const EdgeInsets.only(bottom: 8),
child: Row(
mainAxisAlignment: MainAxisAlignment.center,
children: [
if (iconUrl != null) ...[
Image.network(
iconUrl.toString(),
key: const Key('realm_icon'),
width: 48,
height: 48),
const SizedBox(width: 8),
],
Text(widget.serverSettings.realmName, style: const TextStyle(fontSize: 20)),
]),
),
if (widget.serverSettings.emailAuthEnabled)
OutlinedButton(
onPressed: () => Navigator.push(context, PasswordLoginPage.buildRoute(serverSettings: widget.serverSettings)),
child: const Text('Sign in with password')),
...widget.serverSettings.externalAuthenticationMethods.map(
(authMethod) => switch (authMethod.displayIcon) {
null || '' => OutlinedButton(
onPressed: _testedAuthMethods.contains(authMethod.name) ? () => _openBrowserLogin(authMethod) : null,
child: Text('Sign in with ${authMethod.displayName}'),
),
final displayIcon => OutlinedButton.icon(
onPressed: _testedAuthMethods.contains(authMethod.name) ? () => _openBrowserLogin(authMethod) : null,
icon: Image.network(displayIcon, width: 24, height: 24),
label: Text('Sign in with ${authMethod.displayName}'),
),
}).toList(),
])));
}
}

class PasswordLoginPage extends StatefulWidget {
const PasswordLoginPage({super.key, required this.serverSettings});

final GetServerSettingsResult serverSettings;

static Route<void> buildRoute({required GetServerSettingsResult serverSettings}) {
return _LoginSequenceRoute(
return LoginSequenceRoute(
page: PasswordLoginPage(serverSettings: serverSettings));
}

Expand Down Expand Up @@ -323,7 +399,7 @@ class _PasswordLoginPageState extends State<PasswordLoginPage> {

Navigator.of(context).pushAndRemoveUntil(
HomePage.buildRoute(accountId: accountId),
(route) => (route is! _LoginSequenceRoute),
(route) => (route is! LoginSequenceRoute),
);
} finally {
setState(() {
Expand Down