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
Choose a base branch
from
Open

List Permissions Proposal #21

wants to merge 1 commit into from

Conversation

JedWatson
Copy link
Member

@JedWatson JedWatson 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
Copy link
Contributor

mxstbr 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
Copy link
Member Author

@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
Copy link

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

@webteckie
Copy link

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

@webteckie
Copy link

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

@webteckie
Copy link

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

@JedWatson
Copy link
Member Author

@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;
};

@VinayaSathyanarayana
Copy link

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
Copy link
Member Author

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
Copy link

@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
Copy link

Any news about this feature ?

@Hsn723
Copy link

Hsn723 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
Copy link

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
Copy link

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

@jjhesk
Copy link

jjhesk commented Feb 7, 2017

does it have the ideas proposed from webteckie

@AmrN
Copy link

AmrN commented Mar 9, 2017

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

@niallobrien
Copy link

@AmrN Hopefully.

@Tushar-Gupta
Copy link

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

@cgagnonqc
Copy link

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

@acmoune
Copy link

acmoune 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
Copy link

nvs2394 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.