login: Support web-based auth methods #600

chrisbobbe · 2024-03-28T19:58:13Z

RPReplay_Final1711671338.mov

Fixes: #36

gnprice

Thanks @chrisbobbe! Looking forward to having this in.

Generally the code all looks good. Comments below, mostly small.

As you mention, we'll want some tests on WebAuthPayload. We have a few tests of that kind in zulip-mobile in webAuth-test.js.

Let's also have a screenshot of the new login screen, and a video of the end-to-end auth flow. Those will be helpful for seeing what the UX is like.

gnprice · 2024-03-28T20:08:18Z

lib/widgets/login.dart

 }

-class _PasswordLoginPageState extends State<PasswordLoginPage> {
+class LoginPageState extends State<LoginPage> {


Suggested change

class LoginPageState extends State<LoginPage> {

@visibleForTesting

class LoginPageState extends State<LoginPage> {

That way it's clear this isn't meant for any other part of the app to meddle in the details of.

(And then I think the similar annotation on debugOtpOverride becomes redundant.)

… Ah I see, this type is used by _ZulipAppState.didPushRouteInformation, to call handleWebAuthUrl.

Oddly when I added this @visibleForTesting, that didn't cause any analysis error. I guess in particular that means the one on debugOtpOverride wouldn't be redundant with it after all.

From my comment at lastBuiltKey, though, I think this state type can remain private.

gnprice · 2024-03-28T20:18:58Z

test/example_data.dart

@@ -107,14 +122,14 @@ final User selfUser = user(fullName: 'Self User', email: 'self@example');
 final Account selfAccount = account(
  id: 1001,
  user: selfUser,
-  apiKey: 'asdfqwer',
+  apiKey: 'dQcEJWTq3LczosDkJnRTwf31zniGvMrO',


We can leave a brief note on how these are generated:

Suggested change

apiKey: 'dQcEJWTq3LczosDkJnRTwf31zniGvMrO',

// A Zulip API key is 32 digits of base64.

apiKey: 'dQcEJWTq3LczosDkJnRTwf31zniGvMrO',

gnprice · 2024-03-28T20:20:45Z

test/test_images.dart

+/// before the end of the test.
+// TODO(upstream) simplify callers by using addTearDown: https://github.com/flutter/flutter/issues/123189
+//   See also: https://github.com/flutter/flutter/issues/121917
+FakeImageHttpClient prepareBoringImageHttpClient() {


test [nfc]: Move prepareBoringImageHttpClient to test_images.dart This was already being used for tests outside the test file it was defined in, and we'd like to start using it in an additional different test file. So, define it centrally in this place that makes sense.

Ah good thought, thanks. I'd forgotten there was this natural home for it already.

gnprice · 2024-03-28T20:28:00Z

assets/l10n/app_en.arb

@@ -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, like Google. Uppercase (for languages with letter case)."


nit: On my first reading, I thought "like Google" referred to the divider resembling something Google does.

Probably possible to reword, but I think the ", like Google" part can be left out entirely and it seems just as clear.

gnprice · 2024-03-28T20:28:55Z

assets/l10n/app_en.arb

+  "errorWebAuthOperationalErrorTitle": "Operational error",
+  "@errorWebAuthOperationalErrorTitle": {
+    "description": "Error title when third-party authentication has an operational error (not necessarily caused by invalid credentials)."


Suggested change

"errorWebAuthOperationalErrorTitle": "Operational error",

"@errorWebAuthOperationalErrorTitle": {

"description": "Error title when third-party authentication has an operational error (not necessarily caused by invalid credentials)."

"errorWebAuthOperationalErrorTitle": "Something went wrong",

"@errorWebAuthOperationalErrorTitle": {

"description": "Error title when third-party authentication has an operational error (not necessarily caused by invalid credentials)."

Or other possibilities. But I think "operational error" is likely to be opaque jargon for most users.

gnprice · 2024-03-28T21:08:26Z

lib/widgets/login.dart

+    try {
+      assert (await ZulipBinding.instance.supportsCloseForLaunchMode(_webAuthLaunchMode));
+      await ZulipBinding.instance.closeInAppWebView();
+      if ((debugOtpOverride ?? _otp) == null) {


This fallback between the two fields gets repeated several times, so let's pull it into a little method, like _getOtp.

Then as a bonus, if you want to write this 100% the way the Flutter framework would: that method can use asserts so that outside of debug mode, the debug field never even gets read. (In a hot path, that sort of thing is important if you don't fully trust the optimizer to be able to prove that the field is always null. This is far from a hot path, so the optimization really doesn't matter.)

I'm a little curious how the Flutter framework does this, but in this case I'm not sure how exactly to do it. assert only does anything in debug mode, right, so I'm not sure what change we'd want to make to affect things outside of debug mode.

An example of what I have in mind is UpdateMachine.debugEnableRegisterNotificationToken. The key is that the assert callback sets a local variable that belongs to the outer function.

Then for authentic upstream examples, browse through framework.dart with that pattern in mind. The first one I spot from just starting to read through the Element implementation is Element.debugIsDefunct.

Interesting; thanks! I think I've got it:

String? _otp; String? _getOtp() { String? result = _otp; assert(() { result = LoginPage.debugOtpOverride ?? _otp; return true; }()); return result; }

Yeah, exactly.

I think you can also simplify slightly, deduplicating the use of _otp, by initializing to null, having the assert override that, and then using ?? _otp after that.

gnprice · 2024-03-28T21:13:12Z

lib/widgets/login.dart

+                        onPressed: !_inProgress
+                          ? () => _beginWebAuth(method)
+                          : null,
+                        label: Text(zulipLocalizations.signInWithFoo(method.displayName)));


This code feels a bit cramped from being indented so far to the right. I think it'd benefit from pulling out a piece as a local variable.

Specifically I think the Column, with a name like loginForm, would work well.

gnprice · 2024-03-28T21:28:06Z

lib/widgets/login.dart

+      if (e is PlatformException && e.message != null && e.message!.startsWith('Error while launching')) {
+        // Ignore; I've seen this on my iPhone even when auth succeeds.
+        // Specifically, Apple web auth…which on iOS should be replaced by
+        // Apple native auth; that's #462.
+        // Possibly related:
+        //   https://github.com/flutter/flutter/issues/91660


Hmm, wacky.

Let's perhaps limit this condition more tightly, like only to iOS. That way if there's an error we didn't expect — which might mean the launch really did fail — we'll show the user an error message rather than silently do nothing.

Two other upstream reports that are directly about errors like this one:

[url_launcher] Throw exception when clicked on Stop loading button on Safari flutter/flutter#75691

[url_launcher] - PlatformException on ios 13.3 when opening a docx file flutter/flutter#49162

This looks like possibly the most relevant:
flutter/flutter#75691 (comment)

Does that match the situation you were seeing these errors in?

I'm not sure. In that comment, the person says "if anything fails browser side". But when I watch what goes on in the browser, nothing stands out to me as matching that description.

gnprice · 2024-03-28T21:50:13Z

test/widgets/login_test.dart

+      pushedRoutes = [];
+      final testNavObserver = TestNavigatorObserver()
+        ..onPushed = (route, prevRoute) => pushedRoutes.add(route);
+      await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver],));


nit:

Suggested change

await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver],));

await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver]));

gnprice · 2024-03-28T21:54:03Z

test/widgets/login_test.dart

+
+        // TODO test _inProgress logic?
+
+        final encoded = debugEncodeApiKey(eg.selfAccount.apiKey, LoginPageState.debugOtpOverride!);


nit:

Suggested change

final encoded = debugEncodeApiKey(eg.selfAccount.apiKey, LoginPageState.debugOtpOverride!);

final encoded = debugEncodeApiKey(eg.selfAccount.apiKey, otp);

where otp is a local, and you refer to debugOtpOverride just to set it once.

That way this line doesn't get so long.

chrisbobbe · 2024-03-29T00:17:51Z

Hmm, a test failure in CI:

❌ /home/runner/work/zulip-flutter/zulip-flutter/test/model/store_test.dart: UpdateMachine.load retries registerQueue on NetworkError (failed)
  Bad state: Exceeded timeout 1:00:00.000000 while flushing timers
  package:fake_async/fake_async.dart 217:9   FakeAsync.flushTimers.<fn>
  package:fake_async/fake_async.dart 241:21  FakeAsync._fireTimersWhile
  package:fake_async/fake_async.dart 214:5   FakeAsync.flushTimers
  test/fake_async.dart 29:7                  awaitFakeAsync
  test/model/store_test.dart 176:57          main.<fn>.<fn>

A flake, do you think?

chrisbobbe · 2024-03-29T00:22:39Z

Thanks for the review! Revision pushed, and I've posted a screenshot and video in the PR description.

gnprice · 2024-03-29T00:26:44Z

Hmm, a test failure in CI:

Hmm. Does seem unlikely to be caused by this change — so it may be a pre-existing flake.

If so, then it's a bug in either the test or the code under test, so it'd be good to fix. Would you file a bug to track that flake?

chrisbobbe · 2024-03-29T00:29:14Z

Would you file a bug to track that flake?

Sure: #602

gnprice

Thanks! Generally this all looks great. There's one more test I'd like, and then various small style things.

I also want to try this end-to-end myself, and look closer at those settings in the manifest and Info.plist. But posting these code comments without blocking on those.

gnprice · 2024-04-01T18:58:28Z

lib/api/model/web_auth.dart

+        userId = int.tryParse(userIdStr, radix: 10);
+        if (userId == null) throw const FormatException();
+      }
+
+      final Uri? realm = Uri.tryParse(realmStr);
+      if (realm == null) throw const FormatException();


nit: put these in general-to-specific order, same as in the pattern above

gnprice · 2024-04-01T19:04:20Z

lib/widgets/login.dart

+  const LoginPage({super.key, required this.serverSettings});
+
+  /// A key for the page from the last [buildRoute] call.
+  static final _lastBuiltKey = GlobalKey<_LoginPageState>();


Let's order all these members in the way suggested in the Flutter style guide:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#order-other-class-members-in-a-way-that-makes-sense

I think that'll make it somewhat easier to look at this class and understand the API.

(It looks like the existing code wasn't quite in that order, and perhaps it'd be better if it were. But that matters a lot less when it's so simple; as it gains more complexity, the structure becomes more important for the reader.)

It looks like the existing code wasn't quite in that order

Hmm, really? Here's the class before the commit that adds to it:

class LoginPage extends StatefulWidget { const LoginPage({super.key, required this.serverSettings}); final GetServerSettingsResult serverSettings; static Route<void> buildRoute({required GetServerSettingsResult serverSettings}) { return _LoginSequenceRoute( page: LoginPage(serverSettings: serverSettings)); } @override State<LoginPage> createState() => _LoginPageState(); }

I see the following, matching the order in the style guide:

Constructors […]

[…]

Final fields that are set from the constructor.

Other static methods [i.e. that don't return the same type as the class].

[…]

Methods […]

I see that the updated class, with web-auth things added to it, doesn't match the numbered list; I'll fix that.

I see the following, matching the order in the style guide:

Oh, I guess that's right. I was reading buildRoute as returning the class's type, but it doesn't.

I think it's likely helpful to treat it for the ordering as if it does return the class's type, though. It plays pretty much the role of a factory constructor — it's the thing that application code, at least, should always use instead of calling the actual constructor directly. (Possibly we should even make the constructor private when we have a buildRoute method like this; but that might be annoying in tests.)

gnprice · 2024-04-01T19:08:36Z

lib/widgets/login.dart

+  String? _otp;
+  String? _getOtp() {


This is functionally a getter backed by a mutable field, so let's follow the Flutter style guide's order for those:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#order-other-class-members-in-a-way-that-makes-sense

Also could make this an actual getter/field pair. E.g. String? get _otp and String? __otp;.

Interesting; yeah, I guess it's fine to have a thing that starts with two underscores. 😛

gnprice · 2024-04-01T19:18:56Z

test/api/model/web_auth_test.dart

+    final wellFormed = Uri.parse(
+      'zulip://login?otp_encrypted_api_key=$encryptedApiKey&email=self%40example&user_id=1&realm=https%3A%2F%2Fchat.example%2F');


nit: break line for readability

Suggested change

final wellFormed = Uri.parse(

'zulip://login?otp_encrypted_api_key=$encryptedApiKey&email=self%40example&user_id=1&realm=https%3A%2F%2Fchat.example%2F');

final wellFormed = Uri.parse(

'zulip://login?otp_encrypted_api_key=$encryptedApiKey'

'&email=self%40example&user_id=1&realm=https%3A%2F%2Fchat.example%2F');

gnprice · 2024-04-01T19:21:34Z

test/api/model/web_auth_test.dart

+    });
+
+    test('parse fails when otp_encrypted_api_key is nonsense', () {
+      final queryParams = {...wellFormed.queryParameters}..['otp_encrypted_api_key'] = 'asdf';


nit: key detail is off past 80 columns, so break line:

Suggested change

final queryParams = {...wellFormed.queryParameters}..['otp_encrypted_api_key'] = 'asdf';

final queryParams = {...wellFormed.queryParameters}

..['otp_encrypted_api_key'] = 'asdf';

gnprice · 2024-04-01T19:26:41Z

test/api/model/web_auth_test.dart

+      check(() => WebAuthPayload.parse(input)).throws<FormatException>();
+    });
+
+    test('decodeApiKey fails when otp is nonsense', () {


nit:

Suggested change

test('decodeApiKey fails when otp is nonsense', () {

test('decodeApiKey fails when otp is wrong length', () {

Seems more precise about what's really being checked here. (And an OTP is random noise when it is properly generated, so the reader may wonder what it means for it to be "nonsense".)

gnprice · 2024-04-01T19:27:18Z

test/api/model/web_auth_test.dart

+      'zulip://login?otp_encrypted_api_key=$encryptedApiKey&email=self%40example&user_id=1&realm=https%3A%2F%2Fchat.example%2F');
+
+    test('basic happy case', () {
+      final result = WebAuthPayload.parse(wellFormed);


nit: I like the name "payload" for similar locals elsewhere in this commit — I think that'd be clearer here than "result".

gnprice · 2024-04-01T19:42:45Z

lib/api/model/web_auth.dart

+  }
+}
+
+String generateOtp() {


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.

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.

Greg points out, referring to the Flutter style guide at https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#order-other-class-members-in-a-way-that-makes-sense : zulip#600 (comment) > I think it's likely helpful to treat [`buildRoute`] for the > ordering as if it does return the class's type, though. It plays > pretty much the role of a factory constructor — it's the thing > that application code, at least, should always use instead of > calling the actual constructor directly. (Possibly we should even > make the constructor private when we have a `buildRoute` method > like this; but that might be annoying in tests.)

chrisbobbe · 2024-04-01T21:47:08Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! Small comments on the new test; otherwise all looks good.

There's also those other review steps (#600 (review)) that I want to make before merging.

gnprice · 2024-04-01T21:59:53Z

test/api/model/web_auth_test.dart

+      final bytesThatAppear = <int>{};
+      for (final otp in manyOtps) {
+        final bytes = hex.decode(otp);
+        check(bytes.length).equals(32);


nit: process more on the result of check rather than its argument:

Suggested change

check(bytes.length).equals(32);

check(bytes).length.equals(32);

gnprice · 2024-04-01T22:01:50Z

test/api/model/web_auth_test.dart

+        bytesThatAppear.addAll(bytes);
+      }
+
+      final byteValues = List.generate(256, (index) => index);


nit: can use Iterable.generate instead of allocating a List, and then in fact can leave out the callback argument:

Suggested change

final byteValues = List.generate(256, (index) => index);

final byteValues = Iterable.generate(256);

Then probably clearest to just inline that.

gnprice · 2024-04-01T22:05:46Z

test/api/model/web_auth_test.dart

+    test('smoke, and check all 256 byte values are used', () {
+      const n = 216;


Let's make the principle here a bit more explicit — helpful for anyone thinking of adding another probabilistic test and finding this as an example:

Suggested change

test('smoke, and check all 256 byte values are used', () {

const n = 216;

test('smoke, and check all 256 byte values are used', () {

// This is a probabilistic test. We've chosen `n` so that when the test

// should pass, the probability it fails is < 1e-9. See analysis below.

const n = 216;

gnprice · 2024-04-01T22:11:11Z

test/api/model/web_auth_test.dart

+  group('generateOtp', () {
+    test('smoke, and check all 256 byte values are used', () {


I did a quick test to confirm this test runs quickly, and it does. Running this test file with or without this test case included, 3 runs each, the difference in runtime is less than the noise between runs:

$ time flutter test test/api/model/web_auth_test.dart 00:09 +8: All tests passed! time: 10.660s wall (13.258s u, 0.864s s) $ time flutter test test/api/model/web_auth_test.dart 00:01 +8: All tests passed! time: 2.808s wall (3.231s u, 0.613s s) $ time flutter test test/api/model/web_auth_test.dart 00:01 +8: All tests passed! time: 2.724s wall (3.155s u, 0.596s s) $ time flutter test test/api/model/web_auth_test.dart 00:01 +8: All tests passed! time: 2.702s wall (3.165s u, 0.581s s) $ time flutter test test/api/model/web_auth_test.dart --name WebA 00:01 +7: All tests passed! time: 2.764s wall (3.275s u, 0.580s s) $ time flutter test test/api/model/web_auth_test.dart --name WebA 00:01 +7: All tests passed! time: 2.759s wall (3.110s u, 0.642s s) $ time flutter test test/api/model/web_auth_test.dart --name WebA 00:01 +7: All tests passed! time: 2.710s wall (3.188s u, 0.568s s)

(I made one warmup run before the runs I counted, though I was surprised how big the warmup effect was.)

In particular I'm pretty sure the runtime is <50ms, which is fine.

gnprice · 2024-04-01T22:29:08Z

Hmm, well — this doesn't work for me, when I try it on my device.

I tried rust-lang.zulipchat.com, and hit the GitHub button. In the UI, I got the "Something went wrong" dialog box. In logcat, there was this:

I/UrlLauncher(11715): component name for https://rust-lang.zulipchat.com/accounts/login/social/github?mobile_flow_otp=affc188a02b5d5fb2da77fc4215fab209dc466a71e279dd9589e51f65748f904 is null
I/UrlLauncher(11715): component name for https://flutter.dev is null
I/flutter (11715): Instance of 'Error'

That "component name for" message appears to come from canLaunchUrl. The UrlLauncher tag on the log line is a hint to look in the url_launcher package, and indeed:

greg@dark-matter:~/n/flutter/packages/packages/url_launcher (main u=) 
$ gr -C7 'component name'
url_launcher_android/android/src/main/java/io/flutter/plugins/urllauncher/UrlLauncher.java
65-
66-  @Override
67-  public @NonNull Boolean canLaunchUrl(@NonNull String url) {
68-    Intent launchIntent = new Intent(Intent.ACTION_VIEW);
69-    launchIntent.setData(Uri.parse(url));
70-    String componentName = intentResolver.getHandlerComponentName(launchIntent);
71-    if (BuildConfig.DEBUG) {
72:      Log.i(TAG, "component name for " + url + " is " + componentName);
73-    }
74-    if (componentName == null) {
75-      return false;

If I just comment out our canLaunchUrl call, though:

@@ -321,9 +321,9 @@ class _LoginPageState extends State<LoginPage> {
     try {
       final url = widget.serverSettings.realmUrl.resolve(method.loginUrl)
         .replace(queryParameters: {'mobile_flow_otp': _otp!});
-      if (!(await ZulipBinding.instance.canLaunchUrl(url))) {
-        throw Error();
-      }
+      // if (!(await ZulipBinding.instance.canLaunchUrl(url))) {
+      //   throw Error();
+      // }

then…

it does successfully launch the URL — I get the Zulip server's "choose account" web page, to choose from the different email addresses I have on my GitHub account
when I choose one, I get a bit of system UI asking what app to open with, and I choose "Zulip beta" (this is something normal users, who only have one Zulip app installed at a time, should never see)
then I'm back in the app's own UI, and:
- there's that error dialog again
- but also the progress indicator on the app bar
and when I dismiss the error dialog, it's back on LoginPage as if nothing had happened.

And meanwhile in logcat there's this:

W/CustomTabsClient(11715): Unable to find any Custom Tabs packages, you may need to add a <queries> element to your manifest. See the docs for CustomTabsClient#getPackageName.
I/flutter (11715): 'package:zulip/widgets/login.dart': Failed assertion: line 290 pos 15: 'await ZulipBinding.instance.supportsCloseForLaunchMode(_webAuthLaunchMode)': is not true.

Then I commented out that other check:

-      assert (await ZulipBinding.instance.supportsCloseForLaunchMode(_webAuthLaunchMode));
+      // assert (await ZulipBinding.instance.supportsCloseForLaunchMode(_webAuthLaunchMode));

and tried again, and it works!

gnprice · 2024-04-01T22:30:10Z

So in short it does work if we remove those two checks.

Both checks seem to be trying to confirm that the thing the code is about to do next is expected to work. So it's probably best to just try the thing, and if it doesn't work then that's an error we have to deal with.

gnprice · 2024-04-01T22:34:02Z

This also probably indicates it's a good idea to make a minimal effort to report the actual error message when there's an error, because it's likely there will be errors some users run into and we'll want that information for debugging.

In particular if e is Exception, we can use e.message. The error dialog's title can remain short and generic like "Something went wrong".

gnprice · 2024-04-01T23:51:27Z

android/app/src/main/AndroidManifest.xml

+            <meta-data android:name="flutter_deeplinking_enabled" android:value="true" />
+            <intent-filter>


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.

chrisbobbe · 2024-04-02T01:14:03Z

In particular if e is Exception, we can use e.message.

Hmm, I'm seeing that Exception doesn't have a message.

chrisbobbe · 2024-04-02T01:20:48Z

Thanks for the review! Revision pushed, and see my comment just before this: #600 (comment)

gnprice · 2024-04-02T02:58:15Z

Hmm, I'm seeing that Exception doesn't have a message.

Ah, yeah, I read too quickly:

abstract interface class Exception {
  factory Exception([var message]) => _Exception(message);
}

So it's an interface, but with no members — just a marker interface. And its "message" is only an argument to the factory constructor.

Anyway, we can do so for PlatformException, which is likely to include anything thrown by launchUrl or closeInAppWebView.

We're about to add some more login options.

To sit alongside the existing launchUrl.

The binding classes are getting a bit crowded with all the stuff from url_launcher; perhaps we can pull it all out to a helper as a followup.

And also go a bit further than what the test was doing: simulate a tap on the error dialog's dismiss button.

We're about to add the web-auth feature, and the tests for that feature will want their own separate group.

Soon we'll reuse this setup to test the upcoming web-auth feature, and that feature exercises functionality on _ZulipAppState.

This was already being used for tests outside the test file it was defined in, and we'd like to start using it in an additional different test file. So, define it centrally in this place that makes sense.

Greg points out, referring to the Flutter style guide at https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#order-other-class-members-in-a-way-that-makes-sense : zulip#600 (comment) > I think it's likely helpful to treat [`buildRoute`] for the > ordering as if it does return the class's type, though. It plays > pretty much the role of a factory constructor — it's the thing > that application code, at least, should always use instead of > calling the actual constructor directly. (Possibly we should even > make the constructor private when we have a `buildRoute` method > like this; but that might be annoying in tests.)

Fixes: zulip#36

The top of this area will never be in the top inset, since that's consumed by the app bar. So "minimum 8" really just means always 8. So, express that more directly and transparently.

gnprice · 2024-04-02T19:14:34Z

OK, retested and this version works out of the box. The changes look good, so I'll go ahead and merge. Glad to have this feature in!

Then I'll file a follow-up issue (→ #609) for putting more information in the error dialog, for the sake of debugging whatever errors people turn out to run into. Probably be good to do that soon after, while it's fresh in mind (and also so we have it soon for that debugging.)

chrisbobbe added a-login a-first-hour Issues specific to using the app for the first time beta feedback Things beta users have specifically asked for labels Mar 28, 2024

chrisbobbe requested a review from gnprice March 28, 2024 19:58

gnprice reviewed Mar 28, 2024

View reviewed changes

chrisbobbe force-pushed the pr-web-login branch from b351c0b to ff277e7 Compare March 28, 2024 23:59

chrisbobbe force-pushed the pr-web-login branch from ff277e7 to edf7496 Compare March 29, 2024 00:22

chrisbobbe mentioned this pull request Mar 29, 2024

test: CI flake in "UpdateMachine.load retries registerQueue on NetworkError" #602

Closed

chrisbobbe marked this pull request as ready for review March 29, 2024 00:29

gnprice reviewed Apr 1, 2024

View reviewed changes

chrisbobbe force-pushed the pr-web-login branch from edf7496 to 9d56471 Compare April 1, 2024 21:47

gnprice reviewed Apr 1, 2024

View reviewed changes

chrisbobbe force-pushed the pr-web-login branch from 9d56471 to 9bbdf53 Compare April 2, 2024 01:16

chrisbobbe added 4 commits April 2, 2024 12:11

ZulipApp [nfc]: Convert to a stateful widget

fb8bb4e

login [nfc]: s/PasswordLoginPage/LoginPage/

0795586

We're about to add some more login options.

login: Make content scrollable

70233ef

binding: Add canLaunchUrl from url_launcher

a88135a

To sit alongside the existing launchUrl.

chrisbobbe added 10 commits April 2, 2024 12:11

binding: supportsCloseForLaunchMode, closeInAppWebView from url_launcher

a2ed21b

The binding classes are getting a bit crowded with all the stuff from url_launcher; perhaps we can pull it all out to a helper as a followup.

login test: Use checkErrorDialog

84ff624

And also go a bit further than what the test was doing: simulate a tap on the error dialog's dismiss button.

login test [nfc]: Group username/password login tests together

aa8b91b

We're about to add the web-auth feature, and the tests for that feature will want their own separate group.

login test: Use real ZulipApp instead in tests' setup

a8d4888

Soon we'll reuse this setup to test the upcoming web-auth feature, and that feature exercises functionality on _ZulipAppState.

test [nfc]: Allow customizing eg.serverSettings

e4cf955

test: Use right-shaped API keys for selfAccount and otherAccount

d6d3199

test [nfc]: Move prepareBoringImageHttpClient to test_images.dart

1bd1fd7

This was already being used for tests outside the test file it was defined in, and we'd like to start using it in an additional different test file. So, define it centrally in this place that makes sense.

login: Support web-based auth methods

bd2fc3d

Fixes: zulip#36

login [nfc]: Express some unconditional padding more transparently

76ba652

The top of this area will never be in the top inset, since that's consumed by the app bar. So "minimum 8" really just means always 8. So, express that more directly and transparently.

gnprice force-pushed the pr-web-login branch from 9bbdf53 to 76ba652 Compare April 2, 2024 19:14

gnprice merged commit 76ba652 into zulip:main Apr 2, 2024

gnprice mentioned this pull request Apr 2, 2024

Show user more info on error in web auth #609

Closed

chrisbobbe deleted the pr-web-login branch April 2, 2024 19:42

gnprice mentioned this pull request Apr 2, 2024

login: Use Apple API for web auth on iOS, ASWebAuthenticationSession #611

Open

	class LoginPageState extends State<LoginPage> {
	@visibleForTesting
	class LoginPageState extends State<LoginPage> {

	apiKey: 'dQcEJWTq3LczosDkJnRTwf31zniGvMrO',
	// A Zulip API key is 32 digits of base64.
	apiKey: 'dQcEJWTq3LczosDkJnRTwf31zniGvMrO',

	await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver],));
	await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver]));


		// TODO test _inProgress logic?

		final encoded = debugEncodeApiKey(eg.selfAccount.apiKey, LoginPageState.debugOtpOverride!);

		final wellFormed = Uri.parse(
		'zulip://login?otp_encrypted_api_key=$encryptedApiKey&email=self%40example&user_id=1&realm=https%3A%2F%2Fchat.example%2F');

	final queryParams = {...wellFormed.queryParameters}..['otp_encrypted_api_key'] = 'asdf';
	final queryParams = {...wellFormed.queryParameters}
	..['otp_encrypted_api_key'] = 'asdf';

	test('decodeApiKey fails when otp is nonsense', () {
	test('decodeApiKey fails when otp is wrong length', () {

	check(bytes.length).equals(32);
	check(bytes).length.equals(32);

	final byteValues = List.generate(256, (index) => index);
	final byteValues = Iterable.generate(256);

		test('smoke, and check all 256 byte values are used', () {
		const n = 216;

		group('generateOtp', () {
		test('smoke, and check all 256 byte values are used', () {

		<meta-data android:name="flutter_deeplinking_enabled" android:value="true" />
		<intent-filter>

login: Support web-based auth methods #600

login: Support web-based auth methods #600

Conversation

chrisbobbe commented Mar 28, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Mar 29, 2024

chrisbobbe commented Mar 29, 2024

gnprice commented Mar 29, 2024

chrisbobbe commented Mar 29, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 1, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Apr 1, 2024

gnprice commented Apr 1, 2024

gnprice commented Apr 1, 2024

Choose a reason for hiding this comment

chrisbobbe commented Apr 2, 2024

chrisbobbe commented Apr 2, 2024

gnprice commented Apr 2, 2024

gnprice commented Apr 2, 2024 • edited Loading

chrisbobbe commented Mar 28, 2024 •

edited

Loading

chrisbobbe Mar 28, 2024 •

edited

Loading

gnprice commented Apr 2, 2024 •

edited

Loading