From 5f7482e541146c383eb2ed5c98e200c96da6a6ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Mon, 14 Aug 2023 17:54:10 +0200 Subject: [PATCH] Remove remote users from the license counting and explicitly dissallow them to log in (#22582) * Making all the counts aware of Remote users * Disable login for remote users * Adding tests for login remote_users error * Adding tests for the store * Adding frontend part of not counting remote users in the license * Addressing PR review comment * Adding the new ExternaUserId field to users * Running make migrations-extract * Running make app-layers and make gen-serialized * Revert "Adding the new ExternaUserId field to users" This reverts commit 12e5fd518962a16cdbdb8964179b6cd8e915f230. * Adding GetUserByRemoteID methods * Adding needed migration for users * i18n-extract * Fixing postgres increase remote user id field size migration up and down * run make gen-serialized * Removing migration code * Not count remote users as part of the cloud pricing * Add the cloud subscription when a user gets promote from remote to not-remote * Fixing merge problems --------- Co-authored-by: Mattermost Build --- server/channels/api4/user.go | 9 +++ server/channels/api4/user_test.go | 12 +++ server/channels/app/app_iface.go | 1 + .../app/opentracing/opentracing_layer.go | 22 ++++++ server/channels/app/plugin_api.go | 4 + server/channels/app/user.go | 25 ++++++- server/channels/app/users/users.go | 4 + .../opentracinglayer/opentracinglayer.go | 18 +++++ .../channels/store/retrylayer/retrylayer.go | 21 ++++++ server/channels/store/sqlstore/user_store.go | 45 ++++++++++- server/channels/store/store.go | 1 + .../store/storetest/mocks/UserStore.go | 26 +++++++ server/channels/store/storetest/user_store.go | 74 +++++++++++++++++++ .../channels/store/timerlayer/timerlayer.go | 16 ++++ server/i18n/en.json | 4 + server/public/model/user_count.go | 2 + .../user_list_row_with_error.tsx | 12 +++ webapp/channels/src/i18n/en.json | 1 + webapp/platform/types/src/users.ts | 1 + 19 files changed, 295 insertions(+), 3 deletions(-) diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index 30204b9076..18ec975f23 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -557,12 +557,14 @@ func getFilteredUsersStats(c *Context, w http.ResponseWriter, r *http.Request) { channelID := r.URL.Query().Get("in_channel") includeDeleted := r.URL.Query().Get("include_deleted") includeBotAccounts := r.URL.Query().Get("include_bots") + includeRemoteUsers := r.URL.Query().Get("include_remote_users") rolesString := r.URL.Query().Get("roles") channelRolesString := r.URL.Query().Get("channel_roles") teamRolesString := r.URL.Query().Get("team_roles") includeDeletedBool, _ := strconv.ParseBool(includeDeleted) includeBotAccountsBool, _ := strconv.ParseBool(includeBotAccounts) + includeRemoteUsersBool, _ := strconv.ParseBool(includeRemoteUsers) roles := []string{} var rolesValid bool @@ -593,6 +595,7 @@ func getFilteredUsersStats(c *Context, w http.ResponseWriter, r *http.Request) { options := &model.UserCountOptions{ IncludeDeleted: includeDeletedBool, IncludeBotAccounts: includeBotAccountsBool, + IncludeRemoteUsers: includeRemoteUsersBool, TeamId: teamID, ChannelId: channelID, Roles: roles, @@ -1832,6 +1835,7 @@ func login(c *Context, w http.ResponseWriter, r *http.Request) { "api.user.check_user_mfa.bad_code.app_error", "api.user.login.blank_pwd.app_error", "api.user.login.bot_login_forbidden.app_error", + "api.user.login.remote_users.login.error", "api.user.login.client_side_cert.certificate.app_error", "api.user.login.inactive.app_error", "api.user.login.not_verified.app_error", @@ -1932,6 +1936,11 @@ func login(c *Context, w http.ResponseWriter, r *http.Request) { } } + if user.IsRemote() { + c.Err = model.NewAppError("login", "api.user.login.remote_users.login.error", nil, "", http.StatusUnauthorized) + return + } + c.LogAuditWithUserId(user.Id, "authenticated") err = c.App.DoLogin(c.AppContext, w, r, user, deviceId, false, false, false) diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index f3be8485dc..aa10a94355 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -3842,6 +3842,18 @@ func TestLogin(t *testing.T) { CheckErrorID(t, err, "api.user.login.bot_login_forbidden.app_error") }) + t.Run("remote user login rejected", func(t *testing.T) { + email := th.GenerateTestEmail() + user := model.User{Email: email, Nickname: "Darth Vader", Password: "hello1", Username: GenerateTestUsername(), Roles: model.SystemAdminRoleId + " " + model.SystemUserRoleId, RemoteId: model.NewString("remote-id")} + ruser, _, _ := th.Client.CreateUser(context.Background(), &user) + + _, err := th.SystemAdminClient.UpdateUserPassword(context.Background(), ruser.Id, "", "password") + require.NoError(t, err) + + _, _, err = th.Client.Login(context.Background(), ruser.Email, "password") + CheckErrorID(t, err, "api.user.login.remote_users.login.error") + }) + t.Run("login with terms_of_service set", func(t *testing.T) { termsOfService, appErr := th.App.CreateTermsOfService("terms of service", th.BasicUser.Id) require.Nil(t, appErr) diff --git a/server/channels/app/app_iface.go b/server/channels/app/app_iface.go index 720e8aed52..aca017db9e 100644 --- a/server/channels/app/app_iface.go +++ b/server/channels/app/app_iface.go @@ -820,6 +820,7 @@ type AppIface interface { GetUserAccessTokensForUser(userID string, page, perPage int) ([]*model.UserAccessToken, *model.AppError) GetUserByAuth(authData *string, authService string) (*model.User, *model.AppError) GetUserByEmail(email string) (*model.User, *model.AppError) + GetUserByRemoteID(remoteID string) (*model.User, *model.AppError) GetUserByUsername(username string) (*model.User, *model.AppError) GetUserForLogin(id, loginId string) (*model.User, *model.AppError) GetUserTermsOfService(userID string) (*model.UserTermsOfService, *model.AppError) diff --git a/server/channels/app/opentracing/opentracing_layer.go b/server/channels/app/opentracing/opentracing_layer.go index 70a2a07d75..528379bd5a 100644 --- a/server/channels/app/opentracing/opentracing_layer.go +++ b/server/channels/app/opentracing/opentracing_layer.go @@ -10489,6 +10489,28 @@ func (a *OpenTracingAppLayer) GetUserByEmail(email string) (*model.User, *model. return resultVar0, resultVar1 } +func (a *OpenTracingAppLayer) GetUserByRemoteID(remoteID string) (*model.User, *model.AppError) { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetUserByRemoteID") + + a.ctx = newCtx + a.app.Srv().Store().SetContext(newCtx) + defer func() { + a.app.Srv().Store().SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0, resultVar1 := a.app.GetUserByRemoteID(remoteID) + + if resultVar1 != nil { + span.LogFields(spanlog.Error(resultVar1)) + ext.Error.Set(span, true) + } + + return resultVar0, resultVar1 +} + func (a *OpenTracingAppLayer) GetUserByUsername(username string) (*model.User, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetUserByUsername") diff --git a/server/channels/app/plugin_api.go b/server/channels/app/plugin_api.go index 9f37368356..5fa7b56706 100644 --- a/server/channels/app/plugin_api.go +++ b/server/channels/app/plugin_api.go @@ -257,6 +257,10 @@ func (api *PluginAPI) GetUserByUsername(name string) (*model.User, *model.AppErr return api.app.GetUserByUsername(name) } +func (api *PluginAPI) GetUserByRemoteID(remoteID string) (*model.User, *model.AppError) { + return api.app.GetUserByRemoteID(remoteID) +} + func (api *PluginAPI) GetUsersByUsernames(usernames []string) ([]*model.User, *model.AppError) { return api.app.GetUsersByUsernames(usernames, true, nil) } diff --git a/server/channels/app/user.go b/server/channels/app/user.go index 944443834b..79d2dab001 100644 --- a/server/channels/app/user.go +++ b/server/channels/app/user.go @@ -308,7 +308,7 @@ func (a *App) createUserOrGuest(c request.CTX, user *model.User, guest bool) (*m // table in CWS. This is then used to calculate how much the customers have to pay in addition for the extra users. If the // workspace is currently on a monthly plan, then this function will not do anything. - if a.Channels().License().IsCloud() { + if a.Channels().License().IsCloud() && !ruser.IsRemote() { go func(userId string) { _, err := a.SendSubscriptionHistoryEvent(userId) if err != nil { @@ -440,6 +440,20 @@ func (a *App) GetUserByEmail(email string) (*model.User, *model.AppError) { return user, nil } +func (a *App) GetUserByRemoteID(remoteID string) (*model.User, *model.AppError) { + user, err := a.ch.srv.userService.GetUserByRemoteID(remoteID) + if err != nil { + var nfErr *store.ErrNotFound + switch { + case errors.As(err, &nfErr): + return nil, model.NewAppError("GetUserByRemoteID", MissingAccountError, nil, "", http.StatusNotFound).Wrap(err) + default: + return nil, model.NewAppError("GetUserByRemoteID", MissingAccountError, nil, "", http.StatusInternalServerError).Wrap(err) + } + } + return user, nil +} + func (a *App) GetUserByAuth(authData *string, authService string) (*model.User, *model.AppError) { user, err := a.ch.srv.userService.GetUserByAuth(authData, authService) if err != nil { @@ -1262,6 +1276,15 @@ func (a *App) UpdateUser(c request.CTX, user *model.User, sendNotifications bool a.InvalidateCacheForUser(user.Id) a.onUserProfileChange(user.Id) + if a.Channels().License().IsCloud() && prev.IsRemote() && !user.IsRemote() { + go func(userId string) { + _, err := a.SendSubscriptionHistoryEvent(userId) + if err != nil { + c.Logger().Error("Failed to create/update the SubscriptionHistoryEvent", mlog.Err(err)) + } + }(user.Id) + } + return newUser, nil } diff --git a/server/channels/app/users/users.go b/server/channels/app/users/users.go index 1418ee251a..85617d7372 100644 --- a/server/channels/app/users/users.go +++ b/server/channels/app/users/users.go @@ -105,6 +105,10 @@ func (us *UserService) GetUserByEmail(email string) (*model.User, error) { return us.store.GetByEmail(email) } +func (us *UserService) GetUserByRemoteID(remoteID string) (*model.User, error) { + return us.store.GetByRemoteID(remoteID) +} + func (us *UserService) GetUserByAuth(authData *string, authService string) (*model.User, error) { return us.store.GetByAuth(authData, authService) } diff --git a/server/channels/store/opentracinglayer/opentracinglayer.go b/server/channels/store/opentracinglayer/opentracinglayer.go index 0a3f313d28..5a99018db5 100644 --- a/server/channels/store/opentracinglayer/opentracinglayer.go +++ b/server/channels/store/opentracinglayer/opentracinglayer.go @@ -11148,6 +11148,24 @@ func (s *OpenTracingLayerUserStore) GetByEmail(email string) (*model.User, error return result, err } +func (s *OpenTracingLayerUserStore) GetByRemoteID(remoteID string) (*model.User, error) { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "UserStore.GetByRemoteID") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + result, err := s.UserStore.GetByRemoteID(remoteID) + if err != nil { + span.LogFields(spanlog.Error(err)) + ext.Error.Set(span, true) + } + + return result, err +} + func (s *OpenTracingLayerUserStore) GetByUsername(username string) (*model.User, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "UserStore.GetByUsername") diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index 3e222a5c5c..b00383a475 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -12739,6 +12739,27 @@ func (s *RetryLayerUserStore) GetByEmail(email string) (*model.User, error) { } +func (s *RetryLayerUserStore) GetByRemoteID(remoteID string) (*model.User, error) { + + tries := 0 + for { + result, err := s.UserStore.GetByRemoteID(remoteID) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerUserStore) GetByUsername(username string) (*model.User, error) { tries := 0 diff --git a/server/channels/store/sqlstore/user_store.go b/server/channels/store/sqlstore/user_store.go index 729fc15152..e59d81e648 100644 --- a/server/channels/store/sqlstore/user_store.go +++ b/server/channels/store/sqlstore/user_store.go @@ -1174,6 +1174,26 @@ func (us SqlUserStore) GetByEmail(email string) (*model.User, error) { return &user, nil } +func (us SqlUserStore) GetByRemoteID(remoteID string) (*model.User, error) { + query := us.usersQuery.Where(sq.Eq{"RemoteId": remoteID}) + + queryString, args, err := query.ToSql() + if err != nil { + return nil, errors.Wrap(err, "get_by_remote_id_tosql") + } + + user := model.User{} + if err := us.GetReplicaX().Get(&user, queryString, args...); err != nil { + if err == sql.ErrNoRows { + return nil, errors.Wrap(store.NewErrNotFound("User", fmt.Sprintf("remoteid=%s", remoteID)), "failed to find User") + } + + return nil, errors.Wrapf(err, "failed to get User with RemoteId=%s", remoteID) + } + + return &user, nil +} + func (us SqlUserStore) GetByAuth(authData *string, authService string) (*model.User, error) { if authData == nil || *authData == "" { return nil, store.NewErrInvalidInput("User", "", "empty or nil") @@ -1310,6 +1330,10 @@ func (us SqlUserStore) Count(options model.UserCountOptions) (int64, error) { query = query.Where("u.DeleteAt = 0") } + if !options.IncludeRemoteUsers { + query = query.Where(sq.Or{sq.Eq{"u.RemoteId": ""}, sq.Eq{"u.RemoteId": nil}}) + } + isPostgreSQL := us.DriverName() == model.DatabaseDriverPostgres if options.IncludeBotAccounts { if options.ExcludeRegularUsers { @@ -1365,8 +1389,17 @@ func (us SqlUserStore) AnalyticsActiveCount(timePeriod int64, options model.User query = query.Where(sq.Expr("UserId NOT IN (SELECT UserId FROM Bots)")) } } + + if !options.IncludeRemoteUsers || !options.IncludeDeleted { + query = query.LeftJoin("Users ON s.UserId = Users.Id") + } + + if !options.IncludeRemoteUsers { + query = query.Where(sq.Or{sq.Eq{"Users.RemoteId": ""}, sq.Eq{"Users.RemoteId": nil}}) + } + if !options.IncludeDeleted { - query = query.LeftJoin("Users ON s.UserId = Users.Id").Where("Users.DeleteAt = 0") + query = query.Where("Users.DeleteAt = 0") } queryStr, args, err := query.ToSql() @@ -1393,8 +1426,16 @@ func (us SqlUserStore) AnalyticsActiveCountForPeriod(startTime int64, endTime in } } + if !options.IncludeRemoteUsers || !options.IncludeDeleted { + query = query.LeftJoin("Users ON s.UserId = Users.Id") + } + + if !options.IncludeRemoteUsers { + query = query.Where(sq.Or{sq.Eq{"Users.RemoteId": ""}, sq.Eq{"Users.RemoteId": nil}}) + } + if !options.IncludeDeleted { - query = query.LeftJoin("Users ON s.UserId = Users.Id").Where("Users.DeleteAt = 0") + query = query.Where("Users.DeleteAt = 0") } queryStr, args, err := query.ToSql() diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 347b2e1809..8657bca30d 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -424,6 +424,7 @@ type UserStore interface { GetProfileByGroupChannelIdsForUser(userID string, channelIds []string) (map[string][]*model.User, error) InvalidateProfileCacheForUser(userID string) GetByEmail(email string) (*model.User, error) + GetByRemoteID(remoteID string) (*model.User, error) GetByAuth(authData *string, authService string) (*model.User, error) GetAllUsingAuthService(authService string) ([]*model.User, error) GetAllNotInAuthService(authServices []string) ([]*model.User, error) diff --git a/server/channels/store/storetest/mocks/UserStore.go b/server/channels/store/storetest/mocks/UserStore.go index a0f7c11f6d..48ef53822d 100644 --- a/server/channels/store/storetest/mocks/UserStore.go +++ b/server/channels/store/storetest/mocks/UserStore.go @@ -541,6 +541,32 @@ func (_m *UserStore) GetByEmail(email string) (*model.User, error) { return r0, r1 } +// GetByRemoteID provides a mock function with given fields: remoteID +func (_m *UserStore) GetByRemoteID(remoteID string) (*model.User, error) { + ret := _m.Called(remoteID) + + var r0 *model.User + var r1 error + if rf, ok := ret.Get(0).(func(string) (*model.User, error)); ok { + return rf(remoteID) + } + if rf, ok := ret.Get(0).(func(string) *model.User); ok { + r0 = rf(remoteID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.User) + } + } + + if rf, ok := ret.Get(1).(func(string) error); ok { + r1 = rf(remoteID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // GetByUsername provides a mock function with given fields: username func (_m *UserStore) GetByUsername(username string) (*model.User, error) { ret := _m.Called(username) diff --git a/server/channels/store/storetest/user_store.go b/server/channels/store/storetest/user_store.go index 66212f47b9..baf5522193 100644 --- a/server/channels/store/storetest/user_store.go +++ b/server/channels/store/storetest/user_store.go @@ -3963,6 +3963,15 @@ func testCount(t *testing.T, ss store.Store) { require.NoError(t, err) defer func() { require.NoError(t, ss.User().PermanentDelete(deletedUser.Id)) }() + // Remote User + remoteId := "remote-id" + remoteUser, err := ss.User().Save(&model.User{ + Email: MakeEmail(), + RemoteId: &remoteId, + }) + require.NoError(t, err) + defer func() { require.NoError(t, ss.User().PermanentDelete(remoteUser.Id)) }() + // Bot botUser, err := ss.User().Save(&model.User{ Email: MakeEmail(), @@ -4067,6 +4076,71 @@ func testCount(t *testing.T, ss store.Store) { }, 0, }, + { + "Include remote accounts no deleted accounts and no team id", + model.UserCountOptions{ + IncludeRemoteUsers: true, + IncludeDeleted: false, + TeamId: "", + }, + 5, + }, + { + "Include delete accounts no remote accounts and no team id", + model.UserCountOptions{ + IncludeRemoteUsers: false, + IncludeDeleted: true, + TeamId: "", + }, + 5, + }, + { + "Include remote accounts and deleted accounts and no team id", + model.UserCountOptions{ + IncludeRemoteUsers: true, + IncludeDeleted: true, + TeamId: "", + }, + 6, + }, + { + "Include remote accounts and deleted accounts with existing team id", + model.UserCountOptions{ + IncludeRemoteUsers: true, + IncludeDeleted: true, + TeamId: teamId, + }, + 4, + }, + { + "Include remote accounts and deleted accounts with fake team id", + model.UserCountOptions{ + IncludeRemoteUsers: true, + IncludeDeleted: true, + TeamId: model.NewId(), + }, + 0, + }, + { + "Include remote accounts and deleted accounts with existing team id and view restrictions allowing team", + model.UserCountOptions{ + IncludeRemoteUsers: true, + IncludeDeleted: true, + TeamId: teamId, + ViewRestrictions: &model.ViewUsersRestrictions{Teams: []string{teamId}}, + }, + 4, + }, + { + "Include remote accounts and deleted accounts with existing team id and view restrictions not allowing current team", + model.UserCountOptions{ + IncludeRemoteUsers: true, + IncludeDeleted: true, + TeamId: teamId, + ViewRestrictions: &model.ViewUsersRestrictions{Teams: []string{model.NewId()}}, + }, + 0, + }, { "Filter by system admins only", model.UserCountOptions{ diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index 2fdaf460d1..bbb8c654c2 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -10032,6 +10032,22 @@ func (s *TimerLayerUserStore) GetByEmail(email string) (*model.User, error) { return result, err } +func (s *TimerLayerUserStore) GetByRemoteID(remoteID string) (*model.User, error) { + start := time.Now() + + result, err := s.UserStore.GetByRemoteID(remoteID) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("UserStore.GetByRemoteID", success, elapsed) + } + return result, err +} + func (s *TimerLayerUserStore) GetByUsername(username string) (*model.User, error) { start := time.Now() diff --git a/server/i18n/en.json b/server/i18n/en.json index 86f04cd228..8881c43a5e 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -4291,6 +4291,10 @@ "id": "api.user.login.not_verified.app_error", "translation": "Login failed because email address has not been verified." }, + { + "id": "api.user.login.remote_users.login.error", + "translation": "Login failed because remote users are not allow to log in." + }, { "id": "api.user.login.use_auth_service.app_error", "translation": "Please sign in using {{.AuthService}}." diff --git a/server/public/model/user_count.go b/server/public/model/user_count.go index ee474883a5..950a670044 100644 --- a/server/public/model/user_count.go +++ b/server/public/model/user_count.go @@ -9,6 +9,8 @@ type UserCountOptions struct { IncludeBotAccounts bool // Should include deleted users (of any type) IncludeDeleted bool + // Include remote users + IncludeRemoteUsers bool // Exclude regular users ExcludeRegularUsers bool // Only include users on a specific team. "" for any team. diff --git a/webapp/channels/src/components/user_list_row_with_error/user_list_row_with_error.tsx b/webapp/channels/src/components/user_list_row_with_error/user_list_row_with_error.tsx index 0469ea5ed4..90c4cb3655 100644 --- a/webapp/channels/src/components/user_list_row_with_error/user_list_row_with_error.tsx +++ b/webapp/channels/src/components/user_list_row_with_error/user_list_row_with_error.tsx @@ -2,11 +2,13 @@ // See LICENSE.txt for license information. import React from 'react'; +import {FormattedMessage} from 'react-intl'; import {Link} from 'react-router-dom'; import {ConnectedComponent} from 'react-redux'; import BotTag from 'components/widgets/tag/bot_tag'; +import Tag from 'components/widgets/tag/tag'; import {Client4} from 'mattermost-redux/client'; @@ -154,6 +156,16 @@ export default class UserListRowWithError extends React.PureComponent {this.props.user.is_bot && } + {this.props.user.remote_id && ( + + } + /> + )}