From 4c95d0fbac9738319b661f894e8b277092e6f5ad Mon Sep 17 00:00:00 2001 From: Shoubhik Bose Date: Mon, 5 Jun 2017 21:34:44 +0530 Subject: [PATCH 1/9] update username on relogin --- login/service.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/login/service.go b/login/service.go index 2a30d8e238..7d97d4bf09 100644 --- a/login/service.go +++ b/login/service.go @@ -665,7 +665,8 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st return nil, nil, coreerrors.NewUnauthorizedError(fmt.Sprintf("user '%s' is not approved", claims.Username)) } user = new(account.User) - fillUser(claims, user) + identity = &account.Identity{} + fillUser(claims, user, identity) err = application.Transactional(keycloak.db, func(appl application.Application) error { err := appl.Users().Create(ctx, user) if err != nil { @@ -699,7 +700,7 @@ 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) + fillUser(claims, user, identity) err = keycloak.Users.Save(ctx, user) if err != nil { log.Error(ctx, map[string]interface{}{ @@ -806,10 +807,11 @@ func checkClaims(claims *keycloakTokenClaims) error { return nil } -func fillUser(claims *keycloakTokenClaims, user *account.User) error { +func fillUser(claims *keycloakTokenClaims, user *account.User, identity *account.Identity) error { user.FullName = claims.Name user.Email = claims.Email user.Company = claims.Company + identity.Username = claims.Username if user.ImageURL == "" { image, err := generateGravatarURL(claims.Email) if err != nil { From e3601affc9a2d7c395ac9e1f9bb30a090d898e06 Mon Sep 17 00:00:00 2001 From: Shoubhik Bose Date: Mon, 5 Jun 2017 22:13:26 +0530 Subject: [PATCH 2/9] update username on relogin --- login/service.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/login/service.go b/login/service.go index 7d97d4bf09..47d5b73c69 100644 --- a/login/service.go +++ b/login/service.go @@ -672,12 +672,12 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st if err != nil { return err } - identity = &account.Identity{ - ID: keycloakIdentityID, - Username: claims.Username, - ProviderType: account.KeycloakIDP, - UserID: account.NullUUID{UUID: user.ID, Valid: true}, - User: *user} + + identity.ID = keycloakIdentityID + identity.ProviderType = account.KeycloakIDP + identity.UserID = account.NullUUID{UUID: user.ID, Valid: true} + identity.User = *user + err = appl.Identities().Create(ctx, identity) return err }) From 8645daf71ac678fdce611eda7ebb9672bea4da61 Mon Sep 17 00:00:00 2001 From: Shoubhik Bose Date: Mon, 5 Jun 2017 22:20:26 +0530 Subject: [PATCH 3/9] fix test --- login/service_whitebox_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/login/service_whitebox_test.go b/login/service_whitebox_test.go index 9074e1a396..131b9c565f 100644 --- a/login/service_whitebox_test.go +++ b/login/service_whitebox_test.go @@ -273,12 +273,14 @@ func TestFillUserDoesntOverwriteExistingImageURL(t *testing.T) { resource.Require(t, resource.UnitTest) user := &account.User{FullName: "Vasya Pupkin", Company: "Red Hat", Email: "vpupkin@mail.io", ImageURL: "http://vpupkin.io/image.jpg"} - claims := &keycloakTokenClaims{Name: "new name", Company: "new company", Email: "new email"} - err := fillUser(claims, user) + identity := &account.Identity{Username: "vaysa"} + claims := &keycloakTokenClaims{Username: "new username", Name: "new name", Company: "new company", Email: "new email"} + err := fillUser(claims, user, identity) require.Nil(t, err) assert.Equal(t, "new name", user.FullName) assert.Equal(t, "new company", user.Company) assert.Equal(t, "new email", user.Email) + assert.Equal(t, "new username", identity.Username) assert.Equal(t, "http://vpupkin.io/image.jpg", user.ImageURL) } From 3f0eed77395b7132c640f632e8b3d460844a7624 Mon Sep 17 00:00:00 2001 From: Shoubhik Bose Date: Tue, 6 Jun 2017 00:37:16 +0530 Subject: [PATCH 4/9] persist --- login/service.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/login/service.go b/login/service.go index 47d5b73c69..30eec797ba 100644 --- a/login/service.go +++ b/login/service.go @@ -709,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) + if err != nil { + log.Error(ctx, map[string]interface{}{ + "user_id": identity.ID, + "err": err, + }, "unable to update identity") + return nil, nil, errors.New("Cant' update identity " + err.Error()) + } + } return identity, user, nil } From 88f00cdce0ad871fceb44f899d376e5a06b77b2d Mon Sep 17 00:00:00 2001 From: Shoubhik Bose Date: Tue, 6 Jun 2017 01:27:38 +0530 Subject: [PATCH 5/9] transaction --- login/service.go | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/login/service.go b/login/service.go index 30eec797ba..18a1c855d9 100644 --- a/login/service.go +++ b/login/service.go @@ -701,23 +701,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, identity) - err = keycloak.Users.Save(ctx, user) - if err != nil { - log.Error(ctx, map[string]interface{}{ - "user_id": user.ID, - "err": err, - }, "unable to update user") - return nil, nil, errors.New("Cant' update user " + err.Error()) - } - err = keycloak.Identities.Save(ctx, identity) + err = application.Transactional(keycloak.db, func(appl application.Application) error { + err = appl.Users().Save(ctx, user) + if err != nil { + log.Error(ctx, map[string]interface{}{ + "user_id": user.ID, + "err": err, + }, "unable to update user") + return errors.New("Cant' update user " + err.Error()) + } + err = appl.Identities().Save(ctx, identity) + if err != nil { + log.Error(ctx, map[string]interface{}{ + "user_id": identity.ID, + "err": err, + }, "unable to update identity") + return errors.New("Cant' update identity " + err.Error()) + } + return err + }) if err != nil { log.Error(ctx, map[string]interface{}{ - "user_id": identity.ID, - "err": err, - }, "unable to update identity") - return nil, nil, errors.New("Cant' update identity " + err.Error()) + "keycloak_identity_id": keycloakIdentityID, + "username": claims.Username, + "err": err, + }, "unable to create user/identity") + return nil, nil, errors.New("Cant' create user/identity " + err.Error()) } - } return identity, user, nil } From 3c32c0d42892a966b41c6db8518d218cffc0cb8f Mon Sep 17 00:00:00 2001 From: Shoubhik Bose Date: Tue, 6 Jun 2017 01:51:15 +0530 Subject: [PATCH 6/9] transaction --- login/service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/login/service.go b/login/service.go index 18a1c855d9..c811038d80 100644 --- a/login/service.go +++ b/login/service.go @@ -725,8 +725,8 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st "keycloak_identity_id": keycloakIdentityID, "username": claims.Username, "err": err, - }, "unable to create user/identity") - return nil, nil, errors.New("Cant' create user/identity " + err.Error()) + }, "unable to update user/identity") + return nil, nil, errors.New("Cant' update user/identity " + err.Error()) } } return identity, user, nil From 9ac452f0eb4d8f94f408b9178575da4368e1232d Mon Sep 17 00:00:00 2001 From: Shoubhik Bose Date: Tue, 6 Jun 2017 15:17:29 +0530 Subject: [PATCH 7/9] add conditional block --- login/service.go | 114 ++++++++++++++++++++------------- login/service_whitebox_test.go | 3 +- 2 files changed, 71 insertions(+), 46 deletions(-) diff --git a/login/service.go b/login/service.go index c811038d80..b15caa25b9 100644 --- a/login/service.go +++ b/login/service.go @@ -666,29 +666,38 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st } user = new(account.User) identity = &account.Identity{} - fillUser(claims, user, identity) - err = application.Transactional(keycloak.db, func(appl application.Application) error { - err := appl.Users().Create(ctx, user) - if err != nil { - return err - } - - identity.ID = keycloakIdentityID - identity.ProviderType = account.KeycloakIDP - identity.UserID = account.NullUUID{UUID: user.ID, Valid: true} - identity.User = *user - - err = appl.Identities().Create(ctx, identity) - return err - }) + isChanged, err := fillUser(claims, user, identity) if err != nil { log.Error(ctx, map[string]interface{}{ "keycloak_identity_id": keycloakIdentityID, - "username": claims.Username, - "err": err, + "err": err, }, "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()) + } else if isChanged { + err = application.Transactional(keycloak.db, func(appl application.Application) error { + err := appl.Users().Create(ctx, user) + if err != nil { + return err + } + + identity.ID = keycloakIdentityID + identity.ProviderType = account.KeycloakIDP + identity.UserID = account.NullUUID{UUID: user.ID, Valid: true} + identity.User = *user + + err = appl.Identities().Create(ctx, identity) + return err + }) + if err != nil { + log.Error(ctx, map[string]interface{}{ + "keycloak_identity_id": keycloakIdentityID, + "username": claims.Username, + "err": err, + }, "unable to create user/identity") + return nil, nil, errors.New("Cant' create user/identity " + err.Error()) + } } + } else { identity = &identities[0] user = &identity.User @@ -700,33 +709,41 @@ 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, identity) - err = application.Transactional(keycloak.db, func(appl application.Application) error { - err = appl.Users().Save(ctx, user) - if err != nil { - log.Error(ctx, map[string]interface{}{ - "user_id": user.ID, - "err": err, - }, "unable to update user") - return errors.New("Cant' update user " + err.Error()) - } - err = appl.Identities().Save(ctx, identity) - if err != nil { - log.Error(ctx, map[string]interface{}{ - "user_id": identity.ID, - "err": err, - }, "unable to update identity") - return errors.New("Cant' update identity " + err.Error()) - } - return err - }) + isChanged, err := fillUser(claims, user, identity) if err != nil { log.Error(ctx, map[string]interface{}{ "keycloak_identity_id": keycloakIdentityID, - "username": claims.Username, - "err": err, - }, "unable to update user/identity") - return nil, nil, errors.New("Cant' update user/identity " + err.Error()) + "err": err, + }, "unable to create user/identity") + return nil, nil, errors.New("Cant' update user/identity from claims" + err.Error()) + } else if isChanged { + err = application.Transactional(keycloak.db, func(appl application.Application) error { + err = appl.Users().Save(ctx, user) + if err != nil { + log.Error(ctx, map[string]interface{}{ + "user_id": user.ID, + "err": err, + }, "unable to update user") + return errors.New("Cant' update user " + err.Error()) + } + err = appl.Identities().Save(ctx, identity) + if err != nil { + log.Error(ctx, map[string]interface{}{ + "user_id": identity.ID, + "err": err, + }, "unable to update identity") + return errors.New("Cant' update identity " + err.Error()) + } + return err + }) + if err != nil { + log.Error(ctx, map[string]interface{}{ + "keycloak_identity_id": keycloakIdentityID, + "username": claims.Username, + "err": err, + }, "unable to update user/identity") + return nil, nil, errors.New("Cant' update user/identity " + err.Error()) + } } } return identity, user, nil @@ -826,7 +843,13 @@ func checkClaims(claims *keycloakTokenClaims) error { return nil } -func fillUser(claims *keycloakTokenClaims, user *account.User, identity *account.Identity) error { +func fillUser(claims *keycloakTokenClaims, user *account.User, identity *account.Identity) (bool, error) { + isChanged := false + if user.FullName != claims.Name || user.Email != claims.Email || user.Company != claims.Company || identity.Username != claims.Username || user.ImageURL == "" { + isChanged = true + } else { + return isChanged, nil + } user.FullName = claims.Name user.Email = claims.Email user.Company = claims.Company @@ -838,11 +861,12 @@ func fillUser(claims *keycloakTokenClaims, user *account.User, identity *account "user_full_name": user.FullName, "err": err, }, "error when generating gravatar") - return errors.New("Error when generating gravatar " + err.Error()) + // if there is an error, we will qualify the identity/user as unchanged. + return false, errors.New("Error when generating gravatar " + err.Error()) } user.ImageURL = image } - return nil + return isChanged, nil } // ContextIdentity returns the identity's ID found in given context diff --git a/login/service_whitebox_test.go b/login/service_whitebox_test.go index 131b9c565f..b9858466cb 100644 --- a/login/service_whitebox_test.go +++ b/login/service_whitebox_test.go @@ -275,8 +275,9 @@ func TestFillUserDoesntOverwriteExistingImageURL(t *testing.T) { user := &account.User{FullName: "Vasya Pupkin", Company: "Red Hat", Email: "vpupkin@mail.io", ImageURL: "http://vpupkin.io/image.jpg"} identity := &account.Identity{Username: "vaysa"} claims := &keycloakTokenClaims{Username: "new username", Name: "new name", Company: "new company", Email: "new email"} - err := fillUser(claims, user, identity) + isChanged, err := fillUser(claims, user, identity) require.Nil(t, err) + require.True(t, isChanged) assert.Equal(t, "new name", user.FullName) assert.Equal(t, "new company", user.Company) assert.Equal(t, "new email", user.Email) From 803b4bb5f0ab4c686468acade53286b957aaeef9 Mon Sep 17 00:00:00 2001 From: Shoubhik Bose Date: Tue, 6 Jun 2017 15:24:46 +0530 Subject: [PATCH 8/9] remove conditional block from existing login --- login/service.go | 43 +++++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/login/service.go b/login/service.go index b15caa25b9..0a418e503a 100644 --- a/login/service.go +++ b/login/service.go @@ -666,36 +666,35 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st } user = new(account.User) identity = &account.Identity{} - isChanged, err := fillUser(claims, user, identity) + _, err = fillUser(claims, user, identity) if err != nil { log.Error(ctx, map[string]interface{}{ "keycloak_identity_id": keycloakIdentityID, "err": err, }, "unable to create user/identity") return nil, nil, errors.New("Cant' update user/identity from claims" + err.Error()) - } else if isChanged { - err = application.Transactional(keycloak.db, func(appl application.Application) error { - err := appl.Users().Create(ctx, user) - if err != nil { - return err - } - - identity.ID = keycloakIdentityID - identity.ProviderType = account.KeycloakIDP - identity.UserID = account.NullUUID{UUID: user.ID, Valid: true} - identity.User = *user - - err = appl.Identities().Create(ctx, identity) - return err - }) + } + err = application.Transactional(keycloak.db, func(appl application.Application) error { + err := appl.Users().Create(ctx, user) if err != nil { - log.Error(ctx, map[string]interface{}{ - "keycloak_identity_id": keycloakIdentityID, - "username": claims.Username, - "err": err, - }, "unable to create user/identity") - return nil, nil, errors.New("Cant' create user/identity " + err.Error()) + return err } + + identity.ID = keycloakIdentityID + identity.ProviderType = account.KeycloakIDP + identity.UserID = account.NullUUID{UUID: user.ID, Valid: true} + identity.User = *user + + err = appl.Identities().Create(ctx, identity) + return err + }) + if err != nil { + log.Error(ctx, map[string]interface{}{ + "keycloak_identity_id": keycloakIdentityID, + "username": claims.Username, + "err": err, + }, "unable to create user/identity") + return nil, nil, errors.New("Cant' create user/identity " + err.Error()) } } else { From f54e3ce08f179fe0a37a8056a6834103e10b86c9 Mon Sep 17 00:00:00 2001 From: Shoubhik Bose Date: Tue, 6 Jun 2017 15:38:28 +0530 Subject: [PATCH 9/9] udpate error message --- login/service.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/login/service.go b/login/service.go index 0a418e503a..448b4dde1c 100644 --- a/login/service.go +++ b/login/service.go @@ -672,7 +672,7 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st "keycloak_identity_id": keycloakIdentityID, "err": err, }, "unable to create user/identity") - return nil, nil, errors.New("Cant' update user/identity from claims" + err.Error()) + return nil, nil, errors.New("failed to update user/identity from claims" + err.Error()) } err = application.Transactional(keycloak.db, func(appl application.Application) error { err := appl.Users().Create(ctx, user) @@ -694,7 +694,7 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st "username": claims.Username, "err": err, }, "unable to create user/identity") - return nil, nil, errors.New("Cant' create user/identity " + err.Error()) + return nil, nil, errors.New("failed to create user/identity " + err.Error()) } } else { @@ -714,7 +714,7 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st "keycloak_identity_id": keycloakIdentityID, "err": err, }, "unable to create user/identity") - return nil, nil, errors.New("Cant' update user/identity from claims" + err.Error()) + return nil, nil, errors.New("failed to update user/identity from claims" + err.Error()) } else if isChanged { err = application.Transactional(keycloak.db, func(appl application.Application) error { err = appl.Users().Save(ctx, user) @@ -723,7 +723,7 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st "user_id": user.ID, "err": err, }, "unable to update user") - return errors.New("Cant' update user " + err.Error()) + return errors.New("failed to update user " + err.Error()) } err = appl.Identities().Save(ctx, identity) if err != nil { @@ -731,7 +731,7 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st "user_id": identity.ID, "err": err, }, "unable to update identity") - return errors.New("Cant' update identity " + err.Error()) + return errors.New("failed to update identity " + err.Error()) } return err }) @@ -741,7 +741,7 @@ func (keycloak *KeycloakOAuthProvider) CreateOrUpdateKeycloakUser(accessToken st "username": claims.Username, "err": err, }, "unable to update user/identity") - return nil, nil, errors.New("Cant' update user/identity " + err.Error()) + return nil, nil, errors.New("failed to update user/identity " + err.Error()) } } }