[refactor] Use FilterByParent mixin in BaseEmailView #354 by dee077 · Pull Request #426 · openwisp/openwisp-users

dee077 · 2025-01-22T17:05:16Z

Refactored BaseEmailView to inherit from FilterByParent mixin.

Checklist

I have read the OpenWISP Contributing Guidelines.
I have manually tested the changes proposed in this pull request.

Reference to Existing Issue

Closes #354 .

Description of Changes

I have removed the assert_parent_exists method and in BaseEmailView as It is present in FilterByParent and in the 'assert_parent_exists' method in FilterByParent made some changes which I am not sure to be correct but all the test cases are passing now.

I have a few questions as well

In between FilterByParent, FilterByParentMembership, FilterByParentManaged, and FilterByParentOwned which class to inherit I have gone for FilterByParentOwned.
Is this okay to remove this in the method assert_parent_exists in class FilterByParent? All test cases are passing after the removal as well.
```
   if not self.request.user.is_superuser:
   parent_queryset = self.get_organization_queryset(parent_queryset)
```

Removed the above code because it is causing issues, in the get_organization_queryset method the self.organization_lookup from the OrgLookup class has organization__in inside it but the field in the EmailAddress model is openwisp_user_orginization not organization causing FieldError that's why decided to remove this.

Should add a logic in the OrgLookup class to add organization_lookup correctly or removing the code is fine?

Please give your thoughts on this!

coveralls · 2025-01-27T19:57:03Z

coverage: 97.891% (+0.1%) from 97.787%
when pulling 0e47d5a on dee077:issues/354-refactor-baseemailview
into f6b018e on openwisp:master.

nemesifier · 2025-01-27T20:44:05Z

openwisp_users/api/mixins.py

    def assert_parent_exists(self):
        parent_queryset = self.get_parent_queryset()
-        if not self.request.user.is_superuser:
-            parent_queryset = self.get_organization_queryset(parent_queryset)


This doesn't look like something that was requested, why are you changing this?

This can't be removed as it's intended to restrict non superusers so they can only see objects of their organization and not objects of other organizations. Removing this is absolutely wrong. The fact that no tests are failing for this is bad and requires opening a new issue to add at least one which would fail without this code.

nemesifier · 2025-01-27T20:49:14Z

I have a few questions as well

In between FilterByParent, FilterByParentMembership, FilterByParentManaged, and FilterByParentOwned which class to inherit I have gone for FilterByParentOwned.

Why? What's the reasoning behind this choice?

Is this okay to remove this in the method assert_parent_exists in class FilterByParent? All test cases are passing after the removal as well.
   if not self.request.user.is_superuser:
   parent_queryset = self.get_organization_queryset(parent_queryset)
Removed the above code because it is causing issues, in the get_organization_queryset method the self.organization_lookup from the OrgLookup class has organization__in inside it but the field in the EmailAddress model is openwisp_user_orginization not organization causing FieldError that's why decided to remove this.

The fact that no test fails after the removal is concerning, it shows a lack of test coverage, but I am pretty sure it is not ok to remove this code.

Should add a logic in the OrgLookup class to add organization_lookup correctly or removing the code is fine?

Can you explain why you are compelled to change the internal logic of these classes instead of focusing on what the issue description is asking?

I don't understand your reasoning behind these changes. Please explain so we can understand.

nemesifier · 2025-01-27T20:54:26Z

openwisp_users/api/views.py



-class BaseEmailView(ProtectedAPIMixin, GenericAPIView):
+class BaseEmailView(ProtectedAPIMixin, FilterByParentOwned, GenericAPIView):


I think this should be:

Suggested change

class BaseEmailView(ProtectedAPIMixin, FilterByParentOwned, GenericAPIView):

class BaseEmailView(ProtectedAPIMixin, FilterByParentManaged, GenericAPIView):

That allows organization managers to use the endpoint.

Use FilterByParentManaged here please.

dee077 · 2025-03-04T16:57:36Z

In the get_organization_queryset method,

def get_organization_queryset(self, qs):
        lookup = {self.organization_lookup: getattr(self.request.user, self._user_attr)}
        return qs.filter(**lookup)

self.organization_lookup has the value organization__in, but in the User model, there is no direct link to Organization. It is managed by OrganizationUser to obtain many-to-many relationships between the Organization and User models. So, it adds openwisp_user as the prefix of the organization field and makes the field name openwisp_user_organization. However, we are searching for the field name organization, which cannot be found and causes a field error.

What I have gone through:

Remove this if condition if not self.request.user.is_superuser as it was not used in the previous implementation of BaseEmailView assert_parent_exists method. All test case passes so I have gone for this.
I have tried changing the lookup to check for the openwisp_user_organization field instead, but it causes other test failures. The models defined in /test folder like Config, Book, Shelf etc. have ForeignKey to the organization so test on these expecting field names as organization.

Please provide your thoughts on this

nemesifier

