-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: master
Are you sure you want to change the base?
Conversation
The API is a bit unnecessarily verbose, isn't it? Why not something along the lines of Post.permissions = {
userFieldOptions: () => {},
/* … */
}; |
@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 |
How about |
Would dynamic permissions reflect across all adminUI Screens, including Home screen? |
In idea 1, are you missing the field path argument for those functions which act in a field path? |
I kind of like this. Would it work across inheritance in case I want to group permissions based on inheritance? |
@webteckie in order:
Yes
Yes
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'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 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;
}; |
Some thoughts: We can consider/leverage https://www.npmjs.com/package/acl Global Permissions - Permission at List/Table level : [ "create", "view", "update", "delete", "search" ] Permission at Object/Row level : [ "all", "createdByUser", "ownedBYUser", user_supplied_filter_condition_or_Function ] Permission at Field/Column level : [ "visibleFields" : explicit_list ] // if not mentioned all fields are visible |
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 An example of how this could work would be exporting 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 |
@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):
Likewise, once the Admin User was able to change the state to
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. |
Any news about this feature ? |
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? |
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! |
Hi all, what's the status of this PR? Thanks in advance. |
does it have the ideas proposed from webteckie |
I'm really looking for similar functionality, will this ever make it into keystone? |
@AmrN Hopefully. |
Has there been any update on this feature? I really need it for my project. |
Any update on this? keystonejs/keystone#2111 has interesting stuff that seems more secure. |
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 Is that possible ? |
@acmoune can you explain it. I need this feature for my project |
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
andnodelete
) 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:
Idea 2
You can directly modify the queries used (after filters are applied) for Admin UI endpoints, including:
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:
Example use case
This PR implements these features to achieve two use cases: