Merge pull request #912 from sharelatex/ja-improve-redirect-logic

Add additional functionality to RedirectManager

services/web/app/coffee/infrastructure/RedirectManager.coffee

@@ -1,20 +1,26 @@
 settings = require("settings-sharelatex")
 logger = require("logger-sharelatex")
 
-module.exports = (req, res, next)->
+module.exports = RedirectManager =
-	
+	apply: (webRouter) ->
-	requestedUrl = req.url
+		for redirectUrl, target of settings.redirects
-
+			for method in (target.methods or ['get'])
-	redirectUrl = settings.redirects[requestedUrl]
+				webRouter[method] redirectUrl, RedirectManager.createRedirect(target)
-
-	#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()
 
+	createRedirect: (target) ->
+		(req, res, next) ->
+			code = 302
+			if typeof target is 'string'
+				url = target
+			else
+				if req.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

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)
 
 

services/web/test/acceptance/coffee/RedirectUrlsTests.coffee

@@ -0,0 +1,31 @@
+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 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

services/web/test/acceptance/config/settings.test.coffee

@@ -105,6 +105,24 @@ 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/get_and_post': {
+			methods: ['get', 'post'],
+			url: '/destination/get_and_post'
+		},
+		'/redirect/base_url': {
+			baseUrl: 'https://example.com'
+			url: '/destination/base_url'
+		},
+		'/redirect/params/:id': {
+			url: (params) -> "/destination/#{params.id}/params"
+		}

services/web/test/unit/coffee/infrastructure/RedirectManagerTests.coffee

@@ -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
-