From d787c1694d3ac1b4cfa49ffa6579aa53500333a4 Mon Sep 17 00:00:00 2001 From: Stanislav Mishchyshyn Date: Fri, 20 Dec 2024 16:18:38 +0200 Subject: [PATCH] feat: support http redirect code in transition hook plugin rename guard to transtionhookexecutor --- ilc/client/Client.js | 8 +- ...dManager.js => TransitionHooksExecutor.js} | 11 ++- ...pec.js => TransitionHooksExecutor.spec.js} | 30 ++++---- ilc/common/guard/errors.js | 11 --- ilc/common/guard/errors.ts | 8 ++ ilc/package-lock.json | 8 +- ilc/package.json | 2 +- ilc/server/GuardManager.js | 59 --------------- ...pec.js => TransitionHooksExecutor.spec.js} | 75 ++++++++++++++++--- ilc/server/TransitionHooksExecutor.ts | 65 ++++++++++++++++ .../routes/wildcardRequestHandlerFactory.ts | 9 ++- 11 files changed, 174 insertions(+), 112 deletions(-) rename ilc/client/{GuardManager.js => TransitionHooksExecutor.js} (90%) rename ilc/client/{GuardManager.spec.js => TransitionHooksExecutor.spec.js} (85%) delete mode 100644 ilc/common/guard/errors.js create mode 100644 ilc/common/guard/errors.ts delete mode 100644 ilc/server/GuardManager.js rename ilc/server/{GuardManager.spec.js => TransitionHooksExecutor.spec.js} (71%) create mode 100644 ilc/server/TransitionHooksExecutor.ts diff --git a/ilc/client/Client.js b/ilc/client/Client.js index 709b23486..d6d648951 100644 --- a/ilc/client/Client.js +++ b/ilc/client/Client.js @@ -26,7 +26,7 @@ import Router from './ClientRouter'; import initIlcState from './initIlcState'; import I18n from './i18n'; -import GuardManager from './GuardManager'; +import TransitionHooksExecutor from './TransitionHooksExecutor'; import ParcelApi from './ParcelApi'; import { BundleLoader } from './BundleLoader'; import { registerApplications } from './registerSpaApps'; @@ -62,7 +62,7 @@ export class Client { #router; - #guardManager; + #transitionHooksExecutor; #urlProcessor; @@ -114,7 +114,7 @@ export class Client { singleSpa, this.#transitionManager.handlePageTransition.bind(this.#transitionManager), ); - this.#guardManager = new GuardManager( + this.#transitionHooksExecutor = new TransitionHooksExecutor( this.#router, this.#pluginManager, this.#onCriticalInternalError.bind(this), @@ -268,7 +268,7 @@ export class Client { } #configure() { - addNavigationHook((url) => (this.#guardManager.hasAccessTo(url) ? url : null)); + addNavigationHook((url) => (this.#transitionHooksExecutor.hasAccessTo(url) ? url : null)); addNavigationHook((url) => this.#urlProcessor.process(url)); // TODO: window.ILC.importLibrary - calls bootstrap function with props (if supported), and returns exposed API diff --git a/ilc/client/GuardManager.js b/ilc/client/TransitionHooksExecutor.js similarity index 90% rename from ilc/client/GuardManager.js rename to ilc/client/TransitionHooksExecutor.js index 5b875cb77..49fc177c2 100644 --- a/ilc/client/GuardManager.js +++ b/ilc/client/TransitionHooksExecutor.js @@ -1,7 +1,10 @@ -import errors from '../common/guard/errors'; +import { TransitionHookError } from '../common/guard/errors'; import actionTypes from '../common/guard/actionTypes'; -export default class GuardManager { +/** + * Executes ILC Transition plugin's hooks + */ +export default class TransitionHooksExecutor { #router; #transitionHooksPlugin; #errorHandler; @@ -58,7 +61,7 @@ export default class GuardManager { this.#logger.info( `ILC: Redirect from "${route.reqUrl}" to "${ action.newLocation - }" due to the Route Guard with index #${hooks.indexOf(hook)}`, + }" due to the Transition Hook with index #${hooks.indexOf(hook)}`, ); this.#router.navigateToUrl(action.newLocation); }); @@ -68,7 +71,7 @@ export default class GuardManager { const hookIndex = hooks.indexOf(hook); this.#errorHandler( - new errors.GuardTransitionHookError({ + new TransitionHookError({ message: `An error has occurred while executing "${hookIndex}" transition hook for the following URL: "${url}".`, data: { hookIndex, diff --git a/ilc/client/GuardManager.spec.js b/ilc/client/TransitionHooksExecutor.spec.js similarity index 85% rename from ilc/client/GuardManager.spec.js rename to ilc/client/TransitionHooksExecutor.spec.js index 8e16dd6db..1c594f4ea 100644 --- a/ilc/client/GuardManager.spec.js +++ b/ilc/client/TransitionHooksExecutor.spec.js @@ -1,11 +1,11 @@ import chai from 'chai'; import sinon from 'sinon'; -import errors from '../common/guard/errors'; +import { TransitionHookError } from '../common/guard/errors'; import actionTypes from '../common/guard/actionTypes'; -import GuardManager from './GuardManager'; +import TransitionHooksExecutor from './TransitionHooksExecutor'; -describe('GuardManager', () => { +describe('TransitionHooksExecutor', () => { let clock; const route = Object.freeze({ @@ -68,9 +68,9 @@ describe('GuardManager', () => { transitionHooksPlugin.getTransitionHooks.returns(hooks); router.match.returns({ specialRole: 404 }); - const guardManager = new GuardManager(router, pluginManager, errorHandler, logger); + const transitionHooksExecutor = new TransitionHooksExecutor(router, pluginManager, errorHandler, logger); - chai.expect(guardManager.hasAccessTo('/router/does/not/have/route')).to.be.true; + chai.expect(transitionHooksExecutor.hasAccessTo('/router/does/not/have/route')).to.be.true; }); it(`if none of hooks returns "${actionTypes.stopNavigation}" or "${actionTypes.redirect}" action types`, () => { @@ -85,9 +85,9 @@ describe('GuardManager', () => { transitionHooksPlugin.getTransitionHooks.returns(hooks); router.match.returns(route); - const guardManager = new GuardManager(router, pluginManager, errorHandler, logger); + const transitionHooksExecutor = new TransitionHooksExecutor(router, pluginManager, errorHandler, logger); - chai.expect(guardManager.hasAccessTo(url)).to.be.true; + chai.expect(transitionHooksExecutor.hasAccessTo(url)).to.be.true; for (const hook of hooks) { sinon.assert.calledOnceWithExactly(hook, { @@ -113,12 +113,12 @@ describe('GuardManager', () => { transitionHooksPlugin.getTransitionHooks.returns(hooks); router.match.returns(route); - const guardManager = new GuardManager(router, pluginManager, errorHandler, logger); + const transitionHooksExecutor = new TransitionHooksExecutor(router, pluginManager, errorHandler, logger); - chai.expect(guardManager.hasAccessTo(url)).to.be.false; + chai.expect(transitionHooksExecutor.hasAccessTo(url)).to.be.false; sinon.assert.calledOnce(errorHandler); chai.expect(errorHandler.getCall(0).args[0]).to.have.property('cause', error); - chai.expect(errorHandler.getCall(0).args[0]).to.be.instanceOf(errors.GuardTransitionHookError); + chai.expect(errorHandler.getCall(0).args[0]).to.be.instanceOf(TransitionHookError); chai.expect(errorHandler.getCall(0).args[0].data).to.be.eql({ hookIndex: 1, url, @@ -150,9 +150,9 @@ describe('GuardManager', () => { transitionHooksPlugin.getTransitionHooks.returns(hooks); router.match.returns(route); - const guardManager = new GuardManager(router, pluginManager, errorHandler, logger); + const transitionHooksExecutor = new TransitionHooksExecutor(router, pluginManager, errorHandler, logger); - chai.expect(guardManager.hasAccessTo(url)).to.be.false; + chai.expect(transitionHooksExecutor.hasAccessTo(url)).to.be.false; sinon.assert.calledOnceWithExactly( logger.info, `ILC: Stopped navigation due to the Route Guard with index #${1}`, @@ -183,9 +183,9 @@ describe('GuardManager', () => { transitionHooksPlugin.getTransitionHooks.returns(hooks); router.match.returns(route); - const guardManager = new GuardManager(router, pluginManager, errorHandler, logger); + const transitionHooksExecutor = new TransitionHooksExecutor(router, pluginManager, errorHandler, logger); - chai.expect(guardManager.hasAccessTo(url)).to.be.false; + chai.expect(transitionHooksExecutor.hasAccessTo(url)).to.be.false; sinon.assert.notCalled(router.navigateToUrl); for (const hook of [hooks[0], hooks[1]]) { @@ -204,7 +204,7 @@ describe('GuardManager', () => { sinon.assert.calledWithExactly( logger.info, - `ILC: Redirect from "${route.reqUrl}" to "${url}" due to the Route Guard with index #${1}`, + `ILC: Redirect from "${route.reqUrl}" to "${url}" due to the Transition Hook with index #${1}`, ); sinon.assert.calledWithExactly(router.navigateToUrl, url); }); diff --git a/ilc/common/guard/errors.js b/ilc/common/guard/errors.js deleted file mode 100644 index f6c0ca737..000000000 --- a/ilc/common/guard/errors.js +++ /dev/null @@ -1,11 +0,0 @@ -const { extendError } = require('../utils'); - -const errors = {}; - -errors.GuardError = extendError('GuardError', { defaultData: {} }); -errors.GuardTransitionHookError = extendError('GuardTransitionHookError', { - parent: errors.GuardError, - defaultData: {}, -}); - -module.exports = Object.freeze(errors); diff --git a/ilc/common/guard/errors.ts b/ilc/common/guard/errors.ts new file mode 100644 index 000000000..2e86535f0 --- /dev/null +++ b/ilc/common/guard/errors.ts @@ -0,0 +1,8 @@ +import { extendError } from '../utils'; + +export const TransitionError = extendError('TransitionError', { defaultData: {} }); + +export const TransitionHookError = extendError('TransitionHookError', { + parent: TransitionError, + defaultData: {}, +}); diff --git a/ilc/package-lock.json b/ilc/package-lock.json index ee6e146af..4210e159d 100644 --- a/ilc/package-lock.json +++ b/ilc/package-lock.json @@ -24,7 +24,7 @@ "fast-glob": "^3.3.2", "fastify": "^2.15.3", "http-status-codes": "^2.3.0", - "ilc-plugins-sdk": "^2.1.0", + "ilc-plugins-sdk": "^2.2.0", "ilc-sdk": "^5.2.4", "is-url": "^1.2.4", "js-cookie": "^2.2.1", @@ -5476,9 +5476,9 @@ "dev": true }, "node_modules/ilc-plugins-sdk": { - "version": "2.1.0", - "resolved": "https://registry.npmjs.org/ilc-plugins-sdk/-/ilc-plugins-sdk-2.1.0.tgz", - "integrity": "sha512-6tzVmFTGTwL7pPzQKegfDUBw2APDbkcJLk4p2gpURan0qTy/b5ELdSQP8q4VPNqpTZl5aihw+JIOqbcRGbDC0A==", + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/ilc-plugins-sdk/-/ilc-plugins-sdk-2.2.0.tgz", + "integrity": "sha512-gx9GxyAblTMPlN+MEye0z62jqAp+ING/WxDpN6wxmF7zxhdgxw1lSTP7LG6tlPIiJZOISf1tqzOYWjCVqUC9xw==", "dependencies": { "fast-glob": "^3.3.1", "http-headers": "^3.0.2", diff --git a/ilc/package.json b/ilc/package.json index 1d440b268..0a7f61ef2 100644 --- a/ilc/package.json +++ b/ilc/package.json @@ -35,7 +35,7 @@ "fast-glob": "^3.3.2", "fastify": "^2.15.3", "http-status-codes": "^2.3.0", - "ilc-plugins-sdk": "^2.1.0", + "ilc-plugins-sdk": "^2.2.0", "ilc-sdk": "^5.2.4", "is-url": "^1.2.4", "js-cookie": "^2.2.1", diff --git a/ilc/server/GuardManager.js b/ilc/server/GuardManager.js deleted file mode 100644 index fc72dac07..000000000 --- a/ilc/server/GuardManager.js +++ /dev/null @@ -1,59 +0,0 @@ -const errors = require('../common/guard/errors'); -const actionTypes = require('../common/guard/actionTypes'); - -class GuardManager { - #transitionHooksPlugin; - - constructor(pluginManager) { - this.#transitionHooksPlugin = pluginManager.getTransitionHooksPlugin(); - } - - /** - * @param {FastifyRequest} req - * @return {Promise} - */ - async redirectTo(req) { - const route = req.raw.router.getRoute(); - - if (route.specialRole !== null) { - return null; - } - - const hooks = this.#transitionHooksPlugin.getTransitionHooks(); - - if (hooks.length === 0) { - return null; - } - - for (const hook of hooks) { - try { - const action = await hook({ - route: { - meta: route.meta, - url: route.reqUrl, - hostname: req.hostname, - }, - log: req.log, - req: req.raw, - }); - - if (action.type === actionTypes.redirect && action.newLocation) { - return action.newLocation; - } - } catch (error) { - const hookIndex = hooks.indexOf(hook); - throw new errors.GuardTransitionHookError({ - message: `An error has occurred while executing "${hookIndex}" transition hook.`, - data: { - hookIndex, - }, - cause: error, - }); - } - } - - return null; - } -} - -module.exports = GuardManager; diff --git a/ilc/server/GuardManager.spec.js b/ilc/server/TransitionHooksExecutor.spec.js similarity index 71% rename from ilc/server/GuardManager.spec.js rename to ilc/server/TransitionHooksExecutor.spec.js index 02aafe177..487f2b551 100644 --- a/ilc/server/GuardManager.spec.js +++ b/ilc/server/TransitionHooksExecutor.spec.js @@ -7,12 +7,12 @@ chai.use(chaiAsPromised); const nock = require('nock'); const createApp = require('./app'); const helpers = require('../tests/helpers'); -const errors = require('../common/guard/errors'); +const { TransitionHookError } = require('../common/guard/errors'); const actionTypes = require('../common/guard/actionTypes'); -const GuardManager = require('./GuardManager'); +const { TransitionHooksExecutor } = require('./TransitionHooksExecutor'); const { context } = require('./context/context'); -describe('GuardManager', () => { +describe('TransitionHooksExecutor', () => { const transitionHooksPlugin = Object.freeze({ getTransitionHooks: sinon.stub(), }); @@ -60,6 +60,31 @@ describe('GuardManager', () => { chai.expect(res.headers.location).to.be.equal(newLocation); }); + it(`should redirect with custom HTTP code`, async () => { + let res; + + const newLocation = '/should/be/this/location'; + const hooks = [ + sinon.stub().resolves({ type: actionTypes.continue }), + sinon.stub().resolves({ type: actionTypes.redirect, newLocation, code: 308 }), + sinon.stub().resolves({ type: actionTypes.continue }), + ]; + + pluginManager.getTransitionHooksPlugin.returns(transitionHooksPlugin); + transitionHooksPlugin.getTransitionHooks.returns(hooks); + + const app = createApp(helpers.getRegistryMock(), pluginManager, context); + + try { + res = await app.inject({ method: 'GET', url: '/all' }); + } finally { + app.close(); + } + + chai.expect(res.statusCode).to.be.equal(308); + chai.expect(res.headers.location).to.be.equal(newLocation); + }); + it(`should not redirect when none of hooks resolves with "${actionTypes.redirect}" action type`, async () => { let res; @@ -116,7 +141,7 @@ describe('GuardManager', () => { pluginManager.getTransitionHooksPlugin.returns(transitionHooksPlugin); transitionHooksPlugin.getTransitionHooks.returns([]); - const redirectTo = await new GuardManager(pluginManager).redirectTo(req); + const redirectTo = await new TransitionHooksExecutor(pluginManager).redirectTo(req); chai.expect(redirectTo).to.be.null; }); @@ -133,7 +158,7 @@ describe('GuardManager', () => { getRoute: () => route, }, }); - const redirectTo = await new GuardManager(pluginManager).redirectTo({ ...req, raw: rawReq }); + const redirectTo = await new TransitionHooksExecutor(pluginManager).redirectTo({ ...req, raw: rawReq }); chai.expect(redirectTo).to.be.null; }); @@ -147,7 +172,7 @@ describe('GuardManager', () => { pluginManager.getTransitionHooksPlugin.returns(transitionHooksPlugin); transitionHooksPlugin.getTransitionHooks.returns(hooks); - const redirectTo = await new GuardManager(pluginManager).redirectTo(req); + const redirectTo = await new TransitionHooksExecutor(pluginManager).redirectTo(req); for (const hook of hooks) { chai.expect( @@ -177,9 +202,9 @@ describe('GuardManager', () => { transitionHooksPlugin.getTransitionHooks.returns(hooks); await chai - .expect(new GuardManager(pluginManager).redirectTo(req)) + .expect(new TransitionHooksExecutor(pluginManager).redirectTo(req)) .to.eventually.be.rejected.then((rejectedError) => { - chai.expect(rejectedError).to.be.instanceOf(errors.GuardTransitionHookError); + chai.expect(rejectedError).to.be.instanceOf(TransitionHookError); chai.expect(rejectedError.data).to.be.eql({ hookIndex: 1, }); @@ -213,7 +238,37 @@ describe('GuardManager', () => { pluginManager.getTransitionHooksPlugin.returns(transitionHooksPlugin); transitionHooksPlugin.getTransitionHooks.returns(hooks); - const redirectTo = await new GuardManager(pluginManager).redirectTo(req); + const redirectTo = await new TransitionHooksExecutor(pluginManager).redirectTo(req); + + for (const hook of [hooks[0], hooks[1]]) { + chai.expect( + hook.calledOnceWith({ + route: { meta: route.meta, url: route.reqUrl, hostname: req.hostname }, + req: rawReq, + log, + }), + ).to.be.true; + } + + for (const hook of [hooks[2], hooks[3]]) { + chai.expect(hook.called).to.be.false; + } + + chai.expect(redirectTo).to.eql({ code: 302, location: '/should/be/this/location' }); + }); + it(`if some of hooks resolves with "${actionTypes.redirect}" action type and code`, async () => { + const newLocation = '/should/be/this/location'; + const hooks = [ + sinon.stub().resolves({ type: actionTypes.continue }), + sinon.stub().resolves({ type: actionTypes.redirect, newLocation, code: 307 }), + sinon.stub().resolves({ type: actionTypes.continue }), + sinon.stub().resolves({ type: actionTypes.continue }), + ]; + + pluginManager.getTransitionHooksPlugin.returns(transitionHooksPlugin); + transitionHooksPlugin.getTransitionHooks.returns(hooks); + + const redirectTo = await new TransitionHooksExecutor(pluginManager).redirectTo(req); for (const hook of [hooks[0], hooks[1]]) { chai.expect( @@ -229,7 +284,7 @@ describe('GuardManager', () => { chai.expect(hook.called).to.be.false; } - chai.expect(redirectTo).to.eql(newLocation); + chai.expect(redirectTo).to.eql({ code: 307, location: '/should/be/this/location' }); }); }); }); diff --git a/ilc/server/TransitionHooksExecutor.ts b/ilc/server/TransitionHooksExecutor.ts new file mode 100644 index 000000000..9a78d35a9 --- /dev/null +++ b/ilc/server/TransitionHooksExecutor.ts @@ -0,0 +1,65 @@ +import { PluginManager, TransitionHooksPlugin } from 'ilc-plugins-sdk'; +import { TransitionHookError } from '../common/guard/errors'; + +type TransitionResult = { + location: string; + code: number; +}; + +export class TransitionHooksExecutor { + private readonly transitionHooksPlugin: TransitionHooksPlugin; + + constructor(pluginManager: PluginManager) { + this.transitionHooksPlugin = pluginManager.getTransitionHooksPlugin(); + } + + async redirectTo(req: any): Promise { + const route = req.raw.router.getRoute(); + + if (route.specialRole !== null) { + return null; + } + + const hooks = this.transitionHooksPlugin.getTransitionHooks(); + + if (hooks.length === 0) { + return null; + } + + for (const hook of hooks) { + try { + const action = await hook({ + route: { + meta: route.meta, + url: route.reqUrl, + hostname: req.hostname, + }, + log: req.log, + req: req.raw, + }); + + if (action.type === 'redirect' && action.newLocation) { + const code = action.code ?? 302; + if (code < 300 || code > 308) { + throw new TransitionHookError({ message: 'Invlid redirect code' }); + } + return { + location: action.newLocation, + code, + }; + } + } catch (error) { + const hookIndex = hooks.indexOf(hook); + throw new TransitionHookError({ + message: `An error has occurred while executing "${hookIndex}" transition hook.`, + data: { + hookIndex, + }, + cause: error as Error, + }); + } + } + + return null; + } +} diff --git a/ilc/server/routes/wildcardRequestHandlerFactory.ts b/ilc/server/routes/wildcardRequestHandlerFactory.ts index e47c1e5a4..fa8e52faf 100644 --- a/ilc/server/routes/wildcardRequestHandlerFactory.ts +++ b/ilc/server/routes/wildcardRequestHandlerFactory.ts @@ -5,13 +5,13 @@ import type { RequestHandler } from 'fastify'; import type { Logger, PluginManager } from 'ilc-plugins-sdk'; import { SlotCollection } from '../../common/Slot/SlotCollection'; import UrlProcessor from '../../common/UrlProcessor'; -import GuardManager from '../GuardManager'; import i18n from '../i18n'; import CspBuilderService from '../services/CspBuilderService'; import tailorFactory from '../tailor/factory'; import mergeConfigs from '../tailor/merge-configs'; import parseOverrideConfig from '../tailor/parse-override-config'; import ServerRouter from '../tailor/server-router'; +import { TransitionHooksExecutor } from '../TransitionHooksExecutor'; import { ErrorHandler } from '../types/ErrorHandler'; import { PatchedHttpRequest } from '../types/PatchedHttpRequest'; import { Registry, TransformedRegistryConfig } from '../types/Registry'; @@ -22,7 +22,7 @@ export function wildcardRequestHandlerFactory( errorHandlingService: ErrorHandler, pluginManager: PluginManager, ): RequestHandler { - const guardManager = new GuardManager(pluginManager); + const transitionHooksExecutor = new TransitionHooksExecutor(pluginManager); const autoInjectNrMonitoringConfig = config.get('newrelic.automaticallyInjectBrowserMonitoring'); const autoInjectNrMonitoring = typeof autoInjectNrMonitoringConfig === 'boolean' @@ -73,12 +73,13 @@ export function wildcardRequestHandlerFactory( const unlocalizedUrl = i18n.unlocalizeUrl(finalRegistryConfig.settings.i18n, url); req.raw.router = new ServerRouter(req.log, req.raw, unlocalizedUrl); - const redirectTo = await guardManager.redirectTo(req); + const redirectTo = await transitionHooksExecutor.redirectTo(req); if (redirectTo) { reply.redirect( + redirectTo.code, urlProcessor.process( - i18n.localizeUrl(finalRegistryConfig.settings.i18n, redirectTo, { + i18n.localizeUrl(finalRegistryConfig.settings.i18n, redirectTo.location, { locale: req.raw.ilcState?.locale, }), ),