From e3471639404d133d7e8444b9faeb6978c33e843c Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Wed, 27 Sep 2017 10:28:31 +0200 Subject: [PATCH] Removed bypassing option filtering in User model no issue - the logic here bypasses filtering options! - that is wrong, because if we filter out certain options e.g. include - the tests from the previous commit fail because of this - if we don't fix this logic, the tests won't pass, because as said, you can bypass certain logic e.g. remove roles from include - this has worked before, because we passed the wrong options via the API layer - was introduced here https://github.com/TryGhost/Ghost/commit/014e2c88dd1dd32a59dd023b0b33384c54aec026, because of https://github.com/TryGhost/Ghost/pull/6122 - add proper tests to proof that these queries work!! --- core/server/models/user.js | 8 +--- .../functional/routes/api/public_api_spec.js | 46 +++++++++++++++++++ core/test/functional/routes/api/users_spec.js | 46 +++++++++++++++++++ 3 files changed, 93 insertions(+), 7 deletions(-) diff --git a/core/server/models/user.js b/core/server/models/user.js index f80e82271a..28f519067b 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -336,7 +336,6 @@ User = ghostBookshelf.Model.extend({ findOne: function findOne(dataToClone, options) { var query, status, - optInc, data = _.cloneDeep(dataToClone), lookupRole = data.role; @@ -348,14 +347,9 @@ User = ghostBookshelf.Model.extend({ status = data.status; delete data.status; - options = _.cloneDeep(options || {}); - optInc = options.include; - options.withRelated = _.union(options.withRelated, options.include); - data = this.filterData(data); options = this.filterOptions(options, 'findOne'); - delete options.include; - options.include = optInc; + options.withRelated = _.union(options.withRelated, options.include); // Support finding by role if (lookupRole) { diff --git a/core/test/functional/routes/api/public_api_spec.js b/core/test/functional/routes/api/public_api_spec.js index 1528190018..46a32eee51 100644 --- a/core/test/functional/routes/api/public_api_spec.js +++ b/core/test/functional/routes/api/public_api_spec.js @@ -373,6 +373,52 @@ describe('Public API', function () { }); }); + it('browse user by slug: count.posts', function (done) { + request.get(testUtils.API.getApiQuery('users/slug/ghost/?client_id=ghost-admin&client_secret=not_available&include=count.posts')) + .set('Origin', testUtils.API.getURL()) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.not.exist(res.headers['x-cache-invalidate']); + var jsonResponse = res.body; + + should.exist(jsonResponse.users); + jsonResponse.users.should.have.length(1); + + // We don't expose the email address. + testUtils.API.checkResponse(jsonResponse.users[0], 'user', ['count'], ['email']); + done(); + }); + }); + + it('browse user by id: count.posts', function (done) { + request.get(testUtils.API.getApiQuery('users/1/?client_id=ghost-admin&client_secret=not_available&include=count.posts')) + .set('Origin', testUtils.API.getURL()) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.not.exist(res.headers['x-cache-invalidate']); + var jsonResponse = res.body; + + should.exist(jsonResponse.users); + jsonResponse.users.should.have.length(1); + + // We don't expose the email address. + testUtils.API.checkResponse(jsonResponse.users[0], 'user', ['count'], ['email']); + done(); + }); + }); + it('[unsupported] browse user by email', function (done) { request .get(testUtils.API.getApiQuery('users/email/ghost-author@ghost.org/?client_id=ghost-admin&client_secret=not_available')) diff --git a/core/test/functional/routes/api/users_spec.js b/core/test/functional/routes/api/users_spec.js index 6c762720c2..be82b9ce29 100644 --- a/core/test/functional/routes/api/users_spec.js +++ b/core/test/functional/routes/api/users_spec.js @@ -312,6 +312,52 @@ describe('User API', function () { }); }); + it('can retrieve a user by slug with count.posts', function (done) { + request.get(testUtils.API.getApiQuery('users/slug/joe-bloggs/?include=count.posts')) + .set('Authorization', 'Bearer ' + ownerAccessToken) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.not.exist(res.headers['x-cache-invalidate']); + var jsonResponse = res.body; + should.exist(jsonResponse.users); + should.not.exist(jsonResponse.meta); + + jsonResponse.users.should.have.length(1); + testUtils.API.checkResponse(jsonResponse.users[0], 'user', ['count']); + + done(); + }); + }); + + it('can retrieve a user by id with count.posts', function (done) { + request.get(testUtils.API.getApiQuery('users/1/?include=count.posts')) + .set('Authorization', 'Bearer ' + ownerAccessToken) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.not.exist(res.headers['x-cache-invalidate']); + var jsonResponse = res.body; + should.exist(jsonResponse.users); + should.not.exist(jsonResponse.meta); + + jsonResponse.users.should.have.length(1); + testUtils.API.checkResponse(jsonResponse.users[0], 'user', ['count']); + + done(); + }); + }); + it('can\'t retrieve non existent user by id', function (done) { request.get(testUtils.API.getApiQuery('users/' + ObjectId.generate() + '/')) .set('Authorization', 'Bearer ' + ownerAccessToken)