Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-01-20 22:42:53 -05:00
Code Issues Activity

🔥 Removed certain fields from public user response (#9069)

Browse source 
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
This commit is contained in:
Katharina Irrgang 2017-09-28 15:00:52 +02:00 • committed by Hannah Wolfe
parent 42af268d1b
commit 506a0c3e9e
5 changed files with 87 additions and 56 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
core/server/models/user.js
View file

@ -192,14 +192,28 @@ User = ghostBookshelf.Model.extend({
options = options || {}; options = options || {};
var attrs = ghostBookshelf.Model.prototype.toJSON.call(this, options); var attrs = ghostBookshelf.Model.prototype.toJSON.call(this, options);
// remove password hash for security reasons // remove password hash for security reasons
delete attrs.password; delete attrs.password;
delete attrs.ghost_auth_access_token; 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)) { if (!options || !options.context || (!options.context.user && !options.context.internal)) {
delete attrs.email; 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; return attrs;
}, },
@ -313,7 +327,7 @@ User = ghostBookshelf.Model.extend({
permittedOptionsToReturn = permittedOptionsToReturn.concat(validOptions[methodName]); 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. // Otherwise we expose too much information.
if (options && options.context && options.context.public) { if (options && options.context && options.context.public) {
if (options.include && options.include.indexOf('roles') !== -1) { if (options.include && options.include.indexOf('roles') !== -1) {

1
core/server/permissions/parse-context.js
View file

@ -15,6 +15,7 @@ module.exports = function parseContext(context) {
public: true public: true
}; };
// NOTE: We use the `external` context for subscribers only at the moment.
if (context && (context === 'external' || context.external)) { if (context && (context === 'external' || context.external)) {
parsed.external = true; parsed.external = true;
parsed.public = false; parsed.public = false;

16
core/test/functional/routes/api/public_api_spec.js
View file

@ -322,7 +322,7 @@ describe('Public API', function () {
jsonResponse.users.should.have.length(2); jsonResponse.users.should.have.length(2);
// We don't expose the email address. // 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(); done();
}); });
}); });
@ -345,7 +345,7 @@ describe('Public API', function () {
jsonResponse.users.should.have.length(2); jsonResponse.users.should.have.length(2);
// We don't expose the email address. // 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(); done();
}); });
}); });
@ -368,7 +368,7 @@ describe('Public API', function () {
jsonResponse.users.should.have.length(1); jsonResponse.users.should.have.length(1);
// We don't expose the email address. // 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(); done();
}); });
}); });
@ -391,7 +391,7 @@ describe('Public API', function () {
jsonResponse.users.should.have.length(1); jsonResponse.users.should.have.length(1);
// We don't expose the email address. // 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(); done();
}); });
}); });
@ -414,7 +414,7 @@ describe('Public API', function () {
jsonResponse.users.should.have.length(1); jsonResponse.users.should.have.length(1);
// We don't expose the email address. // 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(); done();
}); });
}); });
@ -453,7 +453,7 @@ describe('Public API', function () {
jsonResponse.users.should.have.length(1); jsonResponse.users.should.have.length(1);
// We don't expose the email address. // 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(); done();
}); });
}); });
@ -476,7 +476,7 @@ describe('Public API', function () {
jsonResponse.users.should.have.length(2); jsonResponse.users.should.have.length(2);
// We don't expose the email address. // 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(); done();
}); });
}); });
@ -499,7 +499,7 @@ describe('Public API', function () {
jsonResponse.users.should.have.length(2); jsonResponse.users.should.have.length(2);
// We don't expose the email address. // 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(); done();
}); });
}); });

26
core/test/integration/api/api_users_spec.js
View file

