Merge pull request #727 from sharelatex/ta-proxy-trailing-id

Proxy Dynamic Paths

services/web/app/coffee/infrastructure/ProxyManager.coffee

@@ -4,33 +4,37 @@ request = require 'request'
 URL = require 'url'
 
 module.exports = ProxyManager =
-	call: (req, res, next) ->
-		requestUrl = URL.parse(req.url)
-		requestPath = requestUrl.pathname # ignore the query part
+	apply: (publicApiRouter) ->
+		for proxyUrl, target of settings.proxyUrls
+			do (target) ->
+				publicApiRouter.get proxyUrl, ProxyManager.createProxy(target)
 
-		target = settings.proxyUrls[requestPath]
-		return next() unless target? # nothing to proxy
+	createProxy:  (target) ->
+		(req, res, next) ->
+			targetUrl = makeTargetUrl(target, req)
+			logger.log targetUrl: targetUrl, reqUrl: req.url, "proxying url"
 
-		targetUrl = makeTargetUrl(target, req.query)
-		logger.log targetUrl: targetUrl, reqUrl: req.url, "proxying url"
+			upstream = request(targetUrl)
+			upstream.on "error", (error) ->
+				logger.error err: error, "error in ProxyManager"
 
-		upstream = request(targetUrl)
-		upstream.on "error", (error) ->
-			logger.error err: error, "error in ProxyManager"
-
-		# TODO: better handling of status code
-		# see https://github.com/overleaf/write_latex/wiki/Streams-and-pipes-in-Node.js
-		upstream.pipe(res)
+			# TODO: better handling of status code
+			# see https://github.com/overleaf/write_latex/wiki/Streams-and-pipes-in-Node.js
+			upstream.pipe(res)
 
 # make a URL from a proxy target.
 # if the query is specified, set/replace the target's query with the given query
-makeTargetUrl = (target, query) ->
-	targetUrl = URL.parse(parseSettingUrl(target))
-	if query? and Object.keys(query).length > 0
-		targetUrl.query = query
+makeTargetUrl = (target, req) ->
+	targetUrl = URL.parse(parseSettingUrl(target, req))
+	if req.query? and Object.keys(req.query).length > 0
+		targetUrl.query = req.query
 		targetUrl.search = null # clear `search` as it takes precedence over `query`
 	targetUrl.format()
 
-parseSettingUrl = (target) ->
+parseSettingUrl = (target, { params }) ->
 	return target if typeof target is 'string'
-	"#{target.baseUrl}#{target.path or ''}"
+	if typeof target.path is 'function'
+		path = target.path(params)
+	else
+		path = target.path
+	"#{target.baseUrl}#{path or ''}"

services/web/app/coffee/infrastructure/Server.coffee

@@ -74,7 +74,7 @@ app.use methodOverride()
 
 app.use metrics.http.monitor(logger)
 app.use RedirectManager
-app.use ProxyManager.call
+ProxyManager.apply(publicApiRouter)
 
 
 webRouter.use cookieParser(Settings.security.sessionSecret)

services/web/config/settings.defaults.coffee

@@ -425,10 +425,6 @@ module.exports = settings =
 	redirects:
 		"/templates/index": "/templates/"
 
