-
Notifications
You must be signed in to change notification settings - Fork 6
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
Support for sign in with local credentials #229
Merged
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
natalian98
commented
Apr 19, 2024
natalian98
force-pushed
the
local-idp-login
branch
from
April 19, 2024 10:08
22f6e82
to
1d9e649
Compare
natalian98
changed the title
Local idp login
Support for sign in with local credentials
Apr 19, 2024
Signed-off-by: David Edler <david.edler@canonical.com>
Signed-off-by: David Edler <david.edler@canonical.com>
natalian98
force-pushed
the
local-idp-login
branch
from
April 23, 2024 12:59
1d9e649
to
9cdad57
Compare
nsklikas
reviewed
Apr 24, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job overall. I left some comments, but most of them don't require changes in the current PR. To sum up the changes that I think are needed in this PR:
- I think we can get rid of the
UiErrorMessages
struct, we can usekCLient.LoginFlow
instead - Would it make sense to just propagate the error description that kratos sends us in
getUiError
instead of creating our own messages? This is part of a larger error handling refactoring we need to do in the login UI, so feel free to disregard it. - Could we automatically import the Kratos users when running the docker compose? (perhaps we could add an entrypoint script or run "job" container like we do for the migration?)
- Add a kratos/service test that tests the service behavior when we get a kratos error response when using the password method, e.g. the user has entered a wrong password
shipperizer
reviewed
Apr 26, 2024
shipperizer
approved these changes
Apr 26, 2024
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.
This PR adds support for signing in with local credentials (kratos
password
method). MFA, recovery and verification will be addressed in future PRs.Testing
Set up docker env:
Run login ui:
Go to http://localhost:2345 and sign in with identity platform. It will redirect to login ui and display the option to sign in with local credentials:
If incorrect credentials are typed, error will be shown:
Logging in with the imported test user (test@example.com / mQ#v9fwHc0@8) logs the user to grafana:
I also tested logging with external idp to make sure the changes don't break it.