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.

Sign up for GitHub

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

Modified permissions scheme #353

Merged
merged 36 commits into from
Oct 29, 2020
Merged

Modified permissions scheme #353

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
59ba055
[wip] permissions with added access/scope scheme (#342)
fmigneault Sep 5, 2020
298bd37
adjust alembic revisions sorted by date
fmigneault Sep 22, 2020
912495e
migration and test of explicit permission conversion (#342) + bump ve…
fmigneault Sep 22, 2020
5d028d6
add permission-type representation to help understanding response det…
fmigneault Sep 24, 2020
cbd65e0
add POST/DELETE PermissionSet request support
fmigneault Sep 24, 2020
eb62b54
PUT request for permissions + handle conflict permission-name via acc…
fmigneault Oct 2, 2020
5ec0265
handle pre-reseolved permission-types via ziggurat permissions during…
fmigneault Oct 2, 2020
a2b85fb
adjust tests with new permission scheme
fmigneault Oct 5, 2020
ff84eac
patch tests with implicit permission names
fmigneault Oct 5, 2020
0a6eb2e
test more combinations
fmigneault Oct 5, 2020
6e79dc8
add test for effective permissions with deny
fmigneault Oct 5, 2020
47f5a53
more permission tests
fmigneault Oct 6, 2020
a9f02f4
more tests for PermissionSet from ziggurat PermissionTuple + fix perm…
fmigneault Oct 6, 2020
41a9b25
[wip] effective permission resolution + setup functional tests + add …
fmigneault Oct 8, 2020
80e31f3
[wip] impl of effective permissions for ServiceAPI
fmigneault Oct 8, 2020
a0fd70b
[wip] functional test of ServiceAPI effective access
fmigneault Oct 9, 2020
0523fac
functional ServiceAPI recursive effective permissions
fmigneault Oct 9, 2020
b4987b6
working func test for ServiceAPI & ServiceWPS extended by Process Res…
fmigneault Oct 14, 2020
6bf0b21
fix effective permission API test
fmigneault Oct 14, 2020
f6e7b8d
ServiceAccess tests + adjust 400->403 for forbidden children resource…
fmigneault Oct 14, 2020
1214c05
[WIP] implementation of ServiceGeoserverWMS for effective permissions
fmigneault Oct 15, 2020
f158f9e
functional ServiceGeoserverWMS with effective permissions
fmigneault Oct 15, 2020
ebdbb48
effective permissions on ServiceTHREDDS & ServiceWFS implementations
fmigneault Oct 15, 2020
7cbc26c
update UI list of available permissions + fixes UI of some alert noti…
fmigneault Oct 15, 2020
c121e1b
fix alembic template to avoid generating linting error-inducing docst…
fmigneault Oct 15, 2020
f762a32
[WIP] display combobox for permission modifiers - not yet applying ch…
fmigneault Oct 16, 2020
2b398db
add changelog about deny permission-access (fixes #235)
fmigneault Oct 16, 2020
5160d29
update UI permission modifiers + style adjustments to reuse tree item…
fmigneault Oct 17, 2020
324fb7b
apply batch permissions + adjust UI arrow children in tree view
fmigneault Oct 19, 2020
06efa6b
add doc details about permission representations (string/JSON)
fmigneault Oct 19, 2020
971cce2
Bump version: 2.1.0 → 3.0.0
fmigneault Oct 19, 2020
2583852
add effective permission tester buttons next to corresponding resourc…
fmigneault Oct 20, 2020
8c4c35f
fix existing UI test + add extra UI test for permission selectors
fmigneault Oct 20, 2020
81b46a2
adjust changelog with temp version tag
fmigneault Oct 20, 2020
9f6dda2
comments and simplifications of the main effecitve permission method
fmigneault Oct 21, 2020
2727e24
update comments of effective permission to be even more explicit abou…
fmigneault Oct 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/permissions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,7 @@ Therefore, it can be noted that all API responses that contain details about per
"access": "allow|deny",
"scope": "match|recursive",
"type": "access|allowed|applied|direct|inherited|effective|owned"
}
]
}

