From 2b7c7e84ef137caeb5a2e5ced7a69525a169a5c8 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Fri, 20 Aug 2021 09:14:59 +0100 Subject: [PATCH] Clsi user id mapping (#4736) * add user-id into clsi project mapping * add user_id on backend group clsi key GitOrigin-RevId: ebbf025f5cd88848b44f35a46045d112ea6b4c3b --- .../src/Features/Compile/ClsiCookieManager.js | 66 ++++++++------ .../app/src/Features/Compile/ClsiManager.js | 91 +++++++++++-------- .../src/Features/Compile/CompileController.js | 3 +- .../src/Compile/ClsiCookieManagerTests.js | 36 +++++--- .../test/unit/src/Compile/ClsiManagerTests.js | 64 ++++++++----- .../src/Compile/CompileControllerTests.js | 2 +- 6 files changed, 163 insertions(+), 99 deletions(-) diff --git a/services/web/app/src/Features/Compile/ClsiCookieManager.js b/services/web/app/src/Features/Compile/ClsiCookieManager.js index bdfe0b3d85..30f06b7266 100644 --- a/services/web/app/src/Features/Compile/ClsiCookieManager.js +++ b/services/web/app/src/Features/Compile/ClsiCookieManager.js @@ -29,43 +29,50 @@ const clsiCookiesEnabled = module.exports = function (backendGroup) { return { - buildKey(project_id) { + buildKey(project_id, user_id) { if (backendGroup != null) { - return `clsiserver:${backendGroup}:${project_id}` + return `clsiserver:${backendGroup}:${project_id}:${user_id}` } else { - return `clsiserver:${project_id}` + return `clsiserver:${project_id}:${user_id}` } }, - _getServerId(project_id, callback) { + _getServerId(project_id, user_id, callback) { if (callback == null) { callback = function (err, serverId) {} } - return rclient.get(this.buildKey(project_id), (err, serverId) => { - if (err != null) { - return callback(err) + 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) + } } - if (serverId == null || serverId === '') { - return this._populateServerIdViaRequest(project_id, callback) - } else { - return callback(null, serverId) - } - }) + ) }, - _populateServerIdViaRequest(project_id, callback) { + _populateServerIdViaRequest(project_id, user_id, callback) { if (callback == null) { callback = function (err, serverId) {} } const url = `${Settings.apis.clsi.url}/project/${project_id}/status` - return request.post(url, (err, res, body) => { + request.post(url, (err, res, body) => { if (err != null) { OError.tag(err, 'error getting initial server id for project', { project_id, }) return callback(err) } - return this.setServerId(project_id, res, function (err, serverId) { + this.setServerId(project_id, user_id, res, function (err, serverId) { if (err != null) { logger.warn( { err, project_id }, @@ -86,7 +93,7 @@ module.exports = function (backendGroup) { return cookies != null ? cookies[Settings.clsiCookie.key] : undefined }, - setServerId(project_id, response, callback) { + setServerId(project_id, user_id, response, callback) { if (callback == null) { callback = function (err, serverId) {} } @@ -96,50 +103,55 @@ module.exports = function (backendGroup) { const serverId = this._parseServerIdFromResponse(response) if (serverId == null) { // We don't get a cookie back if it hasn't changed - return rclient.expire( - this.buildKey(project_id), + rclient.expire( + this.buildKey(project_id, user_id), Settings.clsiCookie.ttl, err => callback(err, undefined) ) } if (rclient_secondary != null) { - this._setServerIdInRedis(rclient_secondary, project_id, serverId) + this._setServerIdInRedis( + rclient_secondary, + project_id, + user_id, + serverId + ) } - return this._setServerIdInRedis(rclient, project_id, serverId, err => + this._setServerIdInRedis(rclient, project_id, user_id, serverId, err => callback(err, serverId) ) }, - _setServerIdInRedis(rclient, project_id, serverId, callback) { + _setServerIdInRedis(rclient, project_id, user_id, serverId, callback) { if (callback == null) { callback = function (err) {} } rclient.setex( - this.buildKey(project_id), + this.buildKey(project_id, user_id), Settings.clsiCookie.ttl, serverId, callback ) }, - clearServerId(project_id, callback) { + clearServerId(project_id, user_id, callback) { if (callback == null) { callback = function (err) {} } if (!clsiCookiesEnabled) { return callback() } - return rclient.del(this.buildKey(project_id), callback) + return rclient.del(this.buildKey(project_id, user_id), callback) }, - getCookieJar(project_id, callback) { + getCookieJar(project_id, user_id, callback) { if (callback == null) { callback = function (err, jar, clsiServerId) {} } if (!clsiCookiesEnabled) { return callback(null, request.jar(), undefined) } - return this._getServerId(project_id, (err, serverId) => { + return this._getServerId(project_id, user_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 f7e9b37531..7c395f49b0 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, opts, callback) + ClsiManager._makeRequest(projectId, userId, opts, callback) }, deleteAuxFiles(projectId, userId, options, clsiserverid, callback) { @@ -150,13 +150,14 @@ 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, redisError => { + ClsiCookieManager.clearServerId(projectId, userId, redisError => { if (clsiErr) { return callback( OError.tag(clsiErr, 'Failed to delete aux files', { projectId }) @@ -194,7 +195,7 @@ const ClsiManager = { } if (options.forceNewClsiServer) { // Clear clsi cookie, then try again - return ClsiCookieManager.clearServerId(projectId, err => { + return ClsiCookieManager.clearServerId(projectId, userId, err => { if (err) { return callback(err) } @@ -270,7 +271,13 @@ const ClsiManager = { ) }, - _makeRequestWithClsiServerId(projectId, opts, clsiserverid, callback) { + _makeRequestWithClsiServerId( + projectId, + userId, + opts, + clsiserverid, + callback + ) { if (clsiserverid) { // ignore cookies and newBackend, go straight to the clsi node opts.qs = Object.assign({ clsiserverid }, opts.qs) @@ -283,17 +290,18 @@ const ClsiManager = { callback(null, response, body) }) } else { - ClsiManager._makeRequest(projectId, opts, callback) + ClsiManager._makeRequest(projectId, userId, opts, callback) } }, - _makeRequest(projectId, opts, callback) { + _makeRequest(projectId, userId, opts, callback) { async.series( { currentBackend(cb) { const startTime = new Date() ClsiCookieManager.getCookieJar( projectId, + userId, (err, jar, clsiServerId) => { if (err != null) { return callback( @@ -318,6 +326,7 @@ const ClsiManager = { ) ClsiCookieManager.setServerId( projectId, + userId, response, (err, newClsiServerId) => { if (err != null) { @@ -350,6 +359,7 @@ const ClsiManager = { const startTime = new Date() ClsiManager._makeNewBackendRequest( projectId, + userId, opts, (err, response, body) => { if (err != null) { @@ -396,7 +406,7 @@ const ClsiManager = { ) }, - _makeNewBackendRequest(projectId, baseOpts, callback) { + _makeNewBackendRequest(projectId, userId, baseOpts, callback) { if (Settings.apis.clsi_new == null || Settings.apis.clsi_new.url == null) { return callback() } @@ -407,42 +417,47 @@ const ClsiManager = { Settings.apis.clsi_new.url ), } - 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() + NewBackendCloudClsiCookieManager.getCookieJar( + projectId, + userId, + (err, jar) => { if (err != null) { return callback( - OError.tag(err, 'error making request to new CLSI', { + OError.tag(err, 'error getting cookie jar for CLSI request', { projectId, - opts, }) ) } - 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) + 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) + } + ) + }) + } + ) }, _getCompilerUrl(compileGroup, projectId, userId, action) { @@ -471,6 +486,7 @@ const ClsiManager = { } ClsiManager._makeRequest( projectId, + userId, opts, (err, response, body, clsiServerId) => { if (err != null) { @@ -645,7 +661,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, (err, jar) => { + ClsiCookieManager.getCookieJar(projectId, userId, (err, jar) => { if (err != null) { return callback( OError.tag(err, 'Failed to get cookie jar', { @@ -876,6 +892,7 @@ 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 5ff2ef98c0..bd06436a08 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/services/web/app/src/Features/Compile/CompileController.js @@ -572,10 +572,11 @@ 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, (err, jar) => { + ClsiCookieManager.getCookieJar(projectId, user_id, (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 49345b3734..6593c8ffe3 100644 --- a/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js +++ b/services/web/test/unit/src/Compile/ClsiCookieManagerTests.js @@ -25,7 +25,8 @@ describe('ClsiCookieManager', function () { get: sinon.stub(), setex: sinon.stub().callsArg(3), } - this.project_id = '123423431321' + this.project_id = '123423431321-proj-id' + this.user_id = 'abc-user-id' this.request = { post: sinon.stub(), cookie: realRequst.cookie, @@ -65,9 +66,10 @@ 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}`) + .calledWith(`clsiserver:${this.project_id}:${this.user_id}`) .should.equal(true) serverId.should.equal('clsi-7') return done() @@ -78,13 +80,14 @@ describe('ClsiCookieManager', function () { it('should _populateServerIdViaRequest if no key is found', function (done) { this.ClsiCookieManager._populateServerIdViaRequest = sinon .stub() - .callsArgWith(1) + .callsArgWith(2) 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) + .calledWith(this.project_id, this.user_id) .should.equal(true) return done() } @@ -94,13 +97,14 @@ describe('ClsiCookieManager', function () { it('should _populateServerIdViaRequest if no key is blank', function (done) { this.ClsiCookieManager._populateServerIdViaRequest = sinon .stub() - .callsArgWith(1) + .callsArgWith(2) 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) + .calledWith(this.project_id, this.user_id) .should.equal(true) return done() } @@ -114,16 +118,18 @@ describe('ClsiCookieManager', function () { this.request.post.callsArgWith(1, null, this.response) return (this.ClsiCookieManager.setServerId = sinon .stub() - .callsArgWith(2, null, 'clsi-9')) + .callsArgWith(3, 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.deep.equal(this.response) + args[1].should.equal(this.user_id) + args[2].should.deep.equal(this.response) return done() } ) @@ -132,6 +138,7 @@ 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() @@ -151,11 +158,12 @@ 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}`, + `clsiserver:${this.project_id}:${this.user_id}`, this.settings.clsiCookie.ttl, 'clsi-8' ) @@ -168,6 +176,7 @@ 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') @@ -186,6 +195,7 @@ describe('ClsiCookieManager', function () { })() return this.ClsiCookieManager.setServerId( this.project_id, + this.user_id, this.response, (err, serverId) => { this.redis.setex.called.should.equal(false) @@ -200,6 +210,7 @@ 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) @@ -227,11 +238,12 @@ 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}`, + `clsiserver:${this.project_id}:${this.user_id}`, this.settings.clsiCookie.ttl, 'clsi-8' ) @@ -246,12 +258,13 @@ describe('ClsiCookieManager', function () { beforeEach(function () { return (this.ClsiCookieManager._getServerId = sinon .stub() - .callsArgWith(1, null, 'clsi-11')) + .callsArgWith(2, 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 @@ -274,6 +287,7 @@ 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 cbed621436..6cc26bc243 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(1, null, this.jar), - setServerId: sinon.stub().callsArgWith(2), + getCookieJar: sinon.stub().callsArgWith(2, null, this.jar), + setServerId: sinon.stub().callsArgWith(3), _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(1, null, 'clsi3') + this.ClsiCookieManager._getServerId.callsArgWith(2, 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(1, null, 'clsi3') + this.ClsiCookieManager._getServerId.callsArgWith(2, null, 'clsi3') }) describe('with a successful compile', function () { @@ -490,6 +490,7 @@ 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}`, @@ -507,7 +508,7 @@ describe('ClsiManager', function () { it('should clear the clsi persistance', function () { this.ClsiCookieManager.clearServerId - .calledWith(this.project_id) + .calledWith(this.project_id, this.user_id) .should.equal(true) }) @@ -924,7 +925,7 @@ describe('ClsiManager', function () { this.ClsiManager._makeRequest = sinon .stub() .callsArgWith( - 2, + 3, null, { statusCode: 204 }, (this.body = { mock: 'foo' }) @@ -941,7 +942,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, { + .calledWith(this.project_id, this.user_id, { method: 'POST', url, json: this.req, @@ -959,7 +960,7 @@ describe('ClsiManager', function () { this.ClsiManager._makeRequest = sinon .stub() .callsArgWith( - 2, + 3, null, { statusCode: 500 }, (this.body = { mock: 'foo' }) @@ -1009,6 +1010,7 @@ 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`, @@ -1043,6 +1045,7 @@ 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`, @@ -1072,6 +1075,7 @@ 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`, @@ -1095,22 +1099,32 @@ describe('ClsiManager', function () { }) it('should process a request with a cookie jar', function (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() - }) + 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() + } + ) }) it('should set the cookie again on response as it might have changed', function (done) { - this.ClsiManager._makeRequest(this.project_id, this.opts, () => { - this.ClsiCookieManager.setServerId - .calledWith(this.project_id, this.response) - .should.equal(true) - 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() + } + ) }) }) @@ -1128,6 +1142,7 @@ 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 => { @@ -1145,12 +1160,13 @@ 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.response) + .calledWith(this.project_id, this.user_id, this.response) .should.equal(true) done() } @@ -1162,6 +1178,7 @@ 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 => { @@ -1179,6 +1196,7 @@ 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 => { @@ -1204,6 +1222,7 @@ 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] @@ -1219,6 +1238,7 @@ 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 416bd9d721..7a4d921b73 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(1, null, this.jar), + getCookieJar: sinon.stub().callsArgWith(2, null, this.jar), } this.SessionManager = { getLoggedInUser: sinon.stub().callsArgWith(1, null, this.user),