From 33487d11b9d81573a86b7f396561440b52d7664f Mon Sep 17 00:00:00 2001 From: Jaanus Sellin Date: Thu, 6 Apr 2023 12:46:54 +0300 Subject: [PATCH] feat: authorization middleware (#3464) --- .../UserProfileContent/UserProfileContent.tsx | 16 ++++---- src/lib/app.ts | 2 +- .../middleware/authorization-middleware.ts | 39 ++++++++++++++++++ src/lib/middleware/oss-authentication.test.ts | 8 +++- src/lib/middleware/oss-authentication.ts | 40 ++++++------------- src/lib/middleware/session-db.ts | 1 + src/lib/routes/logout.test.ts | 22 +++++----- src/lib/routes/logout.ts | 11 ++++- src/test/e2e/api/admin/user/pat.e2e.test.ts | 2 +- 9 files changed, 90 insertions(+), 51 deletions(-) create mode 100644 src/lib/middleware/authorization-middleware.ts diff --git a/frontend/src/component/user/UserProfile/UserProfileContent/UserProfileContent.tsx b/frontend/src/component/user/UserProfile/UserProfileContent/UserProfileContent.tsx index 547a323d19..3976ad6bc0 100644 --- a/frontend/src/component/user/UserProfile/UserProfileContent/UserProfileContent.tsx +++ b/frontend/src/component/user/UserProfile/UserProfileContent/UserProfileContent.tsx @@ -123,13 +123,15 @@ export const UserProfileContent = ({ - - Log out - +
+ + Log out + +
} /> diff --git a/src/lib/app.ts b/src/lib/app.ts index e0d2069ca4..4fab348add 100644 --- a/src/lib/app.ts +++ b/src/lib/app.ts @@ -107,7 +107,7 @@ export default async function getApp( switch (config.authentication.type) { case IAuthType.OPEN_SOURCE: { app.use(baseUriPath, apiTokenMiddleware(config, services)); - ossAuthentication(app, config.server.baseUriPath); + ossAuthentication(app, config.getLogger, config.server.baseUriPath); break; } case IAuthType.ENTERPRISE: { diff --git a/src/lib/middleware/authorization-middleware.ts b/src/lib/middleware/authorization-middleware.ts new file mode 100644 index 0000000000..6a4120cde5 --- /dev/null +++ b/src/lib/middleware/authorization-middleware.ts @@ -0,0 +1,39 @@ +import { IAuthRequest } from '../routes/unleash-types'; +import { NextFunction, Response } from 'express'; +import AuthenticationRequired from '../types/authentication-required'; +import { LogProvider } from '../logger'; + +/* eslint-disable @typescript-eslint/explicit-module-boundary-types */ +const authorizationMiddleware = ( + getLogger: LogProvider, + baseUriPath: string, +): any => { + const logger = getLogger('/middleware/authorization-middleware.ts'); + logger.debug('Enabling Authorization middleware'); + + const generateAuthResponse = async () => + new AuthenticationRequired({ + type: 'password', + path: `${baseUriPath}/auth/simple/login`, + message: 'You must sign in order to use Unleash', + }); + + return async (req: IAuthRequest, res: Response, next: NextFunction) => { + if (req.session && req.session.user) { + req.user = req.session.user; + return next(); + } + if (req.user) { + return next(); + } + if (req.header('authorization')) { + // API clients should get 401 without body + return res.sendStatus(401); + } + // Admin UI users should get auth-response + const authRequired = await generateAuthResponse(); + return res.status(401).json(authRequired); + }; +}; + +export default authorizationMiddleware; diff --git a/src/lib/middleware/oss-authentication.test.ts b/src/lib/middleware/oss-authentication.test.ts index 162e842672..62b73c5d97 100644 --- a/src/lib/middleware/oss-authentication.test.ts +++ b/src/lib/middleware/oss-authentication.test.ts @@ -7,6 +7,10 @@ import ossAuth from './oss-authentication'; import getApp from '../app'; import User from '../types/user'; import sessionDb from './session-db'; +import { Knex } from 'knex'; +import { LogProvider } from '../logger'; + +const getLogger = (() => ({ debug() {} })) as unknown as LogProvider; async function getSetup(preRouterHook) { const base = `/random${Math.round(Math.random() * 1000)}`; @@ -14,7 +18,7 @@ async function getSetup(preRouterHook) { server: { baseUriPath: base }, preRouterHook: (_app) => { preRouterHook(_app); - ossAuth(_app, base); + ossAuth(_app, getLogger, base); _app.get(`${base}/api/protectedResource`, (req, res) => { res.status(200).json({ message: 'OK' }).end(); }); @@ -22,7 +26,7 @@ async function getSetup(preRouterHook) { }); const stores = createStores(); const services = createServices(stores, config); - const unleashSession = sessionDb(config, undefined); + const unleashSession = sessionDb(config, {} as Knex); const app = await getApp(config, stores, services, unleashSession); return { diff --git a/src/lib/middleware/oss-authentication.ts b/src/lib/middleware/oss-authentication.ts index 8828399aaf..ab7e074e13 100644 --- a/src/lib/middleware/oss-authentication.ts +++ b/src/lib/middleware/oss-authentication.ts @@ -1,33 +1,19 @@ -import { Application, NextFunction, Response } from 'express'; -import { IAuthRequest } from '../routes/unleash-types'; -import AuthenticationRequired from '../types/authentication-required'; - -function ossAuthHook(app: Application, baseUriPath: string): void { - const generateAuthResponse = async () => - new AuthenticationRequired({ - type: 'password', - path: `${baseUriPath}/auth/simple/login`, - message: 'You must sign in order to use Unleash', - }); +import { Application } from 'express'; +import authorizationMiddleware from './authorization-middleware'; +import { LogProvider } from '../logger'; +function ossAuthHook( + app: Application, + getLogger: LogProvider, + baseUriPath: string, +): void { app.use( `${baseUriPath}/api`, - async (req: IAuthRequest, res: Response, next: NextFunction) => { - if (req.session && req.session.user) { - req.user = req.session.user; - return next(); - } - if (req.user) { - return next(); - } - if (req.header('authorization')) { - // API clients should get 401 without body - return res.sendStatus(401); - } - // Admin UI users should get auth-response - const authRequired = await generateAuthResponse(); - return res.status(401).json(authRequired); - }, + authorizationMiddleware(getLogger, baseUriPath), + ); + app.use( + `${baseUriPath}/logout`, + authorizationMiddleware(getLogger, baseUriPath), ); } export default ossAuthHook; diff --git a/src/lib/middleware/session-db.ts b/src/lib/middleware/session-db.ts index 97b366992b..63bd0b668a 100644 --- a/src/lib/middleware/session-db.ts +++ b/src/lib/middleware/session-db.ts @@ -37,6 +37,7 @@ function sessionDb( : config.server.baseUriPath, secure: config.secureHeaders, maxAge: age, + sameSite: 'lax', }, }); } diff --git a/src/lib/routes/logout.test.ts b/src/lib/routes/logout.test.ts index e2e464532e..a6c68ed626 100644 --- a/src/lib/routes/logout.test.ts +++ b/src/lib/routes/logout.test.ts @@ -27,7 +27,7 @@ test('should redirect to "/" after logout', async () => { const request = supertest(app); expect.assertions(0); await request - .get(`${baseUriPath}/logout`) + .post(`${baseUriPath}/logout`) .expect(302) .expect('Location', `${baseUriPath}/`); }); @@ -45,7 +45,7 @@ test('should redirect to "/basePath" after logout when baseUriPath is set', asyn const request = supertest(app); expect.assertions(0); await request - .get('/logout') + .post('/logout') .expect(302) .expect('Location', `${baseUriPath}/`); }); @@ -64,7 +64,7 @@ test('should set "Clear-Site-Data" header', async () => { const request = supertest(app); expect.assertions(0); await request - .get(`${baseUriPath}/logout`) + .post(`${baseUriPath}/logout`) .expect(302) .expect('Clear-Site-Data', '"cookies", "storage"'); }); @@ -86,7 +86,7 @@ test('should not set "Clear-Site-Data" header', async () => { const request = supertest(app); expect.assertions(1); await request - .get(`${baseUriPath}/logout`) + .post(`${baseUriPath}/logout`) .expect(302) .expect((res) => expect(res.headers['Clear-Site-Data']).toBeUndefined(), @@ -108,7 +108,7 @@ test('should clear "unleash-session" cookies', async () => { const request = supertest(app); expect.assertions(0); await request - .get(`${baseUriPath}/logout`) + .post(`${baseUriPath}/logout`) .expect(302) .expect( 'Set-Cookie', @@ -134,7 +134,7 @@ test('should clear "unleash-session" cookie even when disabled clear site data', const request = supertest(app); expect.assertions(0); await request - .get(`${baseUriPath}/logout`) + .post(`${baseUriPath}/logout`) .expect(302) .expect( 'Set-Cookie', @@ -162,7 +162,7 @@ test('should call destroy on session', async () => { app.use('/logout', new LogoutController(config, { sessionService }).router); const request = supertest(app); - await request.get(`${baseUriPath}/logout`); + await request.post(`${baseUriPath}/logout`); expect(fakeSession.destroy.mock.calls.length).toBe(1); }); @@ -186,7 +186,7 @@ test('should handle req.logout with callback function', async () => { app.use('/logout', new LogoutController(config, { sessionService }).router); const request = supertest(app); - await request.get(`${baseUriPath}/logout`); + await request.post(`${baseUriPath}/logout`); expect(logoutFunction).toHaveBeenCalledTimes(1); expect(logoutFunction).toHaveBeenCalledWith(expect.anything()); @@ -211,7 +211,7 @@ test('should handle req.logout without callback function', async () => { app.use('/logout', new LogoutController(config, { sessionService }).router); const request = supertest(app); - await request.get(`${baseUriPath}/logout`); + await request.post(`${baseUriPath}/logout`); expect(logoutFunction).toHaveBeenCalledTimes(1); expect(logoutFunction).toHaveBeenCalledWith(); @@ -238,7 +238,7 @@ test('should redirect to alternative logoutUrl', async () => { const request = supertest(app); await request - .get('/logout') + .post('/logout') .expect(302) .expect('Location', '/some-other-path'); }); @@ -282,7 +282,7 @@ test('Should destroy sessions for user', async () => { let activeSessionsBeforeLogout = await sessionStore.getSessionsForUser(1); expect(activeSessionsBeforeLogout).toHaveLength(2); app.use('/logout', new LogoutController(config, { sessionService }).router); - await supertest(app).get('/logout').expect(302); + await supertest(app).post('/logout').expect(302); let activeSessions = await sessionStore.getSessionsForUser(1); expect(activeSessions).toHaveLength(0); }); diff --git a/src/lib/routes/logout.ts b/src/lib/routes/logout.ts index c7dfb78feb..dc72a6564c 100644 --- a/src/lib/routes/logout.ts +++ b/src/lib/routes/logout.ts @@ -1,6 +1,6 @@ import { Response } from 'express'; import { promisify } from 'util'; -import { IUnleashConfig } from '../types'; +import { IUnleashConfig, NONE } from '../types'; import Controller from './controller'; import { IAuthRequest } from './unleash-types'; import { IUnleashServices } from '../types'; @@ -24,7 +24,14 @@ class LogoutController extends Controller { this.baseUri = config.server.baseUriPath; this.clearSiteDataOnLogout = config.session.clearSiteDataOnLogout; this.cookieName = config.session.cookieName; - this.get('/', this.logout); + + this.route({ + method: 'post', + path: '/', + handler: this.logout, + permission: NONE, + acceptAnyContentType: true, + }); } async logout(req: IAuthRequest, res: Response): Promise { diff --git a/src/test/e2e/api/admin/user/pat.e2e.test.ts b/src/test/e2e/api/admin/user/pat.e2e.test.ts index 129fb76627..4a07a87a56 100644 --- a/src/test/e2e/api/admin/user/pat.e2e.test.ts +++ b/src/test/e2e/api/admin/user/pat.e2e.test.ts @@ -222,7 +222,7 @@ test('should not fail creation of PAT when a description already exists for anot }); test('should get user id 1', async () => { - await app.request.get('/logout').expect(302); + await app.request.post('/logout').expect(302); await app.request .get('/api/admin/user') .set('Authorization', firstSecret)