MM-28090 User settings api when ldap sync (#16822)

Automatic Merge

api4/apitestlib.go

@@ -517,6 +517,72 @@ func (th *TestHelper) CreateUserWithClient(client *model.Client4) *model.User {
 	return ruser
 }
 
+func (th *TestHelper) CreateUserWithAuth(authService string) *model.User {
+	id := model.NewId()
+	user := &model.User{
+		Email:         "success+" + id + "@simulator.amazonses.com",
+		Username:      "un_" + id,
+		Nickname:      "nn_" + id,
+		EmailVerified: true,
+		AuthService:   authService,
+	}
+	user, err := th.App.CreateUser(user)
+	if err != nil {
+		panic(err)
+	}
+	return user
+}
+
+func (th *TestHelper) SetupLdapConfig() {
+	th.App.UpdateConfig(func(cfg *model.Config) {
+		*cfg.ServiceSettings.EnableMultifactorAuthentication = true
+		*cfg.LdapSettings.Enable = true
+		*cfg.LdapSettings.EnableSync = true
+		*cfg.LdapSettings.LdapServer = "dockerhost"
+		*cfg.LdapSettings.BaseDN = "dc=mm,dc=test,dc=com"
+		*cfg.LdapSettings.BindUsername = "cn=admin,dc=mm,dc=test,dc=com"
+		*cfg.LdapSettings.BindPassword = "mostest"
+		*cfg.LdapSettings.FirstNameAttribute = "cn"
+		*cfg.LdapSettings.LastNameAttribute = "sn"
+		*cfg.LdapSettings.NicknameAttribute = "cn"
+		*cfg.LdapSettings.EmailAttribute = "mail"
+		*cfg.LdapSettings.UsernameAttribute = "uid"
+		*cfg.LdapSettings.IdAttribute = "cn"
+		*cfg.LdapSettings.LoginIdAttribute = "uid"
+		*cfg.LdapSettings.SkipCertificateVerification = true
+		*cfg.LdapSettings.GroupFilter = ""
+		*cfg.LdapSettings.GroupDisplayNameAttribute = "cN"
+		*cfg.LdapSettings.GroupIdAttribute = "entRyUuId"
+		*cfg.LdapSettings.MaxPageSize = 0
+	})
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+}
+
+func (th *TestHelper) SetupSamlConfig() {
+	th.App.UpdateConfig(func(cfg *model.Config) {
+		*cfg.SamlSettings.Enable = true
+		*cfg.SamlSettings.Verify = false
+		*cfg.SamlSettings.Encrypt = false
+		*cfg.SamlSettings.IdpUrl = "https://does.notmatter.com"
+		*cfg.SamlSettings.IdpDescriptorUrl = "https://localhost/adfs/services/trust"
+		*cfg.SamlSettings.AssertionConsumerServiceURL = "https://localhost/login/sso/saml"
+		*cfg.SamlSettings.ServiceProviderIdentifier = "https://localhost/login/sso/saml"
+		*cfg.SamlSettings.IdpCertificateFile = app.SamlIdpCertificateName
+		*cfg.SamlSettings.PrivateKeyFile = app.SamlPrivateKeyName
+		*cfg.SamlSettings.PublicCertificateFile = app.SamlPublicCertificateName
+		*cfg.SamlSettings.EmailAttribute = "Email"
+		*cfg.SamlSettings.UsernameAttribute = "Username"
+		*cfg.SamlSettings.FirstNameAttribute = "FirstName"
+		*cfg.SamlSettings.LastNameAttribute = "LastName"
+		*cfg.SamlSettings.NicknameAttribute = ""
+		*cfg.SamlSettings.PositionAttribute = ""
+		*cfg.SamlSettings.LocaleAttribute = ""
+		*cfg.SamlSettings.SignatureAlgorithm = model.SAML_SETTINGS_SIGNATURE_ALGORITHM_SHA256
+		*cfg.SamlSettings.CanonicalAlgorithm = model.SAML_SETTINGS_CANONICAL_ALGORITHM_C14N11
+	})
+	th.App.Srv().SetLicense(model.NewTestLicense("saml"))
+}
+
 func (th *TestHelper) CreatePublicChannel() *model.Channel {
 	return th.CreateChannelWithClient(th.Client, model.CHANNEL_OPEN)
 }

api4/user.go

@@ -466,8 +466,19 @@ func setProfileImage(c *Context, w http.ResponseWriter, r *http.Request) {
 		auditRec.AddMeta("filename", imageArray[0].Filename)
 	}
 
-	if user, err := c.App.GetUser(c.Params.UserId); err == nil {
-		auditRec.AddMeta("user", user)
+	user, err := c.App.GetUser(c.Params.UserId)
+	if err != nil {
+		c.SetInvalidUrlParam("user_id")
+		return
+	}
+	auditRec.AddMeta("user", user)
+
+	if (user.IsLDAPUser() || (user.IsSAMLUser() && *c.App.Config().SamlSettings.EnableSyncWithLdap)) &&
+		*c.App.Config().LdapSettings.PictureAttribute != "" {
+		c.Err = model.NewAppError(
+			"uploadProfileImage", "api.user.upload_profile_user.login_provider_attribute_set.app_error",
+			nil, "", http.StatusConflict)
+		return
 	}
 
 	imageData := imageArray[0]
@@ -1109,6 +1120,15 @@ func updateUser(c *Context, w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
+	// Check that the fields being updated are not set by the login provider
+	conflictField := c.App.CheckProviderAttributes(ouser, user.ToPatch())
+	if conflictField != "" {
+		c.Err = model.NewAppError(
+			"updateUser", "api.user.update_user.login_provider_attribute_set.app_error",
+			map[string]interface{}{"Field": conflictField}, "", http.StatusConflict)
+		return
+	}
+
 	// If eMail update is attempted by the currently logged in user, check if correct password was provided
 	if user.Email != "" && ouser.Email != user.Email && c.App.Session().UserId == c.Params.UserId {
 		err = c.App.DoubleCheckPassword(ouser, user.Password)
@@ -1172,6 +1192,14 @@ func patchUser(c *Context, w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
+	conflictField := c.App.CheckProviderAttributes(ouser, patch)
+	if conflictField != "" {
+		c.Err = model.NewAppError(
+			"patchUser", "api.user.patch_user.login_provider_attribute_set.app_error",
+			map[string]interface{}{"Field": conflictField}, "", http.StatusConflict)
+		return
+	}
+
 	// If eMail update is attempted by the currently logged in user, check if correct password was provided
 	if patch.Email != nil && ouser.Email != *patch.Email && c.App.Session().UserId == c.Params.UserId {
 		if patch.Password == nil {

api4/user_test.go

@@ -14,9 +14,11 @@ import (
 
 	"github.com/dgryski/dgoogauth"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 
 	"github.com/mattermost/mattermost-server/v5/app"
+	"github.com/mattermost/mattermost-server/v5/einterfaces/mocks"
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/shared/mail"
 	"github.com/mattermost/mattermost-server/v5/utils/testutils"
@@ -5958,3 +5960,175 @@ func TestReadThreads(t *testing.T) {
 		require.Equal(t, uss3.Threads[0].LastViewedAt, timestamp)
 	})
 }
+
+func TestPatchAndUpdateWithProviderAttributes(t *testing.T) {
+	t.Run("LDAP user", func(t *testing.T) {
+		th := SetupEnterprise(t).InitBasic()
+		defer th.TearDown()
+		user := th.CreateUserWithAuth(model.USER_AUTH_SERVICE_LDAP)
+		ldapMock := &mocks.LdapInterface{}
+		ldapMock.Mock.On(
+			"CheckProviderAttributes",
+			mock.Anything, // app.AppIface
+			mock.Anything, // *model.User
+			mock.Anything, // *model.Patch
+		).Return("")
+		th.App.Srv().Ldap = ldapMock
+		// CheckProviderAttributes should be called for both Patch and Update
+		th.SystemAdminClient.PatchUser(user.Id, &model.UserPatch{})
+		ldapMock.AssertNumberOfCalls(t, "CheckProviderAttributes", 1)
+		th.SystemAdminClient.UpdateUser(user)
+		ldapMock.AssertNumberOfCalls(t, "CheckProviderAttributes", 2)
+	})
+	t.Run("SAML user", func(t *testing.T) {
+		t.Run("with LDAP sync", func(t *testing.T) {
+			th := SetupEnterprise(t).InitBasic()
+			defer th.TearDown()
+			th.SetupLdapConfig()
+			th.SetupSamlConfig()
+			th.App.UpdateConfig(func(cfg *model.Config) {
+				*cfg.SamlSettings.EnableSyncWithLdap = true
+			})
+			user := th.CreateUserWithAuth(model.USER_AUTH_SERVICE_SAML)
+			ldapMock := &mocks.LdapInterface{}
+			ldapMock.Mock.On(
+				"CheckProviderAttributes", mock.Anything, mock.Anything, mock.Anything,
+			).Return("")
+			th.App.Srv().Ldap = ldapMock
+			th.SystemAdminClient.PatchUser(user.Id, &model.UserPatch{})
+			ldapMock.AssertNumberOfCalls(t, "CheckProviderAttributes", 1)
+			th.SystemAdminClient.UpdateUser(user)
+			ldapMock.AssertNumberOfCalls(t, "CheckProviderAttributes", 2)
+		})
+		t.Run("without LDAP sync", func(t *testing.T) {
+			th := SetupEnterprise(t).InitBasic()
+			defer th.TearDown()
+			user := th.CreateUserWithAuth(model.USER_AUTH_SERVICE_SAML)
+			samlMock := &mocks.SamlInterface{}
+			samlMock.Mock.On(
+				"CheckProviderAttributes", mock.Anything, mock.Anything, mock.Anything,
+			).Return("")
+			th.App.Srv().Saml = samlMock
+			th.SystemAdminClient.PatchUser(user.Id, &model.UserPatch{})
+			samlMock.AssertNumberOfCalls(t, "CheckProviderAttributes", 1)
+			th.SystemAdminClient.UpdateUser(user)
+			samlMock.AssertNumberOfCalls(t, "CheckProviderAttributes", 2)
+		})
+	})
+	t.Run("OpenID user", func(t *testing.T) {
+		th := SetupEnterprise(t).InitBasic()
+		defer th.TearDown()
+		user := th.CreateUserWithAuth(model.SERVICE_OPENID)
+		// OAUTH users cannot change these fields
+		for _, fieldName := range []string{
+			"FirstName",
+			"LastName",
+		} {
+			patch := user.ToPatch()
+			patch.SetField(fieldName, "something new")
+			conflictField := th.App.CheckProviderAttributes(user, patch)
+			require.NotEqual(t, "", conflictField)
+		}
+	})
+	t.Run("Patch username", func(t *testing.T) {
+		th := SetupEnterprise(t).InitBasic()
+		defer th.TearDown()
+		// For non-email users, the username must be changed through the provider
+		for _, authService := range []string{
+			model.USER_AUTH_SERVICE_LDAP,
+			model.USER_AUTH_SERVICE_SAML,
+			model.SERVICE_OPENID,
+		} {
+			user := th.CreateUserWithAuth(authService)
+			patch := &model.UserPatch{Username: model.NewString("something new")}
+			conflictField := th.App.CheckProviderAttributes(user, patch)
+			require.NotEqual(t, "", conflictField)
+		}
+	})
+}
+
+func TestSetProfileImageWithProviderAttributes(t *testing.T) {
+	data, err := testutils.ReadTestFile("test.png")
+	require.NoError(t, err)
+
+	type imageTestCase struct {
+		testName      string
+		ldapAttrIsSet bool
+		shouldPass    bool
+	}
+
+	doImageTest := func(t *testing.T, th *TestHelper, user *model.User, testCase imageTestCase) {
+		client := th.SystemAdminClient
+		t.Run(testCase.testName, func(t *testing.T) {
+			th.App.UpdateConfig(func(cfg *model.Config) {
+				if testCase.ldapAttrIsSet {
+					*cfg.LdapSettings.PictureAttribute = "jpegPhoto"
+				} else {
+					*cfg.LdapSettings.PictureAttribute = ""
+				}
+			})
+			ok, resp := client.SetProfileImage(user.Id, data)
+			if testCase.shouldPass {
+				require.True(t, ok)
+				CheckNoError(t, resp)
+			} else {
+				require.False(t, ok)
+				checkHTTPStatus(t, resp, http.StatusConflict, true)
+			}
+		})
+	}
+	doCleanup := func(t *testing.T, th *TestHelper, user *model.User) {
+		info := &model.FileInfo{Path: "users/" + user.Id + "/profile.png"}
+		err = th.cleanupTestFile(info)
+		require.Nil(t, err)
+	}
+
+	t.Run("LDAP user", func(t *testing.T) {
+		testCases := []imageTestCase{
+			{"profile picture attribute is set", true, false},
+			{"profile picture attribute is not set", false, true},
+		}
+		th := SetupEnterprise(t).InitBasic()
+		defer th.TearDown()
+		th.SetupLdapConfig()
+		user := th.CreateUserWithAuth(model.USER_AUTH_SERVICE_LDAP)
+		for _, testCase := range testCases {
+			doImageTest(t, th, user, testCase)
+		}
+		doCleanup(t, th, user)
+	})
+
+	t.Run("SAML user", func(t *testing.T) {
+		th := SetupEnterprise(t).InitBasic()
+		defer th.TearDown()
+		th.SetupLdapConfig()
+		th.SetupSamlConfig()
+		user := th.CreateUserWithAuth(model.USER_AUTH_SERVICE_SAML)
+
+		t.Run("with LDAP sync", func(t *testing.T) {
+			th.App.UpdateConfig(func(cfg *model.Config) {
+				*cfg.SamlSettings.EnableSyncWithLdap = true
+			})
+			testCases := []imageTestCase{
+				{"profile picture attribute is set", true, false},
+				{"profile picture attribute is not set", false, true},
+			}
+			for _, testCase := range testCases {
+				doImageTest(t, th, user, testCase)
+			}
+		})
+		t.Run("without LDAP sync", func(t *testing.T) {
+			th.App.UpdateConfig(func(cfg *model.Config) {
+				*cfg.SamlSettings.EnableSyncWithLdap = false
+			})
+			testCases := []imageTestCase{
+				{"profile picture attribute is set", true, true},
+				{"profile picture attribute is not set", false, true},
+			}
+			for _, testCase := range testCases {
+				doImageTest(t, th, user, testCase)
+			}
+		})
+		doCleanup(t, th, user)
+	})
+}

app/app_iface.go

@@ -55,6 +55,10 @@ type AppIface interface {
 	// The result can be used, for example, to determine the set of users who would be removed from a channel if the
 	// channel were group-constrained with the given groups.
 	ChannelMembersMinusGroupMembers(channelID string, groupIDs []string, page, perPage int) ([]*model.UserWithGroups, int64, *model.AppError)
+	// CheckProviderAttributes returns the empty string if the patch can be applied without
+	// overriding attributes set by the user's login provider; otherwise, the name of the offending
+	// field is returned.
+	CheckProviderAttributes(user *model.User, patch *model.UserPatch) string
 	// ClientConfigWithComputed gets the configuration in a format suitable for sending to the client.
 	ClientConfigWithComputed() map[string]string
 	// ConvertBotToUser converts a bot to user.

app/opentracing/opentracing_layer.go

@@ -1114,6 +1114,23 @@ func (a *OpenTracingAppLayer) CheckPasswordAndAllCriteria(user *model.User, pass
 	return resultVar0
 }
 
+func (a *OpenTracingAppLayer) CheckProviderAttributes(user *model.User, patch *model.UserPatch) string {
+	origCtx := a.ctx
+	span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.CheckProviderAttributes")
+
+	a.ctx = newCtx
+	a.app.Srv().Store.SetContext(newCtx)
+	defer func() {
+		a.app.Srv().Store.SetContext(origCtx)
+		a.ctx = origCtx
+	}()
+
+	defer span.Finish()
+	resultVar0 := a.app.CheckProviderAttributes(user, patch)
+
+	return resultVar0
+}
+
 func (a *OpenTracingAppLayer) CheckRolesExist(roleNames []string) *model.AppError {
 	origCtx := a.ctx
 	span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.CheckRolesExist")

app/user.go

@@ -1183,6 +1183,37 @@ func (a *App) UpdateUserAsUser(user *model.User, asAdmin bool) (*model.User, *mo
 	return updatedUser, nil
 }
 
+// CheckProviderAttributes returns the empty string if the patch can be applied without
+// overriding attributes set by the user's login provider; otherwise, the name of the offending
+// field is returned.
+func (a *App) CheckProviderAttributes(user *model.User, patch *model.UserPatch) string {
+	tryingToChange := func(userValue *string, patchValue *string) bool {
+		return patchValue != nil && *patchValue != *userValue
+	}
+
+	// If any login provider is used, then the username may not be changed
+	if user.AuthService != "" && tryingToChange(&user.Username, patch.Username) {
+		return "username"
+	}
+
+	LdapSettings := &a.Config().LdapSettings
+	SamlSettings := &a.Config().SamlSettings
+
+	conflictField := ""
+	if a.Ldap() != nil &&
+		(user.IsLDAPUser() || (user.IsSAMLUser() && *SamlSettings.EnableSyncWithLdap)) {
+		conflictField = a.Ldap().CheckProviderAttributes(LdapSettings, user, patch)
+	} else if a.Saml() != nil && user.IsSAMLUser() {
+		conflictField = a.Saml().CheckProviderAttributes(SamlSettings, user, patch)
+	} else if user.IsOAuthUser() {
+		if tryingToChange(&user.FirstName, patch.FirstName) || tryingToChange(&user.LastName, patch.LastName) {
+			conflictField = "full name"
+		}
+	}
+
+	return conflictField
+}
+
 func (a *App) PatchUser(userID string, patch *model.UserPatch, asAdmin bool) (*model.User, *model.AppError) {
 	user, err := a.GetUser(userID)
 	if err != nil {

einterfaces/ldap.go

@@ -13,6 +13,7 @@ type LdapInterface interface {
 	GetUserAttributes(id string, attributes []string) (map[string]string, *model.AppError)
 	CheckPassword(id string, password string) *model.AppError
 	CheckPasswordAuthData(authData string, password string) *model.AppError
+	CheckProviderAttributes(LS *model.LdapSettings, ouser *model.User, patch *model.UserPatch) string
 	SwitchToLdap(userId, ldapId, ldapPassword string) *model.AppError
 	StartSynchronizeJob(waitForJobToFinish bool) (*model.Job, *model.AppError)
 	RunTest() *model.AppError

einterfaces/mocks/LdapInterface.go

@@ -46,6 +46,20 @@ func (_m *LdapInterface) CheckPasswordAuthData(authData string, password string)
 	return r0
 }
 
+// CheckProviderAttributes provides a mock function with given fields: LS, ouser, patch
+func (_m *LdapInterface) CheckProviderAttributes(LS *model.LdapSettings, ouser *model.User, patch *model.UserPatch) string {
+	ret := _m.Called(LS, ouser, patch)
+
+	var r0 string
+	if rf, ok := ret.Get(0).(func(*model.LdapSettings, *model.User, *model.UserPatch) string); ok {
+		r0 = rf(LS, ouser, patch)
+	} else {
+		r0 = ret.Get(0).(string)
+	}
+
+	return r0
+}
+
 // DoLogin provides a mock function with given fields: id, password
 func (_m *LdapInterface) DoLogin(id string, password string) (*model.User, *model.AppError) {
 	ret := _m.Called(id, password)

einterfaces/mocks/SamlInterface.go

@@ -39,6 +39,20 @@ func (_m *SamlInterface) BuildRequest(relayState string) (*model.SamlAuthRequest
 	return r0, r1
 }
 
+// CheckProviderAttributes provides a mock function with given fields: SS, ouser, patch
+func (_m *SamlInterface) CheckProviderAttributes(SS *model.SamlSettings, ouser *model.User, patch *model.UserPatch) string {
+	ret := _m.Called(SS, ouser, patch)
+
+	var r0 string
+	if rf, ok := ret.Get(0).(func(*model.SamlSettings, *model.User, *model.UserPatch) string); ok {
+		r0 = rf(SS, ouser, patch)
+	} else {
+		r0 = ret.Get(0).(string)
+	}
+
+	return r0
+}
+
 // ConfigureSP provides a mock function with given fields:
 func (_m *SamlInterface) ConfigureSP() error {
 	ret := _m.Called()

einterfaces/saml.go

@@ -12,4 +12,5 @@ type SamlInterface interface {
 	BuildRequest(relayState string) (*model.SamlAuthRequest, *model.AppError)
 	DoLogin(encodedXML string, relayState map[string]string) (*model.User, *model.AppError)
 	GetMetadata() (string, *model.AppError)
+	CheckProviderAttributes(SS *model.SamlSettings, ouser *model.User, patch *model.UserPatch) string
 }

i18n/en.json

@@ -3586,6 +3586,10 @@
     "id": "api.user.oauth_to_email.not_available.app_error",
     "translation": "Authentication Transfer not configured or available on this server."
   },
+  {
+    "id": "api.user.patch_user.login_provider_attribute_set.app_error",
+    "translation": "Field '{{.Field}}' must be set through user's login provider."
+  },
   {
     "id": "api.user.promote_guest_to_user.no_guest.app_error",
     "translation": "Unable to convert the guest to regular user because is not a guest."
@@ -3726,6 +3730,10 @@
     "id": "api.user.update_user.accepted_guest_domain.app_error",
     "translation": "The email you provided does not belong to an accepted domain for guest accounts. Please contact your administrator or sign up with a different email."
   },
+  {
+    "id": "api.user.update_user.login_provider_attribute_set.app_error",
+    "translation": "Field '{{.Field}}' must be set through user's login provider."
+  },
   {
     "id": "api.user.update_user_auth.invalid_request",
     "translation": "Request is missing either AuthData or AuthService parameter."
@@ -3750,6 +3758,10 @@
     "id": "api.user.upload_profile_user.encode.app_error",
     "translation": "Could not encode profile image."
   },
+  {
+    "id": "api.user.upload_profile_user.login_provider_attribute_set.app_error",
+    "translation": "Profile picture must be set through user's login provider."
+  },
   {
     "id": "api.user.upload_profile_user.no_file.app_error",
     "translation": "No file under 'image' in request."

model/user.go

@@ -710,7 +710,10 @@ func (u *User) IsSSOUser() bool {
 }
 
 func (u *User) IsOAuthUser() bool {
-	return u.AuthService == USER_AUTH_SERVICE_GITLAB
+	return u.AuthService == SERVICE_GITLAB ||
+		u.AuthService == SERVICE_GOOGLE ||
+		u.AuthService == SERVICE_OFFICE365 ||
+		u.AuthService == SERVICE_OPENID
 }
 
 func (u *User) IsLDAPUser() bool {
@@ -725,6 +728,33 @@ func (u *User) GetPreferredTimezone() string {
 	return GetPreferredTimezone(u.Timezone)
 }
 
+func (u *User) ToPatch() *UserPatch {
+	return &UserPatch{
+		Username: &u.Username, Password: &u.Password,
+		Nickname: &u.Nickname, FirstName: &u.FirstName, LastName: &u.LastName,
+		Position: &u.Position, Email: &u.Email,
+		Props: u.Props, NotifyProps: u.NotifyProps,
+		Locale: &u.Locale, Timezone: u.Timezone,
+	}
+}
+
+func (u *UserPatch) SetField(fieldName string, fieldValue string) {
+	switch fieldName {
+	case "FirstName":
+		u.FirstName = &fieldValue
+	case "LastName":
+		u.LastName = &fieldValue
+	case "Nickname":
+		u.Nickname = &fieldValue
+	case "Email":
+		u.Email = &fieldValue
+	case "Position":
+		u.Position = &fieldValue
+	case "Username":
+		u.Username = &fieldValue
+	}
+}
+
 // UserFromJson will decode the input and return a User
 func UserFromJson(data io.Reader) *User {
 	var user *User