Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List Permissions Proposal #21

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@JedWatson
Copy link
Member

commented Sep 2, 2016

This is a proposed implementation of List Permissions that may be supported by Keystone.

To explain what's going on:

Idea 1

You can dynamically change list and field options (specifically hidden, noedit and nodelete) based on the current user / item data. This lets you customise what's displayed in the Admin UI for a particular user (could be a permission group)

To do this, you can set the following methods on the List instance:

userOptions (user) => { /* list options to override for the user */ } 
userFieldOptions (user) => { /* field paths containing options to override for each field */ }
itemOptions (item, user) => { /* list options to override on a per-item basis */ }
itemFieldOptions (item, user) => { /* field paths containing options to override on a per-item basis */ }

Idea 2

You can directly modify the queries used (after filters are applied) for Admin UI endpoints, including:

  • get item(s)
  • update item(s)
  • delete item(s)
  • download export

You could use this, for example, to limit which items a user can see or interact with in any way.

To do this, you could set the following methods on the List instance:

modifyListQuery (user, query) // modify the query by reference, based on the current user
modifyItemData (user, item) // update properties on the item before it is saved

Example use case

This PR implements these features to achieve two use cases:

  • Protecting the demo user from being updated or deleted using generic functionality
  • Adding a concept of Editor Users, and implementing restrictions on Posts, where non-editors can only see their own posts, can't change the state of a post, and can't edit certain fields after the post has been published
@mxstbr

This comment has been minimized.

Copy link
Member

commented Sep 2, 2016

The API is a bit unnecessarily verbose, isn't it?

Why not something along the lines of

Post.permissions = {
  userFieldOptions: () => {},
  /**/
};
@JedWatson

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2016

@mxstbr happy to simplify it, that's why this is a proposal 😄

Not sure how that would be different though, other than nesting the methods under a permission object? Could you propose the full API you'd like to implement the rules I've included in this PR?

@webteckie

This comment has been minimized.

Copy link

commented Sep 2, 2016

How about nocreate ... can that be controlled as well?

@webteckie

This comment has been minimized.

Copy link

commented Sep 2, 2016

Would dynamic permissions reflect across all adminUI Screens, including Home screen?

@webteckie

This comment has been minimized.

Copy link

commented Sep 2, 2016

In idea 1, are you missing the field path argument for those functions which act in a field path?

@webteckie

This comment has been minimized.

Copy link

commented Sep 2, 2016

I kind of like this. Would it work across inheritance in case I want to group permissions based on inheritance?

@mrprkr

This comment has been minimized.

Copy link

commented Sep 2, 2016

This looks really good. Would be great to use the same behaviour as filters too and I'm not sure if it's best to use {canModify: user.isAdmin} vs the existing terminology {noedit: !user.isAdmin}. The latter is a bit strange for permissions because it's a double negative condition. I've pasted below my thoughts on how this could work from a model point of view, rather than middleware for the admin UI.

In this example, we're assigning content lists and users to a territory. On load of the content we want to compare if they match and only show rows that the user is entitled to view.

Let me know what you think

// Create a list of regions / territories
var Territory = new keystone.list('Territory');
Territory.add({
    name: {type: Types.Text},
    country: {type: Types.Select, options: ['Country', 'Country 2']}
}).register();


// User Model
// Users can be an admin, or an editor of a few regions
var User = new keystone.list('User');
User.add({
    name: { type: Types.Name, required: true, index: true },
    email: { type: Types.Email, initial: true, required: true, index: true },
    password: { type: Types.Password, initial: true, required: true },
}, 'Permissions', {
    isAdmin: {type: Boolean, label: 'Keystone Admin', index: true},
    isEditor: {type: Boolean, label: 'Keystone Contributor', index: true, dependsOn: {isAdmin: false}},
    territories: {type: Types.Relationship, ref: 'Territory', many: true, dependsOn: {isEditor: true}}
}).register();


// Some content that has user permissions to view/edit through the admin UI
var ContentList = new keystone.list('ContentList', {
    track: true,
    permissions: {
        // Compare a local field to a field in the user model
        // Show it if they match, or if the user isAdmin
        hidden: [{!this._user.territories, ':territory'}, {!this._user.isAdmin}]

        // General write/delete permissions
        nocreate: [!this._user.isEditor, !this._user.isAdmin],
        noedit: [!this._user.isEditor, !this._user.isAdmin],
        nodelete: [!this._user.isAdmin],
    }
})

// The content model
ContentList.add({
    name: {type: Types.Text},
    // The territory can only be changed by an admin
    territory: {type: Types.Relationship, ref: 'Territory', noedit: !this._user.isAdmin}
}).register();
@JedWatson

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2016

