From 6a77e24adc7199f9c1204cfcb1e216236345df20 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Mon, 22 Mar 2021 14:02:16 -0400 Subject: [PATCH] MM-28090 User settings api when ldap sync (#16822) Automatic Merge --- api4/apitestlib.go | 66 ++++++++++ api4/user.go | 32 ++++- api4/user_test.go | 174 +++++++++++++++++++++++++++ app/app_iface.go | 4 + app/opentracing/opentracing_layer.go | 17 +++ app/user.go | 31 +++++ einterfaces/ldap.go | 1 + einterfaces/mocks/LdapInterface.go | 14 +++ einterfaces/mocks/SamlInterface.go | 14 +++ einterfaces/saml.go | 1 + i18n/en.json | 12 ++ model/user.go | 32 ++++- 12 files changed, 395 insertions(+), 3 deletions(-) diff --git a/api4/apitestlib.go b/api4/apitestlib.go index 1fdcb80363..84f1fbd5d5 100644 --- a/api4/apitestlib.go +++ b/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) } diff --git a/api4/user.go b/api4/user.go index 6939804a84..831f197329 100644 --- a/api4/user.go +++ b/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 { diff --git a/api4/user_test.go b/api4/user_test.go index 9761f4faba..c44fb496ec 100644 --- a/api4/user_test.go +++ b/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) + }) +} diff --git a/app/app_iface.go b/app/app_iface.go index 38f79310a8..2f96f32b87 100644 --- a/app/app_iface.go +++ b/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. diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index 7380be7238..fe4d9558ef 100644 --- a/app/opentracing/opentracing_layer.go +++ b/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") diff --git a/app/user.go b/app/user.go index ab123ad567..8d6c6702ba 100644 --- a/app/user.go +++ b/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 { diff --git a/einterfaces/ldap.go b/einterfaces/ldap.go index ef5fb56b39..5dc09c2d8a 100644 --- a/einterfaces/ldap.go +++ b/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 diff --git a/einterfaces/mocks/LdapInterface.go b/einterfaces/mocks/LdapInterface.go index b768b8a9de..994ce66228 100644 --- a/einterfaces/mocks/LdapInterface.go +++ b/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) diff --git a/einterfaces/mocks/SamlInterface.go b/einterfaces/mocks/SamlInterface.go index a0239a2396..2e53c6fa37 100644 --- a/einterfaces/mocks/SamlInterface.go +++ b/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() diff --git a/einterfaces/saml.go b/einterfaces/saml.go index 157ab013e9..4516b987d4 100644 --- a/einterfaces/saml.go +++ b/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 } diff --git a/i18n/en.json b/i18n/en.json index 2d7e6f025b..bd225ef55a 100644 --- a/i18n/en.json +++ b/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." diff --git a/model/user.go b/model/user.go index 65ecfa0200..e986d96c6f 100644 --- a/model/user.go +++ b/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