login: Prep for web-auth #495
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gnprice
reviewed
Jan 30, 2024
lib/widgets/login.dart
Outdated
| @@ -305,46 +363,31 @@ class _PasswordLoginPageState extends State<PasswordLoginPage> { | |||
| } | |||
|
|
|||
| // TODO(server-7): Rely on user_id from fetchApiKey. | |||
| final int userId = result.userId ?? await _getUserId(result); | |||
| final int userId = result.userId ?? await widget.loginPageState._getUserId(result.email, result.apiKey); | |||
There was a problem hiding this comment.
nit: overlong line
Suggested change
| final int userId = result.userId ?? await widget.loginPageState._getUserId(result.email, result.apiKey); | |
| final int userId = result.userId | |
| ?? await widget.loginPageState._getUserId(result.email, result.apiKey); |
lib/widgets/login.dart
Outdated
| // TODO(#108): give feedback to user on SQL exception, like dupe realm+user | ||
| final accountId = await globalStore.insertAccount(AccountsCompanion.insert( | ||
| realmUrl: realmUrl, | ||
| widget.loginPageState._tryInsertAccountAndNavigate( |
There was a problem hiding this comment.
Suggested change
| widget.loginPageState._tryInsertAccountAndNavigate( | |
| await widget.loginPageState._tryInsertAccountAndNavigate( |
Otherwise the finally block happens sooner than it had been.
lib/widgets/login.dart
Outdated
Comment on lines
455
to
457
| onPressed: widget.loginPageState._inProgress | ||
| ? null | ||
| : _submit, |
There was a problem hiding this comment.
nit: this can still fit on one line:
Suggested change
| onPressed: widget.loginPageState._inProgress | |
| ? null | |
| : _submit, | |
| onPressed: widget.loginPageState._inProgress ? null : _submit, |
and I think it's slightly easier to read that way, since the null is trivial and this brings the _submit (which is the main answer) closer to the onPressed: (which is the question).
| child: Center( | ||
| child: ConstrainedBox( | ||
| constraints: const BoxConstraints(maxWidth: 400), | ||
| child: _UsernamePasswordForm(loginPageState: this))))); |
There was a problem hiding this comment.
For now, we'll plan to put the username/password form and all the
web-auth buttons together on the same page. We don't have to keep it
this way forever, but it has some advantages:
- My feeling is that most web apps do this, and I don't see an
obvious reason to deviate from that pattern here, probably
because:
- In the common case of username/password authentication, users
won't need to tap a "Sign in with password" button and then
context-switch to a new page. The form is just there, right after
your realm URL has been accepted.
- Also, it'll be more convenient to share code among the various
ways of authenticating. Code that manages the server-settings
data, and code that inserts an account in the database and
navigates to the authenticated part of the app.
Sure, happy to try this.
On the third point, I think we can get that pretty much just as well if they're on separate pages too — we can have the code in one place but it's a mixin, or a base class. But I think the other two reasons are good ones.
chrisbobbe
force-pushed
the
pr-web-auth-prep
branch
from
74d8428
to
a524420
Compare
|
Thanks for the review! Revision pushed (and it looks like I forgot to comment about that last week). |
We expect to use this in the OTP protocol in web auth (zulip#281).
To make room for web-auth buttons on the same page. For now, we'll plan to put the username/password form and all the web-auth buttons together on the same page. We don't have to keep it this way forever, but it has some advantages: - My feeling is that most web apps do this, and I don't see an obvious reason to deviate from that pattern here, probably because: - In the common case of username/password authentication, users won't need to tap a "Sign in with password" button and then context-switch to a new page. The form is just there, right after your realm URL has been accepted.
gnprice
force-pushed
the
pr-web-auth-prep
branch
from
a524420
to
e9fccdf
Compare
|
Thanks! Looks good; merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some prep commits toward #36, with: