From f777d9eed959d42d99b645ae8e12a19e966b5256 Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Thu, 16 Jan 2025 17:46:32 -0300 Subject: [PATCH] fix: SAML login redirects to incorrect room when using an invite (#34873) --- .changeset/itchy-pumas-laugh.md | 5 ++ .../definition/IServiceProviderOptions.ts | 4 -- .../meteor-accounts-saml/server/lib/SAML.ts | 16 +----- .../server/lib/ServiceProvider.ts | 8 +-- .../meteor-accounts-saml/server/lib/Utils.ts | 5 +- .../server/lib/generators/AuthorizeRequest.ts | 16 +++--- .../providers/UserProvider/UserProvider.tsx | 12 +++- .../meteor/client/views/invite/InvitePage.tsx | 5 +- .../views/invite/hooks/useSamlInviteToken.ts | 9 +++ .../client/views/root/SAMLLoginRoute.spec.tsx | 42 ++++++-------- .../client/views/root/SAMLLoginRoute.tsx | 17 +++--- apps/meteor/tests/e2e/saml.spec.ts | 56 +++++++++++++++++++ .../app/meteor-accounts-saml/server.tests.ts | 12 +++- 13 files changed, 133 insertions(+), 74 deletions(-) create mode 100644 .changeset/itchy-pumas-laugh.md create mode 100644 apps/meteor/client/views/invite/hooks/useSamlInviteToken.ts 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. 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; } 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..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 @@ -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,15 +53,13 @@ export class AuthorizeRequest { return serviceProviderOptions.authnContextTemplate || defaultAuthnContextTemplate; } - private static getDataForNewRequest(serviceProviderOptions: IServiceProviderOptions): IAuthorizeRequestVariables { - let id = `_${SAMLUtils.generateUniqueID()}`; + private static getDataForNewRequest( + serviceProviderOptions: IServiceProviderOptions, + credentialToken?: string, + ): IAuthorizeRequestVariables { + const id = credentialToken || `_${SAMLUtils.generateUniqueID()}`; const instant = SAMLUtils.generateInstant(); - // Post-auth destination - if (serviceProviderOptions.id) { - id = serviceProviderOptions.id; - } - return { newId: id, instant, 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 a969bf783e5e..97689a832f89 100644 --- a/apps/meteor/client/views/invite/InvitePage.tsx +++ b/apps/meteor/client/views/invite/InvitePage.tsx @@ -7,6 +7,7 @@ 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 InvitePage = (): ReactElement => { @@ -15,14 +16,16 @@ const InvitePage = (): ReactElement => { 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 ; 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 61ef63cfc7ba..489d6acbae3e 100644 --- a/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx +++ b/apps/meteor/client/views/root/SAMLLoginRoute.spec.tsx @@ -4,9 +4,16 @@ 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(); +beforeAll(() => { + jest.spyOn(Storage.prototype, 'getItem'); +}); + beforeEach(() => { jest.clearAllMocks(); navigateStub.mockClear(); @@ -14,10 +21,11 @@ beforeEach(() => { }); it('should redirect to /home', async () => { + mockUseSamlInviteToken.mockReturnValue([null, () => ({})]); render( - + @@ -29,23 +37,8 @@ it('should redirect to /home', async () => { 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 () => { + mockUseSamlInviteToken.mockReturnValue([null, () => ({})]); render( @@ -59,10 +52,11 @@ 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 () => { + mockUseSamlInviteToken.mockReturnValue(['test', () => ({})]); render( - + , @@ -76,7 +70,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 +84,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..35e3c26db504 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'; +import { useSamlInviteToken } from '../invite/hooks/useSamlInviteToken'; + const SAMLLoginRoute = () => { - const rootUrl = useAbsoluteUrl()(''); const router = useRouter(); const dispatchToastMessage = useToastMessageDispatch(); + const [inviteToken] = useSamlInviteToken(); 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; }; diff --git a/apps/meteor/tests/e2e/saml.spec.ts b/apps/meteor/tests/e2e/saml.spec.ts index fe1295ca0b4b..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'); @@ -409,6 +426,45 @@ test.describe('SAML', () => { }); }); + test('Respect redirectUrl on multiple parallel logins', async ({ page, browser }) => { + const page2 = await browser.newPage(); + 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(); + + 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.*/); + + 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'); + + 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 () => { // Test user merge with a custom identifier configured in the fieldmap }); 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]'); }); });