From f89e85231a91228a9fa127d241f59219f4b710c8 Mon Sep 17 00:00:00 2001 From: Ersun Warncke Date: Mon, 24 Sep 2018 18:03:28 -0400 Subject: [PATCH 1/3] check access for doc on read only token --- .../TokenAccess/TokenAccessController.coffee | 38 ++++++++++--------- .../web/app/coffee/Features/V1/V1Api.coffee | 26 +++++++++++++ .../coffee/helpers/MockV1Api.coffee | 3 ++ .../TokenAccessControllerTests.coffee | 17 +++++++++ 4 files changed, 67 insertions(+), 17 deletions(-) create mode 100644 services/web/app/coffee/Features/V1/V1Api.coffee diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee index 08aa4663f1..49a3892855 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee @@ -4,6 +4,7 @@ TokenAccessHandler = require './TokenAccessHandler' Errors = require '../Errors/Errors' logger = require 'logger-sharelatex' settings = require 'settings-sharelatex' +V1Api = require "../V1/V1Api" module.exports = TokenAccessController = @@ -91,23 +92,26 @@ module.exports = TokenAccessController = return next(new Errors.NotFoundError()) TokenAccessController._tryHigherAccess(token, userId, req, res, next) else - if !userId? - logger.log {userId, projectId: project._id}, - "[TokenAccess] adding anonymous user to project with readOnly token" - TokenAccessHandler.grantSessionTokenAccess(req, project._id, token) - req._anonymousAccessToken = token - return TokenAccessController._loadEditor(project._id, req, res, next) - else - if project.owner_ref.toString() == userId + V1Api.request { url: "/api/v1/sharelatex/docs/#{token}/read" }, (err, respose, body) -> + return next err if err? + return res.redirect body.published_path if body.allow == false + if !userId? logger.log {userId, projectId: project._id}, - "[TokenAccess] user is already project owner" - return TokenAccessController._loadEditor(project._id, req, res, next) - logger.log {userId, projectId: project._id}, - "[TokenAccess] adding user to project with readOnly token" - TokenAccessHandler.addReadOnlyUserToProject userId, project._id, (err) -> - if err? - logger.err {err, token, userId, projectId: project._id}, - "[TokenAccess] error adding user to project with readAndWrite token" - return next(err) + "[TokenAccess] adding anonymous user to project with readOnly token" + TokenAccessHandler.grantSessionTokenAccess(req, project._id, token) + req._anonymousAccessToken = token return TokenAccessController._loadEditor(project._id, req, res, next) + else + if project.owner_ref.toString() == userId + logger.log {userId, projectId: project._id}, + "[TokenAccess] user is already project owner" + return TokenAccessController._loadEditor(project._id, req, res, next) + logger.log {userId, projectId: project._id}, + "[TokenAccess] adding user to project with readOnly token" + TokenAccessHandler.addReadOnlyUserToProject userId, project._id, (err) -> + if err? + logger.err {err, token, userId, projectId: project._id}, + "[TokenAccess] error adding user to project with readAndWrite token" + return next(err) + return TokenAccessController._loadEditor(project._id, req, res, next) diff --git a/services/web/app/coffee/Features/V1/V1Api.coffee b/services/web/app/coffee/Features/V1/V1Api.coffee new file mode 100644 index 0000000000..6e781ef147 --- /dev/null +++ b/services/web/app/coffee/Features/V1/V1Api.coffee @@ -0,0 +1,26 @@ +request = require 'request' +settings = require 'settings-sharelatex' + +# TODO: check what happens when these settings aren't defined +DEFAULT_V1_PARAMS = { + baseUrl: settings?.apis?.v1?.url + auth: + user: settings?.apis?.v1?.user + pass: settings?.apis?.v1?.pass + json: true, + timeout: 30 * 1000 +} + +request = request.defaults(DEFAULT_V1_PARAMS) + +module.exports = V1Api = + request: (options, callback) -> + return request(options) if !callback? + request options, (error, response, body) -> + return callback(error, response, body) if error? + if 200 <= response.statusCode < 300 or response.statusCode in (options.expectedStatusCodes or []) + callback null, response, body + else + error = new Error("overleaf v1 returned non-success code: #{response.statusCode}") + error.statusCode = response.statusCode + callback error diff --git a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee index eeafa6a44b..5ecd11a991 100644 --- a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee +++ b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee @@ -81,5 +81,8 @@ module.exports = MockV1Api = .on "error", (error) -> console.error "error starting MockV1Api:", error.message process.exit(1) + + app.get '/api/v1/sharelatex/docs/:token/read', (req, res, next) => + res.json { allow: true } MockV1Api.run() diff --git a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee index 0bdb6d59e6..0d65948225 100644 --- a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee +++ b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee @@ -34,6 +34,9 @@ describe "TokenAccessController", -> overleaf: host: 'http://overleaf.test:5000' } + '../V1/V1Api': @V1Api = { + request: sinon.stub().callsArgWith(1, null, {}, { allow: true }) + } @AuthenticationController.getLoggedInUserId = sinon.stub().returns(@userId.toString()) @@ -394,6 +397,20 @@ describe "TokenAccessController", -> describe 'readOnlyToken', -> beforeEach -> + describe 'when access not allowed by v1 api', -> + beforeEach -> + @req = new MockRequest() + @res = new MockResponse() + @res.redirect = sinon.stub() + @next = sinon.stub() + @TokenAccessHandler.findProjectWithReadOnlyToken = sinon.stub() + .callsArgWith(1, null, @project) + @V1Api.request = sinon.stub().callsArgWith(1, null, {}, { allow: false, published_path: 'doc-url'} ) + @TokenAccessController.readOnlyToken @req, @res, @next + + it 'should redirect to doc-url', -> + expect(@res.redirect.calledWith('doc-url')).to.equal true + describe 'with a user', -> beforeEach -> @AuthenticationController.getLoggedInUserId = sinon.stub().returns(@userId.toString()) From f0c0834b0fdb45b4d729c40032f30177bd4a389a Mon Sep 17 00:00:00 2001 From: Ersun Warncke Date: Tue, 25 Sep 2018 05:42:04 -0400 Subject: [PATCH 2/3] only do v1 access check when api config present --- .../TokenAccess/TokenAccessController.coffee | 5 +- .../TokenAccess/TokenAccessHandler.coffee | 8 +++ .../TokenAccessControllerTests.coffee | 3 +- .../TokenAccessHandlerTests.coffee | 49 ++++++++++++++++++- 4 files changed, 59 insertions(+), 6 deletions(-) diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee index 49a3892855..f75b72d29e 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessController.coffee @@ -4,7 +4,6 @@ TokenAccessHandler = require './TokenAccessHandler' Errors = require '../Errors/Errors' logger = require 'logger-sharelatex' settings = require 'settings-sharelatex' -V1Api = require "../V1/V1Api" module.exports = TokenAccessController = @@ -92,9 +91,9 @@ module.exports = TokenAccessController = return next(new Errors.NotFoundError()) TokenAccessController._tryHigherAccess(token, userId, req, res, next) else - V1Api.request { url: "/api/v1/sharelatex/docs/#{token}/read" }, (err, respose, body) -> + TokenAccessHandler.checkV1Access token, (err, allow_access, redirect_path) -> return next err if err? - return res.redirect body.published_path if body.allow == false + return res.redirect redirect_path unless allow_access if !userId? logger.log {userId, projectId: project._id}, "[TokenAccess] adding anonymous user to project with readOnly token" diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee index ed7f51f0d7..b1778d3f12 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee @@ -4,6 +4,7 @@ PublicAccessLevels = require '../Authorization/PublicAccessLevels' PrivilegeLevels = require '../Authorization/PrivilegeLevels' ObjectId = require("mongojs").ObjectId Settings = require('settings-sharelatex') +V1Api = require "../V1/V1Api" module.exports = TokenAccessHandler = @@ -97,3 +98,10 @@ module.exports = TokenAccessHandler = project.tokens.readAndWrite = '' if privilegeLevel != PrivilegeLevels.READ_ONLY project.tokens.readOnly = '' + + checkV1Access: (token, callback=(err, allow, redirect)->) -> + return callback(null, true) unless Settings.apis?.v1? + V1Api.request { url: "/api/v1/sharelatex/docs/#{token}/read" }, (err, response, body) -> + return callback err if err? + callback null, false, body.published_path if body.allow == false + callback null, true diff --git a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee index 0d65948225..18967534ac 100644 --- a/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee +++ b/services/web/test/unit/coffee/TokenAccess/TokenAccessControllerTests.coffee @@ -396,6 +396,7 @@ describe "TokenAccessController", -> describe 'readOnlyToken', -> beforeEach -> + @TokenAccessHandler.checkV1Access = sinon.stub().callsArgWith(1, null, true) describe 'when access not allowed by v1 api', -> beforeEach -> @@ -405,7 +406,7 @@ describe "TokenAccessController", -> @next = sinon.stub() @TokenAccessHandler.findProjectWithReadOnlyToken = sinon.stub() .callsArgWith(1, null, @project) - @V1Api.request = sinon.stub().callsArgWith(1, null, {}, { allow: false, published_path: 'doc-url'} ) + @TokenAccessHandler.checkV1Access = sinon.stub().callsArgWith(1, null, false, 'doc-url') @TokenAccessController.readOnlyToken @req, @res, @next it 'should redirect to doc-url', -> diff --git a/services/web/test/unit/coffee/TokenAccess/TokenAccessHandlerTests.coffee b/services/web/test/unit/coffee/TokenAccess/TokenAccessHandlerTests.coffee index 722a8496a2..d934f31957 100644 --- a/services/web/test/unit/coffee/TokenAccess/TokenAccessHandlerTests.coffee +++ b/services/web/test/unit/coffee/TokenAccess/TokenAccessHandlerTests.coffee @@ -19,9 +19,11 @@ describe "TokenAccessHandler", -> @req = {} @TokenAccessHandler = SandboxedModule.require modulePath, requires: '../../models/Project': {Project: @Project = {}} - 'settings-sharelatex': {} + 'settings-sharelatex': @settings = {} '../Collaborators/CollaboratorsHandler': @CollaboratorsHandler = {} - + '../V1/V1Api': @V1Api = { + request: sinon.stub() + } describe 'findProjectWithReadOnlyToken', -> beforeEach -> @@ -434,3 +436,46 @@ describe "TokenAccessHandler", -> @TokenAccessHandler.protectTokens(@project, 'owner') expect(@project.tokens.readAndWrite).to.equal 'rw' expect(@project.tokens.readOnly).to.equal 'ro' + + describe 'checkV1Access', -> + beforeEach -> + @callback = sinon.stub() + + describe 'when v1 api not set', -> + beforeEach -> + @TokenAccessHandler.checkV1Access @token, @callback + + it 'should not check access and return true', -> + expect(@V1Api.request.called).to.equal false + expect(@callback.calledWith null, true).to.equal true + + describe 'when v1 api is set', -> + beforeEach -> + @settings.apis = { v1: 'v1' } + + describe 'when access allowed', -> + beforeEach -> + @V1Api.request = sinon.stub().callsArgWith(1, null, {}, { allow: true} ) + @TokenAccessHandler.checkV1Access @token, @callback + + it 'should check api', -> + expect(@V1Api.request.calledWith { url: "/api/v1/sharelatex/docs/#{@token}/read" }).to.equal true + + it 'should callback with true', -> + expect(@callback.calledWith null, true).to.equal true + + describe 'when access denied', -> + beforeEach -> + @V1Api.request = sinon.stub().callsArgWith(1, null, {}, { allow: false, published_path: 'doc-url'} ) + @TokenAccessHandler.checkV1Access @token, @callback + + it 'should callback with false and redirect', -> + expect(@callback.calledWith null, false, 'doc-url').to.equal true + + describe 'on error', -> + beforeEach -> + @V1Api.request = sinon.stub().callsArgWith(1, 'error') + @TokenAccessHandler.checkV1Access @token, @callback + + it 'should callback with error', -> + expect(@callback.calledWith 'error').to.equal true From eeed857dd96640239163d7fe5642111c1fd8a436 Mon Sep 17 00:00:00 2001 From: Ersun Warncke Date: Tue, 25 Sep 2018 06:45:27 -0400 Subject: [PATCH 3/3] change api path --- .../app/coffee/Features/TokenAccess/TokenAccessHandler.coffee | 2 +- services/web/test/acceptance/coffee/helpers/MockV1Api.coffee | 2 +- .../test/unit/coffee/TokenAccess/TokenAccessHandlerTests.coffee | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee b/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee index b1778d3f12..6a45280d42 100644 --- a/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee +++ b/services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee @@ -101,7 +101,7 @@ module.exports = TokenAccessHandler = checkV1Access: (token, callback=(err, allow, redirect)->) -> return callback(null, true) unless Settings.apis?.v1? - V1Api.request { url: "/api/v1/sharelatex/docs/#{token}/read" }, (err, response, body) -> + V1Api.request { url: "/api/v1/sharelatex/docs/#{token}/is_published" }, (err, response, body) -> return callback err if err? callback null, false, body.published_path if body.allow == false callback null, true diff --git a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee index 5ecd11a991..f9a7aed451 100644 --- a/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee +++ b/services/web/test/acceptance/coffee/helpers/MockV1Api.coffee @@ -82,7 +82,7 @@ module.exports = MockV1Api = console.error "error starting MockV1Api:", error.message process.exit(1) - app.get '/api/v1/sharelatex/docs/:token/read', (req, res, next) => + app.get '/api/v1/sharelatex/docs/:token/is_published', (req, res, next) => res.json { allow: true } MockV1Api.run() diff --git a/services/web/test/unit/coffee/TokenAccess/TokenAccessHandlerTests.coffee b/services/web/test/unit/coffee/TokenAccess/TokenAccessHandlerTests.coffee index d934f31957..350dff0638 100644 --- a/services/web/test/unit/coffee/TokenAccess/TokenAccessHandlerTests.coffee +++ b/services/web/test/unit/coffee/TokenAccess/TokenAccessHandlerTests.coffee @@ -459,7 +459,7 @@ describe "TokenAccessHandler", -> @TokenAccessHandler.checkV1Access @token, @callback it 'should check api', -> - expect(@V1Api.request.calledWith { url: "/api/v1/sharelatex/docs/#{@token}/read" }).to.equal true + expect(@V1Api.request.calledWith { url: "/api/v1/sharelatex/docs/#{@token}/is_published" }).to.equal true it 'should callback with true', -> expect(@callback.calledWith null, true).to.equal true