Modified permissions scheme #353

fmigneault · 2020-09-25T18:16:56Z

Fixes #235
Fixes #342

Tasks

Summary of things listed in #342 that are being addressed:

…rsion: 2.0.0 → 2.1.0 to test

…ails

…ess/scope modifiers + perms doc

… responses

dbyrns

So far so good. Everything is now in place to implement the effective permission.
Do you plan to make the generic one up front or update the rest api implementation first?

docs/permissions.rst

magpie/permissions.py

fmigneault · 2020-10-05T14:54:30Z

So far so good. Everything is now in place to implement the effective permission.
Do you plan to make the generic one up front or update the rest api implementation first?

I will implement the generic resolution under ServiceInterface so that all service types can employ it, but realistically will test it using ServiceAPI implementation. ServiceAPI will only need to convert the "path" definition into the generic resolution format.

dbyrns · 2020-10-05T15:15:02Z

but realistically will test it using ServiceAPI implementation. ServiceAPI will only need to convert the "path" definition into the generic resolution format.

Please check also for the thredds service as this is another tree based permissions scheme.

codecov-commenter · 2020-10-06T00:11:43Z

Codecov Report

Merging #353 into master will increase coverage by 0.76%.
The diff coverage is 82.34%.

@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
+ Coverage   68.91%   69.67%   +0.76%     
==========================================
  Files          65       65              
  Lines        7009     7324     +315     
  Branches      950      992      +42     
==========================================
+ Hits         4830     5103     +273     
- Misses       1961     1993      +32     
- Partials      218      228      +10

Impacted Files	Coverage Δ
magpie/api/management/resource/resource_utils.py	`75.80% <0.00%> (ø)`
magpie/models.py	`89.72% <ø> (ø)`
magpie/register.py	`27.22% <18.75%> (+0.18%)`	⬆️
magpie/api/management/group/group_views.py	`62.80% <50.00%> (-3.25%)`	⬇️
magpie/ui/management/views.py	`41.72% <50.00%> (ø)`
magpie/ui/user/views.py	`62.50% <50.00%> (ø)`
magpie/api/management/group/group_utils.py	`67.15% <51.16%> (-2.68%)`	⬇️
magpie/services.py	`46.44% <58.33%> (+1.02%)`	⬆️
magpie/api/management/user/user_views.py	`94.31% <68.42%> (-5.69%)`	⬇️
magpie/permissions.py	`88.19% <87.41%> (-2.98%)`	⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86d535d...6e79dc8. Read the comment docs.

…-type param error

…requirement-dev twitcher to test adapter

fmigneault · 2020-10-20T20:33:25Z

@dbyrns
3.0.0-rc is deployed on https://ogc-ades.crim.ca/magpie for review

dbyrns

Review in progress.

magpie/services.py

fmigneault · 2020-10-21T23:09:53Z

@dbyrns adjusted with comment details as per review

dbyrns · 2020-10-22T12:50:45Z

Regarding ea2455d, you were not supposed to start another PR for that?

dbyrns · 2020-10-22T14:11:26Z

magpie/services.py

-                        # 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):


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

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

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

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)

direct DENY > direct ALLOW (line 314)

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)

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

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.

We then enter User branch. We are in the realm of User-only permissions. So DENY > ALLOW is the only thing to evaluate.

ONLY THEN, once we established we are not evaluating User permissions, we compare only Group with Group.

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.

[...] 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.

[...] 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.

fmigneault · 2020-10-23T02:31:39Z

Regarding ea2455d, you were not supposed to start another PR for that?

Yes, look at the branch name permission-extensions

This one is permissions-scheme

dbyrns

Good job with all that. The tests are pretty exhaustives!
Here is some issues I got with the UI:

Cannot collapse tree using the small arrow (the text do the job but it's less obvious)
The apply button position isn't appropriate for me, it's goal is not clear at first. Renamed to "Apply permissions" and moved to the bottom right under the tree view along with a Cancel button that simply refresh the page could do the trick, don't you think?
I can leave the page with pending changes without any notice. Using https://stackoverflow.com/questions/7317273/warn-user-before-leaving-web-page-with-unsaved-changes and the dirty flag could solve that and even improve issue PAV-415 Improve Magpie UI #2 solution by enabling the button only when changes are done.
Why the effective permission test button is only enabled with inherited permission checked? Is it because the effective resolution doesn't exist for the current user only? Maybe we should check the view inherited group permissions checkbox by default so that we can see this nice feature up front?

dbyrns · 2020-10-26T20:09:58Z

magpie/services.py

-                        # 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):


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

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)

