From 17a180ea8ebce587ec232e273ca363c26e5624e4 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 3 Nov 2017 16:44:37 +0000 Subject: [PATCH 1/2] route history requests based on project either to track changes or to project history service --- .../Features/History/HistoryController.coffee | 24 ++++-- services/web/app/coffee/router.coffee | 6 +- .../History/HistoryControllerTests.coffee | 81 +++++++++++++++++-- 3 files changed, 98 insertions(+), 13 deletions(-) diff --git a/services/web/app/coffee/Features/History/HistoryController.coffee b/services/web/app/coffee/Features/History/HistoryController.coffee index c7c869d674..f623d4a9d3 100644 --- a/services/web/app/coffee/Features/History/HistoryController.coffee +++ b/services/web/app/coffee/Features/History/HistoryController.coffee @@ -2,6 +2,7 @@ logger = require "logger-sharelatex" request = require "request" settings = require "settings-sharelatex" AuthenticationController = require "../Authentication/AuthenticationController" +ProjectDetailsHandler = require "../Project/ProjectDetailsHandler" module.exports = HistoryController = initializeProject: (callback = (error, history_id) ->) -> @@ -27,11 +28,22 @@ module.exports = HistoryController = error = new Error("project-history returned a non-success status code: #{res.statusCode}") callback error + selectHistoryApi: (req, res, next = (error) ->) -> + project_id = req.params?.Project_id + # find out which type of history service this project uses + ProjectDetailsHandler.getDetails project_id, (err, project) -> + return next(err) if err? + if project?.overleaf?.history? + req.useProjectHistory = true + else + req.useProjectHistory = false + next() + proxyToHistoryApi: (req, res, next = (error) ->) -> user_id = AuthenticationController.getLoggedInUserId req - url = HistoryController.buildHistoryServiceUrl() + req.url + url = HistoryController.buildHistoryServiceUrl(req.useProjectHistory) + req.url - logger.log url: url, "proxying to track-changes api" + logger.log url: url, "proxying to history api" getReq = request( url: url method: req.method @@ -40,11 +52,13 @@ module.exports = HistoryController = ) getReq.pipe(res) getReq.on "error", (error) -> - logger.error err: error, "track-changes API error" + logger.error url: url, err: error, "history API error" next(error) - buildHistoryServiceUrl: () -> - if settings.apis.project_history?.enabled + buildHistoryServiceUrl: (useProjectHistory) -> + # choose a history service, either document-level (trackchanges) + # or project-level (project_history) + if settings.apis.project_history?.enabled && useProjectHistory return settings.apis.project_history.url else return settings.apis.trackchanges.url diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 2ebde53748..e5884780cf 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -194,9 +194,9 @@ module.exports = class Router webRouter.post '/project/:Project_id/rename', AuthorizationMiddlewear.ensureUserCanAdminProject, ProjectController.renameProject - webRouter.get "/project/:Project_id/updates", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.proxyToHistoryApi - webRouter.get "/project/:Project_id/doc/:doc_id/diff", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.proxyToHistoryApi - webRouter.post "/project/:Project_id/doc/:doc_id/version/:version_id/restore", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.proxyToHistoryApi + webRouter.get "/project/:Project_id/updates", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.selectHistoryApi, HistoryController.proxyToHistoryApi + webRouter.get "/project/:Project_id/doc/:doc_id/diff", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.selectHistoryApi, HistoryController.proxyToHistoryApi + webRouter.post "/project/:Project_id/doc/:doc_id/version/:version_id/restore", AuthorizationMiddlewear.ensureUserCanReadProject, HistoryController.selectHistoryApi, HistoryController.proxyToHistoryApi webRouter.get '/Project/:Project_id/download/zip', AuthorizationMiddlewear.ensureUserCanReadProject, ProjectDownloadsController.downloadProject webRouter.get '/project/download/zip', AuthorizationMiddlewear.ensureUserCanReadMultipleProjects, ProjectDownloadsController.downloadMultipleProjects diff --git a/services/web/test/UnitTests/coffee/History/HistoryControllerTests.coffee b/services/web/test/UnitTests/coffee/History/HistoryControllerTests.coffee index ae10a34739..0184782e45 100644 --- a/services/web/test/UnitTests/coffee/History/HistoryControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/History/HistoryControllerTests.coffee @@ -15,6 +15,7 @@ describe "HistoryController", -> "settings-sharelatex": @settings = {} "logger-sharelatex": @logger = {log: sinon.stub(), error: sinon.stub()} "../Authentication/AuthenticationController": @AuthenticationController + "../Project/ProjectDetailsHandler": @ProjectDetailsHandler = {} @settings.apis = trackchanges: enabled: false @@ -22,6 +23,29 @@ describe "HistoryController", -> project_history: url: "http://project_history.example.com" + describe "selectHistoryApi", -> + beforeEach -> + @req = { url: "/mock/url", method: "POST" } + @res = "mock-res" + @next = sinon.stub() + + describe "for a project with project history", -> + beforeEach -> + @ProjectDetailsHandler.getDetails = sinon.stub().callsArgWith(1, null, {overleaf:{history:{}}}) + @HistoryController.selectHistoryApi @req, @res, @next + + it "should set the flag for project history to true", -> + @req.useProjectHistory.should.equal true + + describe "for any other project ", -> + beforeEach -> + @ProjectDetailsHandler.getDetails = sinon.stub().callsArgWith(1, null, {}) + @HistoryController.selectHistoryApi @req, @res, @next + + it "should not set the flag for project history to false", -> + @req.useProjectHistory.should.equal false + + describe "proxyToHistoryApi", -> beforeEach -> @req = { url: "/mock/url", method: "POST" } @@ -33,10 +57,13 @@ describe "HistoryController", -> on: (event, handler) -> @events[event] = handler @request.returns @proxy - describe "successfully", -> - describe "with project history enabled", -> + describe "with project history enabled", -> + beforeEach -> + @settings.apis.project_history.enabled = true + + describe "for a project with the project history flag", -> beforeEach -> - @settings.apis.project_history.enabled = true + @req.useProjectHistory = true @HistoryController.proxyToHistoryApi @req, @res, @next it "should get the user id", -> @@ -59,9 +86,53 @@ describe "HistoryController", -> .calledWith(@res) .should.equal true - describe "with project history disabled", -> + describe "for a project without the project history flag", -> beforeEach -> - @settings.apis.project_history.enabled = false + @req.useProjectHistory = false + @HistoryController.proxyToHistoryApi @req, @res, @next + + it "should get the user id", -> + @AuthenticationController.getLoggedInUserId + .calledWith(@req) + .should.equal true + + it "should call the track changes api", -> + @request + .calledWith({ + url: "#{@settings.apis.trackchanges.url}#{@req.url}" + method: @req.method + headers: + "X-User-Id": @user_id + }) + .should.equal true + + it "should pipe the response to the client", -> + @proxy.pipe + .calledWith(@res) + .should.equal true + + describe "with project history disabled", -> + beforeEach -> + @settings.apis.project_history.enabled = false + + describe "for a project with the project history flag", -> + beforeEach -> + @req.useProjectHistory = true + @HistoryController.proxyToHistoryApi @req, @res, @next + + it "should call the track changes api", -> + @request + .calledWith({ + url: "#{@settings.apis.trackchanges.url}#{@req.url}" + method: @req.method + headers: + "X-User-Id": @user_id + }) + .should.equal true + + describe "for a project without the project history flag", -> + beforeEach -> + @req.useProjectHistory = false @HistoryController.proxyToHistoryApi @req, @res, @next it "should call the track changes api", -> From 98fe352a8c70336df430c6571737005b47face46 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 23 Nov 2017 15:14:33 +0000 Subject: [PATCH 2/2] use a separate flag for reading from history the overleaf.history object controls writing to the project history service, we need a separate flag to determine whether to read from it or from track changes. --- .../web/app/coffee/Features/History/HistoryController.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/History/HistoryController.coffee b/services/web/app/coffee/Features/History/HistoryController.coffee index f623d4a9d3..70e0828160 100644 --- a/services/web/app/coffee/Features/History/HistoryController.coffee +++ b/services/web/app/coffee/Features/History/HistoryController.coffee @@ -33,7 +33,7 @@ module.exports = HistoryController = # find out which type of history service this project uses ProjectDetailsHandler.getDetails project_id, (err, project) -> return next(err) if err? - if project?.overleaf?.history? + if project?.overleaf?.history?.display req.useProjectHistory = true else req.useProjectHistory = false