From 1bbc3b4e83d8c3e20672bf8d962fb2980365f26d Mon Sep 17 00:00:00 2001 From: Daniel Schalla Date: Tue, 18 Jun 2024 21:13:29 +0200 Subject: [PATCH] [MM-58560] Configurable session revocation during password resets (#27286) * [MM-58560] Allow for configurable session revocation during password reset * Missing i18n additions * Update Settings Wording * Update Settings Wording #2 * Update default_config.ts for Session Termination --------- Co-authored-by: Mattermost Build --- .../support/server/default_config.ts | 1 + server/channels/app/user.go | 25 +++ server/channels/app/user_test.go | 147 ++++++++++++++++++ .../platform/services/telemetry/telemetry.go | 1 + server/public/model/config.go | 6 + .../admin_console/session_length_settings.tsx | 14 ++ webapp/channels/src/i18n/en.json | 2 + webapp/platform/types/src/config.ts | 1 + 8 files changed, 197 insertions(+) diff --git a/e2e-tests/playwright/support/server/default_config.ts b/e2e-tests/playwright/support/server/default_config.ts index c4f8543de6..7509d6a72c 100644 --- a/e2e-tests/playwright/support/server/default_config.ts +++ b/e2e-tests/playwright/support/server/default_config.ts @@ -133,6 +133,7 @@ const defaultServerConfig: AdminConfig = { CorsDebug: false, AllowCookiesForSubdomains: false, ExtendSessionLengthWithActivity: true, + TerminateSessionsOnPasswordChange: false, SessionLengthWebInDays: 30, SessionLengthWebInHours: 720, SessionLengthMobileInDays: 30, diff --git a/server/channels/app/user.go b/server/channels/app/user.go index db246748d6..7e6ee51e3f 100644 --- a/server/channels/app/user.go +++ b/server/channels/app/user.go @@ -1453,6 +1453,31 @@ func (a *App) UpdatePassword(rctx request.CTX, user *model.User, newPassword str a.InvalidateCacheForUser(user.Id) + if *a.Config().ServiceSettings.TerminateSessionsOnPasswordChange { + // Get currently active sessions if request is user-initiated to retain it + currentSession := "" + if rctx.Session() != nil && rctx.Session().UserId == user.Id { + currentSession = rctx.Session().Id + } + + sessions, err := a.GetSessions(rctx, user.Id) + if err != nil { + return model.NewAppError("UpdatePassword", "api.user.update_password.failed.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + + // Revoke all but current session + for _, session := range sessions { + if session.Id == currentSession { + continue + } + + err := a.RevokeSessionById(rctx, session.Id) + if err != nil { + return model.NewAppError("UpdatePassword", "api.user.update_password.failed.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + } + } + return nil } diff --git a/server/channels/app/user_test.go b/server/channels/app/user_test.go index 3a62336045..8f8a26c9ab 100644 --- a/server/channels/app/user_test.go +++ b/server/channels/app/user_test.go @@ -1210,6 +1210,153 @@ func TestInvalidatePasswordRecoveryTokens(t *testing.T) { }) } +func TestPasswordChangeSessionTermination(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + t.Run("user-initiated password change with termination enabled", func(t *testing.T) { + th.App.UpdateConfig(func(c *model.Config) { + *c.ServiceSettings.TerminateSessionsOnPasswordChange = true + }) + + session, err := th.App.CreateSession(th.Context, &model.Session{ + UserId: th.BasicUser2.Id, + Roles: model.SystemUserRoleId, + }) + require.Nil(t, err) + + session2, err := th.App.CreateSession(th.Context, &model.Session{ + UserId: th.BasicUser2.Id, + Roles: model.SystemUserRoleId, + }) + require.Nil(t, err) + + th.Context.Session().UserId = th.BasicUser2.Id + th.Context.Session().Id = session.Id + + err = th.App.UpdatePassword(th.Context, th.BasicUser2, "Password2") + require.Nil(t, err) + + session, err = th.App.GetSession(session.Token) + require.Nil(t, err) + require.False(t, session.IsExpired()) + + session2, err = th.App.GetSession(session2.Token) + require.Error(t, err) + require.Nil(t, session2) + + // Cleanup + err = th.App.UpdatePassword(th.Context, th.BasicUser2, "Password1") + require.Nil(t, err) + th.Context.Session().UserId = "" + th.Context.Session().Id = "" + }) + + t.Run("user-initiated password change with termination disabled", func(t *testing.T) { + th.App.UpdateConfig(func(c *model.Config) { + *c.ServiceSettings.TerminateSessionsOnPasswordChange = false + }) + + session, err := th.App.CreateSession(th.Context, &model.Session{ + UserId: th.BasicUser2.Id, + Roles: model.SystemUserRoleId, + }) + require.Nil(t, err) + + session2, err := th.App.CreateSession(th.Context, &model.Session{ + UserId: th.BasicUser2.Id, + Roles: model.SystemUserRoleId, + }) + require.Nil(t, err) + + th.Context.Session().UserId = th.BasicUser2.Id + th.Context.Session().Id = session.Id + + err = th.App.UpdatePassword(th.Context, th.BasicUser2, "Password2") + require.Nil(t, err) + + session, err = th.App.GetSession(session.Token) + require.Nil(t, err) + require.False(t, session.IsExpired()) + + session2, err = th.App.GetSession(session2.Token) + require.Nil(t, err) + require.False(t, session2.IsExpired()) + + // Cleanup + err = th.App.UpdatePassword(th.Context, th.BasicUser2, "Password1") + require.Nil(t, err) + th.Context.Session().UserId = "" + th.Context.Session().Id = "" + }) + + t.Run("admin-initiated password change with termination enabled", func(t *testing.T) { + th.App.UpdateConfig(func(c *model.Config) { + *c.ServiceSettings.TerminateSessionsOnPasswordChange = true + }) + + session, err := th.App.CreateSession(th.Context, &model.Session{ + UserId: th.BasicUser2.Id, + Roles: model.SystemUserRoleId, + }) + require.Nil(t, err) + + session2, err := th.App.CreateSession(th.Context, &model.Session{ + UserId: th.BasicUser2.Id, + Roles: model.SystemUserRoleId, + }) + require.Nil(t, err) + + err = th.App.UpdatePassword(th.Context, th.BasicUser2, "Password2") + require.Nil(t, err) + + session, err = th.App.GetSession(session.Token) + require.Error(t, err) + require.Nil(t, session) + + session2, err = th.App.GetSession(session2.Token) + require.Error(t, err) + require.Nil(t, session2) + + // Cleanup + err = th.App.UpdatePassword(th.Context, th.BasicUser2, "Password1") + require.Nil(t, err) + }) + + t.Run("admin-initiated password change with termination disabled", func(t *testing.T) { + th.App.UpdateConfig(func(c *model.Config) { + *c.ServiceSettings.TerminateSessionsOnPasswordChange = false + }) + + session, err := th.App.CreateSession(th.Context, &model.Session{ + UserId: th.BasicUser2.Id, + Roles: model.SystemUserRoleId, + }) + require.Nil(t, err) + + session2, err := th.App.CreateSession(th.Context, &model.Session{ + UserId: th.BasicUser2.Id, + Roles: model.SystemUserRoleId, + }) + require.Nil(t, err) + + err = th.App.UpdatePassword(th.Context, th.BasicUser2, "Password2") + require.Nil(t, err) + + session, err = th.App.GetSession(session.Token) + require.Nil(t, err) + require.False(t, session.IsExpired()) + + session2, err = th.App.GetSession(session2.Token) + require.Nil(t, err) + require.False(t, session2.IsExpired()) + + // Cleanup + err = th.App.UpdatePassword(th.Context, th.BasicUser2, "Password1") + require.Nil(t, err) + }) +} + func TestGetViewUsersRestrictions(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/server/platform/services/telemetry/telemetry.go b/server/platform/services/telemetry/telemetry.go index d9778eb868..424a57f3cd 100644 --- a/server/platform/services/telemetry/telemetry.go +++ b/server/platform/services/telemetry/telemetry.go @@ -430,6 +430,7 @@ func (ts *TelemetryService) trackConfig() { "forward_80_to_443": *cfg.ServiceSettings.Forward80To443, "maximum_login_attempts": *cfg.ServiceSettings.MaximumLoginAttempts, "extend_session_length_with_activity": *cfg.ServiceSettings.ExtendSessionLengthWithActivity, + "terminate_sessions_on_password_change": *cfg.ServiceSettings.TerminateSessionsOnPasswordChange, "session_length_web_in_hours": *cfg.ServiceSettings.SessionLengthWebInHours, "session_length_mobile_in_hours": *cfg.ServiceSettings.SessionLengthMobileInHours, "session_length_sso_in_hours": *cfg.ServiceSettings.SessionLengthSSOInHours, diff --git a/server/public/model/config.go b/server/public/model/config.go index 1d96227e45..f88913cd72 100644 --- a/server/public/model/config.go +++ b/server/public/model/config.go @@ -341,6 +341,7 @@ type ServiceSettings struct { CorsDebug *bool `access:"integrations_cors,write_restrictable,cloud_restrictable"` AllowCookiesForSubdomains *bool `access:"write_restrictable,cloud_restrictable"` ExtendSessionLengthWithActivity *bool `access:"environment_session_lengths,write_restrictable,cloud_restrictable"` + TerminateSessionsOnPasswordChange *bool `access:"environment_session_lengths,write_restrictable,cloud_restrictable"` // Deprecated SessionLengthWebInDays *int `access:"environment_session_lengths,write_restrictable,cloud_restrictable"` // telemetry: none @@ -631,6 +632,11 @@ func (s *ServiceSettings) SetDefaults(isUpdate bool) { s.ExtendSessionLengthWithActivity = NewBool(!isUpdate) } + // Must be manually enabled for existing installations. + if s.TerminateSessionsOnPasswordChange == nil { + s.TerminateSessionsOnPasswordChange = NewBool(!isUpdate) + } + if s.SessionLengthWebInDays == nil { if isUpdate { s.SessionLengthWebInDays = NewInt(180) diff --git a/webapp/channels/src/components/admin_console/session_length_settings.tsx b/webapp/channels/src/components/admin_console/session_length_settings.tsx index 27f2171ce2..f5e87c84c5 100644 --- a/webapp/channels/src/components/admin_console/session_length_settings.tsx +++ b/webapp/channels/src/components/admin_console/session_length_settings.tsx @@ -16,6 +16,7 @@ import TextSetting from './text_setting'; interface State extends BaseState { extendSessionLengthWithActivity: ServiceSettings['ExtendSessionLengthWithActivity']; + terminateSessionsOnPasswordChange: ServiceSettings['TerminateSessionsOnPasswordChange']; sessionLengthWebInHours: ServiceSettings['SessionLengthWebInHours']; sessionLengthMobileInHours: ServiceSettings['SessionLengthMobileInHours']; sessionLengthSSOInHours: ServiceSettings['SessionLengthSSOInHours']; @@ -39,6 +40,8 @@ const messages = defineMessages({ sessionIdleTimeout: {id: 'admin.service.sessionIdleTimeout', defaultMessage: 'Session Idle Timeout (minutes):'}, extendSessionLengthActivity_label: {id: 'admin.service.extendSessionLengthActivity.label', defaultMessage: 'Extend session length with activity: '}, extendSessionLengthActivity_helpText: {id: 'admin.service.extendSessionLengthActivity.helpText', defaultMessage: 'When true, sessions will be automatically extended when the user is active in their Mattermost client. Users sessions will only expire if they are not active in their Mattermost client for the entire duration of the session lengths defined in the fields below. When false, sessions will not extend with activity in Mattermost. User sessions will immediately expire at the end of the session length or idle timeouts defined below. '}, + terminateSessionsOnPasswordChange_label: {id: 'admin.service.terminateSessionsOnPasswordChange.label', defaultMessage: 'Terminate Sessions on Password Change: '}, + terminateSessionsOnPasswordChange_helpText: {id: 'admin.service.terminateSessionsOnPasswordChange.helpText', defaultMessage: 'When true, all sessions of a user will expire if their password is changed by themselves or an administrator.'}, webSessionHours: {id: 'admin.service.webSessionHours', defaultMessage: 'Session Length AD/LDAP and Email (hours):'}, mobileSessionHours: {id: 'admin.service.mobileSessionHours', defaultMessage: 'Session Length Mobile (hours):'}, ssoSessionHours: {id: 'admin.service.ssoSessionHours', defaultMessage: 'Session Length SSO (hours):'}, @@ -73,6 +76,7 @@ export default class SessionLengthSettings extends AdminSettings { const MINIMUM_IDLE_TIMEOUT = 5; config.ServiceSettings.ExtendSessionLengthWithActivity = this.state.extendSessionLengthWithActivity; + config.ServiceSettings.TerminateSessionsOnPasswordChange = this.state.terminateSessionsOnPasswordChange; config.ServiceSettings.SessionLengthWebInHours = this.parseIntNonZero(this.state.sessionLengthWebInHours); config.ServiceSettings.SessionLengthMobileInHours = this.parseIntNonZero(this.state.sessionLengthMobileInHours); config.ServiceSettings.SessionLengthSSOInHours = this.parseIntNonZero(this.state.sessionLengthSSOInHours); @@ -85,6 +89,7 @@ export default class SessionLengthSettings extends AdminSettings { getStateFromConfig(config: AdminConfig) { return { extendSessionLengthWithActivity: config.ServiceSettings.ExtendSessionLengthWithActivity, + terminateSessionsOnPasswordChange: config.ServiceSettings.TerminateSessionsOnPasswordChange, sessionLengthWebInHours: config.ServiceSettings.SessionLengthWebInHours, sessionLengthMobileInHours: config.ServiceSettings.SessionLengthMobileInHours, sessionLengthSSOInHours: config.ServiceSettings.SessionLengthSSOInHours, @@ -140,6 +145,15 @@ export default class SessionLengthSettings extends AdminSettings { setByEnv={this.isSetByEnv('ServiceSettings.ExtendSessionLengthWithActivity')} disabled={this.props.isDisabled} /> + } + helpText={} + value={this.state.terminateSessionsOnPasswordChange} + onChange={this.handleChange} + setByEnv={this.isSetByEnv('ServiceSettings.TerminateSessionsOnPasswordChange')} + disabled={this.props.isDisabled} + /> } diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index ca9ec0cc57..e880e8f3a9 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -2285,6 +2285,8 @@ "admin.service.ssoSessionHours": "Session Length SSO (hours):", "admin.service.ssoSessionHoursDesc": "The number of hours from the last time a user entered their credentials to the expiry of the user's session. If the authentication method is SAML or GitLab, the user may automatically be logged back in to Mattermost if they are already logged in to SAML or GitLab. After changing this setting, the setting will take effect after the next time the user enters their credentials.", "admin.service.ssoSessionHoursDesc.extendLength": "Set the number of hours from the last activity in Mattermost to the expiry of the user’s session for SSO authentication, such as SAML, GitLab and OAuth 2.0. If the authentication method is SAML or GitLab, the user may automatically be logged back in to Mattermost if they are already logged in to SAML or GitLab. After changing this setting, the setting will take effect after the next time the user enters their credentials.", + "admin.service.terminateSessionsOnPasswordChange.helpText": "When true, all sessions of a user will expire if their password is changed by themselves or an administrator. If password change is initiated by user, their current session is not terminated", + "admin.service.terminateSessionsOnPasswordChange.label": "Terminate Sessions on Password Change: ", "admin.service.testingDescription": "When true, /test slash command is enabled to load test accounts, data and text formatting. Changing this requires a server restart before taking effect.", "admin.service.testingTitle": "Enable Testing Commands: ", "admin.service.testSiteURL": "Test Live URL", diff --git a/webapp/platform/types/src/config.ts b/webapp/platform/types/src/config.ts index 509422c7e4..487a5fbcd2 100644 --- a/webapp/platform/types/src/config.ts +++ b/webapp/platform/types/src/config.ts @@ -331,6 +331,7 @@ export type ServiceSettings = { CorsDebug: boolean; AllowCookiesForSubdomains: boolean; ExtendSessionLengthWithActivity: boolean; + TerminateSessionsOnPasswordChange: boolean; SessionLengthWebInDays: number; SessionLengthWebInHours: number; SessionLengthMobileInDays: number;