diff --git a/services/web/app/src/Features/Compile/ClsiCookieManager.js b/services/web/app/src/Features/Compile/ClsiCookieManager.js index 30f06b7266..bdfe0b3d85 100644 --- a/services/web/app/src/Features/Compile/ClsiCookieManager.js +++ b/services/web/app/src/Features/Compile/ClsiCookieManager.js @@ -29,50 +29,43 @@ const clsiCookiesEnabled = module.exports = function (backendGroup) { return { - buildKey(project_id, user_id) { + buildKey(project_id) { if (backendGroup != null) { - return `clsiserver:${backendGroup}:${project_id}:${user_id}` + return `clsiserver:${backendGroup}:${project_id}` } else { - return `clsiserver:${project_id}:${user_id}` + return `clsiserver:${project_id}` } }, - _getServerId(project_id, user_id, callback) { + _getServerId(project_id, callback) { if (callback == null) { callback = function (err, serverId) {} } - return rclient.get( - this.buildKey(project_id, user_id), - (err, serverId) => { - if (err != null) { - return callback(err) - } - if (serverId == null || serverId === '') { - return this._populateServerIdViaRequest( - project_id, - user_id, - callback - ) - } else { - return callback(null, serverId) - } + return rclient.get(this.buildKey(project_id), (err, serverId) => { + if (err != null) { + return callback(err) } - ) + if (serverId == null || serverId === '') { + return this._populateServerIdViaRequest(project_id, callback) + } else { + return callback(null, serverId) + } + }) }, - _populateServerIdViaRequest(project_id, user_id, callback) { + _populateServerIdViaRequest(project_id, callback) { if (callback == null) { callback = function (err, serverId) {} } const url = `${Settings.apis.clsi.url}/project/${project_id}/status` - request.post(url, (err, res, body) => { + return request.post(url, (err, res, body) => { if (err != null) { OError.tag(err, 'error getting initial server id for project', { project_id, }) return callback(err) } - this.setServerId(project_id, user_id, res, function (err, serverId) { + return this.setServerId(project_id, res, function (err, serverId) { if (err != null) { logger.warn( { err, project_id }, @@ -93,7 +86,7 @@ module.exports = function (backendGroup) { return cookies != null ? cookies[Settings.clsiCookie.key] : undefined }, - setServerId(project_id, user_id, response, callback) { + setServerId(project_id, response, callback) { if (callback == null) { callback = function (err, serverId) {} } @@ -103,55 +96,50 @@ module.exports = function (backendGroup) { const serverId = this._parseServerIdFromResponse(response) if (serverId == null) { // We don't get a cookie back if it hasn't changed - rclient.expire( - this.buildKey(project_id, user_id), + return rclient.expire( + this.buildKey(project_id), Settings.clsiCookie.ttl, err => callback(err, undefined) ) } if (rclient_secondary != null) { - this._setServerIdInRedis( - rclient_secondary, - project_id, - user_id, - serverId - ) + this._setServerIdInRedis(rclient_secondary, project_id, serverId) } - this._setServerIdInRedis(rclient, project_id, user_id, serverId, err => + return this._setServerIdInRedis(rclient, project_id, serverId, err => callback(err, serverId) ) }, - _setServerIdInRedis(rclient, project_id, user_id, serverId, callback) { + _setServerIdInRedis(rclient, project_id, serverId, callback) { if (callback == null) { callback = function (err) {} } rclient.setex( - this.buildKey(project_id, user_id), + this.buildKey(project_id), Settings.clsiCookie.ttl, serverId, callback ) }, - clearServerId(project_id, user_id, callback) { + clearServerId(project_id, callback) { if (callback == null) { callback = function (err) {} } if (!clsiCookiesEnabled) { return callback() } - return rclient.del(this.buildKey(project_id, user_id), callback) + return rclient.del(this.buildKey(project_id), callback) }, - getCookieJar(project_id, user_id, callback) { + getCookieJar(project_id, callback) { if (callback == null) { callback = function (err, jar, clsiServerId) {} } if (!clsiCookiesEnabled) { return callback(null, request.jar(), undefined) } - return this._getServerId(project_id, user_id, (err, serverId) => { + return this._getServerId(project_id, (err, serverId) => { if (err != null) { OError.tag(err, 'error getting server id', { project_id, diff --git a/services/web/app/src/Features/Compile/ClsiManager.js b/services/web/app/src/Features/Compile/ClsiManager.js index 7c395f49b0..f7e9b37531 100644 --- a/services/web/app/src/Features/Compile/ClsiManager.js +++ b/services/web/app/src/Features/Compile/ClsiManager.js @@ -132,7 +132,7 @@ const ClsiManager = { url: compilerUrl, method: 'POST', } - ClsiManager._makeRequest(projectId, userId, opts, callback) + ClsiManager._makeRequest(projectId, opts, callback) }, deleteAuxFiles(projectId, userId, options, clsiserverid, callback) { @@ -150,14 +150,13 @@ const ClsiManager = { } ClsiManager._makeRequestWithClsiServerId( projectId, - userId, opts, clsiserverid, clsiErr => { // always clear the project state from the docupdater, even if there // was a problem with the request to the clsi DocumentUpdaterHandler.clearProjectState(projectId, docUpdaterErr => { - ClsiCookieManager.clearServerId(projectId, userId, redisError => { + ClsiCookieManager.clearServerId(projectId, redisError => { if (clsiErr) { return callback( OError.tag(clsiErr, 'Failed to delete aux files', { projectId }) @@ -195,7 +194,7 @@ const ClsiManager = { } if (options.forceNewClsiServer) { // Clear clsi cookie, then try again - return ClsiCookieManager.clearServerId(projectId, userId, err => { + return ClsiCookieManager.clearServerId(projectId, err => { if (err) { return callback(err) } @@ -271,13 +270,7 @@ const ClsiManager = { ) }, - _makeRequestWithClsiServerId( - projectId, - userId, - opts, - clsiserverid, - callback - ) { + _makeRequestWithClsiServerId(projectId, opts, clsiserverid, callback) { if (clsiserverid) { // ignore cookies and newBackend, go straight to the clsi node opts.qs = Object.assign({ clsiserverid }, opts.qs) @@ -290,18 +283,17 @@ const ClsiManager = { callback(null, response, body) }) } else { - ClsiManager._makeRequest(projectId, userId, opts, callback) + ClsiManager._makeRequest(projectId, opts, callback) } }, - _makeRequest(projectId, userId, opts, callback) { + _makeRequest(projectId, opts, callback) { async.series( { currentBackend(cb) { const startTime = new Date() ClsiCookieManager.getCookieJar( projectId, - userId, (err, jar, clsiServerId) => { if (err != null) { return callback( @@ -326,7 +318,6 @@ const ClsiManager = { ) ClsiCookieManager.setServerId( projectId, - userId, response, (err, newClsiServerId) => { if (err != null) { @@ -359,7 +350,6 @@ const ClsiManager = { const startTime = new Date() ClsiManager._makeNewBackendRequest( projectId, - userId, opts, (err, response, body) => { if (err != null) { @@ -406,7 +396,7 @@ const ClsiManager = { ) }, - _makeNewBackendRequest(projectId, userId, baseOpts, callback) { + _makeNewBackendRequest(projectId, baseOpts, callback) { if (Settings.apis.clsi_new == null || Settings.apis.clsi_new.url == null) { return callback() } @@ -417,47 +407,42 @@ const ClsiManager = { Settings.apis.clsi_new.url ), } - NewBackendCloudClsiCookieManager.getCookieJar( - projectId, - userId, - (err, jar) => { + NewBackendCloudClsiCookieManager.getCookieJar(projectId, (err, jar) => { + if (err != null) { + return callback( + OError.tag(err, 'error getting cookie jar for CLSI request', { + projectId, + }) + ) + } + opts.jar = jar + const timer = new Metrics.Timer('compile.newBackend') + request(opts, (err, response, body) => { + timer.done() if (err != null) { return callback( - OError.tag(err, 'error getting cookie jar for CLSI request', { + OError.tag(err, 'error making request to new CLSI', { projectId, + opts, }) ) } - opts.jar = jar - const timer = new Metrics.Timer('compile.newBackend') - request(opts, (err, response, body) => { - timer.done() - if (err != null) { - return callback( - OError.tag(err, 'error making request to new CLSI', { - projectId, - opts, - }) - ) - } - NewBackendCloudClsiCookieManager.setServerId( - projectId, - userId, - response, - err => { - if (err != null) { - return callback( - OError.tag(err, 'error setting server id on new backend', { - projectId, - }) - ) - } - callback(null, response, body) + NewBackendCloudClsiCookieManager.setServerId( + projectId, + response, + err => { + if (err != null) { + return callback( + OError.tag(err, 'error setting server id on new backend', { + projectId, + }) + ) } - ) - }) - } - ) + callback(null, response, body) + } + ) + }) + }) }, _getCompilerUrl(compileGroup, projectId, userId, action) { @@ -486,7 +471,6 @@ const ClsiManager = { } ClsiManager._makeRequest( projectId, - userId, opts, (err, response, body, clsiServerId) => { if (err != null) { @@ -661,7 +645,7 @@ const ClsiManager = { getOutputFileStream(projectId, userId, buildId, outputFilePath, callback) { const url = `${Settings.apis.clsi.url}/project/${projectId}/user/${userId}/build/${buildId}/output/${outputFilePath}` - ClsiCookieManager.getCookieJar(projectId, userId, (err, jar) => { + ClsiCookieManager.getCookieJar(projectId, (err, jar) => { if (err != null) { return callback( OError.tag(err, 'Failed to get cookie jar', { @@ -892,7 +876,6 @@ const ClsiManager = { } ClsiManager._makeRequestWithClsiServerId( projectId, - userId, opts, clsiserverid, (err, response, body) => { diff --git a/services/web/app/src/Features/Compile/CompileController.js b/services/web/app/src/Features/Compile/CompileController.js index bd06436a08..5ff2ef98c0 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/services/web/app/src/Features/Compile/CompileController.js @@ -572,11 +572,10 @@ module.exports = CompileController = { function _getPersistenceOptions(req, projectId, callback) { const { clsiserverid } = req.query - const user_id = SessionManager.getLoggedInUserId(req) if (clsiserverid && typeof clsiserverid === 'string') { callback(null, { qs: { clsiserverid } }) } else { - ClsiCookieManager.getCookieJar(projectId, user_id, (err, jar) => { + ClsiCookieManager.getCookieJar(projectId, (err, jar) => { callback(err, { jar }) }) } diff --git a/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js b/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js index 6593c8ffe3..49345b3734 100644 --- a/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js +++ b/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js @@ -25,8 +25,7 @@ describe('ClsiCookieManager', function () { get: sinon.stub(), setex: sinon.stub().callsArg(3), } - this.project_id = '123423431321-proj-id' - this.user_id = 'abc-user-id' + this.project_id = '123423431321' this.request = { post: sinon.stub(), cookie: realRequst.cookie, @@ -66,10 +65,9 @@ describe('ClsiCookieManager', function () { this.redis.get.callsArgWith(1, null, 'clsi-7') return this.ClsiCookieManager._getServerId( this.project_id, - this.user_id, (err, serverId) => { this.redis.get - .calledWith(`clsiserver:${this.project_id}:${this.user_id}`) + .calledWith(`clsiserver:${this.project_id}`) .should.equal(true) serverId.should.equal('clsi-7') return done() @@ -80,14 +78,13 @@ describe('ClsiCookieManager', function () { it('should _populateServerIdViaRequest if no key is found', function (done) { this.ClsiCookieManager._populateServerIdViaRequest = sinon .stub() - .callsArgWith(2) + .callsArgWith(1) this.redis.get.callsArgWith(1, null) return this.ClsiCookieManager._getServerId( this.project_id, - this.user_id, (err, serverId) => { this.ClsiCookieManager._populateServerIdViaRequest - .calledWith(this.project_id, this.user_id) + .calledWith(this.project_id) .should.equal(true) return done() } @@ -97,14 +94,13 @@ describe('ClsiCookieManager', function () { it('should _populateServerIdViaRequest if no key is blank', function (done) { this.ClsiCookieManager._populateServerIdViaRequest = sinon .stub() - .callsArgWith(2) + .callsArgWith(1) this.redis.get.callsArgWith(1, null, '') return this.ClsiCookieManager._getServerId( this.project_id, - this.user_id, (err, serverId) => { this.ClsiCookieManager._populateServerIdViaRequest - .calledWith(this.project_id, this.user_id) + .calledWith(this.project_id) .should.equal(true) return done() } @@ -118,18 +114,16 @@ describe('ClsiCookieManager', function () { this.request.post.callsArgWith(1, null, this.response) return (this.ClsiCookieManager.setServerId = sinon .stub() - .callsArgWith(3, null, 'clsi-9')) + .callsArgWith(2, null, 'clsi-9')) }) it('should make a request to the clsi', function (done) { return this.ClsiCookieManager._populateServerIdViaRequest( this.project_id, - this.user_id, (err, serverId) => { const args = this.ClsiCookieManager.setServerId.args[0] args[0].should.equal(this.project_id) - args[1].should.equal(this.user_id) - args[2].should.deep.equal(this.response) + args[1].should.deep.equal(this.response) return done() } ) @@ -138,7 +132,6 @@ describe('ClsiCookieManager', function () { it('should return the server id', function (done) { return this.ClsiCookieManager._populateServerIdViaRequest( this.project_id, - this.user_id, (err, serverId) => { serverId.should.equal('clsi-9') return done() @@ -158,12 +151,11 @@ describe('ClsiCookieManager', function () { it('should set the server id with a ttl', function (done) { return this.ClsiCookieManager.setServerId( this.project_id, - this.user_id, this.response, err => { this.redis.setex .calledWith( - `clsiserver:${this.project_id}:${this.user_id}`, + `clsiserver:${this.project_id}`, this.settings.clsiCookie.ttl, 'clsi-8' ) @@ -176,7 +168,6 @@ describe('ClsiCookieManager', function () { it('should return the server id', function (done) { return this.ClsiCookieManager.setServerId( this.project_id, - this.user_id, this.response, (err, serverId) => { serverId.should.equal('clsi-8') @@ -195,7 +186,6 @@ describe('ClsiCookieManager', function () { })() return this.ClsiCookieManager.setServerId( this.project_id, - this.user_id, this.response, (err, serverId) => { this.redis.setex.called.should.equal(false) @@ -210,7 +200,6 @@ describe('ClsiCookieManager', function () { .returns(null) return this.ClsiCookieManager.setServerId( this.project_id, - this.user_id, this.response, (err, serverId) => { this.redis.setex.called.should.equal(false) @@ -238,12 +227,11 @@ describe('ClsiCookieManager', function () { .returns('clsi-8') return this.ClsiCookieManager.setServerId( this.project_id, - this.user_id, this.response, (err, serverId) => { this.redis_secondary.setex .calledWith( - `clsiserver:${this.project_id}:${this.user_id}`, + `clsiserver:${this.project_id}`, this.settings.clsiCookie.ttl, 'clsi-8' ) @@ -258,13 +246,12 @@ describe('ClsiCookieManager', function () { beforeEach(function () { return (this.ClsiCookieManager._getServerId = sinon .stub() - .callsArgWith(2, null, 'clsi-11')) + .callsArgWith(1, null, 'clsi-11')) }) it('should return a jar with the cookie set populated from redis', function (done) { return this.ClsiCookieManager.getCookieJar( this.project_id, - this.user_id, (err, jar) => { jar._jar.store.idx['clsi.example.com']['/'][ this.settings.clsiCookie.key @@ -287,7 +274,6 @@ describe('ClsiCookieManager', function () { })() return this.ClsiCookieManager.getCookieJar( this.project_id, - this.user_id, (err, jar) => { assert.deepEqual(jar, realRequst.jar()) return done() diff --git a/services/web/test/unit/src/Compile/ClsiManagerTests.js b/services/web/test/unit/src/Compile/ClsiManagerTests.js index 6cc26bc243..cbed621436 100644 --- a/services/web/test/unit/src/Compile/ClsiManagerTests.js +++ b/services/web/test/unit/src/Compile/ClsiManagerTests.js @@ -8,8 +8,8 @@ describe('ClsiManager', function () { this.jar = { cookie: 'stuff' } this.ClsiCookieManager = { clearServerId: sinon.stub().yields(), - getCookieJar: sinon.stub().callsArgWith(2, null, this.jar), - setServerId: sinon.stub().callsArgWith(3), + getCookieJar: sinon.stub().callsArgWith(1, null, this.jar), + setServerId: sinon.stub().callsArgWith(2), _getServerId: sinon.stub(), } this.ClsiStateManager = { @@ -73,7 +73,7 @@ describe('ClsiManager', function () { this.ClsiManager._buildRequest = sinon .stub() .callsArgWith(2, null, (this.request = 'mock-request')) - this.ClsiCookieManager._getServerId.callsArgWith(2, null, 'clsi3') + this.ClsiCookieManager._getServerId.callsArgWith(1, null, 'clsi3') }) describe('with a successful compile', function () { @@ -354,7 +354,7 @@ describe('ClsiManager', function () { beforeEach(function () { this.submission_id = 'submission-id' this.clsi_request = 'mock-request' - this.ClsiCookieManager._getServerId.callsArgWith(2, null, 'clsi3') + this.ClsiCookieManager._getServerId.callsArgWith(1, null, 'clsi3') }) describe('with a successful compile', function () { @@ -490,7 +490,6 @@ describe('ClsiManager', function () { this.ClsiManager._makeRequestWithClsiServerId .calledWith( this.project_id, - this.user_id, { method: 'DELETE', url: `${this.settings.apis.clsi.url}/project/${this.project_id}/user/${this.user_id}`, @@ -508,7 +507,7 @@ describe('ClsiManager', function () { it('should clear the clsi persistance', function () { this.ClsiCookieManager.clearServerId - .calledWith(this.project_id, this.user_id) + .calledWith(this.project_id) .should.equal(true) }) @@ -925,7 +924,7 @@ describe('ClsiManager', function () { this.ClsiManager._makeRequest = sinon .stub() .callsArgWith( - 3, + 2, null, { statusCode: 204 }, (this.body = { mock: 'foo' }) @@ -942,7 +941,7 @@ describe('ClsiManager', function () { it('should send the request to the CLSI', function () { const url = `${this.settings.apis.clsi.url}/project/${this.project_id}/user/${this.user_id}/compile` this.ClsiManager._makeRequest - .calledWith(this.project_id, this.user_id, { + .calledWith(this.project_id, { method: 'POST', url, json: this.req, @@ -960,7 +959,7 @@ describe('ClsiManager', function () { this.ClsiManager._makeRequest = sinon .stub() .callsArgWith( - 3, + 2, null, { statusCode: 500 }, (this.body = { mock: 'foo' }) @@ -1010,7 +1009,6 @@ describe('ClsiManager', function () { this.ClsiManager._makeRequestWithClsiServerId .calledWith( this.project_id, - this.user_id, { method: 'GET', url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`, @@ -1045,7 +1043,6 @@ describe('ClsiManager', function () { this.ClsiManager._makeRequestWithClsiServerId .calledWith( this.project_id, - this.user_id, { method: 'GET', url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`, @@ -1075,7 +1072,6 @@ describe('ClsiManager', function () { this.ClsiManager._makeRequestWithClsiServerId .calledWith( this.project_id, - this.user_id, { method: 'GET', url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`, @@ -1099,32 +1095,22 @@ describe('ClsiManager', function () { }) it('should process a request with a cookie jar', function (done) { - this.ClsiManager._makeRequest( - this.project_id, - this.user_id, - this.opts, - () => { - const args = this.request.args[0] - args[0].method.should.equal(this.opts.method) - args[0].url.should.equal(this.opts.url) - args[0].jar.should.equal(this.jar) - done() - } - ) + this.ClsiManager._makeRequest(this.project_id, this.opts, () => { + const args = this.request.args[0] + args[0].method.should.equal(this.opts.method) + args[0].url.should.equal(this.opts.url) + args[0].jar.should.equal(this.jar) + done() + }) }) it('should set the cookie again on response as it might have changed', function (done) { - this.ClsiManager._makeRequest( - this.project_id, - this.user_id, - this.opts, - () => { - this.ClsiCookieManager.setServerId - .calledWith(this.project_id, this.user_id, this.response) - .should.equal(true) - done() - } - ) + this.ClsiManager._makeRequest(this.project_id, this.opts, () => { + this.ClsiCookieManager.setServerId + .calledWith(this.project_id, this.response) + .should.equal(true) + done() + }) }) }) @@ -1142,7 +1128,6 @@ describe('ClsiManager', function () { it('should process a request with a cookie jar', function (done) { this.ClsiManager._makeRequestWithClsiServerId( this.project_id, - this.user_id, this.opts, undefined, err => { @@ -1160,13 +1145,12 @@ describe('ClsiManager', function () { it('should persist the cookie from the response', function (done) { this.ClsiManager._makeRequestWithClsiServerId( this.project_id, - this.user_id, this.opts, undefined, err => { if (err) return done(err) this.ClsiCookieManager.setServerId - .calledWith(this.project_id, this.user_id, this.response) + .calledWith(this.project_id, this.response) .should.equal(true) done() } @@ -1178,7 +1162,6 @@ describe('ClsiManager', function () { it('should not add a cookie jar', function (done) { this.ClsiManager._makeRequestWithClsiServerId( this.project_id, - this.user_id, this.opts, 'node-1', err => { @@ -1196,7 +1179,6 @@ describe('ClsiManager', function () { it('should not persist a cookie on response', function (done) { this.ClsiManager._makeRequestWithClsiServerId( this.project_id, - this.user_id, this.opts, 'node-1', err => { @@ -1222,7 +1204,6 @@ describe('ClsiManager', function () { it('should change the domain on the url', function (done) { this.ClsiManager._makeNewBackendRequest( this.project_id, - this.user_id, this.opts, () => { const args = this.request.args[0] @@ -1238,7 +1219,6 @@ describe('ClsiManager', function () { this.settings.apis.clsi_new = undefined this.ClsiManager._makeNewBackendRequest( this.project_id, - this.user_id, this.opts, err => { expect(err).to.equal(undefined) diff --git a/services/web/test/unit/src/Compile/CompileControllerTests.js b/services/web/test/unit/src/Compile/CompileControllerTests.js index 7a4d921b73..416bd9d721 100644 --- a/services/web/test/unit/src/Compile/CompileControllerTests.js +++ b/services/web/test/unit/src/Compile/CompileControllerTests.js @@ -49,7 +49,7 @@ describe('CompileController', function () { } this.jar = { cookie: 'stuff' } this.ClsiCookieManager = { - getCookieJar: sinon.stub().callsArgWith(2, null, this.jar), + getCookieJar: sinon.stub().callsArgWith(1, null, this.jar), } this.SessionManager = { getLoggedInUser: sinon.stub().callsArgWith(1, null, this.user),