Fix #572 Import parent permissions #585

m6121 · 2023-02-16T14:57:25Z

This PR aims at fixing issue #572

coveralls · 2023-02-16T16:56:04Z

Coverage: 90.469% (+0.04%) from 90.433% when pulling a01ecfd on dev-1.9.2/572-import-parent-permissions into dd0a232 on dev.

MyPyDavid · 2023-02-20T15:30:03Z

rdmo/projects/views/project.py

@@ -126,14 +126,16 @@ def get_context_data(self, **kwargs):
                                                 .filter_catalog(self.object.catalog) \
                                                 .filter_group(self.request.user) \
                                                 .filter_availability(self.request.user).exists()
+        context['ancestors_import'] = ancestors.filter_user(user=self.request.user) \


I've changed the query to filter_user instead of filter.
I thought that sometimes the projects that should be in the import were filtered out by filter?

filter_user takes admins and site_managers into account, so this makes sense here.

On the second thought, I don't think this aproach is correct, since it will not account for "role inheritance". If you are the owner of a parent project, you are automatically owner of all descendants. I think it would be better to use the rules in the templates to filter the projects.

Alternatively we can use something like:

queryset = Project.objects.filter(user=self.request.user) for instance in queryset: queryset |= instance.get_descendants() queryset = queryset.distinct()

which is the query for /projects/.

MyPyDavid · 2023-02-20T15:33:40Z

rdmo/projects/templates/projects/project_detail_header_hierarchy.html

-            {{ node.title }}
-            {% endif %}
-        </a>
+        {% if node.id in user_project_family_ids %}


with this condition, only the projects that a user has permission for are also clickable in the hierarchy tree.

Thought it was closely related to this PR, but it can also be left out ;)

Nice extension. Looks good to me.

I think

{% has_perm 'projects.view_project_object' request.user project as can_view_project %} {% if can_view_project %}

would be better.

jochenklar · 2023-02-21T10:32:14Z

rdmo/projects/views/project.py

@@ -126,14 +126,16 @@ def get_context_data(self, **kwargs):
                                                 .filter_catalog(self.object.catalog) \
                                                 .filter_group(self.request.user) \
                                                 .filter_availability(self.request.user).exists()
+        context['ancestors_import'] = ancestors.filter_user(user=self.request.user) \


On the second thought, I don't think this aproach is correct, since it will not account for "role inheritance". If you are the owner of a parent project, you are automatically owner of all descendants. I think it would be better to use the rules in the templates to filter the projects.

jochenklar · 2023-02-21T10:34:23Z

rdmo/projects/templates/projects/project_detail_header_hierarchy.html

-            {{ node.title }}
-            {% endif %}
-        </a>
+        {% if node.id in user_project_family_ids %}


I think

{% has_perm 'projects.view_project_object' request.user project as can_view_project %} {% if can_view_project %}

would be better.

jochenklar · 2023-02-21T10:37:20Z

rdmo/projects/templates/projects/project_detail_sidebar_parent_import.html

@@ -6,7 +6,7 @@
    <input type="hidden" name="method" value="import_project">

    <select class="form-control" name="source">
-        {% for project in project.get_ancestors %}
+        {% for project in ancestors_import %}


{% has_perm 'projects.view_project_object' request.user project as can_view_project %} {% if can_view_project %}

same as above. We probably need to add some prefetch_related/select_related to the database query.

jochenklar · 2023-02-21T10:38:00Z

rdmo/projects/views/project.py

@@ -126,14 +126,16 @@ def get_context_data(self, **kwargs):
                                                 .filter_catalog(self.object.catalog) \
                                                 .filter_group(self.request.user) \
                                                 .filter_availability(self.request.user).exists()
+        context['ancestors_import'] = ancestors.filter_user(user=self.request.user) \


Alternatively we can use something like:

queryset = Project.objects.filter(user=self.request.user) for instance in queryset: queryset |= instance.get_descendants() queryset = queryset.distinct()

which is the query for /projects/.

m6121 · 2023-02-22T14:45:54Z

Hi @MyPyDavid and @jochenklar, thanks for your reviews and suggestions. I tried to address them.

For the Project hierarchy, the template solution works well.
However, for the import we need to know if at least one project exists that can be imported in order to display respectively hide the import selection field.

Towards the use of prefetch_related/select_related I don't have an idea how to integrate this into the logic at the moment.

m6121 · 2023-02-22T14:51:03Z

Would it be a performance boost if the prefetch related is used as follows?

ancestors_import = []
for instance in ancestors.exclude(id=project.id).prefetch_related('user'):
    if self.request.user.has_perm('projects.view_project_object', instance):
        ancestors_import.append(instance)

…mport-parent-permissions Fix rdmorganiser#572 Import parent permissions

Fix #572

3998b3e

m6121 added this to the 1.9.2 milestone Feb 16, 2023

m6121 self-assigned this Feb 16, 2023

m6121 requested review from jochenklar and MyPyDavid February 16, 2023 14:58

MyPyDavid added 2 commits February 20, 2023 16:12

change ancestors query to filter_user

ee06a5a

add filter to project detail header hierarchy

06d1947

MyPyDavid reviewed Feb 20, 2023

View reviewed changes

m6121 linked an issue Feb 21, 2023 that may be closed by this pull request

Import from parent project list doesn't check permissions #572

Closed

jochenklar requested changes Feb 21, 2023

View reviewed changes

Fix logic according to reviews

a01ecfd

jochenklar merged commit 77b1f79 into dev Feb 23, 2023

jochenklar deleted the dev-1.9.2/572-import-parent-permissions branch February 23, 2023 13:08

CalamityC pushed a commit to CalamityC/rdmo that referenced this pull request Nov 23, 2023

Merge pull request rdmorganiser#585 from rdmorganiser/dev-1.9.2/572-i…

1ae915f

…mport-parent-permissions Fix rdmorganiser#572 Import parent permissions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #572 Import parent permissions #585

Fix #572 Import parent permissions #585

m6121 commented Feb 16, 2023

coveralls commented Feb 16, 2023 •

edited

Loading

MyPyDavid Feb 20, 2023

jochenklar Feb 21, 2023

jochenklar Feb 21, 2023

jochenklar Feb 21, 2023

MyPyDavid Feb 20, 2023

m6121 Feb 20, 2023

jochenklar Feb 21, 2023

jochenklar Feb 21, 2023

jochenklar Feb 21, 2023

jochenklar Feb 21, 2023

jochenklar Feb 21, 2023

m6121 commented Feb 22, 2023

m6121 commented Feb 22, 2023

Fix #572 Import parent permissions #585

Fix #572 Import parent permissions #585

Conversation

m6121 commented Feb 16, 2023

coveralls commented Feb 16, 2023 • edited Loading

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

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

m6121 commented Feb 22, 2023

m6121 commented Feb 22, 2023

coveralls commented Feb 16, 2023 •

edited

Loading