@webteckie in order:

How about nocreate ... can that be controlled as well?

Yes

Would dynamic permissions reflect across all adminUI Screens, including Home screen?

Yes

In idea 1, are you missing the field path argument for those functions which act in a field path?

No, this would be processed once and return an object containing field paths with options that override the general options for each field, for a specific user

I kind of like this. Would it work across inheritance in case I want to group permissions based on inheritance?

I'm not sure yet.

@mrprkr re: terminology, what's going on here is I'm proposing a way to change some list and field options on a per-item and/or per-user basis.

We're not otherwise introducing anything new, so there'd be no changes to terminology or otherwise changes to the features themselves (at least not with this proposal)

I do agree that the double negative is a bit weird, not sure that I prefer booleans that default to true though, which is why that was chosen in the first place. At least we should be consistent.

re: your example, as I'm reading it, we could use the functions I've proposed to achieve what you want, it's just the syntax would be different.

The challenge with what you're suggesting is that the variables are evaluated when the code is executed unless they're wrapped in functions, making it more verbose. For example:

territory: { type: Types.Relationship, ref: 'Territory', noedit: !this._user.isAdmin }

In this case this._user.isAdmin would be evaluated to something when the list is created. You'd need to do something like:

territory: { type: Types.Relationship, ref: 'Territory', noedit: (user) => !user.isAdmin }

... we could do that, but I think it would be more complex to implement as compared to:

ContentList .userFieldOptions = (user) => ({
  territory: { noedit: !user.isAdmin }
});

Using the single method instead of specifying the functions inline will optimise it for larger models, and lets you structure your rules more powerfully, e.g

var protectedFields = ['territory'];
ContentList.userFieldOptions = (user) => {
  var options = {};
  if (!user.isAdmin) protectedFields.forEach(i => { options[i] = { noedit: true } });
  return options;
};
@mrprkr

This comment has been minimized.

Copy link

commented Sep 5, 2016

@JedWatson Looks good to me. At the moment I'm doing pre-save checks like this:

Homepage.js

// Only admins and territory contributors may update this item
HomePage.schema.pre('save', function(next){
    middleware.canEditThisTerritory(this._req_user, this.territoryId, next);
})

middleware.js

// Check your privilage 
exports.canEditThisTerritory = function(user, territoryId, next){
    // Admins can do what they like
    if(user.isAdmin){
        next();
    } else if (!territoryId) {
        // creating a new item
    }
    // Editors need their permissions checked
    else if(user.isEditor){
        // Get a list of the user's permitted territories
        for(var x = 0; x < user.territories.length; x++){
            // Compare it to the one they're trying to save
            if(user.territories[x] == territoryId){
                // If it's a match, no worries
                next();
            }
            if(x == user.territories.length - 1){
                // If not, there's an issue
                next(new Error("You don't have permission to edit this territory"))
            }
        }
    } else {
        // This should never happen, throw an error and log the event
        console.log(user + 'Is trying to access keystone API without permission');
        next(new Error("You don't have permission to access keystone, this attempt has been logged"));
    }
}

Main thing from a usability point of view is to hide items in the UI that don't meet the criteria, so an author doesn't fill out a whole form and then get stuck.

@VinayaSathyanarayana

This comment has been minimized.

Copy link

commented Sep 5, 2016

Some thoughts:

We can consider/leverage https://www.npmjs.com/package/acl

Global Permissions -
SingleTon Object - Only single Object is allowed for this List/Table
NoCreate - Only created through code and not by users
NoDelete - cannot be deleted any any user
Global Permissions supersede other permissions

Permission at List/Table level : [ "create", "view", "update", "delete", "search" ]
// every List/Table specifies which user can do what on the List
// view and "download/export" can be considered same
// List/Table Permissions supersede Object/Row level controls

Permission at Object/Row level : [ "all", "createdByUser", "ownedBYUser", user_supplied_filter_condition_or_Function ]
// implemented using Mongoose Middleware pre and post hooks
// all create/view/update/delete/search should be complient with the filter_condition
// filters can be createdByUser, OwnedByUser, Where Department="DepartmentCode" etc - implemented through a function
// Object/Row level permissions supercede Field/Column level permissions

Permission at Field/Column level : [ "visibleFields" : explicit_list ] // if not mentioned all fields are visible
// maps to the hidden property
// implemented by Keystone ?

@JedWatson

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2016

I'm inclined to implement this sort of low-level support and then do an acl feature separately, probably as an optional package (or maybe by mapping to an existing project like acl)

