Updating user attributes on oauth login (#5324)

Moving update function to app package
Fixing duplicate userID on create user test

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
 }
 

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
+}

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
+}

i18n/en.json

@@ -2339,6 +2339,10 @@
     "id": "api.user.create_oauth_user.not_available.app_error",
     "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"

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 {

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) {