Merge branch 'develop' into refactor/room-menu

RocketChat · Jan 17, 2025 · 79ac2f3 · 79ac2f3
2 parents 73f25fd + f777d9e
commit 79ac2f3
Show file tree

Hide file tree

Showing 13 changed files with 133 additions and 74 deletions.
diff --git a/.changeset/itchy-pumas-laugh.md 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
@@ -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
@@ -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<void> {
-		service.id = samlObject.credentialToken;
-
-		// Allow redirecting to internal domains when login process is complete
-		const { referer } = req.headers;
-		const siteUrl = settings.get<string>('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
@@ -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<string | undefined> {
-		const request = this.generateAuthorizeRequest();
+	public async getAuthorizeUrl(credentialToken: string): Promise<string | undefined> {
+		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
@@ -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<any>): 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
@@ -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
@@ -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
@@ -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 <PageLoading />;

diff --git a/apps/meteor/client/views/invite/hooks/useSamlInviteToken.ts 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
@@ -4,20 +4,28 @@ 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();
 	(Meteor.loginWithSamlToken as jest.Mock<any>).mockClear();
 });
 
 it('should redirect to /home', async () => {
+	mockUseSamlInviteToken.mockReturnValue([null, () => ({})]);
 	render(
 		<MockedServerContext>
 			<MockedUserContext>
-				<RouterContextMock searchParameters={{ redirectUrl: 'http://rocket.chat' }} navigate={navigateStub}>
+				<RouterContextMock navigate={navigateStub}>
 					<SAMLLoginRoute />
 				</RouterContextMock>
 			</MockedUserContext>
@@ -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(
-		<MockedServerContext>
-			<MockedUserContext>
-				<RouterContextMock searchParameters={{ redirectUrl: 'http://rocket.chat' }} navigate={navigateStub}>
-					<SAMLLoginRoute />
-				</RouterContextMock>
-			</MockedUserContext>
-		</MockedServerContext>,
-		{ 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(
 		<MockedServerContext>
 			<RouterContextMock searchParameters={{ redirectUrl: 'http://rocket.chat' }} navigate={navigateStub}>
@@ -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(
 		<MockedServerContext>
-			<RouterContextMock searchParameters={{ redirectUrl: 'http://localhost:3000/invite/test' }} navigate={navigateStub}>
+			<RouterContextMock navigate={navigateStub}>
 				<SAMLLoginRoute />
 			</RouterContextMock>
 		</MockedServerContext>,
@@ -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(
 		<MockedServerContext>
-			<RouterContextMock searchParameters={{ redirectUrl: 'http://rocket.chat' }} navigate={navigateStub}>
+			<RouterContextMock navigate={navigateStub}>
 				<SAMLLoginRoute />
 			</RouterContextMock>
 		</MockedServerContext>,
@@ -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(
 		<MockedUserContext>
-			<RouterContextMock
-				searchParameters={{ redirectUrl: 'http://rocket.chat' }}
-				routeParameters={{ token: 'testToken' }}
-				navigate={navigateStub}
-			>
+			<RouterContextMock routeParameters={{ token: 'testToken' }} navigate={navigateStub}>
 				<SAMLLoginRoute />
 			</RouterContextMock>
 		</MockedUserContext>,

diff --git 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;
 };