-
Notifications
You must be signed in to change notification settings - Fork 86
Update username on re-login #1367
Update username on re-login #1367
Conversation
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.
LGTM so far.
login/service.go
Outdated
@@ -708,6 +709,15 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st | |||
}, "unable to update user") | |||
return nil, nil, errors.New("Cant' update user " + err.Error()) | |||
} | |||
err = keycloak.Identities.Save(ctx, identity) |
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.
Use a transaction to save both User and Identity.
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, done.
Codecov Report
@@ Coverage Diff @@
## master #1367 +/- ##
==========================================
- Coverage 59.96% 59.81% -0.16%
==========================================
Files 119 119
Lines 10872 10906 +34
==========================================
+ Hits 6519 6523 +4
- Misses 3714 3743 +29
- Partials 639 640 +1
Continue to review full report at Codecov.
|
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.
Overall LGTM. Just one comment about checking if the user/identity has changed.
login/service.go
Outdated
@@ -699,14 +700,33 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st | |||
} | |||
// let's update the existing user with the fullname, email and avatar from Keycloak, | |||
// in case the user changed them since the last time he/she logged in | |||
fillUser(claims, user) | |||
err = keycloak.Users.Save(ctx, user) | |||
fillUser(claims, user, identity) |
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.
Let's check if User or Identity actaully changed. If so then update them. If not then we can skip it:
fillUser(claims *keycloakTokenClaims, user *account.User, identity *account.Identity) (userChanged, identityChanged bool, error)
login/service.go
Outdated
}, "unable to create user/identity") | ||
return nil, nil, errors.New("Cant' create user/identity " + err.Error()) | ||
return nil, nil, errors.New("Cant' update user/identity from claims" + err.Error()) |
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.
suggestion: error messages tend to avoid negation, you can use instead failed to, unable to, invalid ...
[test] |
[test] |
fixes #1366
related to fabric8-ui/fabric8-ui#1442