-
-
Notifications
You must be signed in to change notification settings - Fork 226
[fix] Fixed DeviceConnection and Command API endpoints for non-superuser #1093
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
base: master
Are you sure you want to change the base?
Conversation
I found out that the problem appears when checking permissions here https://github.com/openwisp/openwisp-controller/blob/master/openwisp_controller/connection/api/views.py#L85 Unfortunately, I found also a similar problem there https://github.com/openwisp/openwisp-controller/blob/master/openwisp_controller/connection/api/views.py#L146 (TestConnectionApi needs to run with non-superuser user to expose it). => It looks like a "design" problem / a mismatch somewhere in <urls / views / permissions / DB> ? I lack history and context to be able to address this I am afraid. Please advise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll be reviewing this in the coming weeks and come back to you, it's on our TODO list for our next batch of work.
883a5ea
to
91cb3af
Compare
Bug: When an organization admin (non-superuser) attempted to retrieve command details via the REST API, the request resulted in a 500 server error. Fix: Configured `organization_field` and `organization_lookup` in `BaseCommandView`, ensuring that `FilterByParent` and `DjangoModelPermissions` are applied correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pandafy as reported by @asmodehn, BaseDeviceConnection
suffers the same issue which reflects in both DeviceConnenctionListCreateView
(on POST) and on DeviceConnectionDetailView
(on GET).
See the following traceback I got when trying to POST to DeviceConnenctionListCreateView
:
Traceback (most recent call last):
File "/lib/python3.10/site-packages/django/core/handlers/exception.py", line 42, in inner
response = await get_response(request)
File "/lib/python3.10/site-packages/django/core/handlers/base.py", line 284, in _get_response_async
response = await sync_to_async(
File "/usr/lib/python3.10/concurrent/futures/thread.py", line 58, in run
result = self.fn(*self.args, **self.kwargs)
File "/lib/python3.10/site-packages/django/template/response.py", line 114, in render
self.content = self.rendered_content
File "/lib/python3.10/site-packages/rest_framework/response.py", line 74, in rendered_content
ret = renderer.render(self.data, accepted_media_type, context)
File "/lib/python3.10/site-packages/rest_framework/renderers.py", line 726, in render
context = self.get_context(data, accepted_media_type, renderer_context)
File "/lib/python3.10/site-packages/rest_framework/renderers.py", line 657, in get_context
raw_data_post_form = self.get_raw_data_form(data, view, 'POST', request)
File "/lib/python3.10/site-packages/rest_framework/renderers.py", line 540, in get_raw_data_form
if not self.show_form_for_method(view, method, request, instance):
File "/lib/python3.10/site-packages/rest_framework/renderers.py", line 432, in show_form_for_method
view.check_object_permissions(request, obj)
File "/lib/python3.10/site-packages/rest_framework/views.py", line 345, in check_object_permissions
if not permission.has_object_permission(request, self, obj):
File "/home/nemesis/Code/openwisp/openwisp-users/openwisp_users/api/permissions.py", line 33, in has_object_permission
organization = self.get_object_organization(view, obj)
File "/home/nemesis/Code/openwisp/openwisp-users/openwisp_users/api/permissions.py", line 19, in get_object_organization
raise AttributeError(
Exception Type: AttributeError at /api/v1/controller/device/5131f85b-5604-46be-a0da-b45ded388a08/connection/
Exception Value: Organization not found, `organization_field` not implemented correctly.
Another point which needs improvements is that after you reworked the tests there seems to be redundancy, it's really too much test code for a 2 lines fix.
I recommend the following:
- add a failing test case for
DeviceConnenctionListCreateView
(POST) andDeviceConnectionDetailView
(GET) - review the other tests added previously ensuring there's only one test for each case
@@ -387,6 +442,61 @@ def test_create_command_without_connection(self): | |||
) | |||
|
|||
|
|||
# The same tests, but with a normal user | |||
class TestCommandsApiNonAdmin(TestCommandsAPI): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pandafy aren't the tests in this class now redundant with test_endpoints_for_org_operators_own_org
above?
Earlier the test suite was testing only the list endpoint with non-superuser. This created a blind-spot for the detail endpoint. I will try to minimize the test code without much trade-off with coverage. |
Checklist
Description of Changes
Added a test to reveal a problem noticed during openwisp 1.1.1 usage.
When a user, who is not a superuser (but with correct permissions), tried to access the details of a device command, an error is triggered:
This scenario currently seems to be a blind spot in unit tests
Let me know what you think about this...