@ -56,7 +56,7 @@ describe('Users API', function () {
var userData = testUtils.DataGenerator.forModel.users[0]; var userData = testUtils.DataGenerator.forModel.users[0];
models.User.check({email: userData.email, password: userData.password}).then(function (user) { 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) { }).then(function (response) {
response.users[0].created_at.should.be.an.instanceof(Date); response.users[0].created_at.should.be.an.instanceof(Date);
response.users[0].updated_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 () { describe('Browse', function () {
function checkBrowseResponse(response, count, additional, missing) { function checkBrowseResponse(response, count, additional, missing, only, options) {
should.exist(response); should.exist(response);
testUtils.API.checkResponse(response, 'users'); testUtils.API.checkResponse(response, 'users');
should.exist(response.users); should.exist(response.users);
response.users.should.have.length(count); response.users.should.have.length(count);
testUtils.API.checkResponse(response.users[0], 'user', additional, missing); testUtils.API.checkResponse(response.users[0], 'user', additional, missing, only, options);
testUtils.API.checkResponse(response.users[1], 'user', additional, missing); testUtils.API.checkResponse(response.users[1], 'user', additional, missing, only, options);
testUtils.API.checkResponse(response.users[2], 'user', additional, missing); testUtils.API.checkResponse(response.users[2], 'user', additional, missing, only, options);
testUtils.API.checkResponse(response.users[3], 'user', additional, missing); testUtils.API.checkResponse(response.users[3], 'user', additional, missing, only, options);
} }
it('Owner can browse', function (done) { 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) { it('No-auth CAN browse, but only gets filtered active users', function (done) {
UserAPI.browse().then(function (response) { UserAPI.browse().then(function (response) {
checkBrowseResponse(response, 7, null, ['email']); checkBrowseResponse(response, 7, null, null, null, {public: true});
done(); done();
}).catch(done); }).catch(done);
}); });
@ -220,20 +220,18 @@ describe('Users API', function () {
}); });
describe('Read', function () { describe('Read', function () {
function checkReadResponse(response, noEmail) { function checkReadResponse(response, noEmail, additional, missing, only, options) {
should.exist(response); should.exist(response);
should.not.exist(response.meta); should.not.exist(response.meta);
should.exist(response.users); should.exist(response.users);
response.users[0].id.should.eql(testUtils.DataGenerator.Content.users[0].id); response.users[0].id.should.eql(testUtils.DataGenerator.Content.users[0].id);
if (noEmail) { if (noEmail) {
// Email should be missing testUtils.API.checkResponse(response.users[0], 'user', additional, missing, only, options);
testUtils.API.checkResponse(response.users[0], 'user', [], ['email']);
should.not.exist(response.users[0].email);
} else { } 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) { it('Owner can read', function (done) {
@ -268,7 +266,7 @@ describe('Users API', function () {
it('No-auth can read', function (done) { it('No-auth can read', function (done) {
UserAPI.read({id: userIdFor.owner}).then(function (response) { UserAPI.read({id: userIdFor.owner}).then(function (response) {
checkReadResponse(response, true); checkReadResponse(response, true, null, null, null, {public: true});
done(); done();
}).catch(done); }).catch(done);
}); });

84
core/test/utils/api.js
View file

@ -1,44 +1,59 @@
var _ = require('lodash'), var _ = require('lodash'),
url = require('url'), url = require('url'),
moment = require('moment'), moment = require('moment'),
config = require('../../server/config'), config = require('../../server/config'),
schema = require('../../server/data/schema').tables, schema = require('../../server/data/schema').tables,
ApiRouteBase = '/ghost/api/v0.1/', ApiRouteBase = '/ghost/api/v0.1/',
host = config.get('server').host, host = config.get('server').host,
port = config.get('server').port, port = config.get('server').port,
protocol = 'http://', protocol = 'http://',
expectedProperties = { expectedProperties = {
// API top level // API top level
posts: ['posts', 'meta'], posts: ['posts', 'meta'],
tags: ['tags', 'meta'], tags: ['tags', 'meta'],
users: ['users', 'meta'], users: ['users', 'meta'],
settings: ['settings', 'meta'], settings: ['settings', 'meta'],
subscribers: ['subscribers', 'meta'], subscribers: ['subscribers', 'meta'],
roles: ['roles'], roles: ['roles'],
pagination: ['page', 'limit', 'pages', 'total', 'next', 'prev'], pagination: ['page', 'limit', 'pages', 'total', 'next', 'prev'],
slugs: ['slugs'], slugs: ['slugs'],
slug: ['slug'], slug: ['slug'],
// object / model level post: _(schema.posts)
// Post API .keys()
post: _(schema.posts).keys() // by default we only return html
// does not return all formats by default
.without('mobiledoc', 'amp', 'plaintext') .without('mobiledoc', 'amp', 'plaintext')
// swaps author_id to author, and always returns computed properties: url, comment_id, primary_tag // 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') .without('author_id').concat('author', 'url', 'comment_id', 'primary_tag')
.value(), .value(),
// User API always removes the password field user: {
user: _(schema.users).keys().without('password').without('ghost_auth_access_token').value(), 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 API swaps parent_id to parent
tag: _(schema.tags).keys().without('parent_id').concat('parent').value(), tag: _(schema.tags).keys().without('parent_id').concat('parent').value(),
setting: _.keys(schema.settings), setting: _.keys(schema.settings),
subscriber: _.keys(schema.subscribers), subscriber: _.keys(schema.subscribers),
accesstoken: _.keys(schema.accesstokens), accesstoken: _.keys(schema.accesstokens),
role: _.keys(schema.roles), role: _.keys(schema.roles),
permission: _.keys(schema.permissions), permission: _.keys(schema.permissions),
notification: ['type', 'message', 'status', 'id', 'dismissible', 'location'], notification: ['type', 'message', 'status', 'id', 'dismissible', 'location'],
theme: ['name', 'package', 'active'], theme: ['name', 'package', 'active'],
themes: ['themes'], themes: ['themes'],
invites: _(schema.invites).keys().without('token').value() invites: _(schema.invites).keys().without('token').value()
}; };
function getApiQuery(route) { function getApiQuery(route) {
@ -83,8 +98,11 @@ function checkResponseValue(jsonResponse, expectedProperties) {
providedProperties.length.should.eql(expectedProperties.length); providedProperties.length.should.eql(expectedProperties.length);
} }
function checkResponse(jsonResponse, objectType, additionalProperties, missingProperties, onlyProperties) { // @TODO: support options pattern only, it's annoying to call checkResponse(null, null, null, something)
var checkProperties = expectedProperties[objectType]; 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 = onlyProperties ? onlyProperties : checkProperties;
checkProperties = additionalProperties ? checkProperties.concat(additionalProperties) : checkProperties; checkProperties = additionalProperties ? checkProperties.concat(additionalProperties) : checkProperties;