[change] Improve endpoints to download firmware images #306

youhaveme9 · 2025-04-25T16:55:15Z

Checklist

I have read the OpenWISP Contributing Guidelines.
I have manually tested the changes proposed in this pull request.
I have written new test cases for new code and/or updated existing tests for changes to existing code.
I have updated the documentation.

Reference to Existing Issue

Closes #69

Please open a new issue if there isn't an existing issue yet.

pandafy

Looks on the correct track. Check my suggestion below.

pandafy · 2025-04-28T08:01:01Z

openwisp_firmware_upgrader/tests/test_api.py

@@ -970,9 +985,11 @@ def test_firmware_download(self):
        url = reverse('upgrader:api_firmware_download', args=[image.build.pk, image.pk])
        with self.subTest("Test as operator"):


We need to add a sub-test to also test as an administrator.

we are testing it for admin too below

pandafy · 2025-04-28T08:02:47Z

openwisp_firmware_upgrader/tests/test_private_storage.py

+        )
+
+        # Create a custom permission group without view permission
+        from django.contrib.auth.models import Group


Is there any reason why this import isn't at the top of the file?

Load the Group model using the swapper module

import swapper Group = swapper.load_model('openwisp_users', 'Group')

pandafy · 2025-04-28T09:06:23Z

openwisp_firmware_upgrader/private_storage/views.py

+        if not user.is_superuser:
+            perm_codename = get_permission_codename('view', self.model._meta)
+            view_perm = f'{self.model._meta.app_label}.{perm_codename}'
+            has_view_perm = user.has_perm(view_perm, self.object)
+            if not has_view_perm:
+                return False


Did you consider using the PermissionRequiredMixin instead of performing these checks manually?

pandafy · 2025-04-28T09:20:25Z

openwisp_firmware_upgrader/api/serializers.py

+    def to_representation(self, instance):
+        ret = super().to_representation(instance)
+        # Replace 'file' with the URL from file_url
+        if 'file_url' in ret:
+            ret['file'] = ret.pop('file_url')
+        return ret


Since we are already overriding the value of field value in the to_representation, then why do we need to add another field file_url to the serializer? We can directly call the get_file_url

pandafy

I am not convinced with the changes to the tests. Can you clarify my doubts?

pandafy · 2025-05-05T07:48:11Z

openwisp_firmware_upgrader/api/serializers.py

@@ -45,10 +46,26 @@ class Meta:


 class FirmwareImageSerializer(BaseSerializer):
+    file = serializers.FileField(required=False)


Can you explain why defining this field is requried? I thought, it would be added automatically by ModelSerializer.

pandafy · 2025-05-05T08:12:17Z

openwisp_firmware_upgrader/api/serializers.py

+        elif hasattr(instance, 'file'):
+            ret['file'] = instance.file.url


Isn't this the default behaviour? If we leave the 'file' field unchanged in the code, then it will contain the file.url.

I tested this locally with the following code:

In [1]: from openwisp_firmware_upgrader.api.serializers import FirmwareImage, FirmwareImageSerializer ...: ...: fw_image = FirmwareImage.objects.first() ...: fw_image.file.url ...: ...: serializer = FirmwareImageSerializer(instance=fw_image) In [2]: serializer.data Out[2]: {'id': '144d9284-6b80-471b-a2dd-52cbc3c18565', 'file': '/firmware/503ac721-94fd-469f-85a7-977330f50499/openwrt-23.05.0-ramips-mt7621-yuncore_ax820-squashfs-sysupgrade.bin', 'created': '2025-04-28T11:15:07.867479+02:00', 'modified': '2025-04-28T11:15:07.867479+02:00', 'type': 'ramips-mt7621-yuncore_ax820-squashfs-sysupgrade.bin', 'build': UUID('503ac721-94fd-469f-85a7-977330f50499')}

I think, this change makes the serializer inconsistent. We should try to return the URL to the firmware download API even if request is not present in the serializer. In order to remain consistent with DRF, we can return the relative path to the API firmware download view if there's no request object.

pandafy · 2025-05-05T08:19:05Z

openwisp_firmware_upgrader/private_storage/views.py

        )
+        has_view_perm = user.has_perm(perm, self.object)


We don't need to check this here as the PermissionRequiredMixin should handle this.

Let's also skip accessing the object here. We don't enforce object level permission checks in OpenWISP.

pandafy · 2025-05-05T08:23:07Z

openwisp_firmware_upgrader/private_storage/views.py

+    def handle_no_permission(self):
+        """
+        Return empty response for unauthorized API requests
+        """
+        if 'api' in self.request.path:
+            return HttpResponse(status=403)
+        return super().handle_no_permission()


I commented this code, and none of the tests fails. This is bad.

Is this code relevant? Won't raise_exception = True cover this scenario?

pandafy · 2025-05-05T08:27:52Z

openwisp_firmware_upgrader/tests/test_api.py

        url = reverse('upgrader:api_firmware_download', args=[image.build.pk, image.pk])
        with self.subTest(url=url):
            with self.assertNumQueries(1):
                r = client.get(url)
