[Feature] Improve user/group permissions scheme #342
Comments
fmigneault
added
feature
|
@dbyrns I would like to draw attention on the last dot of "API Changes" |
|
Related to Ouranosinc/pavics-sdi#111. |
I think we should first wipe any existing permutation of the permission being assigned. eg: [perm]-deny-match exists and I apply [perm]-allow-match on the same ressource. [perm]-deny-match should be removed and [perm]-allow-match applied. For a given permission we should never have more than one permutation from the set [allow-match, allow-recur, deny-match, deny-recur]. For inheritance, there is nothing to do. The doc should clearly state that deny has a higher priority than allow and thus even if you give access to something, a deny-recur above in the hierarchy will still be enforced. In the UI the effective permission should make things clear and avoid confusion. |
|
@dbyrns What we could do is use the |
|
Another idea. Response wise, we are lucky. Permissions are at the moment returned using format: {
"permission_names": [
"[perm]-[allow|deny]-[match|recur]"
]
"permissions:" [
{
"name": "[perm]",
"access": "[allow|deny]",
"scope": "[match|recur]",
}
]
}For actual 100% backward compatibility, we could add the plain |
|
I'm a bit confused by all that!
So you propose to keep the POST as is, thus requiring one to DELETE, then POST a permission if a flag has to be changed. AND support a PUT that will add the permission or update it with the good flag. To this point I agree and think we are on the good track :).
Here you refer to the GET /.../permissions
Here you partially respond to my question above about how the flags are submitted to the POST/PUT. I understand that the POST could do that for backward compatibility, but for a new route I found it a little bit harsh not you?
And here I'm completely lost :). Do you mean that we would store the name-encoded flags only using a string as before, but with more options? |
|
sorry in advance for the wall of text :/
Yes -ish. Only change is to extend the current POST (not left 100% as is) so that it resolves all variants of
We would indeed need to do explicit DELETE, then POST to replace
There is no dynamic response, both
My intention is to support both str & object format on every method. incoming PUT/POST request perm = get_perm(request)
if isinstance(perm, dict):
perm = perm_as_str(perm) # convert + handle implicit flags
cur_perm = db.get_str_perm_for_[user/group/service/resource]()
if request is "PUT" and cur_perm:
db.delete(cur_perm)
if request is "POST" and cur_perm:
raise Conflict
db.create(perm)
return PUT: 200, POST: 201for all GET requests: cur_perm = db.get_str_perm_for_[user/group/service/resource]():
# extra inheritance resolution (as necessary according to request flags)
all_perm = set(cur_perm) + set(<other_applicable_perms>)
return {
"permission_names": list(all_perm),
"permissions": [perm_as_obj(p) for p in all_perm]
}
Yes. extra detailsProbably better to understand why encoding as string is needed: Because of ziggurat library running under the hood (as well as other pyramid's views implementation), every "permission" is a string. This means that when I call methods they provide to accomplish resource-tree resolution, I obtain only strings (cannot use the object-variant with extra |
|
The wall of text proved to be beneficial :) |
|
I have just skimmed quickly all the previous discussions, I leave it to you for implementation details (ex: POST vs PUT, json payload, etc ...). I just have a few questions: 1 - Can this solution handle the situation in comment Ouranosinc/pavics-sdi#111 (comment) (restrict data access while granting anonymous access to metadata (and browsing)) and that whole issue in general? 2 - In the case of recursive deny, does it make sense to allow something under? 3 - If the parent have match deny but no recursive deny higher, meaning the child is accessible, how does the traversal handle that? Does it stop at the parent and never go down to the child? |
We basically need to tell was Lines 466 to 479 in 4c9d011
extra notePoint 1 made me think about some special use-case: If deny is added to group If non-admin users (aka member of some other group) would need to have access to this resource that was denied access with |
Metadata: Just The actual subtree text (ex:
So if no access control directive is found at the node and all higher ancestors, it is assumed to be deny-match for that node? This sounds reasonable.
This case need to be handled somehow. Ex: the This discussion brings me to wonder, how do you merge contradicting permissions? If a user belongs to all |
|
The use case we have to solve and express in Ouranosinc/pavics-sdi#111 (comment) is the following: Based on the previously posted implementation : bool authorized = False
if deny(file):
return 401
if allow(file):
authorized = True
# Parsing in bottom up order
parents = [dir, root]
for resource in parents:
if deny-recursive(resource):
return 401
if allow-recursive(resource):
authorized = True
if authorized:
return 200For admins, I think that anyway we can return authorized up front whatever the permissions (it is the case right now, no?) If we now take into account the groups, it will means that the deny, allow deny-recursive and allow-recursive fcts are resolved using the inherited flag (taking into account all membership)? So, indeed being part of any group having a deny permission will block you from this ressource. Could it be possible to resolve the previous snippet for each of our group and return the authorization based on an OR basis of each group permission? |
To answer that specifically, up to now, both allow and deny permissions were ORed and deny had priority. So if you are allowed on any of the group you have a pending allowed permission, but if a deny permission applied on any of those groups the pending permission is revoked. I think that we should change the behavior as express in my previous comment. |
Agreed, this will allow setting very strict and aggressive recursive deny for anonymous group but will not block other groups with more forgiving permissions. |
|
In Magpie web UI, I remember there is a button or checkbox "Show inherited permissions", does this button also show the effective combined group permissions for a user? If not, it would be immensely useful to give the admin a way to visually see the effective permissions of a user taking in account all his groups. |
In fact it's exclusively what it does. Right now, it combined the inherited permissions from groups (or not if unchecked) but does not resolve/show the recursive permissions. If you look at the mock UI above, you'll see what we intend to do to better reflect the real effective permissions (what you see is what you get). A combobox will let you chosse the allow|deny/match|recurse flags and the read-only checkbox beside will let you know the resulting permission once all the allow/deny/recurse is resolved. The inherited permissions will still trigger if we include your group membership in the resolution or not. |
Yes. This is the current behavior and will remain in effect.
Yes. That is the intention. I will like to have a test that validates this though, just to ensure that admin-group has more importance than anonymous-group deny-permission when resolving the allow/deny.
No. That's where the issue comes in. Because everyone is a member of anonymous-group, if you set deny-recursive on PrivateDir, you effectively say "block everyone to access PrivateDir and its children" (final decision), and that rule has priority over "allow any GroupA user read on Child1". If denies don't have priority over allows, they will never work (eg: "Anonymous group has a Root recursive-allow" would override "PrivateDir recursive-deny") defeating the original purpose. Keeping in mind that default is to block. So Deny are only for exceptions, and as such must override any previously allowed permission. The combination is something like: This is also how all pyramid permission scheme works:
There is no way to say GroupA is more important than GroupB, so it is not possible to say Allow-GroupA, but Deny-GroupB, and then have a User member of both GroupA and GroupB to be allowed. I would argue that opening everything with recursive root permission, to then deny things one by one is only trying to work in the opposite manner Magpie works. Another set of permissions should be considered. |
I know that's not how it works. I exposed the use case that should work.
Yeah you're absolutely right! If I bring back my example, what's the difference between allow on Root, deny on PrivateDir : I want it to be denied. And deny on PrivateDir and allow on ChildA : I want it to be allowed. In fact, the difference is the depth and since it's our own code that handle the recursive strategy and not pyramid, what could block us to handle it like this: The difference is that the deepest permission always overide recursion, deny or allow. At the same level, deny still has priority. So if we got an allow permission on a file, I doesn't care that the whole branch has been forbidden for one of my group. The drawback is that we would go against every other implementations... There must be a catch? |
|
In fact, I think I just exposed how NTFS works : https://www.ntfs.com/ntfs-permissions-precedence.htm |
|
I agree NTFS' implementation makes a lot of sense and should handle all use cases for the recursive and inherited resolution (permissions closer to the actual item have more weight). Just for completion, I would like to validate how to handle multiple group conflicting permissions on specific entries (no recursion involved and no direct user-permission). The NTFS doc does not explicitly details this properly. It only provides an example of additive different permissions (read+write) in no.4, and user priority > group inheritance in no.2, but not conflicting groups permissions at the same level. More specifically : If groupA has deny-match, while groupB has allow-match, should user with both groupA and groupB memberships be allowed or denied, since they have equal priority. I can see the 2 variants having similarly strong arguments:
Because of case no.1of NTFS ""Deny" permissions generally take precedence over "allow" permissions", I would tend toward resolving to deny as higher priority, but both decisions make sense depending on context and can be justified. |
|
I would, like you, definitely stick with deny has priority over allow. But in this case I guess that pyramid framework will do the resolution for you (at the same level). Hopefully we will not have to work against it. |
|
I would like to thank @tlvu also, since because of your intervention we will get in the end a more versatile design. |
Feature
When looking at permissions on user / group edit pages, a large matrix of checkboxes is listed.
With the eventual addition on 'deny' permission (#235), the list of applicable permissions can become quite large.
(every combination of
deny/deny-match,<perm>/<perm>-match)For example, if a resource has
[read, write]"permissions category" we would then have 8 "permissions variants" (using the previous scheme "match" which means non-recursive while, default is recursive) :This becomes worst the more permissions "category" we have.
API Changes
[perm]-match/[perm]permissions to respective[perm]-allow-[match|recur](
[perm]without modifier remains the[recur]variant for backward compatibility).[perm]-deny-[match|recur]variants to existing sets of services.[perm]-[allow|deny]-[match|recur]), it will be easy to got from string to dict representation(how should we proceed with this??,
eg:
[perm]-allow-matchapplied when[perm]-deny-matchalready exists will not cause any problem from the db side as they are both unique strings. It wouldn't cause problem either when we resolve the effective permission (as deny has priority over allow).This will cause problem in the sense that when
[perm]-allow-matchis applied, if one forgets to check for[perm]-deny-matchalready applied on the same resource, it will still appear as forbidden access although the user just added the allow permission. It is not very intuitive.For each permission we would have the following properties :
The behavior should look like this given resource "/root/dir/file" :
UI Changes
We should change the checkboxes to a combobox of all applicable permissions to make it more succinct.
The currently existing checkbox view is still very relevant for overall viewing of effective permissions.
This could be the visual rendering of results from #341
Because on that view, we will want only the 'allowed' vs 'denied' effective results, the way this result is obtained (recursive or match) does not matter. Checkboxes would be read-only in that case.
Other
readandwrite.The text was updated successfully, but these errors were encountered: