Skip to content

Commit

Permalink
Added staff user limit
Browse files Browse the repository at this point in the history
refs: TryGhost/Product#510

- In the case that host config is provided, keep staff users within the limiti
- The definition of a staff user is a user with a role other than Contributor, and whose status is not inactive
   - Contributors don't count
   - Suspended (status inactive) users don't count
   - Locked users DO count
   - Invited users DO count
- You can't invite more staff users whilst there are pending invites
- You can't unsuspend a user, or change the role on a user in such a way as will take you over your limit
- You can't import staff users - all imported users are automatically set to Contributors
- As part of this work, we are changing the default Ghost user to a Contributor otherwise it uses up a staff user

Note: there is one known active bug with this commit.
- Assume you have one remaining user within your limit. You send an invite, this works.
- You cannot "resend" that invite, it will think you're sending a new invite and hit the limit
- You must "revoke" that invite first, and create a new one
- This bug exists because the resend function uses the add endpoint & does a delete+add, but this hits the permission check before the delete
  • Loading branch information
ErisDS committed Mar 4, 2021
1 parent 7e2da2a commit e30b973
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 4 deletions.
9 changes: 9 additions & 0 deletions core/server/data/importer/importers/data/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const debug = require('ghost-ignition').debug('importer:users');
const _ = require('lodash');
const BaseImporter = require('./base');
const models = require('../../../../models');
const limitService = require('../../../../services/limits');

class UsersImporter extends BaseImporter {
constructor(allDataFromFile) {
Expand Down Expand Up @@ -58,6 +59,14 @@ class UsersImporter extends BaseImporter {
role = {name: 'Author'};
}

// If this site has any sort of staff limit, set all imported users to contributors
// Any other sort of logic for counting staff users would be too complex in this scenario
// So we essentially don't allow importing staff users
// The roles can be changed afterwards if the limit permits
if (limitService.isLimited('staff')) {
role = {name: 'Contributor'};
}

_.each(this.dataToImport, (obj) => {
if (attachedRole.user_id === obj.id) {
if (!_.isArray(obj.roles)) {
Expand Down
2 changes: 1 addition & 1 deletion core/server/data/schema/fixtures/fixtures.json
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@
"name": "Ghost",
"email": "ghost-author@example.com",
"status": "active",
"roles": ["Author"],
"roles": ["Contributor"],
"twitter": "ghost",
"facebook": "ghost",
"location": "The Internet",
Expand Down
9 changes: 8 additions & 1 deletion core/server/models/invite.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const errors = require('@tryghost/errors');
const constants = require('@tryghost/constants');
const security = require('@tryghost/security');
const settingsCache = require('../services/settings/cache');
const limitService = require('../services/limits');
const ghostBookshelf = require('./base');

let Invite;
Expand Down Expand Up @@ -43,9 +44,15 @@ Invite = ghostBookshelf.Model.extend({
return ghostBookshelf.Model.add.call(this, data, options);
},

permissible(inviteModel, action, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasApiKeyPermission) {
async permissible(inviteModel, action, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasApiKeyPermission) {
const isAdd = (action === 'add');

if (isAdd && limitService.isLimited('staff')) {
// CASE: if your site is limited to a certain number of staff users
// Inviting a new user requires we check we won't go over the limit
await limitService.errorIfWouldGoOverLimit('staff');
}

if (!isAdd) {
if (hasUserPermission && hasApiKeyPermission) {
return Promise.resolve();
Expand Down
16 changes: 15 additions & 1 deletion core/server/models/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const validator = require('validator');
const ObjectId = require('bson-objectid');
const ghostBookshelf = require('./base');
const baseUtils = require('./base/utils');
const limitService = require('../services/limits');
const {i18n} = require('../lib/common');
const errors = require('@tryghost/errors');
const security = require('@tryghost/security');
Expand Down Expand Up @@ -655,7 +656,7 @@ User = ghostBookshelf.Model.extend({
});
},

permissible: function permissible(userModelOrId, action, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasApiKeyPermission) {
permissible: async function permissible(userModelOrId, action, context, unsafeAttrs, loadedPermissions, hasUserPermission, hasApiKeyPermission) {
const self = this;
const userModel = userModelOrId;
let origArgs;
Expand Down Expand Up @@ -687,6 +688,11 @@ User = ghostBookshelf.Model.extend({
});
}

// If we have a staff user limit & the user is being unsuspended
if (limitService.isLimited('staff') && action === 'edit' && unsafeAttrs.status && unsafeAttrs.status === 'active' && userModel.get('status') === 'inactive') {
await limitService.errorIfWouldGoOverLimit('staff');
}

if (action === 'edit') {
// Users with the role 'Editor', 'Author', and 'Contributor' have complex permissions when the action === 'edit'
// We now have all the info we need to construct the permissions
Expand Down Expand Up @@ -741,6 +747,13 @@ User = ghostBookshelf.Model.extend({
}));
}

if (limitService.isLimited('staff') && userModel.hasRole('Contributor') && role.name !== 'Contributor') {
// CASE: if your site is limited to a certain number of staff users
// Trying to change the role of a contributor, who doesn't count towards the limit, to any other role requires a limit check
// To check if it's OK to add one more staff user
await limitService.errorIfWouldGoOverLimit('staff');
}

return User.getOwnerUser()
.then((owner) => {
// CASE: owner can assign role to any user
Expand All @@ -765,6 +778,7 @@ User = ghostBookshelf.Model.extend({
// CASE: you are trying to change a role, but you are not owner
// @NOTE: your role is not the same than the role you try to change (!)
// e.g. admin can assign admin role to a user, but not owner

return permissions.canThis(context).assign.role(role)
.then(() => {
if (hasUserPermission && hasApiKeyPermission) {
Expand Down
2 changes: 1 addition & 1 deletion test/unit/data/schema/integrity_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const defaultSettings = require('../../../../core/server/data/schema/default-set
describe('DB version integrity', function () {
// Only these variables should need updating
const currentSchemaHash = '559cdbb49a7eeb5758caf0c6e3bf790d';
const currentFixturesHash = '370d0da0ab7c45050b2ff30bce8896ba';
const currentFixturesHash = '5f6f69931811c407dff01da9ef9667f4';
const currentSettingsHash = 'e1f85186a7c7ed76064b6026f68c6321';
const currentRoutesHash = '3d180d52c663d173a6be791ef411ed01';

Expand Down

0 comments on commit e30b973

Please sign in to comment.