direct DENY > direct ALLOW (line 314)

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)

dbyrns · 2020-10-26T20:36:50Z

magpie/ui/management/views.py

-                removed_perms, new_perms = \
-                    self.edit_user_or_group_resource_permissions(user_name, res_id, is_user=True)
+            elif "edit_permissions" in self.request.POST and not inherit_grp_perms:
+                # FIXME:


Why doing stuff in batch remove the need to add remote ressources?
If I want to add a permission on something that doesn't already exist, it seems like the add_remote fct should be required, no?

I actually never understood that operation... in order to add a new permission on a resource, the resource must exist already (locally), or it wouldn't be listed in the tree to had the permission on it in the first place...

The only use case I can see is to fetch new resources from the remote-service to populate missing ones locally (Magpie's representation of the real resources), which is what the sync-button is for (already available).

This is because david c. did that part, but the trick is that the ui shows ressources that doesn't exist in magpie db. It is using the synched remote ressources db (if I remember correctly) and mix it with the magpie ressources.
So the synch doesn't really populate the magpie ressources, but the remote db and the mix occurs in real time for the ui. So checking a ressource doesn't mean that it already exists. If that ressource came from the remote db it must first be added to the magpie db before setting its permission.

We will need to revisit this whole functionality IMHO. It is not very well maintained.
For example, we cannot update the service type at later date using the API, and creating a service via the UI doesn't provide any selector to specify which one we want.
I have also booted a quick demo THREDDS server and it fails to sync files (with force sync button) due to internal parsing.

dbyrns · 2020-10-26T20:58:00Z

I'm also wondering if I miss something with the test effective permission button... In this screenshot, given that I have a direct deny recursive permission, should I got uncheck marks for the tested ressources?

fmigneault · 2020-10-26T21:21:28Z

Cannot collapse tree using the small arrow (the text do the job but it's less obvious)

Yes. The event is only on the line-text. I struggled a bit to make it work because adding the event directly on the list-item was firing when clicking on the selects. I defined a "hover-area" using the text only to avoid it. I can try looking into adding the event only on the <li> ::marker as well. I guess it should be possible.

The apply button position isn't appropriate for me, it's goal is not clear at first. Renamed to "Apply permissions" and moved to the bottom right under the tree view along with a Cancel button that simply refresh the page could do the trick, don't you think?

I can leave the page with pending changes without any notice. Using https://stackoverflow.com/questions/7317273/warn-user-before-leaving-web-page-with-unsaved-changes and the dirty flag could solve that and even improve issue PAV-415 Improve Magpie UI #2 solution by enabling the button only when changes are done.

I agree with renaming it to make it more explicit. The reason I put the button on top was purposely to avoid the situation of changing permissions and forgetting to press apply at the bottom without going into the full dirty form handling.

Looking at the link, it seems like validating that items were modified is more troublesome than it looks. Quick method doesn't work for selects... need to had many flags to handle edge cases, and then, many different browsers work differently or come keys/mouse-clicks won't work. I feel it is too much effort to be worth it, and we will probably hit edge cases were minor edits spams for "validate to leave page warnings" that I will forcefully disable anyway. Sadly, I think the best will be "make the error once and then you know for the future".

Why the effective permission test button is only enabled with inherited permission checked? Is it because the effective resolution doesn't exist for the current user only? Maybe we should check the view inherited group permissions checkbox by default so that we can see this nice feature up front?

Yes. Effective are by definition user+inherited. When testing, I found that it was really confusing to have only user permissions displayed and have things allowed/denied without the corresponding checkboxes and permissions displayed in the selects.

I find I go into that view way more often to set permissions than to test effective ones. I also find viewing user+group permissions by default is not natural since we arrive there from clicking "Users" button. We would be greeted with a page full of inactive options, and it is not intuitive to know we need to click the checkbox to make them active.

fmigneault · 2020-10-26T21:31:39Z

@dbyrns Regarding the permission, is your user an admin? If so, you got full access for effective permissions.

Using non-admin user:

dbyrns · 2020-10-27T13:26:49Z

@dbyrns Regarding the permission, is your user an admin? If so, you got full access for effective permissions.

Haha, that's it! Everything works fine.

dbyrns · 2020-10-27T13:49:39Z

Regarding my 4 issues :

If you can cover the li that would be cool (the arrow was working before, don't know how however). But if this is a big job, never mind, admins will get used to it.
Ok for renaming. Do you think that moving it to the top-right at least (nearer to the permissions) would be a good compromise?
I did it for a small project and it is really just setting 3 js listeners on document load: combo updated (set dirty), cancel/apply (reset dirty), unloaded (if dirty, warn). It is a nice to have tough and useless if we move to asynch js, so forget it, that was just FYI.
You're right, again :) I do think, however, that we could rename the checkbox for "View group inherited and effective permissions" to get a clue about the nice effective stuff. What do you think?

fmigneault · 2020-10-28T04:58:42Z

If you can cover the li that would be cool (the arrow was working before, don't know how however). But if this is a big job, never mind, admins will get used to it.

I found a way to do it using a fake div area. I couldn't apply the event on the marker directly.
It is deployed on ogc-ades, using 3.1.0-rc so #363 is also included.

Ok for renaming. Do you think that moving it to the top-right at least (nearer to the permissions) would be a good compromise?

It doesn't align very nicely with Sync button when it is there.
Will need to flip the elements around a bit. (not hard, just didn't notice this message update until after deployed)

You're right, again :) I do think, however, that we could rename the checkbox for "View group inherited and effective permissions" to get a clue about the nice effective stuff. What do you think?

Sounds good.

dbyrns · 2020-10-29T15:43:23Z

I still see 3 pending issues :

…t the process

fmigneault · 2020-10-29T17:20:22Z

I still see 3 pending issues :

1. [#353 (comment)](https://github.com/Ouranosinc/Magpie/pull/353#discussion_r512742704)

2. [#353 (comment)](https://github.com/Ouranosinc/Magpie/pull/353#issuecomment-717698950), case #2

3. [#353 (comment)](https://github.com/Ouranosinc/Magpie/pull/353#issuecomment-717698950), case #4

addressed via 2727e24
& 3. addressed via 1322770 of PR Permission extensions #363

dbyrns

Well done! Another great addition to Magpie.

fmigneault force-pushed the permissions-scheme branch from e1339b7 to 460c84a Compare September 28, 2020 23:09

fmigneault requested a review from dbyrns October 1, 2020 23:55

fmigneault added 6 commits October 1, 2020 20:20

[wip] permissions with added access/scope scheme (#342)

59ba055

adjust alembic revisions sorted by date

298bd37

migration and test of explicit permission conversion (#342) + bump ve…

912495e

…rsion: 2.0.0 → 2.1.0 to test

add permission-type representation to help understanding response det…

5d028d6

…ails

add POST/DELETE PermissionSet request support

cbd65e0

PUT request for permissions + handle conflict permission-name via acc…

eb62b54

…ess/scope modifiers + perms doc

fmigneault force-pushed the permissions-scheme branch from 460c84a to eb62b54 Compare October 2, 2020 00:25

handle pre-reseolved permission-types via ziggurat permissions during…

5ec0265

… responses

dbyrns reviewed Oct 3, 2020

View reviewed changes

docs/permissions.rst Outdated Show resolved Hide resolved

magpie/permissions.py Show resolved Hide resolved

fmigneault added 2 commits October 5, 2020 15:17

adjust tests with new permission scheme

a2b85fb

patch tests with implicit permission names

ff84eac

github-actions bot added the ui Something related to the UI operations or display label Oct 5, 2020

fmigneault added 2 commits October 5, 2020 16:34

test more combinations

0a6eb2e

add test for effective permissions with deny

6e79dc8

fmigneault added 4 commits October 6, 2020 15:00

more permission tests

47f5a53

more tests for PermissionSet from ziggurat PermissionTuple + fix perm…

a9f02f4

…-type param error

[wip] effective permission resolution + setup functional tests + add …

41a9b25

…requirement-dev twitcher to test adapter

[wip] impl of effective permissions for ServiceAPI

80e31f3

github-actions bot added the plugin Service plugin label Oct 8, 2020

[wip] functional test of ServiceAPI effective access

a0fd70b

fix existing UI test + add extra UI test for permission selectors

8c4c35f

fmigneault requested a review from dbyrns October 20, 2020 22:36

adjust changelog with temp version tag

81b46a2

fmigneault added a commit that referenced this pull request Oct 21, 2020

add docs relative to thredds-specific config for #361, relates to #353

ff8b6fc

dbyrns reviewed Oct 21, 2020

View reviewed changes

magpie/services.py Show resolved Hide resolved

magpie/services.py Show resolved Hide resolved

magpie/services.py Outdated Show resolved Hide resolved

magpie/services.py Outdated Show resolved Hide resolved

comments and simplifications of the main effecitve permission method

9f6dda2

fmigneault added a commit that referenced this pull request Oct 21, 2020

add docs relative to thredds-specific config for #361, relates to #353

ea2455d

dbyrns reviewed Oct 22, 2020

View reviewed changes

This was referenced Oct 23, 2020

Permission extensions #363

Merged

WMS parsing of THREDDS service #234

Closed

dbyrns reviewed Oct 26, 2020

View reviewed changes

fmigneault requested a review from dbyrns October 28, 2020 21:42

update comments of effective permission to be even more explicit abou…

2727e24

…t the process

fmigneault added a commit that referenced this pull request Oct 29, 2020

add docs relative to thredds-specific config for #361, relates to #353

44ca146

fmigneault merged commit f575987 into master Oct 29, 2020

fmigneault deleted the permissions-scheme branch October 29, 2020 17:21

dbyrns approved these changes Oct 29, 2020

View reviewed changes

tlvu mentioned this pull request Nov 25, 2020

Magpie thredds bird-house/birdhouse-deploy#107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified permissions scheme #353

Modified permissions scheme #353

fmigneault commented Sep 25, 2020 •

edited

Loading

dbyrns left a comment

fmigneault commented Oct 5, 2020 •

edited

Loading

dbyrns commented Oct 5, 2020

codecov-commenter commented Oct 6, 2020

fmigneault commented Oct 20, 2020

dbyrns left a comment •

edited

Loading

fmigneault commented Oct 21, 2020

dbyrns commented Oct 22, 2020

dbyrns Oct 22, 2020

fmigneault Oct 22, 2020

dbyrns Oct 26, 2020

fmigneault Oct 26, 2020

dbyrns Oct 27, 2020

fmigneault commented Oct 23, 2020

dbyrns left a comment

dbyrns Oct 26, 2020

dbyrns Oct 26, 2020

fmigneault Oct 26, 2020

dbyrns Oct 27, 2020

fmigneault Oct 28, 2020

dbyrns commented Oct 26, 2020

fmigneault commented Oct 26, 2020 •

edited

Loading

fmigneault commented Oct 26, 2020

dbyrns commented Oct 27, 2020

dbyrns commented Oct 27, 2020

fmigneault commented Oct 28, 2020 •

edited

Loading

dbyrns commented Oct 29, 2020

fmigneault commented Oct 29, 2020

dbyrns left a comment

Modified permissions scheme #353

Modified permissions scheme #353

Conversation

fmigneault commented Sep 25, 2020 • edited Loading

Tasks

dbyrns left a comment

Choose a reason for hiding this comment

fmigneault commented Oct 5, 2020 • edited Loading

dbyrns commented Oct 5, 2020

codecov-commenter commented Oct 6, 2020

Codecov Report

fmigneault commented Oct 20, 2020

dbyrns left a comment • edited Loading

Choose a reason for hiding this comment

fmigneault commented Oct 21, 2020

dbyrns commented Oct 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmigneault commented Oct 23, 2020

dbyrns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbyrns commented Oct 26, 2020

fmigneault commented Oct 26, 2020 • edited Loading

fmigneault commented Oct 26, 2020

dbyrns commented Oct 27, 2020

dbyrns commented Oct 27, 2020

fmigneault commented Oct 28, 2020 • edited Loading

dbyrns commented Oct 29, 2020

fmigneault commented Oct 29, 2020

dbyrns left a comment

Choose a reason for hiding this comment

fmigneault commented Sep 25, 2020 •

edited

Loading

fmigneault commented Oct 5, 2020 •

edited

Loading

dbyrns left a comment •

edited

Loading

fmigneault commented Oct 26, 2020 •

edited

Loading

fmigneault commented Oct 28, 2020 •

edited

Loading