An example of how this could work would be exporting Users and Roles list definitions from a keystone-roles package, as well as some functions to plug those permissions into your app's lists which would hook into the functions I've described above.

Ideally we keep the features in keystone core as simple and powerful as possible, allowing for developers to build or customise their own permissions solution on top of it.

In particular, I'm trying to keep keystone as unopinionated in its core implementation as possible, then build on that with ready to use (but optional) features that can be added "out of the box", or that serve as a reference implementation if somebody wants to do something more custom.

@VinayaSathyanarayana if you can see something we'd need to integrate acl as you've described that's not supported by the features in this proposal, please let me know!

@webteckie

This comment has been minimized.

Copy link

commented Sep 7, 2016

@JedWatson experimented a little with your branch and...

Given the following I would have expected the Admin User to not be able to edit the state (because he's not an editor -- even though he created the post):

Post.userFieldOptions = (user) => ({
    state: {
        // normal users cannot change the state of a post
        noedit: !user.isEditor,
    },
    author: {
        // author is hidden from normal users
        hidden: !user.isEditor,
    },
});

Likewise, once the Admin User was able to change the state to published the following logic, again, should not let the same Admin User update the name field of the post but it did:

Post.itemFieldOptions = (item, user) => ({
    name: {
        // non-editors cannot change the name of a published post
        noedit: item.state === 'published' && !user.isEditor,
    },
});

Or are there checks in place to let the Admin User do whatever he/she wants? Not sure that should be the case, if so.

@arturkasperek

This comment has been minimized.

Copy link

commented Oct 3, 2016

Any news about this feature ?

@Hsn723

This comment has been minimized.

Copy link

commented Oct 12, 2016

We're using 0.3.x in production and badly need some form of permission, and this seems to be a good compromise until we get full ACL. Looking at the code changes, other than the model-transform dependency (which looks optional?) is it safe to assume we could use this almost as-is on 0.3.x?

@edusaninc

This comment has been minimized.

Copy link

commented Oct 25, 2016

Can anyone point me in the right direction, as to where I can call Jed's field option methods from idea#1 ?

The code changes to the 2 models (Pull Request #21) alone do not implement the List Permissions functionality, is that correct?

I was trying to look for when the lists are queried and created for the initial admin view. Any help would be greatly appreciated!

@niallobrien

This comment has been minimized.

Copy link

commented Dec 6, 2016

Hi all, what's the status of this PR? Thanks in advance.

@jjhesk

This comment has been minimized.

Copy link

commented Feb 7, 2017

does it have the ideas proposed from webteckie

@AmrN

This comment has been minimized.

Copy link

commented Mar 9, 2017

I'm really looking for similar functionality, will this ever make it into keystone?

@niallobrien

This comment has been minimized.

Copy link

commented Mar 10, 2017

@AmrN Hopefully.

@Tushar-Gupta

This comment has been minimized.

Copy link

commented May 21, 2017

Has there been any update on this feature? I really need it for my project.

@cgagnonqc

This comment has been minimized.

Copy link

commented May 19, 2018

Any update on this? keystonejs/keystone#2111 has interesting stuff that seems more secure.

@acmoune

This comment has been minimized.

Copy link

commented Aug 7, 2018

I don't know what you are looking for, but for me there is only one thing missing in the current implementation.

Keystone already support permissions with this:

const User = new keystone.List('User')

User.add({
  name: { type: Types.Name, required: true, index: true },
  email: { type: Types.Email, initial: true, required: true, unique: true, index: true },
  password: { type: Types.Password, initial: true, required: true },
}, 'Permissions', { // << That is it
  isAdmin: { type: Boolean, label: 'Can access Keystone', index: true },
  isRoot: { type: Boolean, label: 'Administrator', index: true, dependsOn: { isRoot: true} },
  isManager: { type: Boolean, label: 'Channel manager', index: true, dependsOn: { isRoot: true} }
})

So you can consider them as roles: Admin, Root, Manager ...

For me the missing part is that I would like to be able to do this:

const Model = new keystone.List('Model', {
  noedit: function() { ... },
  nocreate: function() { ... },
  nodelete: function() { ...},
  hidden: function() { ...}
})

If it could be possible to use a function instead of a boolean to set noedit, nocreate ...
and if that function can be bound some how by Keystone to an object containing the logged in User instance, and the actual model instance (when applicable, for example in noedit and nodelete), so in that function I can do this.user.xxx and this.model.xxx, then I think we should be done with all this.

Is that possible ?

@nvs2394

This comment has been minimized.

Copy link

commented Sep 17, 2018

@acmoune can you explain it. I need this feature for my project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.