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

add type query parameter to filter requested services/resource tree #471

Merged
merged 13 commits into from
Oct 4, 2021

Conversation

fmigneault
Copy link
Collaborator

@fmigneault fmigneault commented Oct 1, 2021

Changes

  • Add type query parameter to multiple requests returning Services or Resources regrouped by ServiceType
    for a given User or Group in order to limit listing in responses and optimise some operations where only a
    subset of details are needed.
  • Using new type query to filter ServiceType, improve Permissions listing in UI pages with faster processing
    because Services that are not required (since they are not currently being displayed by the tab-panel view) can
    be skipped entirely, removing the need to compute their underlying Resource and Permissions tree hierarchy.

References

@fmigneault fmigneault requested a review from dbyrns October 1, 2021 04:39
@github-actions github-actions bot added api Something related to the API operations doc Documentation improvements or building problem tests Test execution or additional use cases ui Something related to the UI operations or display labels Oct 1, 2021
@fmigneault fmigneault mentioned this pull request Oct 1, 2021
… under resources endpoint + add test utils to retrieve perms from HTML resource-tree pages + define test validating displayed perms (fixes #463)
@fmigneault fmigneault mentioned this pull request Oct 4, 2021
20 tasks
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2021

Codecov Report

Merging #471 (08e588f) into master (8e63353) will increase coverage by 0.16%.
The diff coverage is 90.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #471      +/-   ##
==========================================
+ Coverage   79.28%   79.45%   +0.16%     
==========================================
  Files          70       70              
  Lines        8836     8909      +73     
  Branches     1207     1211       +4     
==========================================
+ Hits         7006     7079      +73     
+ Misses       1550     1544       -6     
- Partials      280      286       +6     
Impacted Files Coverage Δ
magpie/adapter/__init__.py 62.79% <ø> (ø)
magpie/api/management/resource/resource_utils.py 80.34% <0.00%> (-0.70%) ⬇️
magpie/models.py 87.82% <50.00%> (-0.16%) ⬇️
magpie/api/management/group/group_utils.py 90.00% <76.47%> (-1.55%) ⬇️
magpie/services.py 81.33% <77.77%> (+0.27%) ⬆️
magpie/api/management/service/service_views.py 95.48% <80.00%> (-1.14%) ⬇️
magpie/ui/management/views.py 48.38% <87.50%> (+1.06%) ⬆️
magpie/__meta__.py 100.00% <100.00%> (ø)
magpie/adapter/magpieowssecurity.py 69.15% <100.00%> (+0.29%) ⬆️
magpie/api/management/group/group_views.py 100.00% <100.00%> (ø)
... and 11 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 75531be...08e588f. Read the comment docs.

dbyrns
dbyrns previously approved these changes Oct 4, 2021
Copy link
Contributor

@dbyrns dbyrns left a comment

Choose a reason for hiding this comment

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

I'm wondering what is changing exactly in the shown permission and found a typo. Else everything looks good.

magpie/api/schemas.py Outdated Show resolved Hide resolved
@@ -465,11 +465,17 @@ def get_user_or_group_resources_permissions_dict(self, user_or_group_name, servi
or :term:`Group` accordingly to specified arguments.
"""
if is_user:
query = "?inherited=true" if is_inherit_groups_permissions else ""
path = schemas.UserResourcesAPI.path.format(user_name=user_or_group_name) + query
# because page can only show a single permission (per name/resource) at a time, apply resolution
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference with before?
The single permission shown should be the effective, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. The displayed permission is either direct or inherited in the combobox, based on the checkbox "show intherited".
The effective permission is obtained by the test button next to it that transforms into checkmark/cross.

The problem was that given a resource X, a user member of both groups A and B, each with different permissions on X was not resolving the most important one to be displayed.
Cases where [(user, X, allow-match), (A, X, allowed-recurive), (B, X, deny-match)] are applied would not be resolved and would simply show the last item in the HTML <select> <options> (that's how it resolves which one to show when many are selected for single-select comboboxes).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hum that's right, we don't have a "word" for the "most important one". But I meant the "local effective", without considering the scope. Anyway, we now have the good behavior. Thank you!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed there is no 'term' in the glossary, but I refer to that concept as resolved in docs.
It is present here: https://pavics-magpie.readthedocs.io/en/latest/permissions.html#types-of-permissions

@fmigneault fmigneault merged commit b32dc9a into master Oct 4, 2021
@fmigneault fmigneault deleted the svc-type branch October 4, 2021 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Something related to the API operations doc Documentation improvements or building problem tests Test execution or additional use cases ui Something related to the UI operations or display
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Bad effective user permission display in UI
3 participants