diff options
-rw-r--r-- | api/user.go | 6 | ||||
-rw-r--r-- | app/user.go | 43 | ||||
-rw-r--r-- | app/user_test.go | 154 | ||||
-rw-r--r-- | i18n/en.json | 4 | ||||
-rw-r--r-- | store/sql_user_store.go | 5 | ||||
-rw-r--r-- | store/sql_user_store_test.go | 29 |
6 files changed, 237 insertions, 4 deletions
diff --git a/api/user.go b/api/user.go index a81aa85a0..d18c4e3c1 100644 --- a/api/user.go +++ b/api/user.go @@ -167,10 +167,16 @@ func LoginByOAuth(c *Context, w http.ResponseWriter, r *http.Request, service st return nil } + if err = app.UpdateOAuthUserAttrs(bytes.NewReader(buf.Bytes()), user, provider, service, c.siteURL); err != nil { + c.Err = err + return nil + } + doLogin(c, w, r, user, "") if c.Err != nil { return nil } + return user } diff --git a/app/user.go b/app/user.go index 9f428089b..53e389947 100644 --- a/app/user.go +++ b/app/user.go @@ -27,6 +27,7 @@ import ( "github.com/golang/freetype" "github.com/mattermost/platform/einterfaces" "github.com/mattermost/platform/model" + "github.com/mattermost/platform/store" "github.com/mattermost/platform/utils" ) @@ -1253,3 +1254,45 @@ func AutocompleteUsersInTeam(teamId string, term string, searchOptions map[strin return autocomplete, nil } + +func UpdateOAuthUserAttrs(userData io.Reader, user *model.User, provider einterfaces.OauthProvider, service string, siteURL string) *model.AppError { + oauthUser := provider.GetUserFromJson(userData) + + if oauthUser == nil { + return model.NewLocAppError("UpdateOAuthUserAttrs", "api.user.update_oauth_user_attrs.get_user.app_error", map[string]interface{}{"Service": service}, "") + } + + userAttrsChanged := false + + if oauthUser.Username != user.Username { + if existingUser, _ := GetUserByUsername(oauthUser.Username); existingUser == nil { + user.Username = oauthUser.Username + userAttrsChanged = true + } + } + + if oauthUser.GetFullName() != user.GetFullName() { + user.FirstName = oauthUser.FirstName + user.LastName = oauthUser.LastName + userAttrsChanged = true + } + + if oauthUser.Email != user.Email { + if existingUser, _ := GetUserByEmail(oauthUser.Email); existingUser == nil { + user.Email = oauthUser.Email + userAttrsChanged = true + } + } + + if userAttrsChanged { + var result store.StoreResult + if result = <-Srv.Store.User().Update(user, true); result.Err != nil { + return result.Err + } + + user = result.Data.([2]*model.User)[0] + InvalidateCacheForUser(user.Id) + } + + return nil +} diff --git a/app/user_test.go b/app/user_test.go index 0dba86241..177f5a638 100644 --- a/app/user_test.go +++ b/app/user_test.go @@ -4,9 +4,14 @@ package app import ( + "bytes" + "encoding/json" + "math/rand" "strings" "testing" + "time" + "github.com/mattermost/platform/einterfaces" "github.com/mattermost/platform/model" "github.com/mattermost/platform/model/gitlab" "github.com/mattermost/platform/utils" @@ -59,7 +64,8 @@ func TestCheckUserDomain(t *testing.T) { func TestCreateOAuthUser(t *testing.T) { th := Setup().InitBasic() - glUser := oauthgitlab.GitLabUser{Id: 1000, Username: model.NewId(), Email: model.NewId() + "@simulator.amazonses.com", Name: "Joram Wilander"} + r := rand.New(rand.NewSource(time.Now().UnixNano())) + glUser := oauthgitlab.GitLabUser{Id: int64(r.Intn(1000)), Username: model.NewId(), Email: model.NewId() + "@simulator.amazonses.com", Name: "Joram Wilander"} json := glUser.ToJson() user, err := CreateOAuthUser(model.USER_AUTH_SERVICE_GITLAB, strings.NewReader(json), th.BasicTeam.Id) @@ -85,3 +91,149 @@ func TestCreateOAuthUser(t *testing.T) { } } + +func TestUpdateOAuthUserAttrs(t *testing.T) { + Setup() + id := model.NewId() + id2 := model.NewId() + gitlabProvider := einterfaces.GetOauthProvider("gitlab") + + username := "user" + id + username2 := "user" + id2 + + email := "user" + id + "@nowhere.com" + email2 := "user" + id2 + "@nowhere.com" + + var user, user2 *model.User + var gitlabUserObj oauthgitlab.GitLabUser + user, gitlabUserObj = createGitlabUser(t, username, email) + user2, _ = createGitlabUser(t, username2, email2) + + t.Run("UpdateUsername", func(t *testing.T) { + t.Run("NoExistingUserWithSameUsername", func(t *testing.T) { + gitlabUserObj.Username = "updateduser" + model.NewId() + gitlabUser := getGitlabUserPayload(gitlabUserObj, t) + data := bytes.NewReader(gitlabUser) + + user = getUserFromDB(user.Id, t) + UpdateOAuthUserAttrs(data, user, gitlabProvider, "gitlab", "http://localhost:8065") + user = getUserFromDB(user.Id, t) + + if user.Username != gitlabUserObj.Username { + t.Fatal("user's username is not updated") + } + }) + + t.Run("ExistinguserWithSameUsername", func(t *testing.T) { + gitlabUserObj.Username = user2.Username + + gitlabUser := getGitlabUserPayload(gitlabUserObj, t) + data := bytes.NewReader(gitlabUser) + + user = getUserFromDB(user.Id, t) + UpdateOAuthUserAttrs(data, user, gitlabProvider, "gitlab", "http://localhost:8065") + user = getUserFromDB(user.Id, t) + + if user.Username == gitlabUserObj.Username { + t.Fatal("user's username is updated though there already exists another user with the same username") + } + }) + }) + + t.Run("UpdateEmail", func(t *testing.T) { + t.Run("NoExistingUserWithSameEmail", func(t *testing.T) { + gitlabUserObj.Email = "newuser" + model.NewId() + "@nowhere.com" + gitlabUser := getGitlabUserPayload(gitlabUserObj, t) + data := bytes.NewReader(gitlabUser) + + user = getUserFromDB(user.Id, t) + UpdateOAuthUserAttrs(data, user, gitlabProvider, "gitlab", "http://localhost:8065") + user = getUserFromDB(user.Id, t) + + if user.Email != gitlabUserObj.Email { + t.Fatal("user's email is not updated") + } + + if !user.EmailVerified { + t.Fatal("user's email should have been verified") + } + }) + + t.Run("ExistingUserWithSameEmail", func(t *testing.T) { + gitlabUserObj.Email = user2.Email + + gitlabUser := getGitlabUserPayload(gitlabUserObj, t) + data := bytes.NewReader(gitlabUser) + + user = getUserFromDB(user.Id, t) + UpdateOAuthUserAttrs(data, user, gitlabProvider, "gitlab", "http://localhost:8065") + user = getUserFromDB(user.Id, t) + + if user.Email == gitlabUserObj.Email { + t.Fatal("user's email is updated though there already exists another user with the same email") + } + }) + }) + + t.Run("UpdateFirstName", func(t *testing.T) { + gitlabUserObj.Name = "Updated User" + gitlabUser := getGitlabUserPayload(gitlabUserObj, t) + data := bytes.NewReader(gitlabUser) + + user = getUserFromDB(user.Id, t) + UpdateOAuthUserAttrs(data, user, gitlabProvider, "gitlab", "http://localhost:8065") + user = getUserFromDB(user.Id, t) + + if user.FirstName != "Updated" { + t.Fatal("user's first name is not updated") + } + }) + + t.Run("UpdateLastName", func(t *testing.T) { + gitlabUserObj.Name = "Updated Lastname" + gitlabUser := getGitlabUserPayload(gitlabUserObj, t) + data := bytes.NewReader(gitlabUser) + + user = getUserFromDB(user.Id, t) + UpdateOAuthUserAttrs(data, user, gitlabProvider, "gitlab", "http://localhost:8065") + user = getUserFromDB(user.Id, t) + + if user.LastName != "Lastname" { + t.Fatal("user's last name is not updated") + } + }) +} + +func getUserFromDB(id string, t *testing.T) *model.User { + if user, err := GetUser(id); err != nil { + t.Fatal("user is not found") + return nil + } else { + return user + } +} + +func getGitlabUserPayload(gitlabUser oauthgitlab.GitLabUser, t *testing.T) []byte { + var payload []byte + var err error + if payload, err = json.Marshal(gitlabUser); err != nil { + t.Fatal("Serialization of gitlab user to json failed") + } + + return payload +} + +func createGitlabUser(t *testing.T, email string, username string) (*model.User, oauthgitlab.GitLabUser) { + r := rand.New(rand.NewSource(time.Now().UnixNano())) + gitlabUserObj := oauthgitlab.GitLabUser{Id: int64(r.Intn(1000)), Username: username, Login: "user1", Email: email, Name: "Test User"} + gitlabUser := getGitlabUserPayload(gitlabUserObj, t) + + var user *model.User + var err *model.AppError + + if user, err = CreateOAuthUser("gitlab", bytes.NewReader(gitlabUser), ""); err != nil { + t.Fatal("unable to create the user") + } + + return user, gitlabUserObj +} diff --git a/i18n/en.json b/i18n/en.json index c4256851f..8aebd4adc 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2340,6 +2340,10 @@ "translation": "{{.Service}} SSO through OAuth 2.0 not available on this server" }, { + "id": "api.user.update_oauth_user_attrs.get_user.app_error", + "translation": "Could not get user from {{.Service}} user object" + }, + { "id": "api.user.create_profile_image.default_font.app_error", "translation": "Could not create default profile image font" }, diff --git a/store/sql_user_store.go b/store/sql_user_store.go index a2a9c8347..838c2a80d 100644 --- a/store/sql_user_store.go +++ b/store/sql_user_store.go @@ -127,7 +127,6 @@ func (us SqlUserStore) Save(user *model.User) StoreChannel { } func (us SqlUserStore) Update(user *model.User, trustedUpdateData bool) StoreChannel { - storeChannel := make(StoreChannel, 1) go func() { @@ -164,7 +163,9 @@ func (us SqlUserStore) Update(user *model.User, trustedUpdateData bool) StoreCha } if user.IsOAuthUser() { - user.Email = oldUser.Email + if !trustedUpdateData { + user.Email = oldUser.Email + } } else if user.IsLDAPUser() && !trustedUpdateData { if user.Username != oldUser.Username || user.Email != oldUser.Email { diff --git a/store/sql_user_store_test.go b/store/sql_user_store_test.go index c46a32ec1..e509653c1 100644 --- a/store/sql_user_store_test.go +++ b/store/sql_user_store_test.go @@ -4,10 +4,11 @@ package store import ( - "github.com/mattermost/platform/model" "strings" "testing" "time" + + "github.com/mattermost/platform/model" ) func TestUserStoreSave(t *testing.T) { @@ -103,6 +104,32 @@ func TestUserStoreUpdate(t *testing.T) { if err := (<-store.User().Update(u2, false)).Err; err == nil { t.Fatal("Update should have failed because you can't modify AD/LDAP fields") } + + u3 := &model.User{} + u3.Email = model.NewId() + oldEmail := u3.Email + u3.AuthService = "gitlab" + Must(store.User().Save(u3)) + Must(store.Team().SaveMember(&model.TeamMember{TeamId: model.NewId(), UserId: u3.Id})) + + u3.Email = model.NewId() + if result := <-store.User().Update(u3, false); result.Err != nil { + t.Fatal("Update should not have failed") + } else { + newUser := result.Data.([2]*model.User)[0] + if newUser.Email != oldEmail { + t.Fatal("Email should not have been updated as the update is not trusted") + } + } + + if result := <-store.User().Update(u3, true); result.Err != nil { + t.Fatal("Update should not have failed") + } else { + newUser := result.Data.([2]*model.User)[0] + if newUser.Email != u3.Email { + t.Fatal("Email should have been updated as the update is trusted") + } + } } func TestUserStoreUpdateUpdateAt(t *testing.T) { |