-
Notifications
You must be signed in to change notification settings - Fork 41
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
refactor: keycloak PR remaining TODOs #742
Conversation
api/utils.go
Outdated
// user represents a user in the users.txt file | ||
// The format of each line in the file is: | ||
// (authentication_type:)?email password | ||
func parseUserFromLine(u user) (username, email, password, authenticationType string) { |
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.
The more I look at this the more I am wondering whether this logic should be part of getUserCredentials
. Would that make sense or are there further complexities?
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.
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.
Looks good, but could you please give this a test with a fresh deployment?
|
||
switch ue.config.AuthenticationType { | ||
case AuthenticationTypeOpenID: | ||
if err := ue.authOpenID(authOpenIDLogin); err != nil { | ||
return fmt.Errorf("error while logging in using OpenID: %w", err) | ||
} | ||
|
||
loggedUser, _, err = ue.client.GetUserByUsername(context.Background(), user.Username, "") |
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.
Ah okay, so Signup
already had the right code. We just hadn't applied it to Login
? That feels a bit weird.
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.
Yeah, in my changes (thinking it wasn't working at that particular time because of a Keycloak issue) I forgot that Login
should store the user the same way as SignUp
does.
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.
Thanks, looks great!
I tested this locally and can confirm that users can login correctly using openID |
Summary
keycloak.env
fileLogin
/SignUp
methods instead ofauthOpenID
method.