login: Implement browser login #281
Conversation
rajveermalviya
force-pushed
the
browser-auth
branch
from
135559a
to
d96954f
Compare
rajveermalviya
force-pushed
the
browser-auth
branch
from
d96954f
to
5fc9fa1
Compare
rajveermalviya
force-pushed
the
browser-auth
branch
from
5fc9fa1
to
42ffaa1
Compare
Sets up the Flutter deeplink support for android in order to capture the browser redirect. Adds the BrowserLoginWidget which is used to co-ordinate the auth flow, storing the otp temporarily, and finally handling the browser redirect to complete the auth flow. Fixes zulip#36
rajveermalviya
force-pushed
the
browser-auth
branch
from
42ffaa1
to
d66151f
Compare
There was a problem hiding this comment.
Thanks @rajveermalviya for building this! This is a significant feature and would be great to have. Sorry for the long delay in review, but I'm looking at it now.
Would you rebase this onto current main? I think that will also mean you can use ZulipApp.navigatorKey rather than having one on BrowserLoginWidget.
A couple of other high-level comments below.
| // TODO: Migrate to `MaterialApp.router` & `Router`, so that we can receive | ||
| // a full Uri instead of just path+query components and also maybe |
There was a problem hiding this comment.
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.
| // final List<ExternalAuthenticationMethod> external_authentication_methods; // TODO handle | ||
| final List<ExternalAuthenticationMethod> externalAuthenticationMethods; |
There was a problem hiding this comment.
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.
| Uint8List nextBytes(int length) { | ||
| if ((length % 4) != 0) { | ||
| throw ArgumentError('\'length\' must be a multiple of 4'); | ||
| } | ||
| final result = Uint32List(length); | ||
| for (int i = 0; i < length; i++) { | ||
| result[i] = nextInt(_pow2_32); | ||
| } | ||
| return result.buffer.asUint8List(0, length); |
There was a problem hiding this comment.
This gets 4x as much data from the RNG as it uses, right?
I think the simpler version suggested on that upstream thread is fine, just doing nextInt(256). (Well, the version there says nextInt(255), but I believe that's an off-by-one bug.) It's possible that's a bit slower than a more complex version along these lines could be, but this isn't a high-performance context.
| String _decodeApiKey(String otp, String otpEncryptedApiKey) { | ||
| final otpHex = hex.decode(otp); | ||
| final otpEncryptedApiKeyHex = hex.decode(otpEncryptedApiKey); | ||
| return String.fromCharCodes(otpHex ^ otpEncryptedApiKeyHex); |
There was a problem hiding this comment.
The ^ extension makes it kind of opaque to see what this is doing. If we were writing code that was dense with this sort of operation, that could be worth it; but there's just one of these, so let's expand it out to something more explicit:
| return String.fromCharCodes(otpHex ^ otpEncryptedApiKeyHex); | |
| if (otpHex.length != otpEncryptedApiKeyHex.length) throw …; | |
| return String.fromCharCodes(Iterable.generate(otpHex.length, | |
| (i) => otpHex[i] ^ otpEncryptedApiKeyHex[i])); |
| /// | ||
| /// This object also stores the temporarily generated OTP required for | ||
| /// the completion of the flow. | ||
| class BrowserLoginWidget extends InheritedWidget { |
There was a problem hiding this comment.
This will need tests. Most critical is to have tests that cover pieces like the parsing logic in loginFromExternalRoute, which could be covered by small-scale unit tests.
But ideally we'd test more of the overall logic of how it fits together: so e.g. there'd be a test that starts out navigated to AuthMethodsPage, taps a button, and confirms we wind up on the appropriate HomePage. Generally I've been happy at how successful we've been in the Flutter app at writing widget tests at that level of complexity. So I'd like to try to have some kind of test at that level for this functionality too.
A key aspect of such a test is that it's not a full-blown integration test — those would also be nice, but are a further level of test complexity which we haven't yet done for anything and I don't expect as part of this issue. That means this test won't involve actual Android or iOS platform code; it'll all be within Dart. For some examples, see test/notifications_test.dart, and other tests that make heavy use of the bindings in test/model/binding.dart.
|
Thanks for your work on this, @rajveermalviya! At this point, I'm going to take over and finish the work on #36, as it was marked a beta-feedback issue and we'd like to finish it ASAP. I'm grateful for your PR and it will inform my work. Thanks again! |
We expect to use this in the OTP protocol in web auth (zulip#281).
We expect to use this in the OTP protocol in web auth (zulip#281).
We expect to use this in the OTP protocol in web auth (zulip#281).
Fixes #36
Implement authentication using browser auth methods, currently only tested and implemented on Android, but I doubt getting it to work on iOS would be an extensive - in theory should only require enabling universal link support in Flutter.
Testing this locally will require the zulip-mobile app uninstalled, because the redirect url from the browser 'zulip://login' will also be handled by zulip-mobile and if that happens the behavior will UB.
Also only some
external_methodsare tested and only those are enabled currently (google, github, gitlab).