From a139c40d25715f109a3cbebd0b81b041084328b4 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Fri, 3 Jan 2025 15:15:05 -0300 Subject: [PATCH 01/13] Remove id and redirectUrl from SAML ServiceProviderOptions --- .../server/definition/IServiceProviderOptions.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/apps/meteor/app/meteor-accounts-saml/server/definition/IServiceProviderOptions.ts b/apps/meteor/app/meteor-accounts-saml/server/definition/IServiceProviderOptions.ts index f48c40432711..e90e36971bc0 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/definition/IServiceProviderOptions.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/definition/IServiceProviderOptions.ts @@ -20,8 +20,4 @@ export interface IServiceProviderOptions { metadataCertificateTemplate: string; metadataTemplate: string; callbackUrl: string; - - // The id and redirectUrl attributes are filled midway through some operations - id?: string; - redirectUrl?: string; } From ed67981a0323bac9a6c132ce3f4dcf15920deea8 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Fri, 3 Jan 2025 15:15:38 -0300 Subject: [PATCH 02/13] Provide id as a param when generating authorization request --- .../app/meteor-accounts-saml/server/lib/SAML.ts | 16 +++------------- .../server/lib/ServiceProvider.ts | 8 ++++---- .../app/meteor-accounts-saml/server/lib/Utils.ts | 5 ++--- .../server/lib/generators/AuthorizeRequest.ts | 13 ++++++++----- .../app/meteor-accounts-saml/server.tests.ts | 12 +++++++++--- 5 files changed, 26 insertions(+), 28 deletions(-) diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts index e3b12ca15aa7..2412feb41afe 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts @@ -54,7 +54,7 @@ export class SAML { case 'sloRedirect': return this.processSLORedirectAction(req, res); case 'authorize': - return this.processAuthorizeAction(req, res, service, samlObject); + return this.processAuthorizeAction(res, service, samlObject); case 'validate': return this.processValidateAction(req, res, service, samlObject); default: @@ -373,25 +373,15 @@ export class SAML { } private static async processAuthorizeAction( - req: IIncomingMessage, res: ServerResponse, service: IServiceProviderOptions, samlObject: ISAMLAction, ): Promise { - service.id = samlObject.credentialToken; - - // Allow redirecting to internal domains when login process is complete - const { referer } = req.headers; - const siteUrl = settings.get('Site_Url'); - if (typeof referer === 'string' && referer.startsWith(siteUrl)) { - service.redirectUrl = referer; - } - const serviceProvider = new SAMLServiceProvider(service); let url: string | undefined; try { - url = await serviceProvider.getAuthorizeUrl(); + url = await serviceProvider.getAuthorizeUrl(samlObject.credentialToken); } catch (err: any) { SAMLUtils.error('Unable to generate authorize url'); SAMLUtils.error(err); @@ -433,7 +423,7 @@ export class SAML { }; await this.storeCredential(credentialToken, loginResult); - const url = Meteor.absoluteUrl(SAMLUtils.getValidationActionRedirectPath(credentialToken, service.redirectUrl)); + const url = Meteor.absoluteUrl(SAMLUtils.getValidationActionRedirectPath(credentialToken)); res.writeHead(302, { Location: url, }); diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts index 0ffb5fe50fdf..c6fc42f4ded5 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts @@ -35,8 +35,8 @@ export class SAMLServiceProvider { return signer.sign(this.serviceProviderOptions.privateKey, 'base64'); } - public generateAuthorizeRequest(): string { - const identifiedRequest = AuthorizeRequest.generate(this.serviceProviderOptions); + public generateAuthorizeRequest(credentialToken: string): string { + const identifiedRequest = AuthorizeRequest.generate(this.serviceProviderOptions, credentialToken); return identifiedRequest.request; } @@ -151,8 +151,8 @@ export class SAMLServiceProvider { } } - public async getAuthorizeUrl(): Promise { - const request = this.generateAuthorizeRequest(); + public async getAuthorizeUrl(credentialToken: string): Promise { + const request = this.generateAuthorizeRequest(credentialToken); SAMLUtils.log('-----REQUEST------'); SAMLUtils.log(request); diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts index 5b58354bfca7..f3b875f41a03 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts @@ -131,10 +131,9 @@ export class SAMLUtils { return newTemplate; } - public static getValidationActionRedirectPath(credentialToken: string, redirectUrl?: string): string { - const redirectUrlParam = redirectUrl ? `&redirectUrl=${encodeURIComponent(redirectUrl)}` : ''; + public static getValidationActionRedirectPath(credentialToken: string): string { // the saml_idp_credentialToken param is needed by the mobile app - return `saml/${credentialToken}?saml_idp_credentialToken=${credentialToken}${redirectUrlParam}`; + return `saml/${credentialToken}?saml_idp_credentialToken=${credentialToken}`; } public static log(obj: any, ...args: Array): void { diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/generators/AuthorizeRequest.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/generators/AuthorizeRequest.ts index 9f264d7e9a05..44fed17c0056 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/generators/AuthorizeRequest.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/generators/AuthorizeRequest.ts @@ -14,8 +14,8 @@ import { An Authorize Request is used to show the Identity Provider login form when the user clicks on the Rocket.Chat SAML login button */ export class AuthorizeRequest { - public static generate(serviceProviderOptions: IServiceProviderOptions): ISAMLRequest { - const data = this.getDataForNewRequest(serviceProviderOptions); + public static generate(serviceProviderOptions: IServiceProviderOptions, credentialToken: string): ISAMLRequest { + const data = this.getDataForNewRequest(serviceProviderOptions, credentialToken); const request = SAMLUtils.fillTemplateData(this.authorizeRequestTemplate(serviceProviderOptions), data); return { @@ -53,13 +53,16 @@ export class AuthorizeRequest { return serviceProviderOptions.authnContextTemplate || defaultAuthnContextTemplate; } - private static getDataForNewRequest(serviceProviderOptions: IServiceProviderOptions): IAuthorizeRequestVariables { + private static getDataForNewRequest( + serviceProviderOptions: IServiceProviderOptions, + credentialToken?: string, + ): IAuthorizeRequestVariables { let id = `_${SAMLUtils.generateUniqueID()}`; const instant = SAMLUtils.generateInstant(); // Post-auth destination - if (serviceProviderOptions.id) { - id = serviceProviderOptions.id; + if (credentialToken) { + id = credentialToken; } return { diff --git a/apps/meteor/tests/unit/app/meteor-accounts-saml/server.tests.ts b/apps/meteor/tests/unit/app/meteor-accounts-saml/server.tests.ts index 2b79daca1f15..df741686feff 100644 --- a/apps/meteor/tests/unit/app/meteor-accounts-saml/server.tests.ts +++ b/apps/meteor/tests/unit/app/meteor-accounts-saml/server.tests.ts @@ -52,29 +52,35 @@ describe('SAML', () => { describe('[AuthorizeRequest]', () => { describe('AuthorizeRequest.generate', () => { it('should use the custom templates to generate the request', () => { - const authorizeRequest = AuthorizeRequest.generate(serviceProviderOptions); + const credentialToken = '__credentialToken__'; + const authorizeRequest = AuthorizeRequest.generate(serviceProviderOptions, credentialToken); + expect(authorizeRequest.id).to.be.equal(credentialToken); expect(authorizeRequest.request).to.be.equal( ' Password ', ); }); it('should include the unique ID on the request', () => { + const credentialToken = '__credentialToken__'; const customOptions = { ...serviceProviderOptions, authRequestTemplate: '__newId__', }; - const authorizeRequest = AuthorizeRequest.generate(customOptions); + const authorizeRequest = AuthorizeRequest.generate(customOptions, credentialToken); + expect(authorizeRequest.id).to.be.equal(credentialToken); expect(authorizeRequest.request).to.be.equal(authorizeRequest.id); }); it('should include the custom options on the request', () => { + const credentialToken = '__credentialToken__'; const customOptions = { ...serviceProviderOptions, authRequestTemplate: '__callbackUrl__ __entryPoint__ __issuer__', }; - const authorizeRequest = AuthorizeRequest.generate(customOptions); + const authorizeRequest = AuthorizeRequest.generate(customOptions, credentialToken); + expect(authorizeRequest.id).to.be.equal(credentialToken); expect(authorizeRequest.request).to.be.equal('[callback-url] [entry-point] [issuer]'); }); }); From 120578eea6d29b6b43194e5f4ac11f0d39ecc6ce Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Tue, 7 Jan 2025 17:03:50 -0300 Subject: [PATCH 03/13] handle SAML invite redirect in the client side --- .../meteor/client/views/invite/InvitePage.tsx | 5 ++- .../invite/hooks/useInviteTokenMutation.ts | 3 ++ .../client/views/root/SAMLLoginRoute.spec.tsx | 44 ++++++++----------- .../client/views/root/SAMLLoginRoute.tsx | 17 +++---- 4 files changed, 32 insertions(+), 37 deletions(-) diff --git a/apps/meteor/client/views/invite/InvitePage.tsx b/apps/meteor/client/views/invite/InvitePage.tsx index a969bf783e5e..6e9285a38e74 100644 --- a/apps/meteor/client/views/invite/InvitePage.tsx +++ b/apps/meteor/client/views/invite/InvitePage.tsx @@ -9,6 +9,8 @@ import PageLoading from '../root/PageLoading'; import { useInviteTokenMutation } from './hooks/useInviteTokenMutation'; import { useValidateInviteQuery } from './hooks/useValidateInviteQuery'; +const KEY = 'invite_token'; + const InvitePage = (): ReactElement => { const { t } = useTranslation(); @@ -28,7 +30,8 @@ const InvitePage = (): ReactElement => { return ; } - if (isValidInvite) { + if (isValidInvite && token) { + localStorage.setItem(KEY, token); return ; } diff --git a/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts b/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts index f6e0c58e7f10..ab7e319a5094 100644 --- a/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts +++ b/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts @@ -13,6 +13,8 @@ export const useInviteTokenMutation = () => { const { mutate } = useMutation({ mutationFn: (token: string) => getInviteRoom({ token }), onSuccess: (result) => { + localStorage.removeItem('invite_token'); + if (!result.room.name) { dispatchToastMessage({ type: 'error', message: t('Failed_to_activate_invite_token') }); router.navigate('/home'); @@ -27,6 +29,7 @@ export const useInviteTokenMutation = () => { router.navigate(`/channel/${result.room.name}`); }, onError: () => { + localStorage.removeItem('invite_token'); dispatchToastMessage({ type: 'error', message: t('Failed_to_activate_invite_token') }); router.navigate('/home'); }, diff --git a/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx b/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx index 61ef63cfc7ba..0ce99add1db5 100644 --- a/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx +++ b/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx @@ -6,18 +6,26 @@ import SAMLLoginRoute from './SAMLLoginRoute'; import RouterContextMock from '../../../tests/mocks/client/RouterContextMock'; const navigateStub = jest.fn(); +const getLocalStorageItemStub = jest.fn(); + +beforeAll(() => { + jest.spyOn(Storage.prototype, 'getItem'); + Storage.prototype.getItem = getLocalStorageItemStub; +}); beforeEach(() => { jest.clearAllMocks(); navigateStub.mockClear(); (Meteor.loginWithSamlToken as jest.Mock).mockClear(); + getLocalStorageItemStub.mockClear(); }); it('should redirect to /home', async () => { + getLocalStorageItemStub.mockReturnValue(undefined); render( - + @@ -25,27 +33,13 @@ it('should redirect to /home', async () => { { legacyRoot: true }, ); + expect(getLocalStorageItemStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenLastCalledWith(expect.objectContaining({ pathname: '/home' }), expect.anything()); }); -it('should redirect to /home when userId is not null', async () => { - render( - - - - - - - , - { legacyRoot: true }, - ); - - expect(navigateStub).toHaveBeenCalledTimes(1); - expect(navigateStub).toHaveBeenLastCalledWith(expect.objectContaining({ pathname: '/home' }), expect.anything()); -}); - -it('should redirect to /home when userId is null and redirectUrl is not within the workspace domain', async () => { +it('should redirect to /home when userId is null and the stored invite token is not valid', async () => { + getLocalStorageItemStub.mockReturnValue(null); render( @@ -59,16 +53,18 @@ it('should redirect to /home when userId is null and redirectUrl is not within t expect(navigateStub).toHaveBeenLastCalledWith(expect.objectContaining({ pathname: '/home' }), expect.anything()); }); -it('should redirect to the provided redirectUrl when userId is null and redirectUrl is within the workspace domain', async () => { +it('should redirect to the invite page with the stored invite token when it is valid', async () => { + getLocalStorageItemStub.mockReturnValue('test'); render( - + , { legacyRoot: true }, ); + expect(getLocalStorageItemStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenLastCalledWith(expect.objectContaining({ pathname: '/invite/test' }), expect.anything()); }); @@ -76,7 +72,7 @@ it('should redirect to the provided redirectUrl when userId is null and redirect it('should call loginWithSamlToken when component is mounted', async () => { render( - + , @@ -90,11 +86,7 @@ it('should call loginWithSamlToken when component is mounted', async () => { it('should call loginWithSamlToken with the token when it is present', async () => { render( - + , diff --git a/apps/meteor/client/views/root/SAMLLoginRoute.tsx b/apps/meteor/client/views/root/SAMLLoginRoute.tsx index 2e12b036a4b9..53ed2b921de7 100644 --- a/apps/meteor/client/views/root/SAMLLoginRoute.tsx +++ b/apps/meteor/client/views/root/SAMLLoginRoute.tsx @@ -1,29 +1,26 @@ -import type { LocationPathname } from '@rocket.chat/ui-contexts'; -import { useRouter, useToastMessageDispatch, useAbsoluteUrl } from '@rocket.chat/ui-contexts'; +import { useRouter, useToastMessageDispatch } from '@rocket.chat/ui-contexts'; import { Meteor } from 'meteor/meteor'; import { useEffect } from 'react'; +const KEY = 'invite_token'; + const SAMLLoginRoute = () => { - const rootUrl = useAbsoluteUrl()(''); const router = useRouter(); const dispatchToastMessage = useToastMessageDispatch(); + const inviteToken = localStorage.getItem(KEY); useEffect(() => { const { token } = router.getRouteParameters(); - const { redirectUrl } = router.getSearchParameters(); Meteor.loginWithSamlToken(token, (error?: unknown) => { if (error) { dispatchToastMessage({ type: 'error', message: error }); } - const decodedRedirectUrl = decodeURIComponent(redirectUrl || ''); - if (decodedRedirectUrl?.startsWith(rootUrl)) { - const redirect = new URL(decodedRedirectUrl); + if (inviteToken) { router.navigate( { - pathname: redirect.pathname as LocationPathname, - search: Object.fromEntries(redirect.searchParams.entries()), + pathname: `/invite/${inviteToken}`, }, { replace: true }, ); @@ -36,7 +33,7 @@ const SAMLLoginRoute = () => { ); } }); - }, [dispatchToastMessage, rootUrl, router]); + }, [dispatchToastMessage, inviteToken, router]); return null; }; From 7809b552b7dca572fbf156bce2112b2c4f8eb579 Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Tue, 7 Jan 2025 17:14:45 -0300 Subject: [PATCH 04/13] Create changeset --- .changeset/itchy-pumas-laugh.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/itchy-pumas-laugh.md diff --git a/.changeset/itchy-pumas-laugh.md b/.changeset/itchy-pumas-laugh.md new file mode 100644 index 000000000000..149083bde64c --- /dev/null +++ b/.changeset/itchy-pumas-laugh.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +Fixes SAML login redirecting to wrong room when using an invite link. From 29732fcadb29940247898f4215555b1118388177 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 8 Jan 2025 13:41:18 -0300 Subject: [PATCH 05/13] improve id assignment --- .../server/lib/generators/AuthorizeRequest.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/generators/AuthorizeRequest.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/generators/AuthorizeRequest.ts index 44fed17c0056..9971750e3df0 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/generators/AuthorizeRequest.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/generators/AuthorizeRequest.ts @@ -57,14 +57,9 @@ export class AuthorizeRequest { serviceProviderOptions: IServiceProviderOptions, credentialToken?: string, ): IAuthorizeRequestVariables { - let id = `_${SAMLUtils.generateUniqueID()}`; + const id = credentialToken || `_${SAMLUtils.generateUniqueID()}`; const instant = SAMLUtils.generateInstant(); - // Post-auth destination - if (credentialToken) { - id = credentialToken; - } - return { newId: id, instant, From 49eb1b358fdde288d84fd3f409b00a5681f9a63f Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 8 Jan 2025 14:17:40 -0300 Subject: [PATCH 06/13] Use sessionStorage instead of localStorage --- apps/meteor/client/views/invite/InvitePage.tsx | 2 +- .../views/invite/hooks/useInviteTokenMutation.ts | 4 ++-- .../client/views/root/SAMLLoginRoute.spec.tsx | 16 ++++++++-------- apps/meteor/client/views/root/SAMLLoginRoute.tsx | 2 +- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/apps/meteor/client/views/invite/InvitePage.tsx b/apps/meteor/client/views/invite/InvitePage.tsx index 6e9285a38e74..945db1d90569 100644 --- a/apps/meteor/client/views/invite/InvitePage.tsx +++ b/apps/meteor/client/views/invite/InvitePage.tsx @@ -31,7 +31,7 @@ const InvitePage = (): ReactElement => { } if (isValidInvite && token) { - localStorage.setItem(KEY, token); + sessionStorage.setItem(KEY, token); return ; } diff --git a/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts b/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts index ab7e319a5094..91d118d9eb78 100644 --- a/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts +++ b/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts @@ -13,7 +13,7 @@ export const useInviteTokenMutation = () => { const { mutate } = useMutation({ mutationFn: (token: string) => getInviteRoom({ token }), onSuccess: (result) => { - localStorage.removeItem('invite_token'); + sessionStorage.removeItem('invite_token'); if (!result.room.name) { dispatchToastMessage({ type: 'error', message: t('Failed_to_activate_invite_token') }); @@ -29,7 +29,7 @@ export const useInviteTokenMutation = () => { router.navigate(`/channel/${result.room.name}`); }, onError: () => { - localStorage.removeItem('invite_token'); + sessionStorage.removeItem('invite_token'); dispatchToastMessage({ type: 'error', message: t('Failed_to_activate_invite_token') }); router.navigate('/home'); }, diff --git a/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx b/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx index 0ce99add1db5..ba650c6f0394 100644 --- a/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx +++ b/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx @@ -6,22 +6,22 @@ import SAMLLoginRoute from './SAMLLoginRoute'; import RouterContextMock from '../../../tests/mocks/client/RouterContextMock'; const navigateStub = jest.fn(); -const getLocalStorageItemStub = jest.fn(); +const getSessionStorageItemStub = jest.fn(); beforeAll(() => { jest.spyOn(Storage.prototype, 'getItem'); - Storage.prototype.getItem = getLocalStorageItemStub; + Storage.prototype.getItem = getSessionStorageItemStub; }); beforeEach(() => { jest.clearAllMocks(); navigateStub.mockClear(); (Meteor.loginWithSamlToken as jest.Mock).mockClear(); - getLocalStorageItemStub.mockClear(); + getSessionStorageItemStub.mockClear(); }); it('should redirect to /home', async () => { - getLocalStorageItemStub.mockReturnValue(undefined); + getSessionStorageItemStub.mockReturnValue(undefined); render( @@ -33,13 +33,13 @@ it('should redirect to /home', async () => { { legacyRoot: true }, ); - expect(getLocalStorageItemStub).toHaveBeenCalledTimes(1); + expect(getSessionStorageItemStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenLastCalledWith(expect.objectContaining({ pathname: '/home' }), expect.anything()); }); it('should redirect to /home when userId is null and the stored invite token is not valid', async () => { - getLocalStorageItemStub.mockReturnValue(null); + getSessionStorageItemStub.mockReturnValue(null); render( @@ -54,7 +54,7 @@ it('should redirect to /home when userId is null and the stored invite token is }); it('should redirect to the invite page with the stored invite token when it is valid', async () => { - getLocalStorageItemStub.mockReturnValue('test'); + getSessionStorageItemStub.mockReturnValue('test'); render( @@ -64,7 +64,7 @@ it('should redirect to the invite page with the stored invite token when it is v { legacyRoot: true }, ); - expect(getLocalStorageItemStub).toHaveBeenCalledTimes(1); + expect(getSessionStorageItemStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenLastCalledWith(expect.objectContaining({ pathname: '/invite/test' }), expect.anything()); }); diff --git a/apps/meteor/client/views/root/SAMLLoginRoute.tsx b/apps/meteor/client/views/root/SAMLLoginRoute.tsx index 53ed2b921de7..b10732212ace 100644 --- a/apps/meteor/client/views/root/SAMLLoginRoute.tsx +++ b/apps/meteor/client/views/root/SAMLLoginRoute.tsx @@ -7,7 +7,7 @@ const KEY = 'invite_token'; const SAMLLoginRoute = () => { const router = useRouter(); const dispatchToastMessage = useToastMessageDispatch(); - const inviteToken = localStorage.getItem(KEY); + const inviteToken = sessionStorage.getItem(KEY); useEffect(() => { const { token } = router.getRouteParameters(); From 359acd4337324bc4f855c25011bd9696fa105aae Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 8 Jan 2025 14:21:01 -0300 Subject: [PATCH 07/13] remove token from storage on homepage (cover aborted login case) --- apps/meteor/client/views/home/HomePage.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/meteor/client/views/home/HomePage.tsx b/apps/meteor/client/views/home/HomePage.tsx index a8a0544e3d87..d3539d8f7d80 100644 --- a/apps/meteor/client/views/home/HomePage.tsx +++ b/apps/meteor/client/views/home/HomePage.tsx @@ -6,8 +6,11 @@ import CustomHomePage from './CustomHomePage'; import DefaultHomePage from './DefaultHomePage'; import { KonchatNotification } from '../../../app/ui/client/lib/KonchatNotification'; +const KEY = 'invite_token'; + const HomePage = (): ReactElement => { useEffect(() => { + sessionStorage.removeItem(KEY); KonchatNotification.getDesktopPermission(); }, []); From 4df8d6eb28d18569bd339d76bc4d7a1fdabcd32d Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 8 Jan 2025 14:32:21 -0300 Subject: [PATCH 08/13] Add e2e test to validate multiple parallel logins Co-authored-by: pierre-lehnen-rc --- apps/meteor/tests/e2e/saml.spec.ts | 31 ++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/apps/meteor/tests/e2e/saml.spec.ts b/apps/meteor/tests/e2e/saml.spec.ts index fe1295ca0b4b..234a60c7c177 100644 --- a/apps/meteor/tests/e2e/saml.spec.ts +++ b/apps/meteor/tests/e2e/saml.spec.ts @@ -409,6 +409,37 @@ test.describe('SAML', () => { }); }); + test('Respect redirectUrl on multiple parallel logins', async ({ page, browser }) => { + const page2 = await browser.newPage({ storageState: Users.samluser2.state }); + const poRegistration2 = new Registration(page2); + + await page2.goto(`/home`); + await expect(page2).toHaveURL('/home'); + + await page.goto(`/invite/${inviteId}`); + await page.getByRole('link', { name: 'Back to Login' }).click(); + + await expect(poRegistration.btnLoginWithSaml).toBeVisible(); + await poRegistration.btnLoginWithSaml.click(); + await expect(page).toHaveURL(/.*\/simplesaml\/module.php\/core\/loginuserpass.php.*/); + + await expect(page2.getByRole('button', { name: 'User menu' })).not.toBeVisible(); + await expect(poRegistration2.btnLoginWithSaml).toBeVisible(); + await poRegistration2.btnLoginWithSaml.click(); + await expect(page2).toHaveURL(/.*\/simplesaml\/module.php\/core\/loginuserpass.php.*/); + + await page.getByLabel('Username').fill('samluser1'); + await page.getByLabel('Password').fill('password'); + await page.locator('role=button[name="Login"]').click(); + + await page2.getByLabel('Username').fill('samluser2'); + await page2.getByLabel('Password').fill('password'); + await page2.locator('role=button[name="Login"]').click(); + + await expect(page).toHaveURL(`/group/${targetInviteGroupName}`); + await expect(page2).toHaveURL('/home'); + }); + test.fixme('User Merge - By Custom Identifier', async () => { // Test user merge with a custom identifier configured in the fieldmap }); From e0f45395989247c5426c49fe1d5c295d9aef9284 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Fri, 10 Jan 2025 11:10:35 -0300 Subject: [PATCH 09/13] logout with user 2 before trying to login again --- apps/meteor/tests/e2e/saml.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/meteor/tests/e2e/saml.spec.ts b/apps/meteor/tests/e2e/saml.spec.ts index 234a60c7c177..6c7d1e1f931f 100644 --- a/apps/meteor/tests/e2e/saml.spec.ts +++ b/apps/meteor/tests/e2e/saml.spec.ts @@ -407,6 +407,7 @@ test.describe('SAML', () => { await test.step('expect to be redirected to the homepage after succesful login', async () => { await expect(page).toHaveURL('/home'); }); + await doLogoutStep(page); }); test('Respect redirectUrl on multiple parallel logins', async ({ page, browser }) => { From b2e7d43d03698c0aa0e3837fa0d5e1b2cd2c5c36 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Fri, 10 Jan 2025 11:20:59 -0300 Subject: [PATCH 10/13] Do not use a storageState for new page in e2e test --- apps/meteor/tests/e2e/saml.spec.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/meteor/tests/e2e/saml.spec.ts b/apps/meteor/tests/e2e/saml.spec.ts index 6c7d1e1f931f..f752fe56a94e 100644 --- a/apps/meteor/tests/e2e/saml.spec.ts +++ b/apps/meteor/tests/e2e/saml.spec.ts @@ -407,11 +407,10 @@ test.describe('SAML', () => { await test.step('expect to be redirected to the homepage after succesful login', async () => { await expect(page).toHaveURL('/home'); }); - await doLogoutStep(page); }); test('Respect redirectUrl on multiple parallel logins', async ({ page, browser }) => { - const page2 = await browser.newPage({ storageState: Users.samluser2.state }); + const page2 = await browser.newPage(); const poRegistration2 = new Registration(page2); await page2.goto(`/home`); From 8529c082f13ef8c4d913a96b51baaa63dd1b8a24 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 15 Jan 2025 23:51:06 -0300 Subject: [PATCH 11/13] Apply FE requested changes --- .../providers/UserProvider/UserProvider.tsx | 12 ++++++++- .../meteor/client/views/invite/InvitePage.tsx | 10 ++++---- .../invite/hooks/useInviteTokenMutation.ts | 3 --- .../views/invite/hooks/useSamlInviteToken.ts | 9 +++++++ .../client/views/root/SAMLLoginRoute.spec.tsx | 14 +++++------ apps/meteor/tests/e2e/saml.spec.ts | 25 +++++++++++++++++++ 6 files changed, 56 insertions(+), 17 deletions(-) create mode 100644 apps/meteor/client/views/invite/hooks/useSamlInviteToken.ts diff --git a/apps/meteor/client/providers/UserProvider/UserProvider.tsx b/apps/meteor/client/providers/UserProvider/UserProvider.tsx index 75f83f4edff0..adbbc07fae54 100644 --- a/apps/meteor/client/providers/UserProvider/UserProvider.tsx +++ b/apps/meteor/client/providers/UserProvider/UserProvider.tsx @@ -1,7 +1,7 @@ import type { IRoom, ISubscription, IUser } from '@rocket.chat/core-typings'; import { useLocalStorage } from '@rocket.chat/fuselage-hooks'; import type { SubscriptionWithRoom } from '@rocket.chat/ui-contexts'; -import { UserContext, useEndpoint } from '@rocket.chat/ui-contexts'; +import { UserContext, useEndpoint, useRouteParameter, useSearchParameter } from '@rocket.chat/ui-contexts'; import { useQueryClient } from '@tanstack/react-query'; import { Meteor } from 'meteor/meteor'; import type { ContextType, ReactElement, ReactNode } from 'react'; @@ -18,6 +18,7 @@ import { afterLogoutCleanUpCallback } from '../../../lib/callbacks/afterLogoutCl import { useReactiveValue } from '../../hooks/useReactiveValue'; import { createReactiveSubscriptionFactory } from '../../lib/createReactiveSubscriptionFactory'; import { useCreateFontStyleElement } from '../../views/account/accessibility/hooks/useCreateFontStyleElement'; +import { useSamlInviteToken } from '../../views/invite/hooks/useSamlInviteToken'; const getUser = (): IUser | null => Meteor.user() as IUser | null; @@ -47,6 +48,9 @@ const UserProvider = ({ children }: UserProviderProps): ReactElement => { const previousUserId = useRef(userId); const [userLanguage, setUserLanguage] = useLocalStorage('userLanguage', ''); const [preferedLanguage, setPreferedLanguage] = useLocalStorage('preferedLanguage', ''); + const [, setSamlInviteToken] = useSamlInviteToken(); + const samlCredentialToken = useSearchParameter('saml_idp_credentialToken'); + const inviteTokenHash = useRouteParameter('hash'); const setUserPreferences = useEndpoint('POST', '/v1/users.setPreferences'); @@ -94,6 +98,12 @@ const UserProvider = ({ children }: UserProviderProps): ReactElement => { } }, [preferedLanguage, setPreferedLanguage, setUserLanguage, user?.language, userLanguage, userId, setUserPreferences]); + useEffect(() => { + if (!samlCredentialToken && !inviteTokenHash) { + setSamlInviteToken(null); + } + }, [inviteTokenHash, samlCredentialToken, setSamlInviteToken]); + const queryClient = useQueryClient(); useEffect(() => { diff --git a/apps/meteor/client/views/invite/InvitePage.tsx b/apps/meteor/client/views/invite/InvitePage.tsx index 945db1d90569..97689a832f89 100644 --- a/apps/meteor/client/views/invite/InvitePage.tsx +++ b/apps/meteor/client/views/invite/InvitePage.tsx @@ -7,31 +7,31 @@ import { useTranslation } from 'react-i18next'; import LoginPage from '../root/MainLayout/LoginPage'; import PageLoading from '../root/PageLoading'; import { useInviteTokenMutation } from './hooks/useInviteTokenMutation'; +import { useSamlInviteToken } from './hooks/useSamlInviteToken'; import { useValidateInviteQuery } from './hooks/useValidateInviteQuery'; -const KEY = 'invite_token'; - const InvitePage = (): ReactElement => { const { t } = useTranslation(); const token = useRouteParameter('hash'); const userId = useUserId(); const { isPending, data: isValidInvite } = useValidateInviteQuery(userId, token); + const [, setToken] = useSamlInviteToken(); const getInviteRoomMutation = useInviteTokenMutation(); useEffect(() => { + setToken(token || null); if (userId && token) { getInviteRoomMutation(token); } - }, [getInviteRoomMutation, token, userId]); + }, [getInviteRoomMutation, setToken, token, userId]); if (isPending) { return ; } - if (isValidInvite && token) { - sessionStorage.setItem(KEY, token); + if (isValidInvite) { return ; } diff --git a/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts b/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts index 91d118d9eb78..f6e0c58e7f10 100644 --- a/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts +++ b/apps/meteor/client/views/invite/hooks/useInviteTokenMutation.ts @@ -13,8 +13,6 @@ export const useInviteTokenMutation = () => { const { mutate } = useMutation({ mutationFn: (token: string) => getInviteRoom({ token }), onSuccess: (result) => { - sessionStorage.removeItem('invite_token'); - if (!result.room.name) { dispatchToastMessage({ type: 'error', message: t('Failed_to_activate_invite_token') }); router.navigate('/home'); @@ -29,7 +27,6 @@ export const useInviteTokenMutation = () => { router.navigate(`/channel/${result.room.name}`); }, onError: () => { - sessionStorage.removeItem('invite_token'); dispatchToastMessage({ type: 'error', message: t('Failed_to_activate_invite_token') }); router.navigate('/home'); }, diff --git a/apps/meteor/client/views/invite/hooks/useSamlInviteToken.ts b/apps/meteor/client/views/invite/hooks/useSamlInviteToken.ts new file mode 100644 index 000000000000..2998e21426b7 --- /dev/null +++ b/apps/meteor/client/views/invite/hooks/useSamlInviteToken.ts @@ -0,0 +1,9 @@ +import { useSessionStorage } from '@rocket.chat/fuselage-hooks'; +import { useRouteParameter } from '@rocket.chat/ui-contexts'; + +const KEY = 'saml_invite_token'; + +export const useSamlInviteToken = () => { + const token = useRouteParameter('hash'); + return useSessionStorage(KEY, token || null); +}; diff --git a/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx b/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx index ba650c6f0394..489d6acbae3e 100644 --- a/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx +++ b/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx @@ -4,24 +4,24 @@ import { Meteor } from 'meteor/meteor'; import SAMLLoginRoute from './SAMLLoginRoute'; import RouterContextMock from '../../../tests/mocks/client/RouterContextMock'; +import { useSamlInviteToken } from '../invite/hooks/useSamlInviteToken'; +jest.mock('../invite/hooks/useSamlInviteToken'); +const mockUseSamlInviteToken = jest.mocked(useSamlInviteToken); const navigateStub = jest.fn(); -const getSessionStorageItemStub = jest.fn(); beforeAll(() => { jest.spyOn(Storage.prototype, 'getItem'); - Storage.prototype.getItem = getSessionStorageItemStub; }); beforeEach(() => { jest.clearAllMocks(); navigateStub.mockClear(); (Meteor.loginWithSamlToken as jest.Mock).mockClear(); - getSessionStorageItemStub.mockClear(); }); it('should redirect to /home', async () => { - getSessionStorageItemStub.mockReturnValue(undefined); + mockUseSamlInviteToken.mockReturnValue([null, () => ({})]); render( @@ -33,13 +33,12 @@ it('should redirect to /home', async () => { { legacyRoot: true }, ); - expect(getSessionStorageItemStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenLastCalledWith(expect.objectContaining({ pathname: '/home' }), expect.anything()); }); it('should redirect to /home when userId is null and the stored invite token is not valid', async () => { - getSessionStorageItemStub.mockReturnValue(null); + mockUseSamlInviteToken.mockReturnValue([null, () => ({})]); render( @@ -54,7 +53,7 @@ it('should redirect to /home when userId is null and the stored invite token is }); it('should redirect to the invite page with the stored invite token when it is valid', async () => { - getSessionStorageItemStub.mockReturnValue('test'); + mockUseSamlInviteToken.mockReturnValue(['test', () => ({})]); render( @@ -64,7 +63,6 @@ it('should redirect to the invite page with the stored invite token when it is v { legacyRoot: true }, ); - expect(getSessionStorageItemStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenCalledTimes(1); expect(navigateStub).toHaveBeenLastCalledWith(expect.objectContaining({ pathname: '/invite/test' }), expect.anything()); }); diff --git a/apps/meteor/tests/e2e/saml.spec.ts b/apps/meteor/tests/e2e/saml.spec.ts index f752fe56a94e..d7e0904fc2cf 100644 --- a/apps/meteor/tests/e2e/saml.spec.ts +++ b/apps/meteor/tests/e2e/saml.spec.ts @@ -18,6 +18,8 @@ import { setSettingValueById } from './utils/setSettingValueById'; import type { BaseTest } from './utils/test'; import { test, expect } from './utils/test'; +const KEY = 'fuselage-sessionStorage-saml_invite_token'; + const resetTestData = async ({ api, cleanupOnly = false }: { api?: any; cleanupOnly?: boolean } = {}) => { // Reset saml users' data on mongo in the beforeAll hook to allow re-running the tests within the same playwright session // This is needed because those tests will modify this data and running them a second time would trigger different code paths @@ -394,13 +396,28 @@ test.describe('SAML', () => { await page.goto(`/invite/${inviteId}`); await page.getByRole('link', { name: 'Back to Login' }).click(); + expect(await page.evaluate((key) => sessionStorage.getItem(key), KEY)).toEqual(JSON.stringify(inviteId)); + await doLoginStep(page, 'samluser1', null); await test.step('expect to be redirected to the invited room after succesful login', async () => { await expect(page).toHaveURL(`/group/${targetInviteGroupName}`); + expect(await page.evaluate((key) => sessionStorage.getItem(key), KEY)).toEqual('null'); }); }); + test('Remove invite token from session storage if invite is not used', async ({ page }) => { + await page.goto(`/invite/${inviteId}`); + await page.getByRole('link', { name: 'Back to Login' }).click(); + + expect(await page.evaluate((key) => sessionStorage.getItem(key), KEY)).toEqual(JSON.stringify(inviteId)); + + await page.goto(`/home`); + await doLoginStep(page, 'samluser2'); + + expect(await page.evaluate((key) => sessionStorage.getItem(key), KEY)).toEqual('null'); + }); + test('Redirect to home after login when no redirectUrl is provided', async ({ page }) => { await doLoginStep(page, 'samluser2'); @@ -419,6 +436,9 @@ test.describe('SAML', () => { await page.goto(`/invite/${inviteId}`); await page.getByRole('link', { name: 'Back to Login' }).click(); + expect(await page.evaluate((key) => sessionStorage.getItem(key), KEY)).toEqual(JSON.stringify(inviteId)); + expect(await page2.evaluate((key) => sessionStorage.getItem(key), KEY)).toEqual('null'); + await expect(poRegistration.btnLoginWithSaml).toBeVisible(); await poRegistration.btnLoginWithSaml.click(); await expect(page).toHaveURL(/.*\/simplesaml\/module.php\/core\/loginuserpass.php.*/); @@ -438,6 +458,11 @@ test.describe('SAML', () => { await expect(page).toHaveURL(`/group/${targetInviteGroupName}`); await expect(page2).toHaveURL('/home'); + + expect(await page.evaluate((key) => sessionStorage.getItem(key), KEY)).toEqual('null'); + expect(await page2.evaluate((key) => sessionStorage.getItem(key), KEY)).toEqual('null'); + + await page2.close(); }); test.fixme('User Merge - By Custom Identifier', async () => { From e0186c97a8b26082225ad7e3311e50aab5a9e6c6 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 15 Jan 2025 23:51:52 -0300 Subject: [PATCH 12/13] use new hook in SAMLLoginRoute --- apps/meteor/client/views/root/SAMLLoginRoute.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/meteor/client/views/root/SAMLLoginRoute.tsx b/apps/meteor/client/views/root/SAMLLoginRoute.tsx index b10732212ace..35e3c26db504 100644 --- a/apps/meteor/client/views/root/SAMLLoginRoute.tsx +++ b/apps/meteor/client/views/root/SAMLLoginRoute.tsx @@ -2,12 +2,12 @@ import { useRouter, useToastMessageDispatch } from '@rocket.chat/ui-contexts'; import { Meteor } from 'meteor/meteor'; import { useEffect } from 'react'; -const KEY = 'invite_token'; +import { useSamlInviteToken } from '../invite/hooks/useSamlInviteToken'; const SAMLLoginRoute = () => { const router = useRouter(); const dispatchToastMessage = useToastMessageDispatch(); - const inviteToken = sessionStorage.getItem(KEY); + const [inviteToken] = useSamlInviteToken(); useEffect(() => { const { token } = router.getRouteParameters(); From 8c733875b9621bbdd117aef8ed1ac60cbc87094c Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Thu, 16 Jan 2025 13:52:21 -0300 Subject: [PATCH 13/13] Remove missing direct sessionStorage usage --- apps/meteor/client/views/home/HomePage.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/apps/meteor/client/views/home/HomePage.tsx b/apps/meteor/client/views/home/HomePage.tsx index d3539d8f7d80..a8a0544e3d87 100644 --- a/apps/meteor/client/views/home/HomePage.tsx +++ b/apps/meteor/client/views/home/HomePage.tsx @@ -6,11 +6,8 @@ import CustomHomePage from './CustomHomePage'; import DefaultHomePage from './DefaultHomePage'; import { KonchatNotification } from '../../../app/ui/client/lib/KonchatNotification'; -const KEY = 'invite_token'; - const HomePage = (): ReactElement => { useEffect(() => { - sessionStorage.removeItem(KEY); KonchatNotification.getDesktopPermission(); }, []);