From 48ed642f9ea820bd77a59bce8e8bccd8e5973703 Mon Sep 17 00:00:00 2001 From: Theo Sanderson Date: Wed, 10 Jan 2024 11:28:16 +0000 Subject: [PATCH 1/7] change login middleware logic --- website/src/middleware/authMiddleware.ts | 6 +-- website/src/utils/isPublicRoute.spec.ts | 37 ------------------- website/src/utils/isPublicRoute.ts | 29 --------------- .../shouldMiddlewareEnforceLogin.spec.ts | 37 +++++++++++++++++++ .../src/utils/shouldMiddlewareEnforceLogin.ts | 21 +++++++++++ 5 files changed, 61 insertions(+), 69 deletions(-) delete mode 100644 website/src/utils/isPublicRoute.spec.ts delete mode 100644 website/src/utils/isPublicRoute.ts create mode 100644 website/src/utils/shouldMiddlewareEnforceLogin.spec.ts create mode 100644 website/src/utils/shouldMiddlewareEnforceLogin.ts diff --git a/website/src/middleware/authMiddleware.ts b/website/src/middleware/authMiddleware.ts index c16bc29dfe..7126ba967d 100644 --- a/website/src/middleware/authMiddleware.ts +++ b/website/src/middleware/authMiddleware.ts @@ -7,7 +7,7 @@ import { type BaseClient, Issuer, type TokenSet } from 'openid-client'; import { getConfiguredOrganisms, getRuntimeConfig } from '../config.ts'; import { getInstanceLogger } from '../logger.ts'; -import { isPublicRoute } from '../utils/isPublicRoute.ts'; +import { shouldMiddlewareEnforceLogin } from '../utils/shouldMiddlewareEnforceLogin.ts'; const { decode, verify } = jsonwebtoken; @@ -51,11 +51,11 @@ export async function getKeycloakClient() { export const authMiddleware = defineMiddleware(async (context, next) => { let token = await getTokenFromCookie(context); - const urlIsPublicRoute = isPublicRoute( + const enforceLogin = shouldMiddlewareEnforceLogin( context.url.pathname, getConfiguredOrganisms().map((it) => it.key), ); - if (urlIsPublicRoute) { + if (!enforceLogin) { if (token === undefined) { context.locals.session = { isLoggedIn: false, diff --git a/website/src/utils/isPublicRoute.spec.ts b/website/src/utils/isPublicRoute.spec.ts deleted file mode 100644 index ff053c91f3..0000000000 --- a/website/src/utils/isPublicRoute.spec.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { describe, expect, test } from 'vitest'; - -import { isPublicRoute } from './isPublicRoute'; -import { testOrganism } from '../../vitest.setup.ts'; - -const otherOrganism = 'otherOrganism'; -const configuredOrganisms = [testOrganism, otherOrganism]; - -describe('isPublicRoute', () => { - test('should return false if not specified', () => { - expect(isPublicRoute('/someRoute', [])).toBe(false); - }); - - test('should return true for empty string', () => { - expect(isPublicRoute('', [])).toBe(true); - }); - - test('should return true if specified as public route', () => { - expectToBePublic(`/${testOrganism}/search`); - expectToBePublic('/'); - expectToBePublic(`/${testOrganism}`); - expectToBePublic(`/${testOrganism}/sequences/id_002156`); - expectToBePublic(`/${testOrganism}/sequences/id_002156`); - expectToBePublic(`/${otherOrganism}/sequences/id_002156`); - }); - - test('should return false on non public routes', () => { - expect(isPublicRoute('/user', configuredOrganisms)).toBe(false); - expect(isPublicRoute('/user/someUsername', configuredOrganisms)).toBe(false); - expect(isPublicRoute(`/${testOrganism}/revise`, configuredOrganisms)).toBe(false); - expect(isPublicRoute(`/${testOrganism}/submit`, configuredOrganisms)).toBe(false); - }); - - function expectToBePublic(path: string) { - expect(isPublicRoute(path, configuredOrganisms), path).toBe(true); - } -}); diff --git a/website/src/utils/isPublicRoute.ts b/website/src/utils/isPublicRoute.ts deleted file mode 100644 index 27c1b66f80..0000000000 --- a/website/src/utils/isPublicRoute.ts +++ /dev/null @@ -1,29 +0,0 @@ -const publicRoutesCache: Record = {}; - -function getPublicRoutes(configuredOrganisms: string[]) { - const cacheKey = configuredOrganisms.join(''); - if (!(cacheKey in publicRoutesCache)) { - const organismSpecificRoutes = configuredOrganisms.flatMap((organism) => [ - new RegExp(`^/${organism}/sequences(?:/.*)?$`), - new RegExp(`^/${organism}/search$`), - new RegExp(`^/${organism}/?$`), - ]); - - publicRoutesCache[cacheKey] = [ - new RegExp('^/?$'), - new RegExp('^/404$'), - new RegExp('^/about$'), - new RegExp('^/api_documentation$'), - new RegExp('^/governance$'), - new RegExp('^/status$'), - new RegExp('^/logout$'), - new RegExp('^/admin/logs.txt$'), - ...organismSpecificRoutes, - ]; - } - return publicRoutesCache[cacheKey]; -} - -export function isPublicRoute(pathname: string, configuredOrganisms: string[]) { - return getPublicRoutes(configuredOrganisms).some((route) => route.test(pathname)); -} diff --git a/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts b/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts new file mode 100644 index 0000000000..cf1ff66367 --- /dev/null +++ b/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts @@ -0,0 +1,37 @@ +import { describe, expect, test } from 'vitest'; + +import { shouldMiddlewareEnforceLogin } from './shouldMiddlewareEnforceLogin'; +import { testOrganism } from '../../vitest.setup.ts'; + +const otherOrganism = 'otherOrganism'; +const configuredOrganisms = [testOrganism, otherOrganism]; + +describe('shouldMiddlewareEnforceLogin', () => { + test('should return false if not specified', () => { + expect(shouldMiddlewareEnforceLogin('/someRoute', [])).toBe(false); + }); + + test('should return false for empty string', () => { + expect(shouldMiddlewareEnforceLogin('', [])).toBe(false); + }); + + test('should return false for various public routes route', () => { + expect(`/${testOrganism}/search`).toBe(false); + expect('/').toBe(false); + expect(`/${testOrganism}`).toBe(false); + expect(`/${testOrganism}/sequences/id_002156`).toBe(false); + expect(`/${testOrganism}/sequences/id_002156`).toBe(false); + expect(`/${otherOrganism}/sequences/id_002156`).toBe(false); + }); + + test('should return true on routes which should force login', () => { + expectForceLogin(isPublicRoute('/user', configuredOrganisms)) + expectForceLogin(isPublicRoute('/user/someUsername', configuredOrganisms)); + expectForceLogin(isPublicRoute(`/${testOrganism}/revise`, configuredOrganisms)); + expectForceLogin(isPublicRoute(`/${testOrganism}/submit`, configuredOrganisms)); + }); + + function expectForceLogin(path: string) { + expect(shouldMiddlewareEnforceLogin(path, configuredOrganisms), path).toBe(true); + } +}); diff --git a/website/src/utils/shouldMiddlewareEnforceLogin.ts b/website/src/utils/shouldMiddlewareEnforceLogin.ts new file mode 100644 index 0000000000..2a93ba4812 --- /dev/null +++ b/website/src/utils/shouldMiddlewareEnforceLogin.ts @@ -0,0 +1,21 @@ +const enforcedLoginRoutesCache: Record = {}; + +function getEnforcedLoginRoutes(configuredOrganisms: string[]) { + const cacheKey = configuredOrganisms.join(''); + if (!(cacheKey in enforcedLoginRoutesCache)) { + const organismSpecificRoutes = configuredOrganisms.flatMap((organism) => [ + new RegExp(`^/${organism}/revise`), + new RegExp(`^/${organism}/submit`), + ]); + + enforcedLoginRoutesCache[cacheKey] = [ + new RegExp('^/user/'), + ...organismSpecificRoutes, + ]; + } + return enforcedLoginRoutesCache[cacheKey]; +} + +export function shouldMiddlewareEnforceLogin(pathname: string, configuredOrganisms: string[]) { + return getEnforcedLoginRoutes(configuredOrganisms).some((route) => route.test(pathname)); +} From d760f1f52c1989c0fd5c7698e7bcf7f5e01d4244 Mon Sep 17 00:00:00 2001 From: Theo Sanderson Date: Wed, 10 Jan 2024 11:35:18 +0000 Subject: [PATCH 2/7] fixes --- .../src/pages/[organism]/revise/index.astro | 6 +++--- .../utils/shouldMiddlewareEnforceLogin.spec.ts | 18 ++++++++---------- .../src/utils/shouldMiddlewareEnforceLogin.ts | 5 +---- 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/website/src/pages/[organism]/revise/index.astro b/website/src/pages/[organism]/revise/index.astro index 42d53ea9a2..9117e2812b 100644 --- a/website/src/pages/[organism]/revise/index.astro +++ b/website/src/pages/[organism]/revise/index.astro @@ -13,9 +13,9 @@ const clientConfig = getRuntimeConfig().public;

Revision Page

- Note: each sequence entry needs to have the associated accession as a metadata field. Furthermore, the sequence - entry that should be revised needs to be already released. If the latter does not hold, delete the accession version - or submit an edited version. + Note: each sequence entry needs to have the associated accession as a metadata field. Furthermore, the sequence entry + that should be revised needs to be already released. If the latter does not hold, delete the accession version or submit + an edited version. diff --git a/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts b/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts index cf1ff66367..db7f7e7e8e 100644 --- a/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts +++ b/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts @@ -16,19 +16,17 @@ describe('shouldMiddlewareEnforceLogin', () => { }); test('should return false for various public routes route', () => { - expect(`/${testOrganism}/search`).toBe(false); - expect('/').toBe(false); - expect(`/${testOrganism}`).toBe(false); - expect(`/${testOrganism}/sequences/id_002156`).toBe(false); - expect(`/${testOrganism}/sequences/id_002156`).toBe(false); - expect(`/${otherOrganism}/sequences/id_002156`).toBe(false); + expect(shouldMiddlewareEnforceLogin(`/${testOrganism}/search`, configuredOrganisms)).toBe(false); + expect(shouldMiddlewareEnforceLogin(`/`, configuredOrganisms)).toBe(false); + expect(shouldMiddlewareEnforceLogin(`/${testOrganism}`, configuredOrganisms)).toBe(false); + expect(shouldMiddlewareEnforceLogin(`/${testOrganism}/sequences/id_002156`, configuredOrganisms)).toBe(false); }); test('should return true on routes which should force login', () => { - expectForceLogin(isPublicRoute('/user', configuredOrganisms)) - expectForceLogin(isPublicRoute('/user/someUsername', configuredOrganisms)); - expectForceLogin(isPublicRoute(`/${testOrganism}/revise`, configuredOrganisms)); - expectForceLogin(isPublicRoute(`/${testOrganism}/submit`, configuredOrganisms)); + expectForceLogin('/user'); + expectForceLogin('/user/someUsername'); + expectForceLogin(`/${testOrganism}/revise`); + expectForceLogin(`/${testOrganism}/submit`); }); function expectForceLogin(path: string) { diff --git a/website/src/utils/shouldMiddlewareEnforceLogin.ts b/website/src/utils/shouldMiddlewareEnforceLogin.ts index 2a93ba4812..f5f46d6950 100644 --- a/website/src/utils/shouldMiddlewareEnforceLogin.ts +++ b/website/src/utils/shouldMiddlewareEnforceLogin.ts @@ -8,10 +8,7 @@ function getEnforcedLoginRoutes(configuredOrganisms: string[]) { new RegExp(`^/${organism}/submit`), ]); - enforcedLoginRoutesCache[cacheKey] = [ - new RegExp('^/user/'), - ...organismSpecificRoutes, - ]; + enforcedLoginRoutesCache[cacheKey] = [new RegExp('^/user/'), ...organismSpecificRoutes]; } return enforcedLoginRoutesCache[cacheKey]; } From c6cbcca69c2f34f0ecf9e05ec491174f59ce97ae Mon Sep 17 00:00:00 2001 From: Theo Sanderson Date: Wed, 10 Jan 2024 11:39:16 +0000 Subject: [PATCH 3/7] refix with write prettier version --- website/src/pages/[organism]/revise/index.astro | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/website/src/pages/[organism]/revise/index.astro b/website/src/pages/[organism]/revise/index.astro index 9117e2812b..42d53ea9a2 100644 --- a/website/src/pages/[organism]/revise/index.astro +++ b/website/src/pages/[organism]/revise/index.astro @@ -13,9 +13,9 @@ const clientConfig = getRuntimeConfig().public;

Revision Page

- Note: each sequence entry needs to have the associated accession as a metadata field. Furthermore, the sequence entry - that should be revised needs to be already released. If the latter does not hold, delete the accession version or submit - an edited version. + Note: each sequence entry needs to have the associated accession as a metadata field. Furthermore, the sequence + entry that should be revised needs to be already released. If the latter does not hold, delete the accession version + or submit an edited version. From 07d066b1ed32fcd6f56cccae41f45a87aadd0a80 Mon Sep 17 00:00:00 2001 From: Theo Sanderson Date: Wed, 10 Jan 2024 11:41:00 +0000 Subject: [PATCH 4/7] add user to forced login routes --- website/src/utils/shouldMiddlewareEnforceLogin.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/website/src/utils/shouldMiddlewareEnforceLogin.ts b/website/src/utils/shouldMiddlewareEnforceLogin.ts index f5f46d6950..e6896d5c3c 100644 --- a/website/src/utils/shouldMiddlewareEnforceLogin.ts +++ b/website/src/utils/shouldMiddlewareEnforceLogin.ts @@ -6,6 +6,7 @@ function getEnforcedLoginRoutes(configuredOrganisms: string[]) { const organismSpecificRoutes = configuredOrganisms.flatMap((organism) => [ new RegExp(`^/${organism}/revise`), new RegExp(`^/${organism}/submit`), + new RegExp(`^/${organism}/user`), ]); enforcedLoginRoutesCache[cacheKey] = [new RegExp('^/user/'), ...organismSpecificRoutes]; From af242891ce77bb6bc352476abceedd9a690af089 Mon Sep 17 00:00:00 2001 From: Theo Sanderson Date: Wed, 10 Jan 2024 11:45:52 +0000 Subject: [PATCH 5/7] fix regex --- website/src/utils/shouldMiddlewareEnforceLogin.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/src/utils/shouldMiddlewareEnforceLogin.ts b/website/src/utils/shouldMiddlewareEnforceLogin.ts index e6896d5c3c..e67875917a 100644 --- a/website/src/utils/shouldMiddlewareEnforceLogin.ts +++ b/website/src/utils/shouldMiddlewareEnforceLogin.ts @@ -9,7 +9,7 @@ function getEnforcedLoginRoutes(configuredOrganisms: string[]) { new RegExp(`^/${organism}/user`), ]); - enforcedLoginRoutesCache[cacheKey] = [new RegExp('^/user/'), ...organismSpecificRoutes]; + enforcedLoginRoutesCache[cacheKey] = [new RegExp('^/user/?'), ...organismSpecificRoutes]; } return enforcedLoginRoutesCache[cacheKey]; } From dbfaccbc957e4534457e270d9f2fc992f00fc1aa Mon Sep 17 00:00:00 2001 From: Theo Sanderson Date: Wed, 10 Jan 2024 11:54:06 +0000 Subject: [PATCH 6/7] typo and reorder --- .../src/utils/shouldMiddlewareEnforceLogin.spec.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts b/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts index db7f7e7e8e..a5ad569460 100644 --- a/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts +++ b/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts @@ -15,13 +15,6 @@ describe('shouldMiddlewareEnforceLogin', () => { expect(shouldMiddlewareEnforceLogin('', [])).toBe(false); }); - test('should return false for various public routes route', () => { - expect(shouldMiddlewareEnforceLogin(`/${testOrganism}/search`, configuredOrganisms)).toBe(false); - expect(shouldMiddlewareEnforceLogin(`/`, configuredOrganisms)).toBe(false); - expect(shouldMiddlewareEnforceLogin(`/${testOrganism}`, configuredOrganisms)).toBe(false); - expect(shouldMiddlewareEnforceLogin(`/${testOrganism}/sequences/id_002156`, configuredOrganisms)).toBe(false); - }); - test('should return true on routes which should force login', () => { expectForceLogin('/user'); expectForceLogin('/user/someUsername'); @@ -29,6 +22,13 @@ describe('shouldMiddlewareEnforceLogin', () => { expectForceLogin(`/${testOrganism}/submit`); }); + test('should return false for various public routes', () => { + expect(shouldMiddlewareEnforceLogin(`/${testOrganism}/search`, configuredOrganisms)).toBe(false); + expect(shouldMiddlewareEnforceLogin(`/`, configuredOrganisms)).toBe(false); + expect(shouldMiddlewareEnforceLogin(`/${testOrganism}`, configuredOrganisms)).toBe(false); + expect(shouldMiddlewareEnforceLogin(`/${testOrganism}/sequences/id_002156`, configuredOrganisms)).toBe(false); + }); + function expectForceLogin(path: string) { expect(shouldMiddlewareEnforceLogin(path, configuredOrganisms), path).toBe(true); } From 8e4dce5cbabdcc8ea069d762afabe0406e38b1fa Mon Sep 17 00:00:00 2001 From: Theo Sanderson Date: Wed, 10 Jan 2024 13:25:48 +0000 Subject: [PATCH 7/7] add no login --- .../utils/shouldMiddlewareEnforceLogin.spec.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts b/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts index a5ad569460..9578f04abe 100644 --- a/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts +++ b/website/src/utils/shouldMiddlewareEnforceLogin.spec.ts @@ -8,11 +8,11 @@ const configuredOrganisms = [testOrganism, otherOrganism]; describe('shouldMiddlewareEnforceLogin', () => { test('should return false if not specified', () => { - expect(shouldMiddlewareEnforceLogin('/someRoute', [])).toBe(false); + expectNoLogin('/someRoute'); }); test('should return false for empty string', () => { - expect(shouldMiddlewareEnforceLogin('', [])).toBe(false); + expectNoLogin(''); }); test('should return true on routes which should force login', () => { @@ -23,13 +23,17 @@ describe('shouldMiddlewareEnforceLogin', () => { }); test('should return false for various public routes', () => { - expect(shouldMiddlewareEnforceLogin(`/${testOrganism}/search`, configuredOrganisms)).toBe(false); - expect(shouldMiddlewareEnforceLogin(`/`, configuredOrganisms)).toBe(false); - expect(shouldMiddlewareEnforceLogin(`/${testOrganism}`, configuredOrganisms)).toBe(false); - expect(shouldMiddlewareEnforceLogin(`/${testOrganism}/sequences/id_002156`, configuredOrganisms)).toBe(false); + expectNoLogin(`/${testOrganism}/search`); + expectNoLogin(`/`); + expectNoLogin(`/${testOrganism}`); + expectNoLogin(`/${testOrganism}/sequences/id_002156`); }); function expectForceLogin(path: string) { expect(shouldMiddlewareEnforceLogin(path, configuredOrganisms), path).toBe(true); } + + function expectNoLogin(path: string) { + expect(shouldMiddlewareEnforceLogin(path, configuredOrganisms), path).toBe(false); + } });