From e0ce988d32761b35d8a18d89c7d90807a167d1c9 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Mon, 10 Sep 2018 17:21:58 +0100 Subject: [PATCH 1/8] Intelligently redirect to v1 if no v2 project found for token --- .../Features/TokenAccess/TokenAccessController.coffee | 10 ++++++++++ services/web/app/coffee/router.coffee | 3 ++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee index b6b65cc7a7..395489c608 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee @@ -3,9 +3,19 @@ AuthenticationController = require '../Authentication/AuthenticationController' TokenAccessHandler = require './TokenAccessHandler' Errors = require '../Errors/Errors' logger = require 'logger-sharelatex' +settings = require 'settings-sharelatex' module.exports = TokenAccessController = + redirectNotFoundErrorToV1: (err, req, res, next) -> + if err instanceof Errors.NotFoundError and settings.overleaf + logger.log { + token: req.params['read_and_write_token'] + }, "[TokenAccess] No project found for token, redirecting to v1" + res.redirect(settings.overleaf.host + req.url) + else + next(err) + _loadEditor: (projectId, req, res, next) -> req.params.Project_id = projectId.toString() return ProjectController.loadEditor(req, res, next) diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index b196c56870..1fc42c354a 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -426,6 +426,7 @@ module.exports = class Router maxRequests: 10, timeInterval: 60 }), - TokenAccessController.readAndWriteToken + TokenAccessController.readAndWriteToken, + TokenAccessController.redirectNotFoundErrorToV1 webRouter.get '*', ErrorController.notFound From 24495f33405112e77fe418a35b35620653bddd56 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 11 Sep 2018 09:57:51 +0100 Subject: [PATCH 2/8] Also redirect not found read tokens to v1 --- services/web/app/coffee/router.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 1fc42c354a..35de9f804f 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -418,7 +418,8 @@ module.exports = class Router maxRequests: 10, timeInterval: 60 }), - TokenAccessController.readOnlyToken + TokenAccessController.readOnlyToken, + TokenAccessController.redirectNotFoundErrorToV1 webRouter.get '/:read_and_write_token([0-9]+[a-z]+)', RateLimiterMiddlewear.rateLimit({ From cf8ae7c28c304715068040ee5f31399be59cdf69 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 11 Sep 2018 16:45:39 +0100 Subject: [PATCH 3/8] Add test for redirecting to v1 if project unimported --- .../web/test/acceptance/coffee/TokenAccessTests.coffee | 7 +++++++ services/web/test/acceptance/config/settings.test.coffee | 3 +++ 2 files changed, 10 insertions(+) diff --git a/services/web/test/acceptance/coffee/TokenAccessTests.coffee b/services/web/test/acceptance/coffee/TokenAccessTests.coffee index 0e2985f75a..d54f19d0eb 100644 --- a/services/web/test/acceptance/coffee/TokenAccessTests.coffee +++ b/services/web/test/acceptance/coffee/TokenAccessTests.coffee @@ -415,3 +415,10 @@ describe 'TokenAccess', -> try_content_access(@other2, @project_id, (response, body) => expect(body.privilegeLevel).to.equal false , done) + + describe 'unimported v1 project', -> + it 'should redirect to v1', (done) -> + unimportedV1Token = '123abc' + try_read_and_write_token_access(@owner, unimportedV1Token, (response, body) => + expect(response.statusCode).to.equal 302 + , done) diff --git a/services/web/test/acceptance/config/settings.test.coffee b/services/web/test/acceptance/config/settings.test.coffee index 4dd820f4a7..4d1fcabe34 100644 --- a/services/web/test/acceptance/config/settings.test.coffee +++ b/services/web/test/acceptance/config/settings.test.coffee @@ -105,3 +105,6 @@ module.exports = path: (params) -> "/universities/list/#{params.id}" '/institutions/domains': { baseUrl: v1Api.url, path: '/university/domains' } '/proxy/missing/baseUrl': path: '/foo/bar' + + overleaf: + host: "http://#{process.env['V1_HOST']}:5000" From 9d600afdf83c5a0eda098ef65587d0c1c1e6ae42 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Wed, 12 Sep 2018 11:06:05 +0100 Subject: [PATCH 4/8] Fix failing tests for token access If project was changed from token access to private, then we want to 404 on v2 (not redirect to v1). So the logic was changed to check if the project exists and if it does then a 404 is returned. If it does not then it redirects to v1. --- services/web/app/coffee/Features/Errors/Errors.coffee | 8 ++++++++ .../Features/TokenAccess/TokenAccessController.coffee | 8 ++++++-- .../Features/TokenAccess/TokenAccessHandler.coffee | 10 +++++++--- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/services/web/app/coffee/Features/Errors/Errors.coffee b/services/web/app/coffee/Features/Errors/Errors.coffee index 94aeaa2a90..720b092012 100644 --- a/services/web/app/coffee/Features/Errors/Errors.coffee +++ b/services/web/app/coffee/Features/Errors/Errors.coffee @@ -82,6 +82,13 @@ EmailExistsError = (message) -> return error EmailExistsError.prototype.__proto__ = Error.prototype +ProjectNotTokenAccessError = (message) -> + error = new Error(message) + error.name = "ProjectNotTokenAccessError" + error.__proto__ = ProjectNotTokenAccessError.prototype + return error +ProjectNotTokenAccessError.prototype.__proto__ = Error.prototype + module.exports = Errors = NotFoundError: NotFoundError ServiceNotConfiguredError: ServiceNotConfiguredError @@ -95,3 +102,4 @@ module.exports = Errors = V1ConnectionError: V1ConnectionError UnconfirmedEmailError: UnconfirmedEmailError EmailExistsError: EmailExistsError + ProjectNotTokenAccessError: ProjectNotTokenAccessError diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee index 395489c608..6b03e6c600 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee @@ -8,7 +8,7 @@ settings = require 'settings-sharelatex' module.exports = TokenAccessController = redirectNotFoundErrorToV1: (err, req, res, next) -> - if err instanceof Errors.NotFoundError and settings.overleaf + if err instanceof Errors.ProjectNotTokenAccessError and settings.overleaf logger.log { token: req.params['read_and_write_token'] }, "[TokenAccess] No project found for token, redirecting to v1" @@ -21,11 +21,15 @@ module.exports = TokenAccessController = return ProjectController.loadEditor(req, res, next) _tryHigherAccess: (token, userId, req, res, next) -> - TokenAccessHandler.findProjectWithHigherAccess token, userId, (err, project) -> + TokenAccessHandler.findProjectWithHigherAccess token, userId, (err, project, projectExists) -> if err? logger.err {err, token, userId}, "[TokenAccess] error finding project with higher access" return next(err) + if !projectExists + logger.log {token, userId}, + "[TokenAccess] no project found for this token" + return next(new Errors.ProjectNotTokenAccessError()) if !project? logger.log {token, userId}, "[TokenAccess] no project with higher access found for this user and token" diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee index 9eb792e55b..ed7f51f0d7 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee @@ -22,7 +22,7 @@ module.exports = TokenAccessHandler = 'publicAccesLevel': PublicAccessLevels.TOKEN_BASED }, {_id: 1, publicAccesLevel: 1, owner_ref: 1}, callback - findProjectWithHigherAccess: (token, userId, callback=(err, project)->) -> + findProjectWithHigherAccess: (token, userId, callback=(err, project, projectExists)->) -> Project.findOne { $or: [ {'tokens.readAndWrite': token}, @@ -32,12 +32,16 @@ module.exports = TokenAccessHandler = if err? return callback(err) if !project? - return callback(null, null) + return callback(null, null, false) # Project doesn't exist, so we handle differently projectId = project._id CollaboratorsHandler.isUserInvitedMemberOfProject userId, projectId, (err, isMember) -> if err? return callback(err) - callback(null, if isMember == true then project else null) + callback( + null, + if isMember == true then project else null, + true # Project does exist, but user doesn't have access + ) addReadOnlyUserToProject: (userId, projectId, callback=(err)->) -> userId = ObjectId(userId.toString()) From 893e2dd2350b236f4a0a09c95c053bcfaf348efd Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Wed, 12 Sep 2018 11:08:28 +0100 Subject: [PATCH 5/8] Add test for location of redirect to v1 --- services/web/test/acceptance/coffee/TokenAccessTests.coffee | 3 +++ services/web/test/acceptance/config/settings.test.coffee | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/services/web/test/acceptance/coffee/TokenAccessTests.coffee b/services/web/test/acceptance/coffee/TokenAccessTests.coffee index d54f19d0eb..b0fbd7a0a1 100644 --- a/services/web/test/acceptance/coffee/TokenAccessTests.coffee +++ b/services/web/test/acceptance/coffee/TokenAccessTests.coffee @@ -421,4 +421,7 @@ describe 'TokenAccess', -> unimportedV1Token = '123abc' try_read_and_write_token_access(@owner, unimportedV1Token, (response, body) => expect(response.statusCode).to.equal 302 + expect(response.headers.location).to.equal( + 'http://overleaf.test:5000/123abc' + ) , done) diff --git a/services/web/test/acceptance/config/settings.test.coffee b/services/web/test/acceptance/config/settings.test.coffee index 4d1fcabe34..1ca1b55e48 100644 --- a/services/web/test/acceptance/config/settings.test.coffee +++ b/services/web/test/acceptance/config/settings.test.coffee @@ -107,4 +107,4 @@ module.exports = '/proxy/missing/baseUrl': path: '/foo/bar' overleaf: - host: "http://#{process.env['V1_HOST']}:5000" + host: "http://overleaf.test:5000" From 0c658127ef944348acc8deb0a13b4d2e17bbb354 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Wed, 12 Sep 2018 12:48:13 +0100 Subject: [PATCH 6/8] Add tests for ProjectNotTokenAccessError --- .../TokenAccessControllerTests.coffee | 44 ++++++++++++++++++- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee index 747822b896..0fe8ab358a 100644 --- a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee +++ b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee @@ -231,6 +231,26 @@ describe "TokenAccessController", -> @AuthenticationController.getLoggedInUserId = sinon.stub().returns(@userId.toString()) + describe 'when project does not exist', -> + beforeEach -> + @req = new MockRequest() + @res = new MockResponse() + @res.redirect = sinon.stub() + @next = sinon.stub() + @req.params['read_and_write_token'] = '123abc' + @TokenAccessHandler.findProjectWithReadAndWriteToken = sinon.stub() + .callsArgWith(1, null, null) + @TokenAccessHandler.findProjectWithHigherAccess = + sinon.stub() + .callsArgWith(2, null, @project, false) + @TokenAccessController.readAndWriteToken @req, @res, @next + + it 'should return a ProjectNotTokenAccessError', (done) -> + expect(@next.callCount).to.equal 1 + expect(@next.firstCall.args[0].name) + .to.equal 'ProjectNotTokenAccessError' + done() + describe 'when token access is off, but user has higher access anyway', -> beforeEach -> @req = new MockRequest() @@ -242,7 +262,7 @@ describe "TokenAccessController", -> .callsArgWith(1, null, null) @TokenAccessHandler.findProjectWithHigherAccess = sinon.stub() - .callsArgWith(2, null, @project) + .callsArgWith(2, null, @project, true) @TokenAccessHandler.addReadAndWriteUserToProject = sinon.stub() .callsArgWith(2, null) @ProjectController.loadEditor = sinon.stub() @@ -479,6 +499,26 @@ describe "TokenAccessController", -> describe 'when findProject does not find a project', -> beforeEach -> + describe 'when project does not exist', -> + beforeEach -> + @req = new MockRequest() + @res = new MockResponse() + @res.redirect = sinon.stub() + @next = sinon.stub() + @req.params['read_and_write_token'] = '123abc' + @TokenAccessHandler.findProjectWithReadOnlyToken = sinon.stub() + .callsArgWith(1, null, null) + @TokenAccessHandler.findProjectWithHigherAccess = + sinon.stub() + .callsArgWith(2, null, @project, false) + @TokenAccessController.readOnlyToken @req, @res, @next + + it 'should return a ProjectNotTokenAccessError', (done) -> + expect(@next.callCount).to.equal 1 + expect(@next.firstCall.args[0].name) + .to.equal 'ProjectNotTokenAccessError' + done() + describe 'when token access is off, but user has higher access anyway', -> beforeEach -> @req = new MockRequest() @@ -490,7 +530,7 @@ describe "TokenAccessController", -> .callsArgWith(1, null, null) @TokenAccessHandler.findProjectWithHigherAccess = sinon.stub() - .callsArgWith(2, null, @project) + .callsArgWith(2, null, @project, true) @TokenAccessHandler.addReadAndWriteUserToProject = sinon.stub() .callsArgWith(2, null) @ProjectController.loadEditor = sinon.stub() From 8a969d1c25c8ee67b6a1172c84c668a58afacf96 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Wed, 12 Sep 2018 18:31:08 +0100 Subject: [PATCH 7/8] Redirect directly from controller instead of via handler --- .../app/coffee/Features/Errors/Errors.coffee | 8 ------- .../TokenAccess/TokenAccessController.coffee | 12 ++-------- services/web/app/coffee/router.coffee | 6 ++--- .../TokenAccessControllerTests.coffee | 24 ++++++++++++------- 4 files changed, 19 insertions(+), 31 deletions(-) diff --git a/services/web/app/coffee/Features/Errors/Errors.coffee b/services/web/app/coffee/Features/Errors/Errors.coffee index 720b092012..94aeaa2a90 100644 --- a/services/web/app/coffee/Features/Errors/Errors.coffee +++ b/services/web/app/coffee/Features/Errors/Errors.coffee @@ -82,13 +82,6 @@ EmailExistsError = (message) -> return error EmailExistsError.prototype.__proto__ = Error.prototype -ProjectNotTokenAccessError = (message) -> - error = new Error(message) - error.name = "ProjectNotTokenAccessError" - error.__proto__ = ProjectNotTokenAccessError.prototype - return error -ProjectNotTokenAccessError.prototype.__proto__ = Error.prototype - module.exports = Errors = NotFoundError: NotFoundError ServiceNotConfiguredError: ServiceNotConfiguredError @@ -102,4 +95,3 @@ module.exports = Errors = V1ConnectionError: V1ConnectionError UnconfirmedEmailError: UnconfirmedEmailError EmailExistsError: EmailExistsError - ProjectNotTokenAccessError: ProjectNotTokenAccessError diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee index 6b03e6c600..9e35c622b0 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee @@ -7,15 +7,6 @@ settings = require 'settings-sharelatex' module.exports = TokenAccessController = - redirectNotFoundErrorToV1: (err, req, res, next) -> - if err instanceof Errors.ProjectNotTokenAccessError and settings.overleaf - logger.log { - token: req.params['read_and_write_token'] - }, "[TokenAccess] No project found for token, redirecting to v1" - res.redirect(settings.overleaf.host + req.url) - else - next(err) - _loadEditor: (projectId, req, res, next) -> req.params.Project_id = projectId.toString() return ProjectController.loadEditor(req, res, next) @@ -29,7 +20,8 @@ module.exports = TokenAccessController = if !projectExists logger.log {token, userId}, "[TokenAccess] no project found for this token" - return next(new Errors.ProjectNotTokenAccessError()) + # Project does not exist, but may be unimported - try it on v1 + return res.redirect(settings.overleaf.host + req.url) if !project? logger.log {token, userId}, "[TokenAccess] no project with higher access found for this user and token" diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 35de9f804f..b196c56870 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -418,8 +418,7 @@ module.exports = class Router maxRequests: 10, timeInterval: 60 }), - TokenAccessController.readOnlyToken, - TokenAccessController.redirectNotFoundErrorToV1 + TokenAccessController.readOnlyToken webRouter.get '/:read_and_write_token([0-9]+[a-z]+)', RateLimiterMiddlewear.rateLimit({ @@ -427,7 +426,6 @@ module.exports = class Router maxRequests: 10, timeInterval: 60 }), - TokenAccessController.readAndWriteToken, - TokenAccessController.redirectNotFoundErrorToV1 + TokenAccessController.readAndWriteToken webRouter.get '*', ErrorController.notFound diff --git a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee index 0fe8ab358a..0bdb6d59e6 100644 --- a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee +++ b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee @@ -30,6 +30,10 @@ describe "TokenAccessController", -> '../Authentication/AuthenticationController': @AuthenticationController = {} './TokenAccessHandler': @TokenAccessHandler = {} 'logger-sharelatex': {log: sinon.stub(), err: sinon.stub()} + 'settings-sharelatex': { + overleaf: + host: 'http://overleaf.test:5000' + } @AuthenticationController.getLoggedInUserId = sinon.stub().returns(@userId.toString()) @@ -234,6 +238,7 @@ describe "TokenAccessController", -> describe 'when project does not exist', -> beforeEach -> @req = new MockRequest() + @req.url = '/123abc' @res = new MockResponse() @res.redirect = sinon.stub() @next = sinon.stub() @@ -245,10 +250,10 @@ describe "TokenAccessController", -> .callsArgWith(2, null, @project, false) @TokenAccessController.readAndWriteToken @req, @res, @next - it 'should return a ProjectNotTokenAccessError', (done) -> - expect(@next.callCount).to.equal 1 - expect(@next.firstCall.args[0].name) - .to.equal 'ProjectNotTokenAccessError' + it 'should redirect to v1', (done) -> + expect(@res.redirect.callCount).to.equal 1 + expect(@res.redirect.firstCall.args[0]) + .to.equal 'http://overleaf.test:5000/123abc' done() describe 'when token access is off, but user has higher access anyway', -> @@ -311,7 +316,7 @@ describe "TokenAccessController", -> .callsArgWith(1, null, null) @TokenAccessHandler.findProjectWithHigherAccess = sinon.stub() - .callsArgWith(2, null, null) + .callsArgWith(2, null, null, true) @TokenAccessHandler.addReadAndWriteUserToProject = sinon.stub() .callsArgWith(2, null) @ProjectController.loadEditor = sinon.stub() @@ -502,6 +507,7 @@ describe "TokenAccessController", -> describe 'when project does not exist', -> beforeEach -> @req = new MockRequest() + @req.url = '/123abc' @res = new MockResponse() @res.redirect = sinon.stub() @next = sinon.stub() @@ -514,9 +520,9 @@ describe "TokenAccessController", -> @TokenAccessController.readOnlyToken @req, @res, @next it 'should return a ProjectNotTokenAccessError', (done) -> - expect(@next.callCount).to.equal 1 - expect(@next.firstCall.args[0].name) - .to.equal 'ProjectNotTokenAccessError' + expect(@res.redirect.callCount).to.equal 1 + expect(@res.redirect.firstCall.args[0]) + .to.equal 'http://overleaf.test:5000/123abc' done() describe 'when token access is off, but user has higher access anyway', -> @@ -578,7 +584,7 @@ describe "TokenAccessController", -> .callsArgWith(1, null, null) @TokenAccessHandler.findProjectWithHigherAccess = sinon.stub() - .callsArgWith(2, null, null) + .callsArgWith(2, null, null, true) @TokenAccessHandler.addReadOnlyUserToProject = sinon.stub() .callsArgWith(2, null) @ProjectController.loadEditor = sinon.stub() From f37040e4a4798d8f1daf0d8213c28fade38b1d6a Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 13 Sep 2018 12:09:03 +0100 Subject: [PATCH 8/8] Only redirect if has overleaf setting --- .../coffee/Features/TokenAccess/TokenAccessController.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee index 9e35c622b0..08aa4663f1 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee @@ -17,7 +17,7 @@ module.exports = TokenAccessController = logger.err {err, token, userId}, "[TokenAccess] error finding project with higher access" return next(err) - if !projectExists + if !projectExists and settings.overleaf logger.log {token, userId}, "[TokenAccess] no project found for this token" # Project does not exist, but may be unimported - try it on v1