From 12f0ec37beeb7b5786aab10f5a5f2d276737f96f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Bou=C3=A7as?= Date: Tue, 14 Aug 2018 23:52:48 +0100 Subject: [PATCH] feat: require currentSecret property when updating client secret --- dadi/lib/controller/clients.js | 34 +++++-- dadi/lib/model/acl/client.js | 75 ++++++++------- test/acceptance/acl/clients-api/put.js | 122 ++++++++++++++++++++++++- 3 files changed, 192 insertions(+), 39 deletions(-) diff --git a/dadi/lib/controller/clients.js b/dadi/lib/controller/clients.js index 5f17e267..994ee409 100644 --- a/dadi/lib/controller/clients.js +++ b/dadi/lib/controller/clients.js @@ -233,12 +233,24 @@ Clients.prototype.handleError = function (res, next) { errors: err.data.map(role => `Invalid role: ${role}`) }) + case 'INVALID_SECRET': + return help.sendBackJSON(400, res, next)(null, { + success: false, + errors: ['The current secret supplied is not valid'] + }) + case 'MISSING_FIELDS': return help.sendBackJSON(400, res, next)(null, { success: false, errors: err.data.map(field => `Missing field: ${field}`) }) + case 'MISSING_SECRET': + return help.sendBackJSON(400, res, next)(null, { + success: false, + errors: ['The current secret must be supplied via a `currentSecret` property'] + }) + case 'PROTECTED_DATA_FIELDS': return help.sendBackJSON(400, res, next)(null, { success: false, @@ -383,18 +395,28 @@ Clients.prototype.postRole = function (req, res, next) { } Clients.prototype.put = function (req, res, next) { + let clientIsAdmin = acl.client.isAdmin(req.dadiApiClient) + let update = req.body + // A client can only be updated by themselves or by an admin. - if ( - !acl.client.isAdmin(req.dadiApiClient) && - req.params.clientId !== req.dadiApiClient.clientId - ) { + if (!clientIsAdmin && req.params.clientId !== req.dadiApiClient.clientId) { return this.handleError(res, next)( acl.createError(req.dadiApiClient) ) } - return this.validateDataObject(req.body.data, req.dadiApiClient).then(() => { - return model.update(req.params.clientId, req.body) + return this.validateDataObject(update.data, req.dadiApiClient).then(() => { + if ( + !clientIsAdmin && + typeof update.secret === 'string' && + typeof update.currentSecret !== 'string' + ) { + return Promise.reject( + new Error('MISSING_SECRET') + ) + } + + return model.update(req.params.clientId, update) }).then(({results}) => { if (results.length === 0) { return Promise.reject( diff --git a/dadi/lib/model/acl/client.js b/dadi/lib/model/acl/client.js index 5fa3d01c..f808a4de 100644 --- a/dadi/lib/model/acl/client.js +++ b/dadi/lib/model/acl/client.js @@ -501,50 +501,61 @@ Client.prototype.setWriteCallback = function (callback) { * @return {Promise} */ Client.prototype.update = function (clientId, update) { + let findQuery = { + clientId + } + let isUpdatingSecret = typeof update.currentSecret === 'string' + + if (isUpdatingSecret) { + findQuery.secret = update.currentSecret + + delete update.currentSecret + } + return this.validate(update, { blockedFields: ['clientId'], partial: true }).then(() => { - let dataMerge = Promise.resolve(null) - - if (update.data) { - dataMerge = this.model.find({ - options: { + return this.model.find({ + options: { + fields: { data: 1 - }, - query: { - clientId - } - }).then(({results}) => { - if (results.length > 0) { - return results[0].data || {} } + }, + query: findQuery + }) + }).then(({results}) => { + if (results.length === 0) { + if (isUpdatingSecret) { + return Promise.reject( + new Error('INVALID_SECRET') + ) + } - return {} - }) + return Promise.reject( + new Error('CLIENT_NOT_FOUND') + ) } - return dataMerge.then(data => { - if (data) { - let mergedData = Object.assign({}, data, update.data) + if (update.data) { + let mergedData = Object.assign({}, results[0].data, update.data) - Object.keys(mergedData).forEach(key => { - if (mergedData[key] === null) { - delete mergedData[key] - } - }) + Object.keys(mergedData).forEach(key => { + if (mergedData[key] === null) { + delete mergedData[key] + } + }) - update.data = mergedData - } + update.data = mergedData + } - return this.model.update({ - query: { - clientId - }, - rawOutput: true, - update, - validate: false - }) + return this.model.update({ + query: { + clientId + }, + rawOutput: true, + update, + validate: false }) }) } diff --git a/test/acceptance/acl/clients-api/put.js b/test/acceptance/acl/clients-api/put.js index 6d98070f..7f756174 100644 --- a/test/acceptance/acl/clients-api/put.js +++ b/test/acceptance/acl/clients-api/put.js @@ -425,7 +425,124 @@ module.exports = () => { }) }) }) - }) + }) + + it('should return 400 when a non-admin client tries to update their secret without supplying the current secret', done => { + let testClient = { + clientId: 'apiClient', + secret: 'someSecret' + } + + help.createACLClient(testClient).then(() => { + client + .post(config.get('auth.tokenUrl')) + .set('content-type', 'application/json') + .send({ + clientId: testClient.clientId, + secret: testClient.secret + }) + .expect(200) + .expect('content-type', 'application/json') + .end((err, res) => { + if (err) return done(err) + + res.body.accessToken.should.be.String + + let bearerToken = res.body.accessToken + + client + .put(`/api/clients/${testClient.clientId}`) + .send({ + secret: 'aNewSecret' + }) + .set('content-type', 'application/json') + .set('Authorization', `Bearer ${bearerToken}`) + .expect('content-type', 'application/json') + .end((err, res) => { + res.statusCode.should.eql(400) + + res.body.success.should.eql(false) + res.body.errors[0].should.eql('The current secret must be supplied via a `currentSecret` property') + + client + .post(config.get('auth.tokenUrl')) + .set('content-type', 'application/json') + .send({ + clientId: testClient.clientId, + secret: 'someSecret' + }) + .expect(200) + .expect('content-type', 'application/json') + .end((err, res) => { + if (err) return done(err) + + res.body.accessToken.should.be.String + + done() + }) + }) + }) + }) + }) + + it('should return 400 when a non-admin client tries to update their secret and the current secret supplied is incorrect', done => { + let testClient = { + clientId: 'apiClient', + secret: 'someSecret' + } + + help.createACLClient(testClient).then(() => { + client + .post(config.get('auth.tokenUrl')) + .set('content-type', 'application/json') + .send({ + clientId: testClient.clientId, + secret: testClient.secret + }) + .expect(200) + .expect('content-type', 'application/json') + .end((err, res) => { + if (err) return done(err) + + res.body.accessToken.should.be.String + + let bearerToken = res.body.accessToken + + client + .put(`/api/clients/${testClient.clientId}`) + .send({ + currentSecret: 'this does not look right', + secret: 'aNewSecret' + }) + .set('content-type', 'application/json') + .set('Authorization', `Bearer ${bearerToken}`) + .expect('content-type', 'application/json') + .end((err, res) => { + res.statusCode.should.eql(400) + + res.body.success.should.eql(false) + res.body.errors[0].should.eql('The current secret supplied is not valid') + + client + .post(config.get('auth.tokenUrl')) + .set('content-type', 'application/json') + .send({ + clientId: testClient.clientId, + secret: 'someSecret' + }) + .expect(200) + .expect('content-type', 'application/json') + .end((err, res) => { + if (err) return done(err) + + res.body.accessToken.should.be.String + + done() + }) + }) + }) + }) + }) }) describe('success states', () => { @@ -456,6 +573,7 @@ module.exports = () => { client .put(`/api/clients/${testClient.clientId}`) .send({ + currentSecret: 'someSecret', secret: 'aNewSecret' }) .set('content-type', 'application/json') @@ -517,6 +635,7 @@ module.exports = () => { client .put('/api/client') .send({ + currentSecret: 'someSecret', secret: 'aNewSecret' }) .set('content-type', 'application/json') @@ -1067,6 +1186,7 @@ module.exports = () => { client .put('/api/client') .send({ + currentSecret: 'someSecret', secret: 'something' }) .set('content-type', 'application/json')