-	proxyUrls:
-		'/institutions/list': { baseUrl: v1Api.url, path: '/universities/list' }
-		'/institutions/domains': { baseUrl: v1Api.url, path: '/university/domains' }
-
 	reloadModuleViewsOnEachRequest: true
 
 	domainLicences: [

services/web/test/acceptance/coffee/ProxyUrls.coffee

@@ -0,0 +1,42 @@
+should = require('chai').should()
+assert = require('chai').assert
+async = require("async")
+request = require "./helpers/request"
+MockV1Api = require "./helpers/MockV1Api"
+
+assertResponse = (path, expectedStatusCode, expectedBody, cb) ->
+	request.get path, (error, response) ->
+		should.not.exist error
+		response.statusCode.should.equal expectedStatusCode
+		assert.deepEqual(JSON.parse(response.body), expectedBody) if expectedBody
+		cb()
+
+describe "ProxyUrls", ->
+	before ->
+		@timeout(1000)
+
+	it 'proxy static URLs', (done) ->
+		async.series [
+			(cb) -> assertResponse '/institutions/list', 200, [], cb
+			(cb) -> assertResponse '/institutions/domains', 200, [], cb
+		],
+		done
+
+	it 'proxy dynamic URLs', (done) ->
+		async.series [
+			(cb) -> assertResponse '/institutions/list/123', 200, { id: 123 }, cb
+			(cb) -> assertResponse '/institutions/list/456', 200, { id: 456 }, cb
+		],
+		done
+
+	it 'return 404 if proxy is not set', (done) ->
+		async.series [
+			(cb) -> assertResponse '/institutions/foobar', 404, null, cb
+		],
+		done
+
+	it 'handle missing baseUrl', (done) ->
+		async.series [
+			(cb) -> assertResponse '/proxy/missing/baseUrl', 500, null, cb
+		],
+		done

services/web/test/acceptance/coffee/helpers/MockV1Api.coffee

@@ -51,6 +51,15 @@ module.exports = MockV1Api =
 		app.delete "/api/v2/users/:userId/affiliations/:email", (req, res, next) =>
 			res.sendStatus 204
 
+		app.get '/universities/list', (req, res, next) ->
+			res.json []
+
+		app.get '/universities/list/:id', (req, res, next) ->
+			res.json id: parseInt(req.params.id)
+
+		app.get '/university/domains', (req, res, next) ->
+			res.json []
+
 		app.listen 5000, (error) ->
 			throw error if error?
 		.on "error", (error) ->

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

@@ -1,3 +1,6 @@
+v1Api =
+	url: "http://#{process.env['V1_HOST'] or 'localhost'}:5000"
+
 module.exports =
 	enableSubscriptions: true
 
@@ -93,3 +96,11 @@ module.exports =
 			collaborators: -1
 			dropbox: true
 			versioning: true
+
+	proxyUrls:
+		'/institutions/list': { baseUrl: v1Api.url, path: '/universities/list' }
+		'/institutions/list/:id':
+			baseUrl: v1Api.url
+			path: (params) -> "/universities/list/#{params.id}"
+		'/institutions/domains': { baseUrl: v1Api.url, path: '/university/domains' }
+		'/proxy/missing/baseUrl': path: '/foo/bar'

services/web/test/unit/coffee/helpers/MockRequest.coffee

@@ -9,6 +9,8 @@ class MockRequest
 	_parsedUrl:{}
 	i18n:
 		translate:->
+	route:
+		path: ''
 
 module.exports = MockRequest
 

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

@@ -24,9 +24,21 @@ describe "ProxyManager", ->
 		@res = new MockResponse()
 		@next = sinon.stub()
 
-	describe 'proxyUrls', ->
+	describe 'apply', ->
+		it 'applies all paths', ->
+			@router = get: sinon.stub()
+			@settings.proxyUrls =
+				'/foo/bar': ''
+				'/foo/:id': ''
+			@proxyManager.apply(@router)
+			sinon.assert.calledTwice(@router.get)
+			assertCalledWith(@router.get, '/foo/bar')
+			assertCalledWith(@router.get, '/foo/:id')
+
+	describe 'createProxy', ->
 		beforeEach ->
 			@req.url = @proxyPath
+			@req.route.path = @proxyPath
 			@req.query = {}
 			@settings.proxyUrls = {}
 
@@ -34,33 +46,44 @@ describe "ProxyManager", ->
 			@next.reset()
 			@request.reset()
 
-		it 'calls next when no match', ->
-			@proxyManager.call(@req, @res, @next)
-			sinon.assert.called(@next)
-			sinon.assert.notCalled(@request)
-
 		it 'does not calls next when match', ->
-			@settings.proxyUrls[@proxyPath] = '/'
-			@proxyManager.call(@req, @res, @next)
+			target = '/'
+			@settings.proxyUrls[@proxyPath] = target
+			@proxyManager.createProxy(target)(@req, @res, @next)
 			sinon.assert.notCalled(@next)
 			sinon.assert.called(@request)
 
 		it 'proxy full URL', ->
 			targetUrl = 'https://user:pass@foo.bar:123/pa/th.ext?query#hash'
 			@settings.proxyUrls[@proxyPath] = targetUrl
-			@proxyManager.call(@req)
+			@proxyManager.createProxy(targetUrl)(@req)
 			assertCalledWith(@request, targetUrl)
 
 		it 'overwrite query', ->
 			targetUrl = 'foo.bar/baz?query'
 			@req.query = { requestQuery: 'important' }
 			@settings.proxyUrls[@proxyPath] = targetUrl
-			@proxyManager.call(@req)
+			@proxyManager.createProxy(targetUrl)(@req)
 			newTargetUrl = 'foo.bar/baz?requestQuery=important'
 			assertCalledWith(@request, newTargetUrl)
 
 		it 'handles target objects', ->
-			targetUrl = { baseUrl: 'api.v1', path: '/pa/th'}
-			@settings.proxyUrls[@proxyPath] = targetUrl
-			@proxyManager.call(@req, @res, @next)
+			target = { baseUrl: 'api.v1', path: '/pa/th'}
+			@settings.proxyUrls[@proxyPath] = target
+			@proxyManager.createProxy(target)(@req, @res, @next)
 			assertCalledWith(@request, 'api.v1/pa/th')
+
+		it 'handles missing baseUrl', ->
+			target = { path: '/pa/th'}
+			@settings.proxyUrls[@proxyPath] = target
+			@proxyManager.createProxy(target)(@req, @res, @next)
+			assertCalledWith(@request, 'undefined/pa/th')
+
+		it 'handles dynamic path', ->
+			target = baseUrl: 'api.v1', path: (params) -> "/resource/#{params.id}"
+			@settings.proxyUrls['/res/:id'] = target
+			@req.url = '/res/123'
+			@req.route.path = '/res/:id'
+			@req.params = id: 123
+			@proxyManager.createProxy(target)(@req, @res, @next)
+			assertCalledWith(@request, 'api.v1/resource/123')