From 40f08d15929fcb58646075aa4d13e9a56c492695 Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 12 Sep 2018 15:25:17 +0100 Subject: [PATCH 1/3] Add additional functionality to RedirectManager --- .../infrastructure/RedirectManager.coffee | 39 ++++++++------- .../app/coffee/infrastructure/Server.coffee | 2 +- .../RedirectManagerTests.coffee | 47 ------------------- 3 files changed, 24 insertions(+), 64 deletions(-) delete mode 100644 services/web/test/unit/coffee/infrastructure/RedirectManagerTests.coffee diff --git a/services/web/app/coffee/infrastructure/RedirectManager.coffee b/services/web/app/coffee/infrastructure/RedirectManager.coffee index 0c9141926d..7fac7ef1ec 100644 --- a/services/web/app/coffee/infrastructure/RedirectManager.coffee +++ b/services/web/app/coffee/infrastructure/RedirectManager.coffee @@ -1,20 +1,27 @@ settings = require("settings-sharelatex") logger = require("logger-sharelatex") -module.exports = (req, res, next)-> - - requestedUrl = req.url - - redirectUrl = settings.redirects[requestedUrl] - - #remove starting slash - if !redirectUrl? and requestedUrl[requestedUrl.length-1] == "/" - requestedUrl = requestedUrl.substring(0, requestedUrl.length - 1) - redirectUrl = settings.redirects[requestedUrl] - - if redirectUrl? - logger.log redirectUrl:redirectUrl, reqUrl:req.url, "redirecting to new path" - res.redirect 301, "#{redirectUrl}" - else - next() +module.exports = RedirectManager = + apply: (webRouter) -> + for redirectUrl, target of settings.redirects + do (target) -> + method = target.method || 'get' + webRouter[method] redirectUrl, RedirectManager.createRedirect(target) + createRedirect: (target) -> + (req, res, next) -> + code = 302 + if typeof target is 'string' + url = target + else + if target.method == "post" + code = 307 + if typeof target.url == "function" + url = target.url(req.params) + if !url + return next() + else + url = target.url + if target.baseUrl? + url = "#{target.baseUrl}#{url}" + res.redirect code, url diff --git a/services/web/app/coffee/infrastructure/Server.coffee b/services/web/app/coffee/infrastructure/Server.coffee index bf9d6652f4..dd70fb1a42 100644 --- a/services/web/app/coffee/infrastructure/Server.coffee +++ b/services/web/app/coffee/infrastructure/Server.coffee @@ -73,7 +73,7 @@ app.use multer(dest: Settings.path.uploadFolder) app.use methodOverride() app.use metrics.http.monitor(logger) -app.use RedirectManager +RedirectManager.apply(webRouter) ProxyManager.apply(publicApiRouter) diff --git a/services/web/test/unit/coffee/infrastructure/RedirectManagerTests.coffee b/services/web/test/unit/coffee/infrastructure/RedirectManagerTests.coffee deleted file mode 100644 index 701f427b90..0000000000 --- a/services/web/test/unit/coffee/infrastructure/RedirectManagerTests.coffee +++ /dev/null @@ -1,47 +0,0 @@ -sinon = require('sinon') -chai = require('chai') -should = chai.should() -expect = chai.expect -path = require("path") -modulePath = "../../../../app/js/infrastructure/RedirectManager.js" -SandboxedModule = require('sandboxed-module') - -describe "redirectToNewTemplate", -> - beforeEach -> - @settings = - redirects: - "/path/here/" : "/path/elsewhere/" - "/no/trailing/slash":"/no/trailing/slash/elsewhere" - "/part/path": "/diff/part/path" - mountPointUrl:"/here" - @redirectManager = SandboxedModule.require modulePath, requires: - "settings-sharelatex":@settings - "logger-sharelatex": - log:-> - err:-> - @res = - redirect: sinon.stub() - @req = {} - - describe "redirect", -> - - it "should perminant redirect if url matches redirect", ()-> - @req.url = "/path/here/" - nextStub = sinon.stub() - @redirectManager @req, @res, nextStub - @res.redirect.calledWith(301, "/path/elsewhere/").should.equal true - nextStub.called.should.equal false - - it "should not redirect on non matching url", (done)-> - @req.url = "non/matching/" - @redirectManager @req, @res, => - @res.redirect.called.should.equal false - done() - - it "should ignore slash at end of url", -> - @req.url = "/no/trailing/slash/" - nextStub = sinon.stub() - @redirectManager @req, @res, nextStub - @res.redirect.calledWith(301, "/no/trailing/slash/elsewhere").should.equal true - nextStub.called.should.equal false - From 83a1039b7e1d82fa2fb75226b90d72d9c19c3ada Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 12 Sep 2018 15:35:30 +0100 Subject: [PATCH 2/3] Add acceptance tests for RedirectManager --- .../coffee/RedirectUrlsTests.coffee | 28 +++++++++++++++++++ .../acceptance/config/settings.test.coffee | 14 ++++++++++ 2 files changed, 42 insertions(+) create mode 100644 services/web/test/acceptance/coffee/RedirectUrlsTests.coffee diff --git a/services/web/test/acceptance/coffee/RedirectUrlsTests.coffee b/services/web/test/acceptance/coffee/RedirectUrlsTests.coffee new file mode 100644 index 0000000000..90c5a62b9b --- /dev/null +++ b/services/web/test/acceptance/coffee/RedirectUrlsTests.coffee @@ -0,0 +1,28 @@ +should = require('chai').should() +assert = require('chai').assert +async = require("async") +request = require "./helpers/request" +MockV1Api = require "./helpers/MockV1Api" + +assertRedirect = (method, path, expectedStatusCode, destination, cb) -> + request[method] path, (error, response) -> + should.not.exist error + response.statusCode.should.equal expectedStatusCode + response.headers.location.should.equal destination + cb() + +describe "RedirectUrls", -> + before -> + @timeout(1000) + + it 'proxy static URLs', (done) -> + assertRedirect 'get', '/redirect/one', 302, '/destination/one', done + + it 'proxy dynamic URLs', (done) -> + assertRedirect 'get', '/redirect/params/42', 302, '/destination/42/params', done + + it 'proxy URLs with baseUrl', (done) -> + assertRedirect 'get', '/redirect/base_url', 302, 'https://example.com/destination/base_url', done + + it 'proxy URLs with POST', (done) -> + assertRedirect 'post', '/redirect/post', 307, '/destination/post', done diff --git a/services/web/test/acceptance/config/settings.test.coffee b/services/web/test/acceptance/config/settings.test.coffee index 1ca1b55e48..400d24d016 100644 --- a/services/web/test/acceptance/config/settings.test.coffee +++ b/services/web/test/acceptance/config/settings.test.coffee @@ -108,3 +108,17 @@ module.exports = overleaf: host: "http://overleaf.test:5000" + + redirects: + '/redirect/one': '/destination/one', + '/redirect/post': { + method: 'post', + url: '/destination/post' + }, + '/redirect/base_url': { + baseUrl: 'https://example.com' + url: '/destination/base_url' + }, + '/redirect/params/:id': { + url: (params) -> "/destination/#{params.id}/params" + } From 15103ac894de40cebabde53e4e72ce8d1a6f908c Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 17 Sep 2018 15:34:35 +0100 Subject: [PATCH 3/3] Support the same URL with multiple methods in redirects --- .../app/coffee/infrastructure/RedirectManager.coffee | 5 ++--- .../test/acceptance/coffee/RedirectUrlsTests.coffee | 7 +++++-- .../web/test/acceptance/config/settings.test.coffee | 10 +++++++--- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/services/web/app/coffee/infrastructure/RedirectManager.coffee b/services/web/app/coffee/infrastructure/RedirectManager.coffee index 7fac7ef1ec..2f9ffd1397 100644 --- a/services/web/app/coffee/infrastructure/RedirectManager.coffee +++ b/services/web/app/coffee/infrastructure/RedirectManager.coffee @@ -4,8 +4,7 @@ logger = require("logger-sharelatex") module.exports = RedirectManager = apply: (webRouter) -> for redirectUrl, target of settings.redirects - do (target) -> - method = target.method || 'get' + for method in (target.methods or ['get']) webRouter[method] redirectUrl, RedirectManager.createRedirect(target) createRedirect: (target) -> @@ -14,7 +13,7 @@ module.exports = RedirectManager = if typeof target is 'string' url = target else - if target.method == "post" + if req.method == "POST" code = 307 if typeof target.url == "function" url = target.url(req.params) diff --git a/services/web/test/acceptance/coffee/RedirectUrlsTests.coffee b/services/web/test/acceptance/coffee/RedirectUrlsTests.coffee index 90c5a62b9b..f1c0f4e8dd 100644 --- a/services/web/test/acceptance/coffee/RedirectUrlsTests.coffee +++ b/services/web/test/acceptance/coffee/RedirectUrlsTests.coffee @@ -24,5 +24,8 @@ describe "RedirectUrls", -> it 'proxy URLs with baseUrl', (done) -> assertRedirect 'get', '/redirect/base_url', 302, 'https://example.com/destination/base_url', done - it 'proxy URLs with POST', (done) -> - assertRedirect 'post', '/redirect/post', 307, '/destination/post', done + it 'proxy URLs with POST with a 307', (done) -> + assertRedirect 'post', '/redirect/get_and_post', 307, '/destination/get_and_post', done + + it 'proxy URLs with multiple support methods', (done) -> + assertRedirect 'get', '/redirect/get_and_post', 302, '/destination/get_and_post', done diff --git a/services/web/test/acceptance/config/settings.test.coffee b/services/web/test/acceptance/config/settings.test.coffee index 400d24d016..521b44cbb6 100644 --- a/services/web/test/acceptance/config/settings.test.coffee +++ b/services/web/test/acceptance/config/settings.test.coffee @@ -105,15 +105,19 @@ module.exports = path: (params) -> "/universities/list/#{params.id}" '/institutions/domains': { baseUrl: v1Api.url, path: '/university/domains' } '/proxy/missing/baseUrl': path: '/foo/bar' + '/proxy/get_and_post': { + methods: ['get', 'post'], + path: '/destination/get_and_post' + } overleaf: host: "http://overleaf.test:5000" redirects: '/redirect/one': '/destination/one', - '/redirect/post': { - method: 'post', - url: '/destination/post' + '/redirect/get_and_post': { + methods: ['get', 'post'], + url: '/destination/get_and_post' }, '/redirect/base_url': { baseUrl: 'https://example.com'