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

New Login Sheet #1064

Merged
merged 50 commits into from
May 18, 2024
Merged

New Login Sheet #1064

merged 50 commits into from
May 18, 2024

Conversation

Sjmarf
Copy link
Contributor

@Sjmarf Sjmarf commented May 14, 2024

This PR implements the login sheet (including two-factor authentication) and the token refresh sheet. For testing purposes, a "Re-authenticate" button has been added to the account list in settings. The token refresh sheet is not yet invoked automatically when the token expires.

Important

This PR is dependent on the changes made in this PR and the changes made in this PR.

Login flow:


Token refresh sheet, which will also show the 2FA page if needed:


The syntax for opening the login sheet is as follows:

// Open the login sheet and ask the user to pick an instance
navigation.openSheet(.login())

// Open the login sheet with the specified instance
navigation.openSheet(.login(.instance(instance))

// Open the reauth sheet 
navigation.openSheet(.login(.reauth(userStub))

Closes #1057

@Sjmarf Sjmarf requested a review from a team as a code owner May 14, 2024 21:04
@Sjmarf Sjmarf requested review from WestonHanners and JakeShirley and removed request for a team May 14, 2024 21:04
Copy link
Member

@EricBAndrews EricBAndrews left a comment

Choose a reason for hiding this comment

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

Looks good! A couple minor notes about naming, hashing, and consistency, and I've found a few minor defects:

  • The flow does not check if the account already exists before adding it.
  • Trying to connect to an invalid instance locks up the app until it resolves, which could be upwards of 30 seconds, and doesn't offer any feedback on failure. There should be a timeout (5s?) and a "connection timed out" notice.

On a UX note, I think selecting an instance from the dropdown should auto-submit "next"--it's very easy to go back and change if the user taps the wrong instance, and makes the common case a tap faster.

@Sjmarf
Copy link
Contributor Author

Sjmarf commented May 16, 2024

The flow does not check if the account already exists before adding it.

If the account already exists, it will update the token of the existing account rather than trying to add a second one. Because LoginCredentialsView is used in the re-auth sheet, this provides the correct behavior in that case. You're correct that it doesn't provide any message if you try to login to an already logged-in account in the regular case - I can add an error message for it, though I doubt anyone will try to do this

Sjmarf and others added 8 commits May 16, 2024 07:24
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
Co-authored-by: Eric Andrews <eric.b.andrews.auto@protonmail.com>
@Sjmarf Sjmarf requested a review from EricBAndrews May 16, 2024 19:25
Copy link
Member

@EricBAndrews EricBAndrews left a comment

Choose a reason for hiding this comment

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

LGTM! 🔥

@Sjmarf Sjmarf merged commit 2537b37 into dev2 May 18, 2024
1 check passed
@Sjmarf Sjmarf deleted the sjmarf/dev2-login-sheet branch May 18, 2024 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants