[fix] Prevent command creation for devices without a DeviceConnection #1016 by DragnEmperor · Pull Request #1021 · openwisp/openwisp-controller

DragnEmperor · 2025-05-05T18:05:45Z

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.

Reference to Existing Issue

Closes #1016 .

Description of Changes

Added a check before creation of command via REST API to ensure device has credentials via 'DeviceConnection'. API throws 400 error if not. To ensure only relevant connection and command types are displayed modified their queryset in serializer's 'init'.

coveralls · 2025-05-06T01:49:24Z

coverage: 98.827% (+0.003%) from 98.824%
when pulling 2860931 on DragnEmperor:issues/1016-command-connection-check
into 957c4a3 on openwisp:master.

codesankalp · 2025-05-07T04:38:26Z

+        if not DeviceConnection.objects.filter(
+            device_id=self.kwargs['device_pk'],
+            credentials__isnull=False,
+        ).exists():
+            raise NotFound(detail='Device has no credentials assigned.')


Can you raise the validation error from the serializer itself?

Sure! does the following format works?
{ "device": [ "Device has no credentials assigned." ] }

codesankalp · 2025-05-07T04:39:32Z

-                '"custom" command is not available for this organization',
-                response.data['input'][0],
+                '"custom" is not a valid choice.',
+                json.loads(response.content)['type'][0],


can you try with response.json() here?

devkapilbansal

Looks good till now, please address below requests from my side

nemesifier

Why don't we do this at model level?
Does it make sense to create a command object for a device which doesn't have any connection info? What do you think @pandafy?

pandafy · 2025-05-09T09:24:48Z

Why don't we do this at model level? Does it make sense to create a command object for a device which doesn't have any connection info? What do you think @pandafy?

Yes, we should add validation at the model level.

pandafy

Looks good, just need some minor improvements.

pandafy · 2025-05-12T10:30:02Z

+            self.fields['connection'].queryset = DeviceConnection.objects.filter(
+                device=device
+            )


Suggested change

self.fields['connection'].queryset = DeviceConnection.objects.filter(

device=device

)

self.fields['connection'].queryset = self.fields['connection'].queryset.filter(

device_id=device.id

)

pandafy · 2025-05-12T10:32:03Z

+        if not DeviceConnection.objects.filter(
+            device=self.device,
+            credentials__isnull=False,
+        ).exists():


Will this be more readable?

Suggested change

if not DeviceConnection.objects.filter(

device=self.device,

credentials__isnull=False,

).exists():

if self.device and self.device.deviceconnection_set.exists():

Let's check if this generates any additional queries.

I'd also prefer this.

pandafy · 2025-05-12T10:45:06Z

+            self.fields['connection'].queryset = DeviceConnection.objects.filter(
+                device=device
+            )


This is altering the labels of the type field in the browsable API.

The option label should be Change password instead of change_password.

pandafy · 2025-05-12T11:02:34Z

+    # for ensuring that only related connections are shown
+    def test_available_connections(self):
+        url = self._get_path('device_command_list', self.device_id)
+        response = self.client.get(url)
+        serializer = CommandSerializer(
+            instance=self.device_conn.device,
+            context={'device_id': self.device_id, 'request': response.wsgi_request},
+        )
+        device = self._create_device(
+            name='default.test.device2', mac_address='12:23:34:45:56:67'
+        )
+        self._create_config(device=device)
+        credentials_2 = self._create_credentials(name='Test Credentials 2')
+        device_conn2 = self._create_device_connection(
+            device=device, credentials=credentials_2
+        )
+        connections = serializer.fields['connection']
+        queryset = list(connections.get_queryset().values_list('id', flat=True))
+        queryset = [str(i) for i in queryset]
+        self.assertIn(str(self.device_conn.id), queryset)
+        self.assertNotIn(device_conn2.id, queryset)
+


We generally test the presence/absence of desired elements in browsable API like this

openwisp-controller/openwisp_controller/config/tests/test_api.py

Lines 767 to 775 in 523f4da

def test_template_list_for_shared_objects(self):

org1 = self._get_org()

self._create_vpn(name='shared-vpn', organization=None)

test_user = self._create_operator(organizations=[org1])

self.client.force_login(test_user)

path = reverse('config_api:template_list')

r = self.client.get(path, {'format': 'api'})

self.assertEqual(r.status_code, 200)

self.assertContains(r, 'shared-vpn</option>')

Thanks! Was looking for something like this

nemesifier · 2025-05-12T19:06:18Z

+        if (
+            device := Device.objects.filter(pk=self.context.get('device_id'))
+            .only('organization_id', 'id')
+            .first()
+        ):
+            self.fields['connection'].queryset = DeviceConnection.objects.filter(
+                device=device
+            )


