Skip to content

login: Fetch all the data we'll want for an accounts table #55

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 6 commits into from
Apr 6, 2023
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
15 changes: 11 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,21 @@ community. See [issue #15][].
In this early prototype, we don't yet have a UI for logging into
a Zulip server. Instead, you supply Zulip credentials at build time.

To do this, log into the Zulip web app for the test account you want
to use, and [download a `.zuliprc` file][download-zuliprc]. Then
create a file `lib/credential_fixture.dart` in this worktree with the
following form:
To do this, log into the Zulip web app for the test account
you want to use, and gather two kinds of information:
* [Download a `.zuliprc` file][download-zuliprc].
This will contain a realm URL, email, and API key.
* Find the account's user ID. You can do this by visiting your
DMs with yourself, and looking at the URL;
it's the number after `pm-with/` or `dm-with/`.

Then create a file `lib/credential_fixture.dart` in this worktree
with the following form, and fill in the gathered information:
```dart
const String realmUrl = '…';
const String email = '…';
const String apiKey = '…';
const int userId = /* … */ -1;
```

Now build and run the app (see "Flutter help" above), and things
Expand Down
10 changes: 5 additions & 5 deletions lib/api/core.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ import 'dart:convert';
import 'package:flutter/foundation.dart';
import 'package:http/http.dart' as http;

abstract class Auth {
String get realmUrl;
class Auth {
const Auth({required this.realmUrl, required this.email, required this.apiKey});

String get email;

String get apiKey;
final String realmUrl;
final String email;
final String apiKey;
}

/// A value for an API request parameter, to use directly without JSON encoding.
Expand Down
7 changes: 6 additions & 1 deletion lib/api/route/account.dart
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,13 @@ Future<FetchApiKeyResult> fetchApiKey({
class FetchApiKeyResult {
final String apiKey;
final String email;
final int? userId; // TODO(server-7)

FetchApiKeyResult({required this.apiKey, required this.email});
FetchApiKeyResult({
required this.apiKey,
required this.email,
this.userId,
});

factory FetchApiKeyResult.fromJson(Map<String, dynamic> json) =>
_$FetchApiKeyResultFromJson(json);
Expand Down
2 changes: 2 additions & 0 deletions lib/api/route/account.g.dart

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

74 changes: 74 additions & 0 deletions lib/api/route/realm.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import 'dart:convert';

import 'package:http/http.dart' as http;
import 'package:json_annotation/json_annotation.dart';

part 'realm.g.dart';

/// https://zulip.com/api/get-server-settings
///
/// Despite the name, this is really a home for certain realm-specific
/// settings, as well as some information about the server as a whole.
///
/// The Zulip server offers this endpoint at the root domain of a server,
/// even when there is no Zulip realm at that domain. This binding, however,
/// only operates on an actual Zulip realm.
// TODO(#35): Perhaps detect realmless root domain, for more specific onboarding feedback.
// See thread, and the zulip-mobile code and chat thread it links to:
// https://github.com/zulip/zulip-flutter/pull/55#discussion_r1160267577
Future<GetServerSettingsResult> getServerSettings({
required String realmUrl,
}) async {
// TODO dedupe this part with LiveApiConnection; make this function testable
final response = await http.get(
Uri.parse("$realmUrl/api/v1/server_settings"));
if (response.statusCode != 200) {
throw Exception('error on GET server_settings: status ${response.statusCode}');
}
final data = utf8.decode(response.bodyBytes);

final json = jsonDecode(data);
return GetServerSettingsResult.fromJson(json);
}

@JsonSerializable(fieldRename: FieldRename.snake)
class GetServerSettingsResult {
Copy link
Collaborator

Choose a reason for hiding this comment

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

zulip-mobile has this comment:

// Current to FL 121.

Should we put a similar comment here? No harm in copying zulip-mobile with 121, I think, since callers don't yet use newer server features.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to use those sparingly. In general I'd prefer that the expectation is that things are up to date.

On this get-server-settings endpoint, for example, now that the current feature level is up to 171, that comment makes it look like it might be pretty stale; but in reality the last change it had (*) was at FL 116, so it's still perfectly up to date. I think most of the Zulip API is like this endpoint in that it doesn't change often, and we can pretty well keep it up to date.

The major exception is settings, which the Git history on zulip-mobile reminds me is where we first started applying this "current to" convention in zulip/zulip-mobile@3971c9c47. I think it may well make sense to use it in a couple of spots that have long lists of settings, once we add those here.

One practice we could start in order to systematically catch anything that's out of date: on each server release, take that as a prompt/reminder to go through the API changelog since the last release, find all the updates that are to API surface we describe, and apply those updates where we haven't already. Looking back at the 5.0-to-6.0 changes as an example, I think that'd be a quite manageable task.

(*) Apart from the ignored_parameters_unsupported thing, which is cross-cutting over the whole API and which we're not interested in anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah OK, that makes sense.

final Map<String, bool> authenticationMethods;
// final List<ExternalAuthenticationMethod> external_authentication_methods; // TODO handle

final int zulipFeatureLevel;
final String zulipVersion;
final String? zulipMergeBase; // TODO(server-5)

final bool pushNotificationsEnabled;
final bool isIncompatible;

final bool emailAuthEnabled;
final bool requireEmailFormatUsernames;
final String realmUri;
final String realmName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

realmName can be missing, kind of awkwardly. We have this comment in zulip-mobile:

  // When missing, the user requested the root domain of a Zulip server, but
  // there is no realm there. User error.
  //
  // (Also, the server has `ROOT_DOMAIN_LANDING_PAGE = False`, the default.
  // But the mobile client doesn't care about that; we just care that there
  // isn't a realm at the root.)
  //
  // Discussion:
  //   https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/.2Fserver_settings.3A.20.60realm_name.60.2C.20etc.2E/near/1334042
  //
  // TODO(server-future): Expect the error to be encoded in a proper error
  //   response instead of this missing property.
  // TODO(server-future): Then, when all supported servers give that error,
  //   make this property required.

and this bit of logic to handle when it's missing:

  if (realm_name == null) {
    // See comment on realm_name in ApiResponseServerSettings.
    //
    // This error copies the proper error that servers *sometimes* give when
    // the root domain is requested and there's no realm there. [1]
    // Specifically, servers give this when
    // `ROOT_DOMAIN_LANDING_PAGE = True`. That circumstance makes no
    // difference to mobile.
    //
    // [1] Observed empirically. For details, see
    //   https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/.2Fserver_settings.3A.20.60realm_name.60.2C.20etc.2E/near/1332900 .
    throw new ApiError(400, { code: 'BAD_REQUEST', msg: 'Subdomain required', result: 'error' });
  }

Probably we don't need the runtime code right now, but maybe worth a comment mentioning that there's some complexity to be handled until server-future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, my thinking was that this result type would describe only what we get when asking at an actual Zulip realm's domain, not a Zulip server's root domain when there's no realm there. I should make that explicit in dartdoc, because the https://zulip.com/api/get-server-settings doc is written with the opposite focus.

When you talk to a server's root domain when no realm is there, you'll get a response that's missing several of these required fields, and so it'll throw an exception. That seems appropriate if one thinks of this as about a realm — other API endpoints don't work on such a domain either. It's basically the same as one expects if the entered URL doesn't belong to a Zulip server at all.

I'll also leave a TODO comment that as part of #35 it would be good to give feedback when the user enters such a domain. I'm not sure we'll necessarily take care of that in the main round of #35 when we close that issue — in particular, I'm not aware of any existing server one can make
that mistake with other than zulipchat.com, and that one has ROOT_DOMAIN_LANDING_PAGE = True so it fails differently — but if not then we can file a more specific issue for it then.

final String realmIcon;
final String realmDescription;
final bool? realmWebPublicAccessEnabled; // TODO(server-5)

GetServerSettingsResult({
required this.authenticationMethods,
required this.zulipFeatureLevel,
required this.zulipVersion,
this.zulipMergeBase,
required this.pushNotificationsEnabled,
required this.isIncompatible,
required this.emailAuthEnabled,
required this.requireEmailFormatUsernames,
required this.realmUri,
required this.realmName,
required this.realmIcon,
required this.realmDescription,
this.realmWebPublicAccessEnabled,
});

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

Map<String, dynamic> toJson() => _$GetServerSettingsResultToJson(this);
}
46 changes: 46 additions & 0 deletions lib/api/route/realm.g.dart

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

37 changes: 37 additions & 0 deletions lib/api/route/users.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import 'dart:convert';

import 'package:json_annotation/json_annotation.dart';

import '../core.dart';

part 'users.g.dart';

/// https://zulip.com/api/get-own-user, abridged
///
/// This route's return type is simplified because we use it only
/// as a workaround on old servers.
Future<GetOwnUserResult> getOwnUser(ApiConnection connection) async {
final data = await connection.get('users/me', {});
return GetOwnUserResult.fromJson(jsonDecode(data));
}

@JsonSerializable(fieldRename: FieldRename.snake)
class GetOwnUserResult {
final int userId;

// There are many more properties in this route's result.
// But we use this route only as a workaround on old servers:
// https://github.com/zulip/zulip/issues/24980
// https://chat.zulip.org/#narrow/stream/378-api-design/topic/user.20ID.20in.20fetch-api-key/near/1540592
// for which `userId` is the only property we need.
// TODO(server-7): Drop getOwnUser entirely, relying on userId from fetchApiKey.

GetOwnUserResult({
required this.userId,
});

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

Map<String, dynamic> toJson() => _$GetOwnUserResultToJson(this);
}
17 changes: 17 additions & 0 deletions lib/api/route/users.g.dart

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

29 changes: 19 additions & 10 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -201,16 +201,21 @@ class PerAccountStore extends ChangeNotifier {
}

@immutable
class Account implements Auth {
const Account(
{required this.realmUrl, required this.email, required this.apiKey});

@override
final String realmUrl;
@override
final String email;
@override
final String apiKey;
class Account extends Auth {
const Account({
required super.realmUrl,
required super.email,
required super.apiKey,
required this.userId,
required this.zulipFeatureLevel,
required this.zulipVersion,
required this.zulipMergeBase,
});

final int userId;
final int zulipFeatureLevel;
final String zulipVersion;
final String? zulipMergeBase;
}

class LiveGlobalStore extends GlobalStore {
Expand Down Expand Up @@ -240,6 +245,10 @@ const Account _fixtureAccount = Account(
realmUrl: credentials.realmUrl,
email: credentials.email,
apiKey: credentials.apiKey,
userId: credentials.userId,
zulipFeatureLevel: 169,
zulipVersion: '6.0-1235-g061f1dc43b',
zulipMergeBase: '6.0-1235-g061f1dc43b',
);

/// A [PerAccountStore] which polls an event queue to stay up to date.
Expand Down
50 changes: 42 additions & 8 deletions lib/widgets/login.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import 'package:flutter/material.dart';

import '../api/core.dart';
import '../api/route/account.dart';
import '../api/route/realm.dart';
import '../api/route/users.dart';
import '../model/store.dart';
import 'app.dart';
import 'store.dart';
Expand Down Expand Up @@ -32,17 +35,27 @@ class _AddAccountPageState extends State<AddAccountPage> {
super.dispose();
}

void _onSubmitted(BuildContext context, String value) {
Future<void> _onSubmitted(BuildContext context, String value) async {
final Uri? url = Uri.tryParse(value);
switch (url) {
case Uri(scheme: 'https' || 'http'):
// TODO(#35): validate realm URL further?
// TODO(#36): support login methods beyond email/password
Navigator.push(context,
EmailPasswordLoginPage.buildRoute(realmUrl: url));
break;
default:
// TODO(#35): give feedback to user on bad realm URL
return;
}

// TODO(#35): show feedback that we're working, while fetching server settings
final serverSettings = await getServerSettings(realmUrl: url.toString());
if (context.mounted) {} // https://github.com/dart-lang/linter/issues/4007
else {
return;
}

// TODO(#36): support login methods beyond email/password
Navigator.push(context,
EmailPasswordLoginPage.buildRoute(realmUrl: url, serverSettings: serverSettings));
}

@override
Expand All @@ -69,13 +82,16 @@ class _AddAccountPageState extends State<AddAccountPage> {
}

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

final Uri realmUrl;
final GetServerSettingsResult serverSettings;

static Route<void> buildRoute({required Uri realmUrl}) {
static Route<void> buildRoute({
required Uri realmUrl, required GetServerSettingsResult serverSettings}) {
return _LoginSequenceRoute(builder: (context) =>
EmailPasswordLoginPage(realmUrl: realmUrl));
EmailPasswordLoginPage(realmUrl: realmUrl, serverSettings: serverSettings));
}

@override
Expand All @@ -86,6 +102,14 @@ class _EmailPasswordLoginPageState extends State<EmailPasswordLoginPage> {
final GlobalKey<FormFieldState<String>> _emailKey = GlobalKey();
final GlobalKey<FormFieldState<String>> _passwordKey = GlobalKey();

Future<int> _getUserId(FetchApiKeyResult fetchApiKeyResult) async {
final FetchApiKeyResult(:email, :apiKey) = fetchApiKeyResult;
final auth = Auth(
realmUrl: widget.realmUrl.toString(), email: email, apiKey: apiKey);
final connection = LiveApiConnection(auth: auth); // TODO make this widget testable
return (await getOwnUser(connection)).userId;
}

void _submit() async {
final context = _emailKey.currentContext!;
final realmUrl = widget.realmUrl;
Expand All @@ -106,13 +130,23 @@ class _EmailPasswordLoginPageState extends State<EmailPasswordLoginPage> {
debugPrint(e.toString());
return;
}

// TODO(server-7): Rely on user_id from fetchApiKey.
final int userId = result.userId ?? await _getUserId(result);
if (context.mounted) {} // https://github.com/dart-lang/linter/issues/4007
else {
return;
}

final account = Account(
realmUrl: realmUrl.toString(), email: result.email, apiKey: result.apiKey);
realmUrl: realmUrl.toString(),
email: result.email,
apiKey: result.apiKey,
userId: userId,
zulipFeatureLevel: widget.serverSettings.zulipFeatureLevel,
zulipVersion: widget.serverSettings.zulipVersion,
zulipMergeBase: widget.serverSettings.zulipMergeBase,
);
final globalStore = GlobalStoreWidget.of(context);
final accountId = await globalStore.insertAccount(account);
if (context.mounted) {} // https://github.com/dart-lang/linter/issues/4007
Expand Down
Loading