From 8f0694c6e6295858b1fe7ac731a1ae6013c39fe5 Mon Sep 17 00:00:00 2001 From: fcaps Date: Mon, 27 Nov 2023 18:27:44 +0100 Subject: [PATCH] refactoring auth with auto refresh token and api-client --- config/app.js | 1 + fafApp.js | 48 ++++++++- lib/ApiErrors.js | 1 + lib/JavaApiClient.js | 57 ++++++++++ lib/LeaderboardRepository.js | 19 ++-- lib/LeaderboardServiceFactory.js | 14 --- package.json | 2 + routes/middleware.js | 8 ++ routes/views/auth.js | 45 +------- routes/views/leaderboardRouter.js | 18 ++-- src/frontend/js/entrypoint/leaderboards.js | 5 + tests/JavaApiClient.test.js | 115 +++++++++++++++++++++ yarn.lock | 21 +++- 13 files changed, 284 insertions(+), 70 deletions(-) create mode 100644 lib/ApiErrors.js create mode 100644 lib/JavaApiClient.js delete mode 100644 lib/LeaderboardServiceFactory.js create mode 100644 tests/JavaApiClient.test.js diff --git a/config/app.js b/config/app.js index 89f4b120..6c795552 100644 --- a/config/app.js +++ b/config/app.js @@ -11,6 +11,7 @@ const appConfig = { tokenLifespan: process.env.TOKEN_LIFESPAN || 43200 }, oauth: { + strategy: 'faforever', clientId: process.env.OAUTH_CLIENT_ID || '12345', clientSecret: process.env.OAUTH_CLIENT_SECRET || '12345', url: oauthUrl, diff --git a/fafApp.js b/fafApp.js index 87fd455e..014cb527 100644 --- a/fafApp.js +++ b/fafApp.js @@ -1,4 +1,4 @@ -const appConfig = require("./config/app") +const appConfig = require('./config/app') const express = require('express') const bodyParser = require('body-parser') const session = require('express-session') @@ -15,6 +15,9 @@ const clanRouter = require("./routes/views/clanRouter") const accountRouter = require("./routes/views/accountRouter") const dataRouter = require('./routes/views/dataRouter'); const setupCronJobs = require("./scripts/cron-jobs") +const OidcStrategy = require('passport-openidconnect') +const axios = require('axios') +const refresh = require('passport-oauth2-refresh') const copyFlashHandler = (req, res, next) => { res.locals.message = req.flash(); @@ -33,6 +36,44 @@ const errorHandler = (err, req, res, next) => { res.status(500).render('errors/500'); } +const configureAuth = () => { + passport.serializeUser((user, done) => done(null, user)) + passport.deserializeUser((user, done) => done(null, user)) + + const authStrategy = new OidcStrategy({ + issuer: appConfig.oauth.url + '/', + tokenURL: appConfig.oauth.url + '/oauth2/token', + authorizationURL: appConfig.oauth.publicUrl + '/oauth2/auth', + userInfoURL: appConfig.oauth.url + '/userinfo?schema=openid', + clientID: appConfig.oauth.clientId, + clientSecret: appConfig.oauth.clientSecret, + callbackURL: `${appConfig.host}/${appConfig.oauth.callback}`, + scope: ['openid', 'offline', 'public_profile', 'write_account_data'] + }, function (iss, sub, profile, jwtClaims, accessToken, refreshToken, params, verified) { + axios.get( + appConfig.apiUrl + '/me', + { + headers: { Authorization: `Bearer ${accessToken}` } + }).then((res) => { + const user = res.data + user.token = accessToken + user.refreshToken = refreshToken + user.data.attributes.token = accessToken + user.data.id = user.data.attributes.userId + + return verified(null, user) + }).catch(e => { + console.error('[Error] views/auth.js::passport::verify failed with "' + e.toString() + '"') + + return verified(null, null) + }) + } + ) + + passport.use(appConfig.oauth.strategy, authStrategy) + refresh.use(appConfig.oauth.strategy, authStrategy) +} + module.exports.setupCronJobs = () => { setupCronJobs() } @@ -64,7 +105,6 @@ module.exports.setup = (app) => { app.set('view engine', 'pug') app.set('port', appConfig.expressPort) - app.use(middleware.injectServices) app.use(middleware.initLocals) app.use(express.static('public', { @@ -93,6 +133,10 @@ module.exports.setup = (app) => { })) app.use(passport.initialize()) app.use(passport.session()) + configureAuth() + + app.use(middleware.injectServices) + app.use(flash()) app.use(middleware.username) app.use(middleware.webpackAsset) diff --git a/lib/ApiErrors.js b/lib/ApiErrors.js new file mode 100644 index 00000000..05e64490 --- /dev/null +++ b/lib/ApiErrors.js @@ -0,0 +1 @@ +module.exports.AuthFailed = class AuthFailed extends Error {} diff --git a/lib/JavaApiClient.js b/lib/JavaApiClient.js new file mode 100644 index 00000000..4443244f --- /dev/null +++ b/lib/JavaApiClient.js @@ -0,0 +1,57 @@ +const { Axios } = require('axios') +const refresh = require('passport-oauth2-refresh') +const { AuthFailed } = require('./ApiErrors') +const appConfig = require('../config/app') + +const getRefreshToken = (user) => { + return new Promise((resolve, reject) => { + refresh.requestNewAccessToken(appConfig.oauth.strategy, user.refreshToken, function (err, accessToken, refreshToken) { + if (err || !accessToken || !refreshToken) { + return reject(new AuthFailed('Failed to refresh token')) + } + + return resolve([accessToken, refreshToken]) + }) + }) +} + +module.exports = (javaApiBaseURL, user) => { + let tokenRefreshRunning = null + const client = new Axios({ + baseURL: javaApiBaseURL + }) + + client.interceptors.request.use( + async config => { + config.headers = { + Authorization: `Bearer ${user.token}` + } + + return config + }) + + client.interceptors.response.use(async (res) => { + if (!res.config._refreshTokenRequest && res.config && res.status === 401) { + res.config._refreshTokenRequest = true + + if (!tokenRefreshRunning) { + tokenRefreshRunning = getRefreshToken(user) + } + + const [token, refreshToken] = await tokenRefreshRunning + + user.token = token + user.refreshToken = refreshToken + + return client.request(res.config) + } + + if (res.status === 401) { + throw new AuthFailed('Token no longer valid and refresh did not help') + } + + return res + }) + + return client +} diff --git a/lib/LeaderboardRepository.js b/lib/LeaderboardRepository.js index d8f75f3b..1fb50f1f 100644 --- a/lib/LeaderboardRepository.js +++ b/lib/LeaderboardRepository.js @@ -45,13 +45,18 @@ class LeaderboardRepository { let leaderboardData = [] data.data.forEach((item, index) => { - leaderboardData.push({ - rating: item.attributes.rating, - totalgames: item.attributes.totalGames, - wonGames: item.attributes.wonGames, - date: item.attributes.updateTime, - label: data.included[index].attributes.login, - }) + try { + leaderboardData.push({ + rating: item.attributes.rating, + totalgames: item.attributes.totalGames, + wonGames: item.attributes.wonGames, + date: item.attributes.updateTime, + label: data.included[index]?.attributes.login || 'unknown user', + }) + } catch (e) { + console.error('LeaderboardRepository::mapResponse failed on item with "' + e.toString() + '"') + } + }) return leaderboardData diff --git a/lib/LeaderboardServiceFactory.js b/lib/LeaderboardServiceFactory.js deleted file mode 100644 index 1d9ad1a0..00000000 --- a/lib/LeaderboardServiceFactory.js +++ /dev/null @@ -1,14 +0,0 @@ -const LeaderboardService = require("./LeaderboardService"); -const LeaderboardRepository = require("./LeaderboardRepository"); -const {Axios} = require("axios"); -const cacheService = require('./CacheService') - -module.exports = (javaApiBaseURL, token) => { - const config = { - baseURL: javaApiBaseURL, - headers: {Authorization: `Bearer ${token}`} - }; - const javaApiClient = new Axios(config) - - return new LeaderboardService(cacheService, new LeaderboardRepository(javaApiClient)) -} diff --git a/package.json b/package.json index bb50b868..6a4e7259 100644 --- a/package.json +++ b/package.json @@ -16,6 +16,7 @@ "node-fetch": "^2.6.7", "npm-check": "^6.0.1", "passport": "^0.6.0", + "passport-oauth2-refresh": "^2.2.0", "passport-openidconnect": "^0.1.1", "pug": "3.0.2", "request": "2.88.2", @@ -42,6 +43,7 @@ "jest": "^29.7.0", "load-grunt-config": "4.0.1", "load-grunt-tasks": "5.1.0", + "nock": "^13.3.8", "octokit": "^3.1.2", "supertest": "^6.3.3", "webpack": "^5.89.0", diff --git a/routes/middleware.js b/routes/middleware.js index 5db9fa2d..a07e39e3 100755 --- a/routes/middleware.js +++ b/routes/middleware.js @@ -1,4 +1,7 @@ const WordpressServiceFactory = require("../lib/WordpressServiceFactory"); +const JavaApiClientFactory = require('../lib/JavaApiClient') +const LeaderboardService = require('../lib/LeaderboardService') +const cacheService = require('../lib/CacheService') const appConfig = require("../config/app"); const wordpressService = WordpressServiceFactory(appConfig.wordpressUrl) const fs = require('fs'); @@ -58,5 +61,10 @@ exports.injectServices = function(req, res, next) { wordpressService: wordpressService } + if (req.isAuthenticated()) { + req.services.javaApiClient = JavaApiClientFactory(appConfig.apiUrl, req.user) + req.services.leaderboardService = new LeaderboardService(cacheService, new LeaderboardRepository(req.services.javaApiClient)) + } + next() } diff --git a/routes/views/auth.js b/routes/views/auth.js index 75a7f1bf..7c5aff04 100644 --- a/routes/views/auth.js +++ b/routes/views/auth.js @@ -1,44 +1,9 @@ const appConfig = require('../../config/app') -const passport = require('passport'); -const OidcStrategy = require('passport-openidconnect'); -const express = require("express"); -const axios = require("axios"); -const router = express.Router(); +const passport = require('passport') +const express = require("express") +const router = express.Router() -passport.serializeUser((user, done) => done(null, user)) -passport.deserializeUser((user, done) => done(null, user)) - -passport.use('faforever', new OidcStrategy({ - issuer: appConfig.oauth.url + '/', - tokenURL: appConfig.oauth.url + '/oauth2/token', - authorizationURL: appConfig.oauth.publicUrl + '/oauth2/auth', - userInfoURL: appConfig.oauth.url + '/userinfo?schema=openid', - clientID: appConfig.oauth.clientId, - clientSecret: appConfig.oauth.clientSecret, - callbackURL: `${appConfig.host}/${appConfig.oauth.callback}`, - scope: ['openid', 'public_profile', 'write_account_data'] - }, function (iss, sub, profile, jwtClaims, accessToken, refreshToken, params, verified) { - - axios.get( - appConfig.apiUrl + '/me', - { - headers: {'Authorization': `Bearer ${accessToken}`} - }).then((res) => { - const user = res.data - user.token = accessToken - user.data.attributes.token = accessToken; - user.data.id = user.data.attributes.userId; - - return verified(null, user); - }).catch(e => { - console.error('[Error] views/auth.js::passport::verify failed with "' + e.toString() + '"'); - - return verified(null, null); - }); - } -)); - -router.get('/login', passport.authenticate('faforever')); +router.get('/login', passport.authenticate(appConfig.oauth.strategy)); router.get( '/' + appConfig.oauth.callback, @@ -47,7 +12,7 @@ router.get( return next() }, - passport.authenticate('faforever', {failureRedirect: '/login', failureFlash: true}), + passport.authenticate(appConfig.oauth.strategy, {failureRedirect: '/login', failureFlash: true}), (req, res) => { res.redirect(res.locals.returnTo || '/') } diff --git a/routes/views/leaderboardRouter.js b/routes/views/leaderboardRouter.js index 0f67fa5a..0a667ff0 100644 --- a/routes/views/leaderboardRouter.js +++ b/routes/views/leaderboardRouter.js @@ -1,9 +1,8 @@ -const appConfig = require('../../config/app') const express = require('express'); const router = express.Router(); -const LeaderboardServiceFactory = require('../../lib/LeaderboardServiceFactory') const {AcquireTimeoutError} = require('../../lib/MutexService'); const middlewares = require('../middleware') +const {AuthFailed} = require('../../lib/ApiErrors') const getLeaderboardId = (leaderboardName) => { @@ -33,14 +32,21 @@ router.get('/:leaderboard.json', middlewares.isAuthenticated(null, true), async return res.status(404).json({error: 'Leaderboard "' + req.params.leaderboard + '" does not exist'}) } - const token = req.user.data.attributes.token - const leaderboardService = LeaderboardServiceFactory(appConfig.apiUrl, token) - - return res.json(await leaderboardService.getLeaderboard(leaderboardId)) + return res.json(await req.services.leaderboardService.getLeaderboard(leaderboardId)) } catch (e) { if (e instanceof AcquireTimeoutError) { return res.status(503).json({error: 'timeout reached'}) } + + if (e instanceof AuthFailed) { + req.logout(function(err) { + if (err) { + throw err + } + }) + + return res.status(400).json({error: 'authentication failed, reload site'}) + } console.error('[error] leaderboardRouter::get:leaderboard.json failed with "' + e.toString() + '"') diff --git a/src/frontend/js/entrypoint/leaderboards.js b/src/frontend/js/entrypoint/leaderboards.js index 180e9f0a..d98100f6 100644 --- a/src/frontend/js/entrypoint/leaderboards.js +++ b/src/frontend/js/entrypoint/leaderboards.js @@ -17,6 +17,11 @@ let currentDate = new Date(minusTimeFilter).toISOString(); async function leaderboardOneJSON(leaderboardFile) { //Check which category is active const response = await fetch(`leaderboards/${leaderboardFile}.json`); + + if (response.status === 400) { + window.location.href = '/leaderboards' + } + currentLeaderboard = leaderboardFile; const data = await response.json(); return await data; diff --git a/tests/JavaApiClient.test.js b/tests/JavaApiClient.test.js new file mode 100644 index 00000000..1a6c9d26 --- /dev/null +++ b/tests/JavaApiClient.test.js @@ -0,0 +1,115 @@ +const JavaApiClientFactory = require('../lib/JavaApiClient') +const appConfig = require('../config/app') +const refresh = require('passport-oauth2-refresh') +const OidcStrategy = require('passport-openidconnect') +const nock = require('nock') +const { AuthFailed } = require('../lib/ApiErrors') + +beforeEach(() => { + refresh.use(appConfig.oauth.strategy, new OidcStrategy({ + issuer: 'me', + tokenURL: 'http://auth-localhost/oauth2/token', + authorizationURL: 'http://auth-localhost/oauth2/auth', + clientID: 'test', + clientSecret: 'test', + scope: ['openid', 'offline'] + }, () => {})) +}) + +afterEach(() => { + refresh._strategies = {} + jest.restoreAllMocks() +}) + +test('multiple calls with stale token will trigger refresh only once', async () => { + const client = JavaApiClientFactory('http://api-localhost', { + token: '123', + refreshToken: '456' + }) + + const refreshSpy = jest.spyOn(refresh, 'requestNewAccessToken') + const apiScope = nock('http://api-localhost') + .get('/example') + .times(2) + .reply(401, 'nope') + .get('/example') + .times(2) + .reply(200, 'OK') + + const authScope = nock('http://auth-localhost') + .post('/oauth2/token') + .times(1) + .reply(200, { access_token: 'new_tok', refresh_token: 'new_ref' }) + + const response = client.get('/example').then((res) => { + expect(res.request.headers.authorization).toBe('Bearer new_tok') + }) + + const response2 = client.get('/example').then((res) => { + expect(res.request.headers.authorization).toBe('Bearer new_tok') + }) + + await Promise.all([response, response2]) + + expect(refreshSpy).toBeCalledTimes(1) + + apiScope.done() + authScope.done() +}) + +test('refresh will throw on error', async () => { + const client = JavaApiClientFactory('http://api-localhost', { + token: '123', + refreshToken: '456' + }) + + const refreshSpy = jest.spyOn(refresh, 'requestNewAccessToken') + const apiScope = nock('http://api-localhost') + .get('/example') + .reply(401, 'nope') + + const authScope = nock('http://auth-localhost') + .post('/oauth2/token') + .times(1) + .reply(400, null) + + let thrown = false + try { + await client.get('/example') + } catch (e) { + expect(e).toBeInstanceOf(AuthFailed) + thrown = true + } + + expect(thrown).toBe(true) + expect(refreshSpy).toBeCalledTimes(1) + + apiScope.done() + authScope.done() +}) + +test('refresh will not loop to death', async () => { + const client = JavaApiClientFactory('http://api-localhost', { + token: '123', + refreshToken: '456' + }) + + const apiScope = nock('http://api-localhost') + .get('/example') + .times(2) + .reply(401, 'nope') + + const authScope = nock('http://auth-localhost') + .post('/oauth2/token') + .times(1) + .reply(200, { access_token: 'new_tok', refresh_token: 'new_ref' }) + + try { + await client.get('/example') + } catch (e) { + expect(e).toBeInstanceOf(AuthFailed) + } + + apiScope.done() + authScope.done() +}) diff --git a/yarn.lock b/yarn.lock index d89a1f37..67f352e2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5271,7 +5271,7 @@ json-stable-stringify-without-jsonify@^1.0.1: resolved "https://registry.yarnpkg.com/json-stable-stringify-without-jsonify/-/json-stable-stringify-without-jsonify-1.0.1.tgz#9db7b59496ad3f3cfef30a75142d2d930ad72651" integrity sha512-Bdboy+l7tA3OGW6FjyFHWkP5LuByj1Tk33Ljyq0axyzdk9//JSi2u3fP1QSmd1KNwq6VOKYGlAu87CisVir6Pw== -json-stringify-safe@~5.0.1: +json-stringify-safe@^5.0.1, json-stringify-safe@~5.0.1: version "5.0.1" resolved "https://registry.yarnpkg.com/json-stringify-safe/-/json-stringify-safe-5.0.1.tgz#1296a2d58fd45f19a0f6ce01d65701e2c735b6eb" integrity sha512-ZClg6AaYvamvYEE82d3Iyd3vSSIjQ+odgjaTzRuO3s7toCdFKczob2i0zCh7JE8kWn17yvAWhUVxvqGwUalsRA== @@ -5912,6 +5912,15 @@ neo-async@^2.6.2: resolved "https://registry.yarnpkg.com/neo-async/-/neo-async-2.6.2.tgz#b4aafb93e3aeb2d8174ca53cf163ab7d7308305f" integrity sha512-Yd3UES5mWCSqR+qNT93S3UoYUkqAZ9lLg8a7g9rimsWmYGK8cVToA4/sF3RrshdyV3sAGMXVUmpMYOw+dLpOuw== +nock@^13.3.8: + version "13.3.8" + resolved "https://registry.yarnpkg.com/nock/-/nock-13.3.8.tgz#7adf3c66f678b02ef0a78d5697ae8bc2ebde0142" + integrity sha512-96yVFal0c/W1lG7mmfRe7eO+hovrhJYd2obzzOZ90f6fjpeU/XNvd9cYHZKZAQJumDfhXgoTpkpJ9pvMj+hqHw== + dependencies: + debug "^4.1.0" + json-stringify-safe "^5.0.1" + propagate "^2.0.0" + node-cache@^5.1.2: version "5.1.2" resolved "https://registry.yarnpkg.com/node-cache/-/node-cache-5.1.2.tgz#f264dc2ccad0a780e76253a694e9fd0ed19c398d" @@ -6376,6 +6385,11 @@ pascalcase@^0.1.1: resolved "https://registry.yarnpkg.com/pascalcase/-/pascalcase-0.1.1.tgz#b363e55e8006ca6fe21784d2db22bd15d7917f14" integrity sha512-XHXfu/yOQRy9vYOtUDVMN60OEJjW013GoObG1o+xwQTpB9eYJX/BjXMsdW13ZDPruFhYYn0AG22w0xgQMwl3Nw== +passport-oauth2-refresh@^2.2.0: + version "2.2.0" + resolved "https://registry.yarnpkg.com/passport-oauth2-refresh/-/passport-oauth2-refresh-2.2.0.tgz#e60dd4e84e8df3c6ead87b6aab0754dec7a89aca" + integrity sha512-yXwXHL7ZZH0s2oknnjugfvwzCB5mpJ5ZNpzkb+b/sTsHeZFbx2BXfzvwsoD4aq6gq/aWuCxBV89ef+L/cjjrjg== + passport-openidconnect@^0.1.1: version "0.1.1" resolved "https://registry.yarnpkg.com/passport-openidconnect/-/passport-openidconnect-0.1.1.tgz#83921ff5f87f634079f65262dada834af1972244" @@ -6661,6 +6675,11 @@ prompts@^2.0.1: kleur "^3.0.3" sisteransi "^1.0.5" +propagate@^2.0.0: + version "2.0.1" + resolved "https://registry.yarnpkg.com/propagate/-/propagate-2.0.1.tgz#40cdedab18085c792334e64f0ac17256d38f9a45" + integrity sha512-vGrhOavPSTz4QVNuBNdcNXePNdNMaO1xj9yBeH1ScQPjk/rhg9sSlCXPhMkFuaNNW/syTvYqsnbIJxMBfRbbag== + proxy-addr@~2.0.7: version "2.0.7" resolved "https://registry.yarnpkg.com/proxy-addr/-/proxy-addr-2.0.7.tgz#f19fe69ceab311eeb94b42e70e8c2070f9ba1025"