feat: authorization middleware (#3464)

frontend/src/component/user/UserProfile/UserProfileContent/UserProfileContent.tsx

@@ -123,13 +123,15 @@ export const UserProfileContent = ({
 
                 <StyledDivider />
 
-                <StyledLogoutButton
+                <form method="POST" action={`${basePath}/logout`}>
-                    variant="outlined"
+                    <StyledLogoutButton
-                    color="primary"
+                        type="submit"
-                    href={`${basePath}/logout`}
+                        variant="outlined"
-                >
+                        color="primary"
-                    Log out
+                    >
-                </StyledLogoutButton>
+                        Log out
+                    </StyledLogoutButton>
+                </form>
             </StyledPaper>
         }
     />

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: {

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;

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 {

src/lib/middleware/oss-authentication.ts

@@ -1,33 +1,19 @@
-import { Application, NextFunction, Response } from 'express';
+import { Application } from 'express';
-import { IAuthRequest } from '../routes/unleash-types';
+import authorizationMiddleware from './authorization-middleware';
-import AuthenticationRequired from '../types/authentication-required';
+import { LogProvider } from '../logger';
-
-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',
-        });
 
+function ossAuthHook(
+    app: Application,
+    getLogger: LogProvider,
+    baseUriPath: string,
+): void {
     app.use(
         `${baseUriPath}/api`,
-        async (req: IAuthRequest, res: Response, next: NextFunction) => {
+        authorizationMiddleware(getLogger, baseUriPath),
-            if (req.session && req.session.user) {
+    );
-                req.user = req.session.user;
+    app.use(
-                return next();
+        `${baseUriPath}/logout`,
-            }
+        authorizationMiddleware(getLogger, baseUriPath),
-            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 ossAuthHook;

src/lib/middleware/session-db.ts

@@ -37,6 +37,7 @@ function sessionDb(
                     : config.server.baseUriPath,
             secure: config.secureHeaders,
             maxAge: age,
+            sameSite: 'lax',
         },
     });
 }

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);
 });

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<void> {

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)