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

Unified auth experience #5940

Merged
merged 60 commits into from
Nov 12, 2024
Merged

Unified auth experience #5940

merged 60 commits into from
Nov 12, 2024

Conversation

lovincyrus
Copy link
Contributor

@lovincyrus lovincyrus commented Oct 18, 2024

@lovincyrus lovincyrus force-pushed the cyrus/unified-auth branch 3 times, most recently from 402228b to 8cc397f Compare October 28, 2024 17:32
@lovincyrus lovincyrus marked this pull request as ready for review October 29, 2024 00:04
@ericpgreen2
Copy link
Contributor

On staging, when I enter an email address and click "Continue with email", nothing happens.

@lovincyrus
Copy link
Contributor Author

On staging, when I enter an email address and click "Continue with email", nothing happens.

Try again with Enter to submit password. I'll add the click event to the Continue.

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

Can we only designate a sign-in option as "last used" once the user has successfully used it to log-in? See this Jam for two problems:

  1. The "last used" parenthetical switches before the next page is loaded
  2. I do not complete the Microsoft SSO flow, so I'd consider it "never used" not "last used"

web-auth/src/components/DiscordCTA.svelte Outdated Show resolved Hide resolved
web-auth/src/components/utils.ts Outdated Show resolved Hide resolved
web-auth/src/components/Auth.svelte Outdated Show resolved Hide resolved
@lovincyrus
Copy link
Contributor Author

lovincyrus commented Nov 1, 2024

Can we only designate a sign-in option as "last used" once the user has successfully used it to log-in? See this Jam for two problems:

  1. The "last used" parenthetical switches before the next page is loaded
  2. I do not complete the Microsoft SSO flow, so I'd consider it "never used" not "last used"

Will only set last used in a success callback

To do this the right way, we need to persist it with cookie. See https://github.com/rilldata/rill/blob/main/admin/server/auth/handlers.go#L165

@lovincyrus
Copy link
Contributor Author

I can remove the last usage implementation in this ref and revisit this

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

LGTM. @djbarnwal, since you're closer to this code, can you please also give it a review?

Copy link
Member

@djbarnwal djbarnwal left a comment

Choose a reason for hiding this comment

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

https://jam.dev/c/647b8271-4469-4694-9f53-c0ec127c6b3d
Submitting email doesn't work for me. Tried firefox and chrome.

@ericokuma
Copy link

UXQA items:

  1. First time logging in or signing up always started with Log in with Email page
Screenshot 2024-11-07 at 5 01 52 PM

We don't want that. We want to show this page instead:
Screenshot 2024-11-08 at 6 22 38 AM

  1. auth error of type "invalid_request": the connection was not found

Steps to repro:

  • Enter eric.okuma@rilldata.com into email field
  • Hit Continue
  • See that it says email is SSO-supported
  • Hit Continue, get error

@lovincyrus

@lovincyrus
Copy link
Contributor Author

  1. First time logging in or signing up always started with Log in with Email page

This is a bug caused from the latest commit. Will fix

@lovincyrus
Copy link
Contributor Author

2. auth error of type "invalid_request": the connection was not found

I believe this is expected in staging. I was able to repro this error in the previous staging build.

CleanShot.2024-11-11.at.11.15.51.mp4

@lovincyrus
Copy link
Contributor Author

lovincyrus commented Nov 11, 2024

Are legacy users allowed to use google or microsoft auth? @djbarnwal Nvm, I just checked the prod rill dash auth pages. The auth options are in there.

@lovincyrus lovincyrus merged commit 6760fa5 into main Nov 12, 2024
7 checks passed
@lovincyrus lovincyrus deleted the cyrus/unified-auth branch November 12, 2024 17:54
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.

4 participants