Skip to content

Username/password login page#1533

Merged
david-crespo merged 5 commits into
mainfrom
password-login
May 27, 2023
Merged

Username/password login page#1533
david-crespo merged 5 commits into
mainfrom
password-login

Conversation

@david-crespo

Copy link
Copy Markdown
Collaborator

Closes #1457

The main thing of note here is that I'm only putting this page at /login/:silo/local because that way, we know what silo you're trying to log into. This will work with the API as-is (with the trivial addition of telling it to serve the console bundle at that endpoint).

The next step is to serve this page at /login and have it post to /v1/login. In that situation, the API gets to figure out what silo you want when it handles the POST. Console does not have to care about the silo, except for the fact that we want to display the silo name on the page. We could probably add the page at /login without silo name already in this PR, and then once we have a way to get the silo name we can add it.

image

Comment thread app/pages/LoginPage.tsx
const addToast = useToast()
const { silo } = useSiloSelector()

const form = useForm({ defaultValues })

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It is rather pleasant to work with useForm directly like this. Something to think about. I wonder if our helpers like SideModalForm would be nicer to work with if we called useForm (or a wrapper hook that returns the same thing) outside them and passed the result in so the entire form instance was available to the calling code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed!

</div>
<Logo className="absolute bottom-8 left-1/2 hidden -translate-x-1/2 sm-:block" />
</main>
</>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

nothing interesting here, just the extraction of the layout

Comment thread libs/api/hooks.ts
// if logged out, hit /login to trigger login redirect
if (result.statusCode === 401) {
// Exception: 401 on password login POST needs to be handled in-page
if (result.statusCode === 401 && method !== 'loginLocal') {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Without this change, we get a login redirect on bad login attempt, which is obviously not right. This method whitelist approach looks hacky, but it's exercised by the e2e test that looks for the error message on the login form. Will keep an eye on it, but I don't see a reason to get elaborate, for example by allowing the calling code to pass in a flag to exempt it from the login redirect. That would allow this function to not be aware of the method, but it would also require an annoying amount of plumbing through useApiMutation.

This was referenced May 25, 2023
@david-crespo david-crespo enabled auto-merge (squash) May 27, 2023 05:00
@david-crespo david-crespo merged commit 1eb450f into main May 27, 2023
@david-crespo david-crespo deleted the password-login branch May 27, 2023 05:06
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.

Username/password login page for local silo login

2 participants