From 968931de1e7559ba1a624669b132c0cd4d55d7ec Mon Sep 17 00:00:00 2001 From: Yassine Bounekhla Date: Fri, 20 Jun 2025 09:52:32 -0400 Subject: [PATCH 1/2] fix users not being redirected to login on session expiry --- .github/ISSUE_TEMPLATE/webtestplan.md | 2 + .../services/websession/websession.test.ts | 42 +++++++++++++++++++ .../src/services/websession/websession.ts | 23 ++++++---- 3 files changed, 59 insertions(+), 8 deletions(-) create mode 100644 web/packages/teleport/src/services/websession/websession.test.ts diff --git a/.github/ISSUE_TEMPLATE/webtestplan.md b/.github/ISSUE_TEMPLATE/webtestplan.md index 3a3c94dc8bf34..4937c61de1c59 100644 --- a/.github/ISSUE_TEMPLATE/webtestplan.md +++ b/.github/ISSUE_TEMPLATE/webtestplan.md @@ -132,6 +132,8 @@ For each, test the invite, reset, and login flows - [ ] Verify that error message is shown if an invite/reset is expired/invalid - [ ] Verify that account is locked after several unsuccessful login attempts +- [ ] Verify that after logging in, user is automatically redirected to the login page when the session expires. + #### Auth Connectors For help with setting up auth connectors, check out the [Quick GitHub/SAML/OIDC Setup Tips] diff --git a/web/packages/teleport/src/services/websession/websession.test.ts b/web/packages/teleport/src/services/websession/websession.test.ts new file mode 100644 index 0000000000000..bec420eef267b --- /dev/null +++ b/web/packages/teleport/src/services/websession/websession.test.ts @@ -0,0 +1,42 @@ +/** + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import history from 'teleport/services/history'; + +import websession from '.'; +import api from '../api'; + +test('user should be redirected to login even if session delete call fails', async () => { + const mockPromise = { + then: jest.fn().mockReturnThis(), + catch: jest.fn().mockReturnThis(), + finally: jest.fn().mockImplementation(callback => { + callback(); + return mockPromise; + }), + [Symbol.toStringTag]: 'Promise', + }; + + jest.spyOn(api, 'delete').mockReturnValue(mockPromise as any); + const goToLoginSpy = jest.spyOn(history, 'goToLogin').mockImplementation(); + jest.spyOn(websession, 'clear').mockImplementation(); + + websession.logout(); + + expect(goToLoginSpy).toHaveBeenCalled(); +}); diff --git a/web/packages/teleport/src/services/websession/websession.ts b/web/packages/teleport/src/services/websession/websession.ts index bea98b37ed9f2..df57c017f7395 100644 --- a/web/packages/teleport/src/services/websession/websession.ts +++ b/web/packages/teleport/src/services/websession/websession.ts @@ -36,14 +36,21 @@ let sesstionCheckerTimerId = null; const session = { logout(rememberLocation = false) { - api.delete(cfg.api.webSessionPath).then(response => { - this.clear(); - if (response.samlSloUrl) { - window.open(response.samlSloUrl, '_self'); - } else { - history.goToLogin({ rememberLocation }); - } - }); + let samlSloUrl = ''; + + api + .delete(cfg.api.webSessionPath) + .then(response => { + samlSloUrl = response.samlSloUrl; + }) + .finally(() => { + this.clear(); + if (samlSloUrl) { + window.open(samlSloUrl, '_self'); + } else { + history.goToLogin({ rememberLocation }); + } + }); }, logoutWithoutSlo({ From 91077cbeecfab2da7b46d9efde8e8c2a46ebe18c Mon Sep 17 00:00:00 2001 From: Yassine Bounekhla Date: Thu, 26 Jun 2025 15:26:06 -0400 Subject: [PATCH 2/2] CR --- .../src/services/websession/websession.test.ts | 17 +++++------------ .../src/services/websession/websession.ts | 9 ++++++++- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/web/packages/teleport/src/services/websession/websession.test.ts b/web/packages/teleport/src/services/websession/websession.test.ts index bec420eef267b..9a13dadc6d515 100644 --- a/web/packages/teleport/src/services/websession/websession.test.ts +++ b/web/packages/teleport/src/services/websession/websession.test.ts @@ -16,27 +16,20 @@ * along with this program. If not, see . */ +import { waitFor } from '@testing-library/react'; + import history from 'teleport/services/history'; import websession from '.'; import api from '../api'; test('user should be redirected to login even if session delete call fails', async () => { - const mockPromise = { - then: jest.fn().mockReturnThis(), - catch: jest.fn().mockReturnThis(), - finally: jest.fn().mockImplementation(callback => { - callback(); - return mockPromise; - }), - [Symbol.toStringTag]: 'Promise', - }; - - jest.spyOn(api, 'delete').mockReturnValue(mockPromise as any); + jest.spyOn(console, 'error').mockImplementation(); + jest.spyOn(api, 'delete').mockRejectedValue(new Error('some error')); const goToLoginSpy = jest.spyOn(history, 'goToLogin').mockImplementation(); jest.spyOn(websession, 'clear').mockImplementation(); websession.logout(); - expect(goToLoginSpy).toHaveBeenCalled(); + await waitFor(() => expect(goToLoginSpy).toHaveBeenCalled()); }); diff --git a/web/packages/teleport/src/services/websession/websession.ts b/web/packages/teleport/src/services/websession/websession.ts index df57c017f7395..c4b3bb0fb975b 100644 --- a/web/packages/teleport/src/services/websession/websession.ts +++ b/web/packages/teleport/src/services/websession/websession.ts @@ -41,7 +41,14 @@ const session = { api .delete(cfg.api.webSessionPath) .then(response => { - samlSloUrl = response.samlSloUrl; + samlSloUrl = response?.samlSloUrl; + }) + .catch(err => { + // This request can fail if the session is already expired, which isn't an issue, but we should still catch the error. + logger.error( + 'Failed to delete session. This can happen if the session has already expired.', + err + ); }) .finally(() => { this.clear();