Expand Down
23 changes: 17 additions & 6 deletions magpie/services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,12 @@ def effective_permissions(self, user, resource, permissions=None, allow_match=Tr
# if user is owner (directly or via groups), all permissions are set,
# but continue processing this resource until end in case user explicit deny reverts it
if perm_tup.perm_name == ALL_PERMISSIONS:
# FIXME:
# This block needs to be validated if support of ownership rules are added.
# Conditions must be revised according to wanted behaviour...
# General idea for now is that explict user/group deny should be prioritized over resource
# ownership permissions since these can be attributed to *any user* while explicit deny are
# definitely set by an admin-level user.
for perm in requested_perms:
all_perm = PermissionSet(perm, perm_set.access, perm.scope, perm.type)
if perm_set.access == Access.DENY:
fmigneault marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -298,20 +304,25 @@ def effective_permissions(self, user, resource, permissions=None, allow_match=Tr
if not match and perm_set.scope == Scope.MATCH:
continue

# less obvious use case for both of the following user/group blocks:
# no need to check explicitly for ALLOW since it was either already set during previous iteration
# (at that moment, perm=None) or a DENY was already set, but it takes precedence over it anyway

# user direct permissions have priority over inherited ones from groups
if perm_set.type == PermissionType.DIRECT:
perm = effective_perms.get(perm_name)
# explicit user deny overrides user allow if any already found
# explicit user DENY overrides user ALLOW if any already found
# if permission name not already found, ALLOW/DENY is set regardless (first occurrence)
if perm is None or perm.type == PermissionType.INHERITED or perm_set.access == Access.DENY:
effective_perms[perm_name] = perm_set
continue # final decision for this user, skip any group permissions

# otherwise check if for another group permission and yet no user permission
# otherwise check for group permission
# like previously, explicit DENY overrides ALLOW if permission name was already found
# if permission name not already found, ALLOW/DENY is set regardless (first occurrence)
perm = effective_perms.get(perm_name)
if perm is None or perm.type == PermissionType.INHERITED:
# explicit deny overrides allow if any already found
if perm is None or perm_set.access == Access.DENY:
effective_perms[perm_name] = perm_set
if perm is None or (perm.type == PermissionType.INHERITED and perm_set.access == Access.DENY):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, much easier to follow. I think it still miss a point about the perm.type == PermissionType.INHERITED.
If the perm is direct (line 312), an allow permission overrides any access type that has been inherited. It means that a direct "allow" takes precedance over inherited "deny",
Here, to override any inherited permissions the access type must be deny. It means that a "deny" group permission take precedance over other "allow" group permission.
In short, we should better explain the difference between INHERITED or DENY of line 316 vs INHERITED and DENY of line 324

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that is what comments on lines 311 and 318 describe,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to be more explicit. The following 3 rules should be clear while reading the comments:

  1. direct > inherited (line 311 is fine and line 318 cover the fact that once direct is set we're done, but line 314 put emphasis on DENY override ALLOW while opposite is true too (the reason why we got a type == inherited or access == DENY is not covered)
  2. direct DENY > direct ALLOW (line 314)
  3. inherited DENY > inherited ALLOW (line 321 is confusing because it's stating like previously while this time it's between group (the reason why we got a type == inherited and access == DENY is not covered)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is because of the way I parse the sequence that doesn't feel ambiguous for me.

  1. User > Group - so if Group was set (ALLOW or DENY) we don't care, we set whatever User permission says.
    For this reason, the opposite use case is never true.
  2. We then enter User branch. We are in the realm of User-only permissions. So DENY > ALLOW is the only thing to evaluate.
  1. ONLY THEN, once we established we are not evaluating User permissions, we compare only Group with Group.
  2. Again, the only thing to check is DENY > ALLOW resolution.

It is mostly for this reason I had 2 ifs before. It gave a clean distinction between the 2 phases (eval User > Group) and then, for each (DENY > ALLOW).
If you think about it, the original 2nd if of the Group block is identical to the 2nd one of the User group
if perm is None or perm.type == PermissionType.INHERITED or perm_set.access == Access.DENY:
Only thing is that we have a redundant check for perm.type == PermissionType.INHERITED since we evaluated that on the 1st if of the Group block, which is why it became if perm is None or perm_set.access == Access.DENY:

Making the if all-in-one for Group portion makes it more complicated to evaluate in my opinion because it introduces the mixing of Type and Access simultaneously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. [...] For this reason, the opposite use case is never true.

I meant if group was DENY, but user is ALLOW, allow override deny. Just explaining that would explain perfectly why we have a inherited or deny. For me line 311, 314 and 318 doesn't make that obvious.

  1. [...] So DENY > ALLOW is the only thing to evaluate.

This statement cover the second part of the if of l316. And the comment just above cover only that part. My point is reallly to cover the inherited part too. Comment should look like that : "permission is set if not already found (first occurence), if inherited regardless of access (direct > inherited) or deny (deny > allow)"

3 [...] we compare only Group with Group

Exact. But comment said "like previously" which compare direct with group and direct with direct.
I would change the comment to put the emphasis on the group with group and deny > allow to reflect the if, something like : "otherwise check for group permission, permission is set if not already found (first occurence) or if inherited (compare group with group only) AND deny (deny > allow)"

Sorry to be bothering, but I'm only looking to have better comments explaining what is going on for non-initiated, not to refactor the whole part.

effective_perms[perm_name] = perm_set

# don't bother moving to parent if everything is resolved already
if len(effective_perms) == len(requested_perms):
Expand Down