In the get_organization_queryset method,
def get_organization_queryset(self, qs):
        lookup = {self.organization_lookup: getattr(self.request.user, self._user_attr)}
        return qs.filter(**lookup)
self.organization_lookup has the value organization__in, but in the User model, there is no direct link to Organization. It is managed by OrganizationUser to obtain many-to-many relationships between the Organization and User models. So, it adds openwisp_user as the prefix of the organization field and makes the field name openwisp_user_organization. However, we are searching for the field name organization, which cannot be found and causes a field error.

Override what's needed in BaseEmailView to get the desired result.

What I have gone through:

Remove this if condition if not self.request.user.is_superuser as it was not used in the previous implementation of BaseEmailView assert_parent_exists method. All test case passes so I have gone for this.

Not the right approach, as already stated.

I have tried changing the lookup to check for the openwisp_user_organization field instead, but it causes other test failures. The models defined in /test folder like Config, Book, Shelf etc. have ForeignKey to the organization so test on these expecting field names as organization.

You have to override the methods in BaseEmailView, not in the base class (which is super wrong and would cause problems to the entire set of OpenWISP modules).

nemesifier · 2025-03-04T22:37:36Z

openwisp_users/api/mixins.py

    def assert_parent_exists(self):
        parent_queryset = self.get_parent_queryset()
-        if not self.request.user.is_superuser:
-            parent_queryset = self.get_organization_queryset(parent_queryset)


This can't be removed as it's intended to restrict non superusers so they can only see objects of their organization and not objects of other organizations. Removing this is absolutely wrong. The fact that no tests are failing for this is bad and requires opening a new issue to add at least one which would fail without this code.

nemesifier · 2025-03-04T22:37:52Z

openwisp_users/api/views.py



-class BaseEmailView(ProtectedAPIMixin, GenericAPIView):
+class BaseEmailView(ProtectedAPIMixin, FilterByParentOwned, GenericAPIView):


Use FilterByParentManaged here please.

Refactored BaseEmailView to inherit from FilterByParent and for that updated get_organization_queryset to override FilterByParent's method. Fixes openwisp#354

nemesifier

@dee077 I was not satisfied with the refactoring and I rewrote it.

@pandafy what do you think about this refactoring? See my comments below.

nemesifier · 2025-03-05T15:34:19Z

openwisp_users/api/views.py

        super().initial(*args, **kwargs)
        self.assert_parent_exists()

-    def assert_parent_exists(self):


This method is shipped by FilterByParent so we can remove it.

nemesifier · 2025-03-05T15:34:54Z

openwisp_users/api/views.py

-        return qs_user.filter(pk=self.kwargs['pk'])
+        qs = User.objects.filter(pk=self.kwargs['pk'])
+        if self.request.user.is_superuser:
+            return qs


if the user performing the request is superuser, just return the parent without further checks (superusers can do anything).

nemesifier · 2025-03-05T15:37:00Z

openwisp_users/api/views.py

+            return qs
+        return self.get_organization_queryset(qs)
+
+    def get_organization_queryset(self, qs):


The goal of this method, if I am not mistaken, is to ensure that the parent object is related to one of the organizations the user performing the API request manages, otherwise the API shall return 404 because nothing is found (the query doens't return any result).

nemesifier · 2025-03-05T15:37:26Z

openwisp_users/api/views.py

+        return self.get_organization_queryset(qs)
+
+    def get_organization_queryset(self, qs):
+        orgs = self.request.user.organizations_managed


Therefore we use this handy method to get the list of organization IDs the user manages.

nemesifier · 2025-03-05T15:38:03Z

openwisp_users/api/views.py

+        orgs = self.request.user.organizations_managed
+        return qs.filter(
+            # exclude superusers
+            is_superuser=False,


We exclude superusers as before, nothing should have changed here, organization managers can't mess with superusers, that's the point.

nemesifier · 2025-03-05T15:38:59Z

openwisp_users/api/views.py

+            # exclude superusers
+            is_superuser=False,
+            # ensure user is member of the org
+            openwisp_users_organizationuser__organization_id__in=orgs


Here we basically ensure the parent user is member of one of the organizations managed by the user performing the API reqeust.

nemesifier · 2025-03-05T15:39:08Z

openwisp_users/api/views.py

+            is_superuser=False,
+            # ensure user is member of the org
+            openwisp_users_organizationuser__organization_id__in=orgs
+        ).distinct()


Not sure this is really needed anymore.

The distinct is required. There could be a scenario where org manager manages two organizations and an end user is part of both organizations. Then, that user will appear twice in the queryset.

In [2]: org_ids = Organization.objects.values_list('id', flat=True) In [3]: org_ids Out[3]: (0.000) SELECT "openwisp_users_organization"."id" FROM "openwisp_users_organization" ORDER BY "openwisp_users_organization"."name" ASC LIMIT 21; args=(); alias=default <QuerySet [UUID('57197e42-b7a9-4342-b1ef-672d7fd6ed59'), UUID('33150131-ad21-40fb-8642-365499fb01d9')]> In [4]: User.objects.filter(openwisp_users_organizationuser__organization_id__in=org_ids) Out[4]: (0.000) SELECT "openwisp_users_user"."password", "openwisp_users_user"."last_login", "openwisp_users_user"."is_superuser", "openwisp_users_user"."username", "openwisp_users_user"."first_name", "openwisp_users_user"."last_name", "openwisp_users_user"."is_staff", "openwisp_users_user"."is_active", "openwisp_users_user"."date_joined", "openwisp_users_user"."id", "openwisp_users_user"."email", "openwisp_users_user"."bio", "openwisp_users_user"."url", "openwisp_users_user"."company", "openwisp_users_user"."location", "openwisp_users_user"."phone_number", "openwisp_users_user"."birth_date", "openwisp_users_user"."notes", "openwisp_users_user"."language", "openwisp_users_user"."password_updated" FROM "openwisp_users_user" INNER JOIN "openwisp_users_organizationuser" ON ("openwisp_users_user"."id" = "openwisp_users_organizationuser"."user_id") WHERE "openwisp_users_organizationuser"."organization_id" IN (SELECT U0."id" FROM "openwisp_users_organization" U0) LIMIT 21; args=(); alias=default <QuerySet [<User: orguser>, <User: orguser>]>

Thanks for double checking! 👍

pandafy · 2025-03-07T15:36:16Z

openwisp_users/api/views.py

+            # exclude superusers
+            is_superuser=False,
+            # ensure user is member of the org
+            openwisp_users_organizationuser__organization_id__in=orgs


This will probably cause issues when testing the feature with sample app. I have run

Suggested change

openwisp_users_organizationuser__organization_id__in=orgs

f'{app_label}_organizationuser__organization_id__in=orgs

Thanks, updated.

nemesifier · 2025-03-07T16:03:23Z

openwisp_users/api/views.py

+            is_superuser=False,
+            # ensure user is member of the org
+            openwisp_users_organizationuser__organization_id__in=orgs
+        ).distinct()


