From 506a0c3e9e51ff4a2ae5bfe0afe84cc5d0bcee18 Mon Sep 17 00:00:00 2001 From: Katharina Irrgang Date: Thu, 28 Sep 2017 15:00:52 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=A5=20=20=20Removed=20certain=20fields?= =?UTF-8?q?=20from=20public=20user=20response=20(#9069)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no issue * Comment current state of toJSON for user model - currently the user model does not return the email if the context is app/external/public OR if there is no context object at all - i am not 100% sure why if there is no context we should not return the email address - i think no context means internal access - maybe change this condition cc @ErisDS * Extend our access rules plugin - we already have a instance method to determine which context is used - this relies on passing options into `.forge` - but we almost never pass the context into the forge call - added @TODO - provide another static method to determine the context based on the options object passed from outside * Use the new static function for existing code * Add comment where the external context is used * Remove certain fields from a public request (User model only) * Tests: support `checkResponse` for a public request - start with an optional option pattern - i would love to get rid of checkResponse('user', null, null, null) - still support old style for now - a resoure can define the default response fields and public response fields * Tests: adapt public api test * Tests: adapt api user test - use new option pattern for `checkResponse` - eww null, null, null, null.... * Revert the usage of the access rules plugin --- core/server/models/user.js | 16 +++- core/server/permissions/parse-context.js | 1 + .../functional/routes/api/public_api_spec.js | 16 ++-- core/test/integration/api/api_users_spec.js | 26 +++--- core/test/utils/api.js | 84 +++++++++++-------- 5 files changed, 87 insertions(+), 56 deletions(-) diff --git a/core/server/models/user.js b/core/server/models/user.js index 28f519067b..352b25ecb4 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -192,14 +192,28 @@ User = ghostBookshelf.Model.extend({ options = options || {}; var attrs = ghostBookshelf.Model.prototype.toJSON.call(this, options); + // remove password hash for security reasons delete attrs.password; delete attrs.ghost_auth_access_token; + // NOTE: We don't expose the email address for for external, app and public context. + // @TODO: Why? External+Public is actually the same context? Was also mentioned here https://github.com/TryGhost/Ghost/issues/9043 if (!options || !options.context || (!options.context.user && !options.context.internal)) { delete attrs.email; } + // We don't expose these fields when fetching data via the public API. + if (options && options.context && options.context.public) { + delete attrs.created_at; + delete attrs.created_by; + delete attrs.updated_at; + delete attrs.updated_by; + delete attrs.last_seen; + delete attrs.status; + delete attrs.ghost_auth_id; + } + return attrs; }, @@ -313,7 +327,7 @@ User = ghostBookshelf.Model.extend({ permittedOptionsToReturn = permittedOptionsToReturn.concat(validOptions[methodName]); } - // CASE: The `include` paramater is allowed when using the public API, but not the `roles` value. + // CASE: The `include` parameter is allowed when using the public API, but not the `roles` value. // Otherwise we expose too much information. if (options && options.context && options.context.public) { if (options.include && options.include.indexOf('roles') !== -1) { diff --git a/core/server/permissions/parse-context.js b/core/server/permissions/parse-context.js index 237319994a..acc1a9cd4a 100644 --- a/core/server/permissions/parse-context.js +++ b/core/server/permissions/parse-context.js @@ -15,6 +15,7 @@ module.exports = function parseContext(context) { public: true }; + // NOTE: We use the `external` context for subscribers only at the moment. if (context && (context === 'external' || context.external)) { parsed.external = true; parsed.public = false; diff --git a/core/test/functional/routes/api/public_api_spec.js b/core/test/functional/routes/api/public_api_spec.js index 46a32eee51..5b5fb0bfe9 100644 --- a/core/test/functional/routes/api/public_api_spec.js +++ b/core/test/functional/routes/api/public_api_spec.js @@ -322,7 +322,7 @@ describe('Public API', function () { jsonResponse.users.should.have.length(2); // We don't expose the email address. - testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, ['email']); + testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, null, null, {public: true}); done(); }); }); @@ -345,7 +345,7 @@ describe('Public API', function () { jsonResponse.users.should.have.length(2); // We don't expose the email address. - testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, ['email']); + testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, null, null, {public: true}); done(); }); }); @@ -368,7 +368,7 @@ describe('Public API', function () { jsonResponse.users.should.have.length(1); // We don't expose the email address. - testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, ['email']); + testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, null, null, {public: true}); done(); }); }); @@ -391,7 +391,7 @@ describe('Public API', function () { jsonResponse.users.should.have.length(1); // We don't expose the email address. - testUtils.API.checkResponse(jsonResponse.users[0], 'user', ['count'], ['email']); + testUtils.API.checkResponse(jsonResponse.users[0], 'user', ['count'], null, null, {public: true}); done(); }); }); @@ -414,7 +414,7 @@ describe('Public API', function () { jsonResponse.users.should.have.length(1); // We don't expose the email address. - testUtils.API.checkResponse(jsonResponse.users[0], 'user', ['count'], ['email']); + testUtils.API.checkResponse(jsonResponse.users[0], 'user', ['count'], null, null, {public: true}); done(); }); }); @@ -453,7 +453,7 @@ describe('Public API', function () { jsonResponse.users.should.have.length(1); // We don't expose the email address. - testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, ['email']); + testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, null, null, {public: true}); done(); }); }); @@ -476,7 +476,7 @@ describe('Public API', function () { jsonResponse.users.should.have.length(2); // We don't expose the email address. - testUtils.API.checkResponse(jsonResponse.users[0], 'user', ['count'], ['email']); + testUtils.API.checkResponse(jsonResponse.users[0], 'user', ['count'], null, null, {public: true}); done(); }); }); @@ -499,7 +499,7 @@ describe('Public API', function () { jsonResponse.users.should.have.length(2); // We don't expose the email address. - testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, ['email']); + testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, null, null, {public: true}); done(); }); }); diff --git a/core/test/integration/api/api_users_spec.js b/core/test/integration/api/api_users_spec.js index 2fd8d0471a..4ce2fb90b6 100644 --- a/core/test/integration/api/api_users_spec.js +++ b/core/test/integration/api/api_users_spec.js @@ -56,7 +56,7 @@ describe('Users API', function () { var userData = testUtils.DataGenerator.forModel.users[0]; models.User.check({email: userData.email, password: userData.password}).then(function (user) { - return UserAPI.read({id: user.id}); + return UserAPI.read(_.merge({id: user.id}, context.internal)); }).then(function (response) { response.users[0].created_at.should.be.an.instanceof(Date); response.users[0].updated_at.should.be.an.instanceof(Date); @@ -67,15 +67,15 @@ describe('Users API', function () { }); describe('Browse', function () { - function checkBrowseResponse(response, count, additional, missing) { + function checkBrowseResponse(response, count, additional, missing, only, options) { should.exist(response); testUtils.API.checkResponse(response, 'users'); should.exist(response.users); response.users.should.have.length(count); - testUtils.API.checkResponse(response.users[0], 'user', additional, missing); - testUtils.API.checkResponse(response.users[1], 'user', additional, missing); - testUtils.API.checkResponse(response.users[2], 'user', additional, missing); - testUtils.API.checkResponse(response.users[3], 'user', additional, missing); + testUtils.API.checkResponse(response.users[0], 'user', additional, missing, only, options); + testUtils.API.checkResponse(response.users[1], 'user', additional, missing, only, options); + testUtils.API.checkResponse(response.users[2], 'user', additional, missing, only, options); + testUtils.API.checkResponse(response.users[3], 'user', additional, missing, only, options); } it('Owner can browse', function (done) { @@ -108,7 +108,7 @@ describe('Users API', function () { it('No-auth CAN browse, but only gets filtered active users', function (done) { UserAPI.browse().then(function (response) { - checkBrowseResponse(response, 7, null, ['email']); + checkBrowseResponse(response, 7, null, null, null, {public: true}); done(); }).catch(done); }); @@ -220,20 +220,18 @@ describe('Users API', function () { }); describe('Read', function () { - function checkReadResponse(response, noEmail) { + function checkReadResponse(response, noEmail, additional, missing, only, options) { should.exist(response); should.not.exist(response.meta); should.exist(response.users); response.users[0].id.should.eql(testUtils.DataGenerator.Content.users[0].id); if (noEmail) { - // Email should be missing - testUtils.API.checkResponse(response.users[0], 'user', [], ['email']); - should.not.exist(response.users[0].email); + testUtils.API.checkResponse(response.users[0], 'user', additional, missing, only, options); } else { - testUtils.API.checkResponse(response.users[0], 'user'); + testUtils.API.checkResponse(response.users[0], 'user', additional, missing, only, options); + response.users[0].created_at.should.be.an.instanceof(Date); } - response.users[0].created_at.should.be.an.instanceof(Date); } it('Owner can read', function (done) { @@ -268,7 +266,7 @@ describe('Users API', function () { it('No-auth can read', function (done) { UserAPI.read({id: userIdFor.owner}).then(function (response) { - checkReadResponse(response, true); + checkReadResponse(response, true, null, null, null, {public: true}); done(); }).catch(done); }); diff --git a/core/test/utils/api.js b/core/test/utils/api.js index eba481819b..62780952f6 100644 --- a/core/test/utils/api.js +++ b/core/test/utils/api.js @@ -1,44 +1,59 @@ -var _ = require('lodash'), - url = require('url'), - moment = require('moment'), - config = require('../../server/config'), - schema = require('../../server/data/schema').tables, - ApiRouteBase = '/ghost/api/v0.1/', - host = config.get('server').host, - port = config.get('server').port, - protocol = 'http://', +var _ = require('lodash'), + url = require('url'), + moment = require('moment'), + config = require('../../server/config'), + schema = require('../../server/data/schema').tables, + ApiRouteBase = '/ghost/api/v0.1/', + host = config.get('server').host, + port = config.get('server').port, + protocol = 'http://', expectedProperties = { // API top level - posts: ['posts', 'meta'], - tags: ['tags', 'meta'], - users: ['users', 'meta'], - settings: ['settings', 'meta'], - subscribers: ['subscribers', 'meta'], - roles: ['roles'], - pagination: ['page', 'limit', 'pages', 'total', 'next', 'prev'], - slugs: ['slugs'], - slug: ['slug'], - // object / model level - // Post API - post: _(schema.posts).keys() - // does not return all formats by default + posts: ['posts', 'meta'], + tags: ['tags', 'meta'], + users: ['users', 'meta'], + settings: ['settings', 'meta'], + subscribers: ['subscribers', 'meta'], + roles: ['roles'], + pagination: ['page', 'limit', 'pages', 'total', 'next', 'prev'], + slugs: ['slugs'], + slug: ['slug'], + post: _(schema.posts) + .keys() + // by default we only return html .without('mobiledoc', 'amp', 'plaintext') // swaps author_id to author, and always returns computed properties: url, comment_id, primary_tag .without('author_id').concat('author', 'url', 'comment_id', 'primary_tag') .value(), - // User API always removes the password field - user: _(schema.users).keys().without('password').without('ghost_auth_access_token').value(), + user: { + default: _(schema.users).keys().without('password').without('ghost_auth_access_token').value(), + public: _(schema.users) + .keys() + .without( + 'password', + 'email', + 'ghost_auth_access_token', + 'ghost_auth_id', + 'created_at', + 'created_by', + 'updated_at', + 'updated_by', + 'last_seen', + 'status' + ) + .value() + }, // Tag API swaps parent_id to parent - tag: _(schema.tags).keys().without('parent_id').concat('parent').value(), - setting: _.keys(schema.settings), + tag: _(schema.tags).keys().without('parent_id').concat('parent').value(), + setting: _.keys(schema.settings), subscriber: _.keys(schema.subscribers), accesstoken: _.keys(schema.accesstokens), - role: _.keys(schema.roles), - permission: _.keys(schema.permissions), + role: _.keys(schema.roles), + permission: _.keys(schema.permissions), notification: ['type', 'message', 'status', 'id', 'dismissible', 'location'], - theme: ['name', 'package', 'active'], - themes: ['themes'], - invites: _(schema.invites).keys().without('token').value() + theme: ['name', 'package', 'active'], + themes: ['themes'], + invites: _(schema.invites).keys().without('token').value() }; function getApiQuery(route) { @@ -83,8 +98,11 @@ function checkResponseValue(jsonResponse, expectedProperties) { providedProperties.length.should.eql(expectedProperties.length); } -function checkResponse(jsonResponse, objectType, additionalProperties, missingProperties, onlyProperties) { - var checkProperties = expectedProperties[objectType]; +// @TODO: support options pattern only, it's annoying to call checkResponse(null, null, null, something) +function checkResponse(jsonResponse, objectType, additionalProperties, missingProperties, onlyProperties, options) { + options = options || {}; + + var checkProperties = options.public ? (expectedProperties[objectType].public || expectedProperties[objectType]) : (expectedProperties[objectType].default || expectedProperties[objectType]); checkProperties = onlyProperties ? onlyProperties : checkProperties; checkProperties = additionalProperties ? checkProperties.concat(additionalProperties) : checkProperties;