From 3bd75733f175c084df28de02a2ccec20a31cd1ed Mon Sep 17 00:00:00 2001 From: "Adolfo R. Brandes" Date: Sat, 29 Aug 2026 12:35:47 -0300 Subject: [PATCH] fix: resolve header auth links from route roles The header's Login and Register buttons linked straight to the LMS using loginUrl and lmsBaseUrl, so an installed authentication app was bypassed even though it registers the login and register route roles. They now resolve those roles first, keeping the previous site config values as fallbacks, and navigate in the client when the resolved URL is a route in this site. Fixes #305 Co-Authored-By: Claude --- shell/constants.ts | 2 + .../anonymous-menu/LoginButton.test.tsx | 61 +++++++++++++++++++ shell/header/anonymous-menu/LoginButton.tsx | 10 ++- .../anonymous-menu/RegisterButton.test.tsx | 53 ++++++++++++++++ .../header/anonymous-menu/RegisterButton.tsx | 10 ++- shell/header/anonymous-menu/utils.ts | 19 ++++++ 6 files changed, 151 insertions(+), 4 deletions(-) create mode 100644 shell/header/anonymous-menu/LoginButton.test.tsx create mode 100644 shell/header/anonymous-menu/RegisterButton.test.tsx create mode 100644 shell/header/anonymous-menu/utils.ts diff --git a/shell/constants.ts b/shell/constants.ts index 516cce56..1e5b79bd 100644 --- a/shell/constants.ts +++ b/shell/constants.ts @@ -1,2 +1,4 @@ export const homeRole = 'org.openedx.frontend.role.home'; export const providesChromelessRolesId = 'org.openedx.frontend.provides.chromelessRoles.v1'; +export const loginRole = 'org.openedx.frontend.role.login'; +export const registerRole = 'org.openedx.frontend.role.register'; diff --git a/shell/header/anonymous-menu/LoginButton.test.tsx b/shell/header/anonymous-menu/LoginButton.test.tsx new file mode 100644 index 00000000..e59cadc6 --- /dev/null +++ b/shell/header/anonymous-menu/LoginButton.test.tsx @@ -0,0 +1,61 @@ +import '@testing-library/jest-dom'; +import { render, screen } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +import { getUrlByRouteRole, useSiteConfig } from '../../../runtime'; +import { IntlProvider } from '../../../runtime/i18n'; +import LoginButton from './LoginButton'; + +jest.mock('../../../runtime', () => ({ + ...jest.requireActual('../../../runtime'), + getUrlByRouteRole: jest.fn(), + useSiteConfig: jest.fn(), +})); + +const mockGetUrlByRouteRole = getUrlByRouteRole as jest.MockedFunction; +const mockUseSiteConfig = useSiteConfig as jest.MockedFunction; + +function renderLoginButton() { + return render( + + + + + + ); +} + +describe('LoginButton', () => { + beforeEach(() => { + mockUseSiteConfig.mockReturnValue({ loginUrl: 'http://localhost:18000/login' } as any); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('links to the route provided by an authentication app', () => { + mockGetUrlByRouteRole.mockReturnValue('/authn/login'); + + renderLoginButton(); + + expect(mockGetUrlByRouteRole).toHaveBeenCalledWith('org.openedx.frontend.role.login'); + expect(screen.getByRole('link', { name: 'Login' })).toHaveAttribute('href', '/authn/login'); + }); + + it('links to an external login route when one is configured', () => { + mockGetUrlByRouteRole.mockReturnValue('https://auth.example.com/login'); + + renderLoginButton(); + + expect(screen.getByRole('link', { name: 'Login' })).toHaveAttribute('href', 'https://auth.example.com/login'); + }); + + it('falls back to loginUrl when no app provides the login role', () => { + mockGetUrlByRouteRole.mockReturnValue(null); + + renderLoginButton(); + + expect(screen.getByRole('link', { name: 'Login' })).toHaveAttribute('href', 'http://localhost:18000/login'); + }); +}); diff --git a/shell/header/anonymous-menu/LoginButton.tsx b/shell/header/anonymous-menu/LoginButton.tsx index da319f11..5000762b 100644 --- a/shell/header/anonymous-menu/LoginButton.tsx +++ b/shell/header/anonymous-menu/LoginButton.tsx @@ -1,13 +1,19 @@ import { Button } from '@openedx/paragon'; -import { useSiteConfig, useIntl } from '../../../runtime'; +import { getUrlByRouteRole, useSiteConfig, useIntl } from '../../../runtime'; +import { loginRole } from '../../constants'; import messages from '../../Shell.messages'; +import { getLinkProps } from './utils'; export default function LoginButton({ ...props }) { const config = useSiteConfig(); const intl = useIntl(); + // Prefer the route provided by an installed authentication app, falling back + // to the login service configured for the site. + const url = getUrlByRouteRole(loginRole) ?? config.loginUrl; + return ( - ); diff --git a/shell/header/anonymous-menu/RegisterButton.test.tsx b/shell/header/anonymous-menu/RegisterButton.test.tsx new file mode 100644 index 00000000..d52189a8 --- /dev/null +++ b/shell/header/anonymous-menu/RegisterButton.test.tsx @@ -0,0 +1,53 @@ +import '@testing-library/jest-dom'; +import { render, screen } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +import { getUrlByRouteRole, useSiteConfig } from '../../../runtime'; +import { IntlProvider } from '../../../runtime/i18n'; +import RegisterButton from './RegisterButton'; + +jest.mock('../../../runtime', () => ({ + ...jest.requireActual('../../../runtime'), + getUrlByRouteRole: jest.fn(), + useSiteConfig: jest.fn(), +})); + +const mockGetUrlByRouteRole = getUrlByRouteRole as jest.MockedFunction; +const mockUseSiteConfig = useSiteConfig as jest.MockedFunction; + +function renderRegisterButton() { + return render( + + + + + + ); +} + +describe('RegisterButton', () => { + beforeEach(() => { + mockUseSiteConfig.mockReturnValue({ lmsBaseUrl: 'http://localhost:18000' } as any); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('links to the route provided by an authentication app', () => { + mockGetUrlByRouteRole.mockReturnValue('/authn/register'); + + renderRegisterButton(); + + expect(mockGetUrlByRouteRole).toHaveBeenCalledWith('org.openedx.frontend.role.register'); + expect(screen.getByRole('link', { name: 'Sign Up' })).toHaveAttribute('href', '/authn/register'); + }); + + it('falls back to the LMS registration page when no app provides the register role', () => { + mockGetUrlByRouteRole.mockReturnValue(null); + + renderRegisterButton(); + + expect(screen.getByRole('link', { name: 'Sign Up' })).toHaveAttribute('href', 'http://localhost:18000/register'); + }); +}); diff --git a/shell/header/anonymous-menu/RegisterButton.tsx b/shell/header/anonymous-menu/RegisterButton.tsx index a836c9f7..7d1aad17 100644 --- a/shell/header/anonymous-menu/RegisterButton.tsx +++ b/shell/header/anonymous-menu/RegisterButton.tsx @@ -1,14 +1,20 @@ import { Button } from '@openedx/paragon'; -import { useSiteConfig, useIntl } from '../../../runtime'; +import { getUrlByRouteRole, useSiteConfig, useIntl } from '../../../runtime'; +import { registerRole } from '../../constants'; import messages from '../../Shell.messages'; +import { getLinkProps } from './utils'; export default function RegisterButton({ ...props }) { const config = useSiteConfig(); const intl = useIntl(); + // Prefer the route provided by an installed authentication app, falling back + // to the registration page served by the LMS. + const url = getUrlByRouteRole(registerRole) ?? `${config.lmsBaseUrl}/register`; + return ( - ); diff --git a/shell/header/anonymous-menu/utils.ts b/shell/header/anonymous-menu/utils.ts new file mode 100644 index 00000000..202a0b9e --- /dev/null +++ b/shell/header/anonymous-menu/utils.ts @@ -0,0 +1,19 @@ +import { ElementType } from 'react'; +import { Link } from 'react-router-dom'; + +interface LinkProps { + as?: ElementType; + to?: string; + href?: string; +} + +/** + * Builds the props needed to link to a URL, keeping navigation inside the + * client when the URL is a route in this site rather than an external one. + */ +export function getLinkProps(url: string): LinkProps { + if (url.startsWith('/')) { + return { as: Link, to: url }; + } + return { href: url }; +}