From 95c83866c5a1d2a2e7535a5e1bd1e02ce2ff2999 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Mon, 31 May 2021 10:30:04 +0200 Subject: [PATCH] Merge pull request #4112 from overleaf/tm-private-api-basic-auth Add requireBasicAuth middleware and refactor httpAuth to use it GitOrigin-RevId: 7f68c0dc4a40102bfe4a97711def517e465ec7fd --- .../src/Features/Analytics/AnalyticsRouter.js | 2 +- .../AuthenticationController.js | 30 +++++++----- .../app/src/Features/Editor/EditorRouter.js | 4 +- .../Subscription/SubscriptionRouter.js | 2 +- services/web/app/src/router.js | 46 +++++++++--------- .../AuthenticationControllerTests.js | 47 ++++++++++++++----- 6 files changed, 81 insertions(+), 50 deletions(-) diff --git a/services/web/app/src/Features/Analytics/AnalyticsRouter.js b/services/web/app/src/Features/Analytics/AnalyticsRouter.js index dd8953977a..b4a8af81b9 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsRouter.js +++ b/services/web/app/src/Features/Analytics/AnalyticsRouter.js @@ -16,7 +16,7 @@ module.exports = { publicApiRouter.use( '/analytics/uniExternalCollaboration', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), AnalyticsProxy.call('/uniExternalCollaboration') ) }, diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 8dfc47e1e0..fe2edd9119 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -316,7 +316,7 @@ const AuthenticationController = { } if (req.headers.authorization != null) { - AuthenticationController.httpAuth(req, res, next) + AuthenticationController.requirePrivateApiAuth()(req, res, next) } else if (AuthenticationController.isUserLoggedIn(req)) { next() } else { @@ -361,17 +361,23 @@ const AuthenticationController = { return next() }, - httpAuth: basicAuth(function (user, pass) { - const expectedPassword = Settings.httpAuthUsers[user] - const isValid = - expectedPassword && - expectedPassword.length === pass.length && - crypto.timingSafeEqual(Buffer.from(expectedPassword), Buffer.from(pass)) - if (!isValid) { - logger.err({ user, pass }, 'invalid login details') - } - return isValid - }), + requireBasicAuth: function (userDetails) { + return basicAuth(function (user, pass) { + const expectedPassword = userDetails[user] + const isValid = + expectedPassword && + expectedPassword.length === pass.length && + crypto.timingSafeEqual(Buffer.from(expectedPassword), Buffer.from(pass)) + if (!isValid) { + logger.err({ user, pass }, 'invalid login details') + } + return isValid + }) + }, + + requirePrivateApiAuth() { + return AuthenticationController.requireBasicAuth(Settings.httpAuthUsers) + }, setRedirectInSession(req, value) { if (value == null) { diff --git a/services/web/app/src/Features/Editor/EditorRouter.js b/services/web/app/src/Features/Editor/EditorRouter.js index e5113c5b28..35e012e4aa 100644 --- a/services/web/app/src/Features/Editor/EditorRouter.js +++ b/services/web/app/src/Features/Editor/EditorRouter.js @@ -57,7 +57,7 @@ module.exports = { ) apiRouter.post( '/project/:Project_id/doc/:entity_id/convert-to-file', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), validate({ body: Joi.object({ userId: Joi.objectId().required(), @@ -71,7 +71,7 @@ module.exports = { // whenever a user joins a project, like updating the deleted status. apiRouter.post( '/project/:Project_id/join', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), RateLimiterMiddleware.rateLimit({ endpointName: 'join-project', params: ['Project_id'], diff --git a/services/web/app/src/Features/Subscription/SubscriptionRouter.js b/services/web/app/src/Features/Subscription/SubscriptionRouter.js index 7a683cade9..16244cc2b7 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionRouter.js +++ b/services/web/app/src/Features/Subscription/SubscriptionRouter.js @@ -136,7 +136,7 @@ module.exports = { // Currently used in acceptance tests only, as a way to trigger the syncing logic return publicApiRouter.post( '/user/:user_id/features/sync', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), SubscriptionController.refreshUserFeatures ) }, diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index 83ee901d9e..690795c16d 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -231,7 +231,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { ) privateApiRouter.get( '/user/:user_id/personal_info', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), UserInfoController.getPersonalInfo ) @@ -564,7 +564,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { ) privateApiRouter.post( '/project/:Project_id/history/resync', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), HistoryController.resyncProjectHistory ) @@ -639,28 +639,28 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { ) privateApiRouter.post( '/internal/expire-deleted-projects-after-duration', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), ProjectController.expireDeletedProjectsAfterDuration ) privateApiRouter.post( '/internal/expire-deleted-users-after-duration', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), UserController.expireDeletedUsersAfterDuration ) privateApiRouter.post( '/internal/project/:projectId/expire-deleted-project', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), ProjectController.expireDeletedProject ) privateApiRouter.post( '/internal/users/:userId/expire', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), UserController.expireDeletedUser ) privateApiRouter.get( '/user/:userId/tag', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), TagsController.apiGetAllTags ) webRouter.get( @@ -733,35 +733,35 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { // Deprecated in favour of /internal/project/:project_id but still used by versioning privateApiRouter.get( '/project/:project_id/details', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), ProjectApiController.getProjectDetails ) // New 'stable' /internal API end points privateApiRouter.get( '/internal/project/:project_id', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), ProjectApiController.getProjectDetails ) privateApiRouter.get( '/internal/project/:Project_id/zip', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), ProjectDownloadsController.downloadProject ) privateApiRouter.get( '/internal/project/:project_id/compile/pdf', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), CompileController.compileAndDownloadPdf ) privateApiRouter.post( '/internal/deactivateOldProjects', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), InactiveProjectController.deactivateOldProjects ) privateApiRouter.post( '/internal/project/:project_id/deactivate', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), InactiveProjectController.deactivateProject ) @@ -775,40 +775,40 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { req.params = params next() }, - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), CompileController.getFileFromClsi ) privateApiRouter.get( '/project/:Project_id/doc/:doc_id', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), DocumentController.getDocument ) privateApiRouter.post( '/project/:Project_id/doc/:doc_id', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), DocumentController.setDocument ) privateApiRouter.post( '/user/:user_id/update/*', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), TpdsController.mergeUpdate ) privateApiRouter.delete( '/user/:user_id/update/*', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), TpdsController.deleteUpdate ) privateApiRouter.post( '/project/:project_id/contents/*', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), TpdsController.updateProjectContents ) privateApiRouter.delete( '/project/:project_id/contents/*', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), TpdsController.deleteProjectContents ) @@ -884,7 +884,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { // overleaf-integration module), but may expand beyond that role. publicApiRouter.post( '/api/clsi/compile/:submission_id', - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), CompileController.compileSubmission ) publicApiRouter.get( @@ -898,7 +898,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { req.params = params next() }, - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), CompileController.getFileFromClsiWithoutUser ) publicApiRouter.post( @@ -908,7 +908,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { maxRequests: 1, timeInterval: 60, }), - AuthenticationController.httpAuth, + AuthenticationController.requirePrivateApiAuth(), InstitutionsController.confirmDomain ) diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index e261c7f6c1..bb8009d43c 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -630,7 +630,10 @@ describe('AuthenticationController', function () { describe('requireGlobalLogin', function () { beforeEach(function () { this.req.headers = {} - this.AuthenticationController.httpAuth = sinon.stub() + this.middleware = sinon.stub() + this.AuthenticationController.requirePrivateApiAuth = sinon + .stub() + .returns(this.middleware) this.setRedirect = sinon.spy( this.AuthenticationController, 'setRedirectInSession' @@ -684,8 +687,8 @@ describe('AuthenticationController', function () { ) }) - it('should pass the request onto httpAuth', function () { - this.AuthenticationController.httpAuth + it('should pass the request onto requirePrivateApiAuth middleware', function () { + this.middleware .calledWith(this.req, this.res, this.next) .should.equal(true) }) @@ -726,7 +729,16 @@ describe('AuthenticationController', function () { }) }) - describe('httpAuth', function () { + describe('requireBasicAuth', function () { + beforeEach(function () { + this.basicAuthUsers = { + 'basic-auth-user': 'basic-auth-password', + } + this.middleware = this.AuthenticationController.requireBasicAuth( + this.basicAuthUsers + ) + }) + describe('with http auth', function () { it('should error with incorrect user', function (done) { this.req.headers = { @@ -736,12 +748,12 @@ describe('AuthenticationController', function () { expect(status).to.equal('Unauthorized') done() } - this.AuthenticationController.httpAuth(this.req, this.req) + this.middleware(this.req, this.req) }) it('should error with incorrect password', function (done) { this.req.headers = { - authorization: `Basic ${Buffer.from('valid-test-user:nope').toString( + authorization: `Basic ${Buffer.from('basic-auth-user:nope').toString( 'base64' )}`, } @@ -749,12 +761,12 @@ describe('AuthenticationController', function () { expect(status).to.equal('Unauthorized') done() } - this.AuthenticationController.httpAuth(this.req, this.req) + this.middleware(this.req, this.req) }) it('should fail with empty pass', function (done) { this.req.headers = { - authorization: `Basic ${Buffer.from(`invalid-test-user:`).toString( + authorization: `Basic ${Buffer.from(`basic-auth-user:`).toString( 'base64' )}`, } @@ -762,20 +774,33 @@ describe('AuthenticationController', function () { expect(status).to.equal('Unauthorized') done() } - this.AuthenticationController.httpAuth(this.req, this.req) + this.middleware(this.req, this.req) }) it('should succeed with correct user/pass', function (done) { this.req.headers = { authorization: `Basic ${Buffer.from( - `valid-test-user:${this.httpAuthUsers['valid-test-user']}` + `basic-auth-user:${this.basicAuthUsers['basic-auth-user']}` ).toString('base64')}`, } - this.AuthenticationController.httpAuth(this.req, this.res, done) + this.middleware(this.req, this.res, done) }) }) }) + describe('requirePrivateApiAuth', function () { + beforeEach(function () { + this.AuthenticationController.requireBasicAuth = sinon.stub() + }) + + it('should call requireBasicAuth with the private API user details', function () { + this.AuthenticationController.requirePrivateApiAuth() + this.AuthenticationController.requireBasicAuth + .calledWith(this.httpAuthUsers) + .should.equal(true) + }) + }) + describe('_redirectToLoginOrRegisterPage', function () { beforeEach(function () { this.middleware = this.AuthenticationController.requireLogin(