diff --git a/.changeset/eleven-news-stare.md b/.changeset/eleven-news-stare.md new file mode 100644 index 00000000000..8bf62b7aeaf --- /dev/null +++ b/.changeset/eleven-news-stare.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +Fixed issue with login via SAML not redirecting to invite link diff --git a/apps/meteor/.meteorMocks/index.ts b/apps/meteor/.meteorMocks/index.ts new file mode 100644 index 00000000000..e70ffa7f7c4 --- /dev/null +++ b/apps/meteor/.meteorMocks/index.ts @@ -0,0 +1,5 @@ +import sinon from 'sinon'; + +export const Meteor = { + loginWithSamlToken: sinon.stub(), +}; 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 acc67681fcd..f48c4043271 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/definition/IServiceProviderOptions.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/definition/IServiceProviderOptions.ts @@ -21,6 +21,7 @@ export interface IServiceProviderOptions { metadataTemplate: string; callbackUrl: string; - // The id attribute is filled midway through some operations + // 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 f62ab71f230..76747b59910 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(res, service, samlObject); + return this.processAuthorizeAction(req, res, service, samlObject); case 'validate': return this.processValidateAction(req, res, service, samlObject); default: @@ -378,12 +378,20 @@ 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; @@ -430,7 +438,7 @@ export class SAML { }; await this.storeCredential(credentialToken, loginResult); - const url = Meteor.absoluteUrl(SAMLUtils.getValidationActionRedirectPath(credentialToken)); + const url = Meteor.absoluteUrl(SAMLUtils.getValidationActionRedirectPath(credentialToken, service.redirectUrl)); res.writeHead(302, { Location: url, }); 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 70df22120b7..984c7bc458a 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts @@ -131,9 +131,10 @@ export class SAMLUtils { return newTemplate; } - public static getValidationActionRedirectPath(credentialToken: string): string { + public static getValidationActionRedirectPath(credentialToken: string, redirectUrl?: string): string { + const redirectUrlParam = redirectUrl ? `&redirectUrl=${encodeURIComponent(redirectUrl)}` : ''; // the saml_idp_credentialToken param is needed by the mobile app - return `saml/${credentialToken}?saml_idp_credentialToken=${credentialToken}`; + return `saml/${credentialToken}?saml_idp_credentialToken=${credentialToken}${redirectUrlParam}`; } public static log(obj: any, ...args: Array): void { diff --git a/apps/meteor/client/views/root/SAMLLoginRoute.tsx b/apps/meteor/client/views/root/SAMLLoginRoute.tsx index 5bf205c1fa2..61ceb82a5ee 100644 --- a/apps/meteor/client/views/root/SAMLLoginRoute.tsx +++ b/apps/meteor/client/views/root/SAMLLoginRoute.tsx @@ -1,19 +1,42 @@ -import { useRouter, useToastMessageDispatch, useUserId } from '@rocket.chat/ui-contexts'; +import type { LocationPathname } from '@rocket.chat/ui-contexts'; +import { useRouter, useToastMessageDispatch, useUserId, useAbsoluteUrl } from '@rocket.chat/ui-contexts'; import { Meteor } from 'meteor/meteor'; import { useEffect } from 'react'; const SAMLLoginRoute = () => { + const rootUrl = useAbsoluteUrl()(''); const router = useRouter(); const dispatchToastMessage = useToastMessageDispatch(); 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); + router.navigate( + { + pathname: redirect.pathname as LocationPathname, + search: Object.fromEntries(redirect.searchParams.entries()), + }, + { replace: true }, + ); + } else { + router.navigate( + { + pathname: '/home', + }, + { replace: true }, + ); + } }); - }, [dispatchToastMessage, router]); + }, [dispatchToastMessage, rootUrl, router]); const userId = useUserId(); useEffect(() => { diff --git a/apps/meteor/jest.config.ts b/apps/meteor/jest.config.ts index 81938441d72..aaa76299d26 100644 --- a/apps/meteor/jest.config.ts +++ b/apps/meteor/jest.config.ts @@ -22,6 +22,7 @@ const config: Config = { '\\.css$': 'identity-obj-proxy', '^react($|/.+)': '/node_modules/react$1', '^@tanstack/(.+)': '/node_modules/@tanstack/$1', + '^meteor/(.*)': '/.meteorMocks/index.ts', }, }, { diff --git a/apps/meteor/tests/e2e/saml.spec.ts b/apps/meteor/tests/e2e/saml.spec.ts index a85060a59ea..c50abf19bbc 100644 --- a/apps/meteor/tests/e2e/saml.spec.ts +++ b/apps/meteor/tests/e2e/saml.spec.ts @@ -1,6 +1,7 @@ import child_process from 'child_process'; import path from 'path'; +import { faker } from '@faker-js/faker'; import { Page } from '@playwright/test'; import { v2 as compose } from 'docker-compose' import { MongoClient } from 'mongodb'; @@ -48,6 +49,10 @@ const resetTestData = async (cleanupOnly = false) => { await Promise.all( [ + { + _id: 'Accounts_AllowAnonymousRead', + value: false, + }, { _id: 'SAML_Custom_Default_logout_behaviour', value: 'SAML', @@ -93,6 +98,9 @@ test.describe('SAML', () => { let poRegistration: Registration; let samlRoleId: string; + let targetInviteGroupId: string; + let targetInviteGroupName: string; + let inviteId: string; const containerPath = path.join(__dirname, 'containers', 'saml'); @@ -116,6 +124,19 @@ test.describe('SAML', () => { }); }); + test.beforeAll(async ({ api }) => { + const groupResponse = await api.post('/groups.create', { name: faker.string.uuid() }); + expect(groupResponse.status()).toBe(200); + const { group } = await groupResponse.json(); + targetInviteGroupId = group._id; + targetInviteGroupName = group.name; + + const inviteResponse = await api.post('/findOrCreateInvite', { rid: targetInviteGroupId, days: 1, maxUses: 0 }); + expect(inviteResponse.status()).toBe(200); + const { _id } = await inviteResponse.json(); + inviteId = _id; + }); + test.afterAll(async ({ api }) => { await compose.down({ cwd: containerPath, @@ -139,6 +160,10 @@ test.describe('SAML', () => { } }); + test.afterAll(async ({ api }) => { + expect((await api.post('/groups.delete', { roomId: targetInviteGroupId })).status()).toBe(200); + }); + test.beforeEach(async ({ page }) => { poRegistration = new Registration(page); @@ -175,7 +200,7 @@ test.describe('SAML', () => { }); }); - const doLoginStep = async (page: Page, username: string) => { + const doLoginStep = async (page: Page, username: string, redirectUrl = '/home') => { await test.step('expect successful login', async () => { await poRegistration.btnLoginWithSaml.click(); // Redirect to Idp @@ -187,7 +212,7 @@ test.describe('SAML', () => { await page.locator('role=button[name="Login"]').click(); // Redirect back to rocket.chat - await expect(page).toHaveURL('/home'); + await expect(page).toHaveURL(redirectUrl); await expect(page.getByRole('button', { name: 'User menu' })).toBeVisible(); }); @@ -314,6 +339,26 @@ test.describe('SAML', () => { }); }); + test('Redirect to a specific group after login when using a valid invite link', async ({ page }) => { + await page.goto(`/invite/${inviteId}`); + await page.getByRole('link', { name: 'Back to Login' }).click(); + + await doLoginStep(page, 'samluser1', `${constants.BASE_URL}/invite/${inviteId}`); + + await test.step('expect to be redirected to the invited room after succesful login', async () => { + await expect(page).toHaveURL(`/group/${targetInviteGroupName}`); + }); + }); + + test('Redirect to home after login when no redirectUrl is provided', async ({ page }) => { + await doLoginStep(page, 'samluser2'); + + await test.step('expect to be redirected to the homepage after succesful login', async () => { + await expect(page).toHaveURL('/home'); + }); + }); + + 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/mocks/client/RouterContextMock.tsx b/apps/meteor/tests/mocks/client/RouterContextMock.tsx index 61cbf5f1090..2831fe226ad 100644 --- a/apps/meteor/tests/mocks/client/RouterContextMock.tsx +++ b/apps/meteor/tests/mocks/client/RouterContextMock.tsx @@ -59,9 +59,17 @@ type RouterContextMockProps = { children?: ReactNode; navigate?: (toOrDelta: number | To) => void; currentPath?: MutableRefObject; + searchParameters?: Record; + routeParameters?: Record; }; -const RouterContextMock = ({ children, navigate, currentPath }: RouterContextMockProps): ReactElement => { +const RouterContextMock = ({ + children, + navigate, + currentPath, + searchParameters = {}, + routeParameters = {}, +}: RouterContextMockProps): ReactElement => { const history = useRef<{ stack: To[]; index: number }>({ stack: ['/'], index: 0 }); if (currentPath) { @@ -75,8 +83,8 @@ const RouterContextMock = ({ children, navigate, currentPath }: RouterContextMoc subscribeToRouteChange: () => () => undefined, getLocationPathname: () => '/', getLocationSearch: () => '', - getRouteParameters: () => ({}), - getSearchParameters: () => ({}), + getRouteParameters: () => routeParameters, + getSearchParameters: () => searchParameters, getRouteName: () => 'home', buildRoutePath, navigate: diff --git a/apps/meteor/tests/unit/client/views/root/SAMLLoginRoute.spec.tsx b/apps/meteor/tests/unit/client/views/root/SAMLLoginRoute.spec.tsx new file mode 100644 index 00000000000..bfd0d9ec0a6 --- /dev/null +++ b/apps/meteor/tests/unit/client/views/root/SAMLLoginRoute.spec.tsx @@ -0,0 +1,106 @@ +import { MockedServerContext, MockedUserContext } from '@rocket.chat/mock-providers'; +import { render } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { Meteor } from 'meteor/meteor'; +import React from 'react'; +import sinon from 'sinon'; + +import SAMLLoginRoute from '../../../../../client/views/root/SAMLLoginRoute'; +import RouterContextMock from '../../../../mocks/client/RouterContextMock'; + +const loginWithSamlTokenStub = Meteor.loginWithSamlToken as sinon.SinonStub; +const navigateStub = sinon.stub(); + +describe('views/root/SAMLLoginRoute', () => { + beforeEach(() => { + jest.clearAllMocks(); + navigateStub.resetHistory(); + loginWithSamlTokenStub.reset(); + loginWithSamlTokenStub.callsFake((_token, callback) => callback()); + }); + + it('should redirect to /home when userId is not null', async () => { + render( + + + + + + + , + ); + + expect(navigateStub.calledTwice).toBe(true); + expect( + navigateStub.calledWith( + sinon.match({ + pathname: '/home', + }), + ), + ).toBe(true); + }); + + it('should redirect to /home when userId is null and redirectUrl is not within the workspace domain', async () => { + render( + + + + + , + ); + + expect( + navigateStub.calledOnceWith( + sinon.match({ + pathname: '/home', + }), + ), + ).toBe(true); + }); + + it('should redirect to the provided redirectUrl when userId is null and redirectUrl is within the workspace domain', async () => { + render( + + + + + , + ); + + expect( + navigateStub.calledOnceWith( + sinon.match({ + pathname: '/invite/test', + }), + ), + ).toBe(true); + }); + + it('should call loginWithSamlToken when component is mounted', async () => { + render( + + + + + , + ); + + expect(loginWithSamlTokenStub.calledOnceWith(undefined)).toBe(true); + }); + + it('should call loginWithSamlToken with the token when it is present', async () => { + render( + + + + + , + ); + + expect(loginWithSamlTokenStub.calledOnceWith('testToken')).toBe(true); + }); +});