[fix] Fixed enforce_required_templates bug in change of device org #445

Closes #445

Author: pandafy

openwisp_controller/config/admin.py

@@ -347,6 +347,14 @@ def clean_templates(self):
                 reverse=False,
                 model=config.templates.model,
                 pk_set=templates,
+                # The template validation retrieves the device object
+                # from the database. Even if the organization of the device
+                # is changed by the user, the validation uses the old
+                # organization of the device because the device is not
+                # saved yet. The raw POST data is passed here so that
+                # validation can be performed using up to date data of
+                # the device object.
+                raw_data=self.data,
             )
         return templates
 

openwisp_controller/config/api/serializers.py

@@ -201,22 +201,23 @@ class Meta(BaseMeta):
         ]
 
     def update(self, instance, validated_data):
-        config_data = None
-
+        config_data = validated_data.pop('config', {})
+        config_templates = [
+            template.pk for template in config_data.get('templates', [])
+        ]
+        raw_data_for_signal_handlers = {
+            'organization': validated_data.get('organization', instance.organization)
+        }
         if self.initial_data.get('config.backend') and instance._has_config() is False:
-            config_data = dict(validated_data.pop('config'))
-            config_templates = [
-                template.pk for template in config_data.pop('templates')
-            ]
+            config_data = dict(config_data)
             with transaction.atomic():
-                config = Config.objects.create(device=instance, **config_data)
+                config = Config(device=instance, **config_data)
                 config.templates.add(*config_templates)
                 config.full_clean()
                 config.save()
             return super().update(instance, validated_data)
 
-        if validated_data.get('config'):
-            config_data = validated_data.pop('config')
+        if config_data:
             instance.config.backend = config_data.get(
                 'backend', instance.config.backend
             )
@@ -227,9 +228,7 @@ def update(self, instance, validated_data):
 
             if 'templates' in config_data:
                 if config_data.get('templates'):
