From 435fe11115c137247bdc08ec6a36bb46beb8c88f Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 27 Sep 2018 17:38:35 +0100 Subject: [PATCH 1/2] Check if v1 project was exported if not found This prevents a redirect loop for projects which were exported but then deleted on v2. v2 would not find the project, redirect to v1, which would find that it was exported and redirect back to v2. --- .../TokenAccess/TokenAccessController.coffee | 10 ++++-- .../TokenAccess/TokenAccessHandler.coffee | 6 ++++ .../coffee/helpers/MockV1Api.coffee | 3 ++ .../TokenAccessControllerTests.coffee | 34 +++++++++++++------ 4 files changed, 39 insertions(+), 14 deletions(-) diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee index e335b19181..c6123cecf6 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee @@ -1,6 +1,7 @@ ProjectController = require "../Project/ProjectController" AuthenticationController = require '../Authentication/AuthenticationController' TokenAccessHandler = require './TokenAccessHandler' +V1Api = require '../V1/V1Api' Errors = require '../Errors/Errors' logger = require 'logger-sharelatex' settings = require 'settings-sharelatex' @@ -36,9 +37,12 @@ module.exports = TokenAccessController = return next(err) if !projectExists and settings.overleaf logger.log {token, userId}, - "[TokenAccess] no project found for this token" - return res.redirect(302, "/sign_in_to_v1?return_to=/#{token}") - if !project? + "[TokenAccess] no project found for this token" + TokenAccessHandler.checkV1ProjectExported token, (err, exported) -> + return next err if err? + return next(new Errors.NotFoundError()) if exported + return res.redirect(302, "/sign_in_to_v1?return_to=/#{token}") + else if !project? logger.log {token, userId}, "[TokenAccess] no token-based project found for readAndWrite token" if !userId? diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee index 8d9825da2c..100786f776 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee @@ -116,3 +116,9 @@ module.exports = TokenAccessHandler = return callback err if err? callback null, false, body.published_path if body.allow == false callback null, true + + checkV1ProjectExported: (token, callback = (err, exists) ->) -> + return callback(null, false) unless Settings.apis?.v1? + V1Api.request { url: "/api/v1/sharelatex/docs/#{token}/exported_to_v2" }, (err, response, body) -> + return callback err if err? + callback null, body.exported diff --git a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee index f9a7aed451..fcaf7a80df 100644 --- a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee +++ b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee @@ -85,4 +85,7 @@ module.exports = MockV1Api = app.get '/api/v1/sharelatex/docs/:token/is_published', (req, res, next) => res.json { allow: true } + app.get '/api/v1/sharelatex/docs/:token/exported_to_v2', (req, res, next) => + res.json { exported: false } + MockV1Api.run() diff --git a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee index aa2e8c4ede..09123b5939 100644 --- a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee +++ b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee @@ -248,18 +248,30 @@ describe "TokenAccessController", -> @req.params['read_and_write_token'] = '123abc' @TokenAccessHandler.findProjectWithReadAndWriteToken = sinon.stub() .callsArgWith(1, null, null, false) - @TokenAccessHandler.findProjectWithHigherAccess = - sinon.stub() - .callsArgWith(2, null, @project) - @TokenAccessController.readAndWriteToken @req, @res, @next - it 'should redirect to v1', (done) -> - expect(@res.redirect.callCount).to.equal 1 - expect(@res.redirect.calledWith( - 302, - '/sign_in_to_v1?return_to=/123abc' - )).to.equal true - done() + describe 'when project was not exported from v1', -> + beforeEach -> + @TokenAccessHandler.checkV1ProjectExported = sinon.stub() + .callsArgWith(1, null, false) + @TokenAccessController.readAndWriteToken @req, @res, @next + + it 'should redirect to v1', (done) -> + expect(@res.redirect.callCount).to.equal 1 + expect(@res.redirect.calledWith( + 302, + '/sign_in_to_v1?return_to=/123abc' + )).to.equal true + done() + + describe 'when project was exported from v1', -> + beforeEach -> + @TokenAccessHandler.checkV1ProjectExported = sinon.stub() + .callsArgWith(1, null, false) + @TokenAccessController.readAndWriteToken @req, @res, @next + + it 'should call next with a not-found error', (done) -> + expect(@next.callCount).to.equal 0 + done() describe 'when token access is off, but user has higher access anyway', -> beforeEach -> From 1330c8da733f67e66e7befc2975e53db3cc0cb3b Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Fri, 28 Sep 2018 11:31:07 +0100 Subject: [PATCH 2/2] Also check if v1 project exported if not found for read-only tokens --- .../TokenAccess/TokenAccessController.coffee | 9 ++- .../acceptance/coffee/TokenAccessTests.coffee | 2 +- .../TokenAccessControllerTests.coffee | 60 ++++++++++++++++--- 3 files changed, 60 insertions(+), 11 deletions(-) diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee index c6123cecf6..d58b28cb30 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee @@ -87,9 +87,12 @@ module.exports = TokenAccessController = return next(err) if !projectExists and settings.overleaf logger.log {token, userId}, - "[TokenAccess] no project found for this token" - return res.redirect(302, settings.overleaf.host + '/read/' + token) - if !project? + "[TokenAccess] no project found for this token" + TokenAccessHandler.checkV1ProjectExported token, (err, exported) -> + return next err if err? + return next(new Errors.NotFoundError()) if exported + return res.redirect(302, "/sign_in_to_v1?return_to=/read/#{token}") + else if !project? logger.log {token, userId}, "[TokenAccess] no project found for readOnly token" if !userId? diff --git a/services/web/test/acceptance/coffee/TokenAccessTests.coffee b/services/web/test/acceptance/coffee/TokenAccessTests.coffee index 31be9523ba..832281f6a3 100644 --- a/services/web/test/acceptance/coffee/TokenAccessTests.coffee +++ b/services/web/test/acceptance/coffee/TokenAccessTests.coffee @@ -431,6 +431,6 @@ describe 'TokenAccess', -> try_read_only_token_access(@owner, unimportedV1Token, (response, body) => expect(response.statusCode).to.equal 302 expect(response.headers.location).to.equal( - 'http://overleaf.test:5000/read/abcd' + '/sign_in_to_v1?return_to=/read/abcd' ) , done) diff --git a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee index 09123b5939..6d9728536d 100644 --- a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee +++ b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee @@ -533,6 +533,44 @@ describe "TokenAccessController", -> done() describe 'when findProject does not find a project', -> + describe 'when project does not exist', -> + beforeEach -> + @req = new MockRequest() + @res = new MockResponse() + @res.redirect = sinon.stub() + @next = sinon.stub() + @req.params['read_only_token'] = 'abcd' + @TokenAccessHandler.findProjectWithReadOnlyToken = sinon.stub() + .callsArgWith(1, null, null, false) + @TokenAccessHandler.checkV1ProjectExported = sinon.stub() + .callsArgWith(1, null, false) + @TokenAccessController.readOnlyToken @req, @res, @next + + it 'should redirect to v1', (done) -> + expect(@res.redirect.callCount).to.equal 1 + expect(@res.redirect.calledWith( + 302, + '/sign_in_to_v1?return_to=/read/abcd' + )).to.equal true + done() + + describe 'when project was exported from v1', -> + beforeEach -> + @req = new MockRequest() + @res = new MockResponse() + @res.redirect = sinon.stub() + @next = sinon.stub() + @req.params['read_only_token'] = 'abcd' + @TokenAccessHandler.findProjectWithReadOnlyToken = sinon.stub() + .callsArgWith(1, null, null, false) + @TokenAccessHandler.checkV1ProjectExported = sinon.stub() + .callsArgWith(1, null, true) + @TokenAccessController.readOnlyToken @req, @res, @next + + it 'should call next with a not-found error', (done) -> + expect(@next.callCount).to.equal 1 + done() + describe 'when token access is off, but user has higher access anyway', -> beforeEach -> @req = new MockRequest() @@ -761,6 +799,8 @@ describe "TokenAccessController", -> @req.params['read_only_token'] = @readOnlyToken @TokenAccessHandler.findProjectWithReadOnlyToken = sinon.stub() .callsArgWith(1, null, null) + @TokenAccessHandler.checkV1ProjectExported = sinon.stub() + .callsArgWith(1, null, false) @TokenAccessHandler.addReadOnlyUserToProject = sinon.stub() .callsArgWith(2, null) @ProjectController.loadEditor = sinon.stub() @@ -788,11 +828,17 @@ describe "TokenAccessController", -> .to.equal 0 done() - it 'should redirect to v1', (done) -> - expect(@res.redirect.callCount).to.equal 1 - expect(@res.redirect.calledWith( - 302, - "http://overleaf.test:5000/read/#{@readOnlyToken}" - )).to.equal true - done() + describe 'when project was exported to v2', -> + beforeEach -> + @TokenAccessHandler.checkV1ProjectExported = sinon.stub() + .callsArgWith(1, null, true) + @TokenAccessController.readOnlyToken @req, @res, @next + + it 'should redirect to v1', (done) -> + expect(@res.redirect.callCount).to.equal 1 + expect(@res.redirect.calledWith( + 302, + "/sign_in_to_v1?return_to=/read/#{@readOnlyToken}" + )).to.equal true + done()