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

feat(login): display notification on bad credentials #256

Merged
merged 3 commits into from
Aug 30, 2021

Conversation

andrewazores
Copy link
Member

Fixes #237

@andrewazores andrewazores added the feat New feature or request label Aug 30, 2021
@andrewazores andrewazores requested a review from jan-law August 30, 2021 15:54
@jan-law
Copy link
Contributor

jan-law commented Aug 30, 2021

Is there a way to test this PR without running CRC? I don't have enough RAM on my laptop to run CRC :/

@andrewazores
Copy link
Member Author

andrewazores commented Aug 30, 2021 via email

Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

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

UI looks good! The code also makes sense.

For my own learning, would the 2.0 release also 'forget' your credentials after you refresh the browser tab? After a user logs into Cryostat, how would they log out if they wanted to switch users?

@andrewazores
Copy link
Member Author

Correct, 2.0 UI will still forget your credentials. If you need to switch users you can open a new tab or refresh the current one.

The UI could save the user's credentials into a cookie or localStorage and have a user menu with logout button, but this just hasn't been implemented. PRs accepted, if you're interested, but if it's going into 2.0 the merge window has become very short :-)

Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

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

I'll make a note to look into saving credentials into cookies or localStorage after the release. For now, PR looks good.

@andrewazores andrewazores merged commit da7c1ef into cryostatio:main Aug 30, 2021
@andrewazores andrewazores deleted the auth-fail-notify branch August 30, 2021 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authentication Failure should produce a notification
2 participants