-                    new_config_templates = [
-                        template.pk for template in config_data.get('templates')
-                    ]
+                    new_config_templates = config_templates
                     old_config_templates = [
                         template
                         for template in instance.config.templates.values_list(
@@ -256,8 +255,23 @@ def update(self, instance, validated_data):
                     instance.config.templates.clear()
                     instance.config.templates.add(*[])
 
-            instance.config.full_clean()
-            instance.config.save()
+        elif hasattr(instance, 'config') and validated_data.get('organization'):
+            if instance.organization != validated_data.get('organization'):
+                # config.device.organization is used for validating
+                # the organization of templates. It is also used for adding
+                # default and required templates configured for an organization.
+                # The value of the organization field is set here to
+                # prevent access of the old value stored in the database
+                # while performing above operations.
+                instance.config.device.organization = validated_data.get('organization')
+                instance.config.templates.clear()
+                Config.enforce_required_templates(
+                    action='post_clear',
+                    instance=instance.config,
+                    sender=instance.config.templates,
+                    pk_set=None,
+                    raw_data=raw_data_for_signal_handlers,
+                )
         return super().update(instance, validated_data)
 
 

openwisp_controller/config/base/config.py

@@ -163,15 +163,21 @@ def _get_templates_from_pk_set(cls, pk_set):
         return templates
 
     @classmethod
-    def clean_templates(cls, action, instance, pk_set, **kwargs):
+    def clean_templates(cls, action, instance, pk_set, raw_data=None, **kwargs):
         """
         validates resulting configuration of config + templates
         raises a ValidationError if invalid
         must be called from forms or APIs
         this method is called from a django signal (m2m_changed)
         see config.apps.ConfigConfig.connect_signals
+
+        raw_data contains the non-validated data that is submitted to
+        a form or API.
         """
-        templates = cls.clean_templates_org(action, instance, pk_set, **kwargs)
+        raw_data = raw_data or {}
+        templates = cls.clean_templates_org(
+            action, instance, pk_set, raw_data=raw_data, **kwargs
+        )
         if not templates:
             return
         backend = instance.get_backend_instance(template_instances=templates)
@@ -254,19 +260,25 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
                     client.delete()
 
     @classmethod
-    def clean_templates_org(cls, action, instance, pk_set, **kwargs):
+    def clean_templates_org(cls, action, instance, pk_set, raw_data=None, **kwargs):
+        """
+        raw_data contains the non-validated data that is submitted to
+        a form or API.
+        """
         if action != 'pre_add':
             return False
+        raw_data = raw_data or {}
         templates = cls._get_templates_from_pk_set(pk_set)
         # when using the admin, templates will be a list
         # we need to get the queryset from this list in order to proceed
         if not isinstance(templates, models.QuerySet):
             template_model = cls.templates.rel.model
             pk_list = [template.pk for template in templates]
             templates = template_model.objects.filter(pk__in=pk_list)
-        # lookg for invalid templates
+        # looking for invalid templates
+        organization = raw_data.get('organization', instance.device.organization)
         invalids = (
-            templates.exclude(organization=instance.device.organization)
+            templates.exclude(organization=organization)
             .exclude(organization=None)
             .values('name')
         )
@@ -287,17 +299,23 @@ def clean_templates_org(cls, action, instance, pk_set, **kwargs):
         return templates
 
     @classmethod
-    def enforce_required_templates(cls, action, instance, pk_set, **kwargs):
+    def enforce_required_templates(
+        cls, action, instance, pk_set, raw_data=None, **kwargs
+    ):
         """
         This method is called from a django signal (m2m_changed),
         see config.apps.ConfigConfig.connect_signals.
         It raises a PermissionDenied if a required template
         is unassigned from a config.
         It adds back required templates on post_clear events
         (post-clear is used by sortedm2m to assign templates).
+
+        raw_data contains the non-validated data that is submitted to
+        a form or API.
         """
         if action not in ['pre_remove', 'post_clear']:
             return False
+        raw_data = raw_data or {}
         template_query = models.Q(required=True, backend=instance.backend)
         # trying to remove a required template will raise PermissionDenied
         if action == 'pre_remove':
@@ -309,12 +327,12 @@ def enforce_required_templates(cls, action, instance, pk_set, **kwargs):
         if action == 'post_clear':
             # retrieve required templates related to this
             # device and ensure they're always present
+            organization = raw_data.get('organization', instance.device.organization)
             required_templates = (
                 cls.get_template_model()
                 .objects.filter(template_query)
                 .filter(
-                    models.Q(organization=instance.device.organization)
-                    | models.Q(organization=None)
+                    models.Q(organization=organization) | models.Q(organization=None)
                 )
             )
             if required_templates.exists():

openwisp_controller/config/tests/test_admin.py

@@ -551,6 +551,29 @@ def test_change_device_required_template(self):
             self.assertEqual(c.name, 'test-device-templates-cleared')
             self.assertTrue(c.templates.filter(pk=t.pk).exists())
 
+    def test_change_device_org_required_templates(self):
+        org1 = self._create_org(name='org1')
+        org2 = self._create_org(name='org2')
+        template = self._create_template(organization=org1, config={'interfaces': []})
+        device = self._create_device(organization=org1)
+        config = self._create_config(device=device)
+        path = reverse(f'admin:{self.app_label}_device_change', args=[device.pk])
+        params = self._get_device_params(org=org1)
+        params.update(
+            {
+                'name': 'test-device-changed',
+                'config-0-id': str(config.pk),
+                'config-0-device': str(device.pk),
+                'config-0-templates': str(template.pk),
+                'config-INITIAL_FORMS': 1,
+                'organization': str(org2.pk),
+            }
+        )
+        response = self.client.post(path, params, follow=True)
+        self.assertEqual(response.status_code, 200)
+        config.refresh_from_db()
+        self.assertEqual(config.templates.count(), 0)
+
     def test_download_device_config(self):
         d = self._create_device(name='download')
         self._create_config(device=d)

openwisp_controller/config/tests/test_api.py

@@ -265,6 +265,30 @@ def test_device_patch_with_templates_of_different_org(self):
                         '''
         self.assertTrue(' '.join(validation_msg.split()) in error.exception.message)
 
+    def test_device_change_organization_required_templates(self):
+        org1 = self._create_org(name='org1')
+        org2 = self._create_org(name='org2')
+        org1_template = self._create_template(
+            name='org1-template', organization=org1, required=True
+        )
+        org2_template = self._create_template(
+            name='org2-template', organization=org2, required=True
+        )
+        device = self._create_device(organization=org1)
+        config = self._create_config(device=device)
+        self.assertEqual(config.templates.count(), 1)
+        self.assertEqual(config.templates.first(), org1_template)
+
+        path = reverse('config_api:device_detail', args=[device.pk])
+        data = {'organization': org2.pk}
+        response = self.client.patch(path, data=data, content_type='application/json')
+        self.assertEqual(response.status_code, 200)
+        device.refresh_from_db()
+        config.refresh_from_db()
+        self.assertEqual(device.organization, org2)
+        self.assertEqual(config.templates.count(), 1)
+        self.assertEqual(config.templates.first(), org2_template)
+
     def test_device_patch_api(self):
         d1 = self._create_device(name='test-device')
         path = reverse('config_api:device_detail', args=[d1.pk])