-            self.assertEqual(r.status_code, 401)
+            self.assertEqual(r.status_code, 403)


This is wrong. We should fix the view so it returns 401 instead of 403 for unauthenticated users.

pandafy · 2025-05-05T09:54:03Z

openwisp_firmware_upgrader/tests/test_private_storage.py

@@ -68,9 +74,41 @@ def test_staff_operator_with_same_organization(self):
            user=staff_user, organization=self.test_org, is_admin=True
        )
        self.client.force_login(staff_user)
-        self._download_firmware_assert_status(200)
+        self._download_firmware_assert_status(403)


Why did you change this? Aren't operators allowed to view the FirmwareImage?

openwisp-firmware-upgrader/openwisp_firmware_upgrader/migrations/__init__.py

Lines 28 to 34 in ee878d5

operators_read_only_admins_manage = [

'build',

'devicefirmware',

'firmwareimage',

'batchupgradeoperation',

'upgradeoperation',

]

pandafy · 2025-05-05T09:57:03Z

openwisp_firmware_upgrader/tests/test_private_storage.py


    def test_superuser(self):
        user = self._get_admin()
        self.client.force_login(user)
        self._download_firmware_assert_status(200)
+
+    def test_view_permission_check(self):


Are you sure this test is correct? I believe the code comments are outdated.

A user being Organization Admin and a user having Administrator group are two different things. Is this clear?

Yes clear now

openwisp_firmware_upgrader/tests/test_api.py

pandafy · 2025-05-12T10:01:00Z

openwisp_firmware_upgrader/private_storage/views.py

+        # Get object first since it's needed for organization check
+        self.object = self.get_object()
+
+        # Superusers can always access
+        if user.is_superuser:
+            return True
+
+        return user.is_staff and user.is_manager(
+            self.object.build.category.organization
        )


Does the logic stays same as before?

pandafy · 2025-05-12T10:05:01Z

openwisp_firmware_upgrader/tests/test_private_storage.py

+        # Add view permission first
+        content_type = ContentType.objects.get_for_model(FirmwareImage)
+        perm_codename = get_permission_codename('view', FirmwareImage._meta)
+        view_perm = Permission.objects.get(
+            content_type=content_type, codename=perm_codename
+        )
+        staff_user.user_permissions.add(view_perm)
+
+        self._create_org_user(user=staff_user, organization=org, is_admin=True)
+        self._download_firmware_assert_status(200)
+
+        # Remove org manager status
+        org_user = staff_user.openwisp_users_organization.get(
+            organization_users__organization=org
+        )
+        org_user.is_admin = False
+        org_user.save()
+
+        # Remove staff status
+        staff_user.is_staff = False
+        staff_user.save()
+
+        # Restore org manager status
+        org_user.is_admin = True
+        org_user.save()
+
+        # No access (missing staff status)
+        self._download_firmware_assert_status(403)
+
+        # Remove view permission
+        staff_user.user_permissions.remove(view_perm)
+
+        # No access (missing view permission)
+        self._download_firmware_assert_status(403)


Looks good, can you refactor the code to use subTest? It helps to organize code for testing different scenarios. You can look around the code for reference.

https://github.com/search?q=repo%3Aopenwisp%2Fopenwisp-firmware-upgrader+subTest&type=code

youhaveme9 added 3 commits April 25, 2025 15:53

[change] Improved endpoints to download firmware images

18775b0

[fix] Fixed failing tests

761bdd2

[fix] Fixed failing tests

2824f49

pandafy requested changes Apr 28, 2025

View reviewed changes

pandafy added this to OpenWISP Contributor's Board Apr 28, 2025

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

pandafy moved this from To do (general) to In progress in OpenWISP Contributor's Board Apr 28, 2025

[fix] Changed permission checker to PermissionRequiredMixin

30c8aef

pandafy requested changes May 5, 2025

View reviewed changes

[fix] Removed manual permission check

5deae9a

pandafy requested changes May 12, 2025

View reviewed changes

[tests] Added subtests for test_view_permission_check

3543a8a

		@@ -970,9 +985,11 @@ def test_firmware_download(self):
		url = reverse('upgrader:api_firmware_download', args=[image.build.pk, image.pk])
		with self.subTest("Test as operator"):

		@@ -45,10 +46,26 @@ class Meta:


		class FirmwareImageSerializer(BaseSerializer):
		file = serializers.FileField(required=False)

		elif hasattr(instance, 'file'):
		ret['file'] = instance.file.url

	operators_read_only_admins_manage = [
	'build',
	'devicefirmware',
	'firmwareimage',
	'batchupgradeoperation',
	'upgradeoperation',
	]

Uh oh!

[change] Improve endpoints to download firmware images #306

Are you sure you want to change the base?

[change] Improve endpoints to download firmware images #306

Uh oh!

Conversation

youhaveme9 commented Apr 25, 2025

Checklist

Reference to Existing Issue

Uh oh!

pandafy 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!

pandafy 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!