What about something like:

Suggested change

if (

device := Device.objects.filter(pk=self.context.get('device_id'))

.only('organization_id', 'id')

.first()

):

self.fields['connection'].queryset = DeviceConnection.objects.filter(

device=device

)

device_id = self.context.get('device_id')

if device_id:

self.fields['connection'].queryset = DeviceConnection.objects.filter(

device_id=self.context.get('device_id')

)```

Yes would reduce query in case we are using it somewhere where context not provided by view

nemesifier · 2025-05-12T19:07:08Z

+        if not DeviceConnection.objects.filter(
+            device=self.device,
+            credentials__isnull=False,
+        ).exists():


I'd also prefer this.

pandafy

I have performed manual testing and it works as expected.

pandafy · 2025-05-14T12:31:38Z

+            device = self._create_device(
+                name='default.test.device2', mac_address='11:22:33:44:55:66'
+            )
+            new_command = Command(
+                device=device, type='custom', input={'command': 'echo test'}
+            )


Instead of creating new objects, can you we please do this?

device.deviceconnection_set.delete() command.type = 'custom' command.input = {'command': 'echo test'} with self.assertRaises(ValidationError) as context_manager: command.full_clean()

This is more of a personal preference. And. this keeps the entire test consistent.

With this we need to make sure that this subTest is executed last, if any new subTests are added here in future, as then those subtests will raise error. Just a heads up

It should be okay, I guess.

pandafy · 2025-05-14T12:33:10Z

-                '"custom" command is not available for this organization',
-                response.data['input'][0],
+                '"custom" is not a valid choice.',
+                response.json()['type'][0],


Why are you changing response.data to response.json()?

pandafy · 2025-05-14T12:34:21Z

+            # filter command types based on the device's organization
+            allowed_commands = Command.get_org_choices(device.organization_id)
+            # this translates the commands to their labels
+            commands_map = dict(COMMAND_CHOICES)
+            self.fields['type'].choices = [
+                (i, commands_map[i]) for i in commands_map if i in allowed_commands
+            ]


Can we move this to a class method in Command model?

Yes. Makes sense

Should i combine this with get_org_choices as they seem to to do the same thing?

I think, that is an acceptable solution. I am afraid, that will be backward incompatible though.

We would also need to update this code

openwisp-controller/openwisp_controller/connection/base/models.py

Lines 473 to 475 in 523f4da

if self.type not in self.get_org_choices(

organization_id=self.device.organization_id

):

cc @nemesifier

Yes, i think this is the only place where it is used, so it should be easier now to do this change. Should I proceed?

Yes, go ahead!

pandafy

Great work @DragnEmperor! 👏🏼

LGTM!

codesankalp · 2025-05-20T15:36:56Z

            str(organization_id), ORGANIZATION_ENABLED_COMMANDS.get('__all__')
        )
+        commands_map = dict(COMMAND_CHOICES)
+        return [(i, commands_map[i]) for i in commands_map if i in allowed_commands]


nitpick: we can iterate over commands list and search it in dict.

Suggested change

return [(i, commands_map[i]) for i in commands_map if i in allowed_commands]

return [(cmd, commands_map[cmd]) for cmd in allowed_commands if cmd in commands_map]

…penwisp#1016 Added a check before creation to ensure device has credentials via 'DeviceConnection'. To ensure only relevant connection and command types are displayed modified their queryset in serializer's 'init'. Closes openwisp#1016 Signed-off-by: DragnEmperor <dragnemperor@gmail.com>

As we are now restricting the choices at serializer level, the checks in api test cases required a small change to handle this. Added a new test case for creating command for a device without device connection. Signed-off-by: DragnEmperor <dragnemperor@gmail.com>

Instead of raising the credentials not assigned error from views, its better to raise it from serializer as part of validations. Also updated the tests to handle this change. Signed-off-by: DragnEmperor <dragnemperor@gmail.com>

Added a test to ensure only the connections of device are available on browser api form. Migrated the validation error from serializer to model level. Signed-off-by: DragnEmperor <dragnemperor@gmail.com>

Modified checks in model for better readability. Choices labels are now being shown in browseable API form using `COMMAND_CHOICES`. `test_available_commands` is modified to check using browseable api form instead of serializer. Also, added additional test case for Connection model. Signed-off-by: DragnEmperor <dragnemperor@gmail.com>

Modified `get_org_choices` method to fetch choices as list of tuples to support labels wherever possible. Refactored test cases Signed-off-by: DragnEmperor <dragnemperor@gmail.com>

Instead of looping through the `commands_map`, its better to loop the allowed commands to reduce overhead. Corrected tuple format of `ORGANIZATION_ENABLED_COMMANDS` for `test_execute_user_registered_command_without_input`. Signed-off-by: DragnEmperor <dragnemperor@gmail.com>

nemesifier · 2025-05-23T18:55:45Z

Thanks! @DragnEmperor @pandafy @codesankalp 👏

DragnEmperor force-pushed the issues/1016-command-connection-check branch from a73d01f to 17d17e6 Compare May 6, 2025 01:32

DragnEmperor force-pushed the issues/1016-command-connection-check branch from 17d17e6 to 6cab435 Compare May 6, 2025 02:25

codesankalp requested changes May 7, 2025

View reviewed changes

DragnEmperor force-pushed the issues/1016-command-connection-check branch from 6cab435 to d6ec29a Compare May 7, 2025 12:40

devkapilbansal requested changes May 8, 2025

View reviewed changes

Comment thread openwisp_controller/connection/api/serializers.py

nemesifier reviewed May 8, 2025

View reviewed changes

pandafy requested changes May 12, 2025

View reviewed changes

pandafy added this to OpenWISP Contributor's Board May 12, 2025

github-project-automation Bot moved this to To do (general) in OpenWISP Contributor's Board May 12, 2025

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

nemesifier reviewed May 12, 2025

View reviewed changes

DragnEmperor force-pushed the issues/1016-command-connection-check branch from ce2b540 to 66d3cf9 Compare May 13, 2025 09:19

pandafy self-requested a review May 13, 2025 17:01

pandafy requested changes May 14, 2025

View reviewed changes

DragnEmperor force-pushed the issues/1016-command-connection-check branch 2 times, most recently from 3997b12 to 2106d23 Compare May 15, 2025 19:03

pandafy previously approved these changes May 20, 2025

View reviewed changes

pandafy moved this from In progress to Needs review in OpenWISP Contributor's Board May 20, 2025

codesankalp reviewed May 20, 2025

View reviewed changes

DragnEmperor dismissed pandafy’s stale review via b0e5398 May 22, 2025 20:54

DragnEmperor added 6 commits May 23, 2025 20:24

[refactor] Migrate error raising to serializer

fac87bb

Instead of raising the credentials not assigned error from views, its better to raise it from serializer as part of validations. Also updated the tests to handle this change. Signed-off-by: DragnEmperor <dragnemperor@gmail.com>

[tests] Added test ensuring device's related connections

48124b7

Added a test to ensure only the connections of device are available on browser api form. Migrated the validation error from serializer to model level. Signed-off-by: DragnEmperor <dragnemperor@gmail.com>

[chores] Added allowed commands to model method

7d42f3d

Modified `get_org_choices` method to fetch choices as list of tuples to support labels wherever possible. Refactored test cases Signed-off-by: DragnEmperor <dragnemperor@gmail.com>

DragnEmperor force-pushed the issues/1016-command-connection-check branch from b0e5398 to 7d42f3d Compare May 23, 2025 14:54

pandafy approved these changes May 23, 2025

View reviewed changes

codesankalp approved these changes May 23, 2025

View reviewed changes

nemesifier assigned DragnEmperor May 23, 2025

nemesifier approved these changes May 23, 2025

View reviewed changes

nemesifier merged commit 4836913 into openwisp:master May 23, 2025
15 checks passed

github-project-automation Bot moved this from Needs review to Done in OpenWISP Contributor's Board May 23, 2025

nemesifier mentioned this pull request May 23, 2025

[fix:1.1] Prevent command creation for devices without a DeviceConnection #1055

Merged

	def test_template_list_for_shared_objects(self):
	org1 = self._get_org()
	self._create_vpn(name='shared-vpn', organization=None)
	test_user = self._create_operator(organizations=[org1])
	self.client.force_login(test_user)
	path = reverse('config_api:template_list')
	r = self.client.get(path, {'format': 'api'})
	self.assertEqual(r.status_code, 200)
	self.assertContains(r, 'shared-vpn</option>')

	if self.type not in self.get_org_choices(
	organization_id=self.device.organization_id
	):

	return [(i, commands_map[i]) for i in commands_map if i in allowed_commands]
	return [(cmd, commands_map[cmd]) for cmd in allowed_commands if cmd in commands_map]

Uh oh!

Conversation

DragnEmperor commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Reference to Existing Issue

Description of Changes

Uh oh!

coveralls commented May 6, 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!

Choose a reason for hiding this comment

Uh oh!

devkapilbansal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

pandafy commented May 9, 2025

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!

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!

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!

nemesifier commented May 23, 2025

Uh oh!

Uh oh!

Reviewers

DragnEmperor commented May 5, 2025 •

edited

Loading

coveralls commented May 6, 2025 •

edited

Loading