tqt/Ghost
2
1
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2023-12-13 21:00:40 +01:00
Code Issues 1 Releases Wiki Activity

Updated permissions service to handle api keys (#9967)

Browse source 
refs #9865

- Enabled the permissions module to lookup permissions based on an api_key id.
- Updated the "can this" part of the permissions service to load permissions for any api key in the context, and correctly use that to determine whether an action is permissible. It also updates the permissible interface that models can implement to pass in the hasApiKeyPermissions param.
This commit is contained in:
Fabien O'Carroll 2019-01-18 12:17:12 +01:00 committed by Naz Gargol
parent 42a1313bff
commit fc73cd71bb
8 changed files with 148 additions and 11 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

30
core/server/services/permissions/can-this.js
View file

@ -50,8 +50,10 @@ CanThisResult.prototype.buildObjectTypeHandlers = function (objTypes, actType, c
return permissionLoad.then(function (loadedPermissions) {
// Iterate through the user permissions looking for an affirmation
var userPermissions = loadedPermissions.user ? loadedPermissions.user.permissions : null,
apiKeyPermissions = loadedPermissions.apiKey ? loadedPermissions.apiKey.permissions : null,
appPermissions = loadedPermissions.app ? loadedPermissions.app.permissions : null,
hasUserPermission,
hasApiKeyPermission,
hasAppPermission,
checkPermission = function (perm) {
var permObjId;
@ -82,6 +84,14 @@ CanThisResult.prototype.buildObjectTypeHandlers = function (objTypes, actType, c
hasUserPermission = _.some(userPermissions, checkPermission);
}
// Check api key permissions if they were passed
hasApiKeyPermission = true;
if (!_.isNull(apiKeyPermissions)) {
// api key request have no user, but we want the user permissions checks to pass
hasUserPermission = true;
hasApiKeyPermission = _.some(apiKeyPermissions, checkPermission);
}
// Check app permissions if they were passed
hasAppPermission = true;
if (!_.isNull(appPermissions)) {
@ -91,11 +101,11 @@ CanThisResult.prototype.buildObjectTypeHandlers = function (objTypes, actType, c
// Offer a chance for the TargetModel to override the results
if (TargetModel && _.isFunction(TargetModel.permissible)) {
return TargetModel.permissible(
modelId, actType, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasAppPermission
modelId, actType, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasAppPermission, hasApiKeyPermission
);
}
if (hasUserPermission && hasAppPermission) {
if (hasUserPermission && hasApiKeyPermission && hasAppPermission) {
return;
}
@ -110,10 +120,11 @@ CanThisResult.prototype.buildObjectTypeHandlers = function (objTypes, actType, c
CanThisResult.prototype.beginCheck = function (context) {
var self = this,
userPermissionLoad,
apiKeyPermissionLoad,
appPermissionLoad,
permissionsLoad;
// Get context.user and context.app
// Get context.user, context.api_key_id and context.app
context = parseContext(context);
if (actionsMap.empty()) {
@ -128,6 +139,14 @@ CanThisResult.prototype.beginCheck = function (context) {
userPermissionLoad = Promise.resolve(null);
}
// Kick off loading of api key permissions if necessary
if (context.api_key_id) {
apiKeyPermissionLoad = providers.apiKey(context.api_key_id);
} else {
// Resolve null if no context.api_key_id
apiKeyPermissionLoad = Promise.resolve(null);
}
// Kick off loading of app permissions if necessary
if (context.app) {
appPermissionLoad = providers.app(context.app);
@ -137,10 +156,11 @@ CanThisResult.prototype.beginCheck = function (context) {
}
// Wait for both user and app permissions to load
permissionsLoad = Promise.all([userPermissionLoad, appPermissionLoad]).then(function (result) {
permissionsLoad = Promise.all([userPermissionLoad, apiKeyPermissionLoad, appPermissionLoad]).then(function (result) {
return {
user: result[0],
app: result[1]
apiKey: result[1],
app: result[2]
};
});

8
core/server/services/permissions/parse-context.js
View file

@ -3,7 +3,7 @@
*
* Utility function, to expand strings out into objects.
* @param {Object|String} context
* @return {{internal: boolean, external: boolean, user: integer|null, app: integer|null, public: boolean}}
* @return {{internal: boolean, external: boolean, user: integer|null, app: integer|null, public: boolean, api_key_id: ObjectId|null}}
*/
module.exports = function parseContext(context) {
// Parse what's passed to canThis.beginCheck for standard user and app scopes
@ -11,6 +11,7 @@ module.exports = function parseContext(context) {
internal: false,
external: false,
user: null,
api_key_id: null,
app: null,
public: true
};
@ -31,6 +32,11 @@ module.exports = function parseContext(context) {
parsed.public = false;
}
if (context && context.api_key_id) {
parsed.api_key_id = context.api_key_id;
parsed.public = false;
}
if (context && context.app) {
parsed.app = context.app;
parsed.public = false;

18
core/server/services/permissions/providers.js
View file

@ -53,5 +53,23 @@ module.exports = {
return {permissions: foundApp.related('permissions').models};
});
},
apiKey(id) {
return models.ApiKey.findOne({id}, {withRelated: ['role', 'role.permissions']})
.then((foundApiKey) => {
if (!foundApiKey) {
throw new common.errors.NotFoundError({
message: common.i18n.t('errors.models.api_key.apiKeyNotFound')
});
}
// api keys have a belongs_to relationship to a role and no individual permissions
// so there's no need for permission deduplication
const permissions = foundApiKey.related('role').related('permissions').models;
const roles = [foundApiKey.toJSON().role];
return {permissions, roles};
});
}
};

3
core/server/translations/en.json
View file

@ -270,6 +270,9 @@
"onlyOwnerCanTransferOwnerRole": "Only owners are able to transfer the owner role.",
"onlyAdmCanBeAssignedOwnerRole": "Only administrators can be assigned the owner role."
},
"api_key": {
"apiKeyNotFound": "API Key not found"
},
"base": {
"index": {
"missingContext": "missing context"

6
core/test/unit/api/v0.1/utils_spec.js
View file

@ -608,7 +608,7 @@ describe('API Utils', function () {
describe('handlePublicPermissions', function () {
it('should return empty options if passed empty options', function (done) {
apiUtils.handlePublicPermissions('tests', 'test')({}).then(function (options) {
options.should.eql({context: {app: null, external: false, internal: false, public: true, user: null}});
options.should.eql({context: {app: null, external: false, internal: false, public: true, user: null, api_key_id: null}});
done();
}).catch(done);
});
@ -617,7 +617,7 @@ describe('API Utils', function () {
var aPPStub = sandbox.stub(apiUtils, 'applyPublicPermissions').returns(Promise.resolve({}));
apiUtils.handlePublicPermissions('tests', 'test')({}).then(function (options) {
aPPStub.calledOnce.should.eql(true);
options.should.eql({context: {app: null, external: false, internal: false, public: true, user: null}});
options.should.eql({context: {app: null, external: false, internal: false, public: true, user: null, api_key_id: null}});
done();
}).catch(done);
});
@ -633,7 +633,7 @@ describe('API Utils', function () {
apiUtils.handlePublicPermissions('tests', 'test')({context: {user: 1}}).then(function (options) {
cTStub.calledOnce.should.eql(true);
cTMethodStub.test.test.calledOnce.should.eql(true);
options.should.eql({context: {app: null, external: false, internal: false, public: false, user: 1}});
options.should.eql({context: {app: null, external: false, internal: false, public: false, user: 1, api_key_id: null}});
done();
}).catch(done);
});

32
core/test/unit/services/permissions/can-this_spec.js
View file

@ -423,6 +423,32 @@ describe('Permissions', function () {
});
});
describe('API Key-based permissions', function () {
// TODO change to using fake models in tests!
// Permissions need to be NOT fundamentally baked into Ghost, but a separate module, at some point
// It can depend on bookshelf, but should NOT use hard coded model knowledge.
// We use the tag model here because it doesn't have permissible, once that changes, these tests must also change
it('With permissions: can edit non-specific tag (no permissible function on model)', function (done) {
const apiKeyProviderStub = sandbox.stub(providers, 'apiKey').callsFake(() => {
// Fake the response from providers.user, which contains permissions and roles
return Promise.resolve({
permissions: models.Permissions.forge(testUtils.DataGenerator.Content.permissions).models,
// This should be JSON, so no need to run it through the model layer. 5 === admin api key
roles: [testUtils.DataGenerator.Content.roles[5]]
});
});
permissions.canThis({api_key_id: 123}) // api key context
.edit
.tag({id: 1}) // tag id in model syntax
.then(function (res) {
apiKeyProviderStub.callCount.should.eql(1);
should.not.exist(res);
done();
})
.catch(done);
});
});
describe('App-based permissions (requires user as well)', function () {
// @TODO: revisit this - do we really need to have USER permissions AND app permissions?
it('No permissions: cannot edit tag with app only (no permissible function on model)', function (done) {
@ -553,7 +579,7 @@ describe('Permissions', function () {
})
.catch(function (err) {
permissibleStub.callCount.should.eql(1);
permissibleStub.firstCall.args.should.have.lengthOf(7);
permissibleStub.firstCall.args.should.have.lengthOf(8);
permissibleStub.firstCall.args[0].should.eql(1);
permissibleStub.firstCall.args[1].should.eql('edit');
@ -562,6 +588,7 @@ describe('Permissions', function () {
permissibleStub.firstCall.args[4].should.be.an.Object();
permissibleStub.firstCall.args[5].should.be.true();
permissibleStub.firstCall.args[6].should.be.true();
permissibleStub.firstCall.args[7].should.be.true();
userProviderStub.callCount.should.eql(1);
err.message.should.eql('Hello World!');
@ -587,7 +614,7 @@ describe('Permissions', function () {
.post({id: 1}) // tag id in model syntax
.then(function (res) {
permissibleStub.callCount.should.eql(1);
permissibleStub.firstCall.args.should.have.lengthOf(7);
permissibleStub.firstCall.args.should.have.lengthOf(8);
permissibleStub.firstCall.args[0].should.eql(1);
permissibleStub.firstCall.args[1].should.eql('edit');
permissibleStub.firstCall.args[2].should.be.an.Object();
@ -595,6 +622,7 @@ describe('Permissions', function () {
permissibleStub.firstCall.args[4].should.be.an.Object();
permissibleStub.firstCall.args[5].should.be.true();
permissibleStub.firstCall.args[6].should.be.true();
permissibleStub.firstCall.args[7].should.be.true();
userProviderStub.callCount.should.eql(1);
should.not.exist(res);

21
core/test/unit/services/permissions/parse-context_spec.js
View file

@ -8,6 +8,7 @@ describe('Permissions', function () {
internal: false,
external: false,
user: null,
api_key_id: null,
app: null,
public: true
});
@ -15,6 +16,7 @@ describe('Permissions', function () {
internal: false,
external: false,
user: null,
api_key_id: null,
app: null,
public: true
});
@ -25,6 +27,7 @@ describe('Permissions', function () {
internal: false,
external: false,
user: null,
api_key_id: null,
app: null,
public: true
});
@ -32,6 +35,7 @@ describe('Permissions', function () {
internal: false,
external: false,
user: null,
api_key_id: null,
app: null,
public: true
});
@ -42,6 +46,18 @@ describe('Permissions', function () {
internal: false,
external: false,
user: 1,
api_key_id: null,
app: null,
public: false
});
});
it('should return api_key_id if api_key_id provided', function () {
parseContext({api_key_id: 1}).should.eql({
internal: false,
external: false,
user: null,
api_key_id: 1,
app: null,
public: false
});
@ -52,6 +68,7 @@ describe('Permissions', function () {
internal: false,
external: false,
user: null,
api_key_id: null,
app: 5,
public: false
});
@ -62,6 +79,7 @@ describe('Permissions', function () {
internal: true,
external: false,
user: null,
api_key_id: null,
app: null,
public: false
});
@ -70,6 +88,7 @@ describe('Permissions', function () {
internal: true,
external: false,
user: null,
api_key_id: null,
app: null,
public: false
});
@ -80,6 +99,7 @@ describe('Permissions', function () {
internal: false,
external: true,
user: null,
api_key_id: null,
app: null,
public: false
});
@ -88,6 +108,7 @@ describe('Permissions', function () {
internal: false,
external: true,
user: null,
api_key_id: null,
app: null,
public: false
});

41
core/test/unit/services/permissions/providers_spec.js
View file

@ -174,6 +174,47 @@ describe('Permission Providers', function () {
});
});
describe('API Key', function () {
it('errors if api_key cannot be found', function (done) {
let findApiKeySpy = sandbox.stub(models.ApiKey, 'findOne');
findApiKeySpy.returns(new Promise.resolve());
providers.apiKey(1)
.then(() => {
done(new Error('Should have thrown an api key not found error'));
})
.catch((err) => {
findApiKeySpy.callCount.should.eql(1);
err.errorType.should.eql('NotFoundError');
done();
});
});
it('can load api_key with role, and role.permissions', function (done) {
const findApiKeySpy = sandbox.stub(models.ApiKey, 'findOne').callsFake(function () {
const fakeApiKey = models.ApiKey.forge(testUtils.DataGenerator.Content.api_keys[0]);
const fakeAdminRole = models.Role.forge(testUtils.DataGenerator.Content.roles[0]);
const fakeAdminRolePermissions = models.Permissions.forge(testUtils.DataGenerator.Content.permissions);
fakeAdminRole.relations = {
permissions: fakeAdminRolePermissions
};
fakeApiKey.relations = {
role: fakeAdminRole
};
fakeApiKey.withRelated = ['role', 'role.permissions'];
return Promise.resolve(fakeApiKey);
});
providers.apiKey(1).then((res) => {
findApiKeySpy.callCount.should.eql(1);
res.should.be.an.Object().with.properties('permissions', 'roles');
res.roles.should.be.an.Array().with.lengthOf(1);
res.permissions[0].should.be.an.Object().with.properties('attributes', 'id');
res.roles[0].should.be.an.Object().with.properties('id', 'name', 'description');
res.permissions[0].should.be.instanceOf(models.Base.Model);
res.roles[0].should.not.be.instanceOf(models.Base.Model);
done();
}).catch(done);
});
});
describe('App', function () {
// @TODO make this consistent or sane or something!
// Why is this an empty array, when the success is an object?