Thanks for double checking! 👍

nemesifier · 2025-03-07T16:33:28Z

openwisp_users/api/views.py

+            # exclude superusers
+            is_superuser=False,
+            # ensure user is member of the org
+            openwisp_users_organizationuser__organization_id__in=orgs


Thanks, updated.

pandafy

LGTM!

dee077 force-pushed the issues/354-refactor-baseemailview branch from 63dd5f9 to d961dc3 Compare January 22, 2025 17:17

nemesifier requested changes Jan 27, 2025

View reviewed changes

nemesifier reviewed Jan 27, 2025

View reviewed changes

nemesifier requested changes Mar 4, 2025

View reviewed changes

dee077 force-pushed the issues/354-refactor-baseemailview branch from b574634 to df1ae06 Compare March 5, 2025 09:34

[refactor] Inherit from FilterByParent openwisp#354

635d776

Refactored BaseEmailView to inherit from FilterByParent and for that updated get_organization_queryset to override FilterByParent's method. Fixes openwisp#354

dee077 force-pushed the issues/354-refactor-baseemailview branch from df1ae06 to 635d776 Compare March 5, 2025 13:05

nemesifier added 2 commits March 5, 2025 12:09

Merge branch 'master' into issues/354-refactor-baseemailview

0878753

[refactor] Rewrote queries

efe6a73

nemesifier reviewed Mar 5, 2025

View reviewed changes

pandafy reviewed Mar 7, 2025

View reviewed changes

[fix] Fix for sample_app + QA

0e47d5a

nemesifier approved these changes Mar 7, 2025

View reviewed changes

pandafy approved these changes Mar 7, 2025

View reviewed changes

nemesifier assigned nemesifier, pandafy and dee077 Mar 7, 2025

nemesifier added this to OpenWISP Contributor's Board Mar 7, 2025

nemesifier added the enhancement label Mar 7, 2025

github-project-automation bot moved this to To do (general) in OpenWISP Contributor's Board Mar 7, 2025

nemesifier merged commit 40677ab into openwisp:master Mar 7, 2025
12 checks passed

github-project-automation bot moved this from To do (general) to Done in OpenWISP Contributor's Board Mar 7, 2025



		class BaseEmailView(ProtectedAPIMixin, GenericAPIView):
		class BaseEmailView(ProtectedAPIMixin, FilterByParentOwned, GenericAPIView):

	openwisp_users_organizationuser__organization_id__in=orgs
	f'{app_label}_organizationuser__organization_id__in=orgs

Uh oh!

Conversation

dee077 commented Jan 22, 2025 • edited by nemesifier Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Reference to Existing Issue

Description of Changes

I have a few questions as well

Please give your thoughts on this!

Uh oh!

coveralls commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier commented Jan 27, 2025

I have a few questions as well

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dee077 commented Mar 4, 2025

Please provide your thoughts on this

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pandafy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dee077 commented Jan 22, 2025 •

edited by nemesifier

Loading

coveralls commented Jan 27, 2025 •

edited

Loading