From e53b82f985e4b7bd8ec9696cbe78563b0cecad6b Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Tue, 27 Jun 2023 21:11:32 +0530 Subject: [PATCH 01/10] [feature] Added configuration variables to DeviceGroup #738 Closes #738 --- openwisp_controller/config/admin.py | 23 ++---- openwisp_controller/config/api/serializers.py | 2 + openwisp_controller/config/base/config.py | 6 +- openwisp_controller/config/base/device.py | 8 ++ .../config/base/device_group.py | 20 +++++ .../config/migrations/0036_device_group.py | 6 ++ .../migrations/0048_devicegroup_context.py | 31 ++++++++ .../config/static/config/js/widget.js | 16 +++- .../config/tests/test_admin.py | 39 ++++++++-- openwisp_controller/config/tests/test_api.py | 7 +- .../config/tests/test_device.py | 7 +- .../config/tests/test_views.py | 22 ++++-- openwisp_controller/config/tests/utils.py | 2 +- openwisp_controller/config/views.py | 78 ++++++++++++++----- .../sample_config/migrations/0001_initial.py | 20 +++++ 15 files changed, 228 insertions(+), 59 deletions(-) create mode 100644 openwisp_controller/config/migrations/0048_devicegroup_context.py diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 791e1c843..8803aac3d 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -26,10 +26,7 @@ from openwisp_ipam.filters import SubnetFilter from swapper import load_model -from openwisp_controller.config.views import ( - get_relevant_templates, - get_template_default_values, -) +from openwisp_controller.config.views import get_default_values, get_relevant_templates from openwisp_users.admin import OrganizationAdmin from openwisp_users.multitenancy import MultitenantOrgFilter from openwisp_utils.admin import ( @@ -677,9 +674,9 @@ def get_urls(self): name='get_relevant_templates', ), path( - 'get-template-default-values/', - self.admin_site.admin_view(get_template_default_values), - name='get_template_default_values', + 'get-default-values/', + self.admin_site.admin_view(get_default_values), + name='get_default_values', ), ] + super().get_urls() for inline in self.inlines + self.conditional_inlines: @@ -1002,14 +999,7 @@ def save(self, *args, **kwargs): class Meta(BaseForm.Meta): model = DeviceGroup - widgets = {'meta_data': DeviceGroupJsonSchemaWidget} - labels = {'meta_data': _('Metadata')} - help_texts = { - 'meta_data': _( - 'Group meta data, use this field to store data which is related' - 'to this group and can be retrieved via the REST API.' - ) - } + widgets = {'meta_data': DeviceGroupJsonSchemaWidget, 'context': FlatJsonWidget} class DeviceGroupAdmin(MultitenantAdminMixin, BaseAdmin): @@ -1026,11 +1016,12 @@ class DeviceGroupAdmin(MultitenantAdminMixin, BaseAdmin): 'organization', 'description', 'templates', + 'context', 'meta_data', 'created', 'modified', ] - search_fields = ['name', 'description', 'meta_data'] + search_fields = ['name', 'description', 'meta_data', 'context'] list_filter = [MultitenantOrgFilter, DeviceGroupFilter] multitenant_shared_relations = ('templates',) diff --git a/openwisp_controller/config/api/serializers.py b/openwisp_controller/config/api/serializers.py index 9822f9289..701dfb943 100644 --- a/openwisp_controller/config/api/serializers.py +++ b/openwisp_controller/config/api/serializers.py @@ -304,6 +304,7 @@ def get_queryset(self): class DeviceGroupSerializer(BaseSerializer): + context = serializers.JSONField(required=False, initial={}) meta_data = serializers.JSONField(required=False, initial={}) templates = FilterGroupTemplates(many=True) _templates = None @@ -316,6 +317,7 @@ class Meta(BaseMeta): 'organization', 'description', 'templates', + 'context', 'meta_data', 'created', 'modified', diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index 23ad7b3a5..ce2b0f773 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -620,7 +620,11 @@ def get_context(self, system=False): return c def get_system_context(self): - return self.get_context(system=True) + context = {} + context.update(self.get_context(system=True)) + if hasattr(self, 'device') and self.device._get_group(): + context.update(self.device._get_group().get_context()) + return context def manage_group_templates( self, templates, old_templates, ignore_backend_filter=False diff --git a/openwisp_controller/config/base/device.py b/openwisp_controller/config/base/device.py index 054b0ac53..671b4970e 100644 --- a/openwisp_controller/config/base/device.py +++ b/openwisp_controller/config/base/device.py @@ -127,6 +127,9 @@ def __str__(self): def _has_config(self): return hasattr(self, 'config') + def _has_group(self): + return hasattr(self, 'config') + def _get_config_attr(self, attr): """ gets property or calls method of related config object @@ -143,6 +146,11 @@ def _get_config(self): else: return self.get_config_model()(device=self) + def _get_group(self): + if self._has_group(): + return self.group + return self.get_group_model()(device=self) + def get_context(self): config = self._get_config() return config.get_context() diff --git a/openwisp_controller/config/base/device_group.py b/openwisp_controller/config/base/device_group.py index 5378a8265..95e8d3042 100644 --- a/openwisp_controller/config/base/device_group.py +++ b/openwisp_controller/config/base/device_group.py @@ -1,4 +1,5 @@ import collections +from copy import deepcopy import jsonschema from django.core.exceptions import ValidationError @@ -39,6 +40,22 @@ class AbstractDeviceGroup(OrgMixin, TimeStampedEditableModel): default=dict, load_kwargs={'object_pairs_hook': collections.OrderedDict}, dump_kwargs={'indent': 4}, + help_text=_( + 'Group meta data, use this field to store data which is related' + ' to this group and can be retrieved via the REST API.' + ), + verbose_name=_('Metadata'), + ) + context = JSONField( + blank=True, + default=dict, + load_kwargs={'object_pairs_hook': collections.OrderedDict}, + dump_kwargs={'indent': 4}, + help_text=_( + 'This field can be used to add meta data for the group' + ' or to add "Configuration Variables" to the devices.' + ), + verbose_name=_('Configuration Variables'), ) def __str__(self): @@ -58,6 +75,9 @@ def clean(self): except SchemaError as e: raise ValidationError({'input': e.message}) + def get_context(self): + return deepcopy(self.context) + @classmethod def templates_changed(cls, instance, old_templates, templates, *args, **kwargs): group_templates_changed.send( diff --git a/openwisp_controller/config/migrations/0036_device_group.py b/openwisp_controller/config/migrations/0036_device_group.py index 10c12ea2c..1bb00482d 100644 --- a/openwisp_controller/config/migrations/0036_device_group.py +++ b/openwisp_controller/config/migrations/0036_device_group.py @@ -62,6 +62,12 @@ class Migration(migrations.Migration): default=dict, dump_kwargs={'ensure_ascii': False, 'indent': 4}, load_kwargs={'object_pairs_hook': collections.OrderedDict}, + help_text=( + 'Group meta data, use this field to store data which is' + ' related to this group and can be retrieved via the' + ' REST API.' + ), + verbose_name='Metadata', ), ), ( diff --git a/openwisp_controller/config/migrations/0048_devicegroup_context.py b/openwisp_controller/config/migrations/0048_devicegroup_context.py new file mode 100644 index 000000000..6dcb6af03 --- /dev/null +++ b/openwisp_controller/config/migrations/0048_devicegroup_context.py @@ -0,0 +1,31 @@ +# Generated by Django 3.2.19 on 2023-06-27 14:45 + +import collections + +import jsonfield.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('config', '0047_add_organizationlimits'), + ] + + operations = [ + migrations.AddField( + model_name='devicegroup', + name='context', + field=jsonfield.fields.JSONField( + blank=True, + default=dict, + dump_kwargs={'ensure_ascii': False, 'indent': 4}, + help_text=( + 'This field can be used to add meta data for the group' + ' or to add "Configuration Variables" to the devices.' + ), + load_kwargs={'object_pairs_hook': collections.OrderedDict}, + verbose_name='Configuration Variables', + ), + ), + ] diff --git a/openwisp_controller/config/static/config/js/widget.js b/openwisp_controller/config/static/config/js/widget.js index e8d647021..a57db88fb 100644 --- a/openwisp_controller/config/static/config/js/widget.js +++ b/openwisp_controller/config/static/config/js/widget.js @@ -4,7 +4,7 @@ django._jsonEditors = new Map(); var inFullScreenMode = false, prevDefaultValues = {}, - defaultValuesUrl = window.location.origin + '/admin/config/device/get-template-default-values/', + defaultValuesUrl = window.location.origin + '/admin/config/device/get-default-values/', removeDefaultValues = function(contextValue, defaultValues) { // remove default values when template is removed. Object.keys(prevDefaultValues).forEach(function (key) { @@ -55,9 +55,14 @@ } }, getDefaultValues = function (isLoading=false) { - var pks = $('input[name="config-0-templates"]').attr('value'); - if (pks) { - $.get(defaultValuesUrl, {pks: pks}) + var templatePks = $('input[name="config-0-templates"]').attr('value'), + groupPk = $('#id_group').val(); + if (templatePks) { + var payload = {pks: templatePks}; + if (groupPk) { + payload.group = groupPk; + } + $.get(defaultValuesUrl, payload) .done( function (data) { updateContext(isLoading, data.default_values); }) @@ -430,6 +435,9 @@ $('.sortedm2m-items').on('change', function() { getDefaultValues(); }); + $('#id_group').on('change', function() { + getDefaultValues(); + }); }); // deletes maxLength on ip address schema if address contains variable diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index be3e0aaaf..cd2552154 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -2,6 +2,7 @@ import json import os from unittest.mock import patch +from uuid import uuid4 from django.contrib.admin.models import LogEntry from django.contrib.auth import get_user_model @@ -1426,9 +1427,12 @@ def test_clone_template(self): self.assertIn('test-template (Clone)', str(response.content)) self.assertEqual(LogEntry.objects.all().count(), 3) - def test_get_template_default_values(self): + def test_get_default_values(self): t1 = self._create_template(name='t1', default_values={'name1': 'test1'}) - path = reverse('admin:get_template_default_values') + group = self._create_device_group( + context={'name1': 'device group', 'name2': 'should not appear'} + ) + path = reverse('admin:get_default_values') with self.subTest('get default values for one template'): with self.assertNumQueries(3): @@ -1445,19 +1449,38 @@ def test_get_template_default_values(self): expected = {'default_values': {'name1': 'test1', 'name2': 'test2'}} self.assertEqual(r.json(), expected) - def test_get_template_default_values_invalid_pks(self): - path = reverse('admin:get_template_default_values') - expected = {'error': 'invalid template pks were received'} + with self.subTest('get default values conflicting with device group'): + with self.assertNumQueries(4): + response = self.client.get( + path, {'pks': f'{t1.pk}', 'group': str(group.pk)} + ) + self.assertEqual(response.status_code, 200) + expected = {'default_values': {'name1': 'device group'}} + response_data = response.json() + self.assertEqual(response_data, expected) + self.assertNotIn('name2', response_data) + + def test_get_default_values_invalid_pks(self): + path = reverse('admin:get_default_values') + expected = { + 'template': {'error': 'invalid template pks were received'}, + 'group': {'error': 'invalid group pk were received'}, + } - with self.subTest('test with invalid pk'): + with self.subTest('test with invalid template pk'): r = self.client.get(path, {'pks': 'invalid'}) self.assertEqual(r.status_code, 400) - self.assertEqual(r.json(), expected) + self.assertEqual(r.json(), expected['template']) with self.subTest('test with absent pk'): r = self.client.get(path) self.assertEqual(r.status_code, 400) - self.assertEqual(r.json(), expected) + self.assertEqual(r.json(), expected['template']) + + with self.subTest('test with invalid group pk'): + r = self.client.get(path, {'group': 'invalid', 'pks': str(uuid4())}) + self.assertEqual(r.status_code, 400) + self.assertEqual(r.json(), expected['group']) def _test_system_context_field_helper(self, path): r = self.client.get(path) diff --git a/openwisp_controller/config/tests/test_api.py b/openwisp_controller/config/tests/test_api.py index 139a0b462..a246775ec 100644 --- a/openwisp_controller/config/tests/test_api.py +++ b/openwisp_controller/config/tests/test_api.py @@ -81,6 +81,7 @@ class ApiTestMixin: 'description': 'Group for APs of default organization', 'organization': 'None', 'meta_data': {'captive_portal_url': 'https://example.com'}, + 'context': {'SSID': 'OpenWISP'}, 'templates': [], } @@ -900,6 +901,7 @@ def test_devicegroup_create_api(self): self.assertEqual(response.data['name'], data['name']) self.assertEqual(response.data['description'], data['description']) self.assertEqual(response.data['meta_data'], data['meta_data']) + self.assertEqual(response.data['context'], data['context']) self.assertEqual(response.data['organization'], org.pk) self.assertEqual(response.data['templates'], [template.pk]) @@ -936,7 +938,7 @@ def _assert_devicegroup_list_filter(response=None, device_group=None): self.assertEqual(response.status_code, 200) data = response.data self.assertEqual(data['count'], 1) - self.assertEqual(len(data['results'][0]), 8) + self.assertEqual(len(data['results'][0]), 9) self.assertEqual(data['results'][0]['id'], str(device_group.pk)) self.assertEqual(data['results'][0]['name'], str(device_group.name)) self.assertEqual( @@ -972,6 +974,7 @@ def test_devicegroup_detail_api(self): self.assertEqual(response.data['name'], device_group.name) self.assertEqual(response.data['description'], device_group.description) self.assertDictEqual(response.data['meta_data'], device_group.meta_data) + self.assertDictEqual(response.data['context'], device_group.context) self.assertEqual( response.data['organization'], device_group.organization.pk ) @@ -1013,6 +1016,7 @@ def test_devicegroup_commonname(self): self.assertEqual(response.data['name'], device_group.name) self.assertEqual(response.data['description'], device_group.description) self.assertDictEqual(response.data['meta_data'], device_group.meta_data) + self.assertDictEqual(response.data['context'], device_group.context) self.assertEqual( response.data['organization'], device_group.organization.pk ) @@ -1026,6 +1030,7 @@ def test_devicegroup_commonname(self): self.assertEqual(response.data['name'], device_group.name) self.assertEqual(response.data['description'], device_group.description) self.assertDictEqual(response.data['meta_data'], device_group.meta_data) + self.assertDictEqual(response.data['context'], device_group.context) self.assertEqual( response.data['organization'], device_group.organization.pk ) diff --git a/openwisp_controller/config/tests/test_device.py b/openwisp_controller/config/tests/test_device.py index d07cd931a..d7d5498d3 100644 --- a/openwisp_controller/config/tests/test_device.py +++ b/openwisp_controller/config/tests/test_device.py @@ -310,10 +310,15 @@ def test_change_org(self): self.assertEqual(config.device.organization_id, org2.pk) def test_device_get_system_context(self): - d = self._create_device(organization=self._get_org()) + org = self._get_org() + device_group = self._create_device_group( + organization=org, context={'SSID': 'OpenWISP'} + ) + d = self._create_device(organization=org, group=device_group) self._create_config(context={'test': 'name'}, device=d) d.refresh_from_db() system_context = d.get_system_context() + self.assertEqual(system_context['SSID'], 'OpenWISP') self.assertNotIn('test', system_context.keys()) def test_management_ip_changed_not_emitted_on_creation(self): diff --git a/openwisp_controller/config/tests/test_views.py b/openwisp_controller/config/tests/test_views.py index 14b7d0915..e79271f39 100644 --- a/openwisp_controller/config/tests/test_views.py +++ b/openwisp_controller/config/tests/test_views.py @@ -6,14 +6,18 @@ from openwisp_users.tests.utils import TestOrganizationMixin from ...tests.utils import TestAdminMixin -from .utils import CreateConfigTemplateMixin +from .utils import CreateConfigTemplateMixin, CreateDeviceGroupMixin Template = load_model('config', 'Template') User = get_user_model() class TestViews( - CreateConfigTemplateMixin, TestAdminMixin, TestOrganizationMixin, TestCase + CreateConfigTemplateMixin, + CreateDeviceGroupMixin, + TestAdminMixin, + TestOrganizationMixin, + TestCase, ): """ tests for config.views @@ -241,7 +245,7 @@ def test_get_relevant_templates_400(self): ) self.assertEqual(response.status_code, 404) - def get_template_default_values_authorization(self): + def test_get_default_values_authorization(self): org1 = self._get_org() org1_template = self._create_template( organization=org1, default_values={'org1': 'secret1'} @@ -253,8 +257,11 @@ def get_template_default_values_authorization(self): shared_template = self._create_template( name='shared-template', default_values={'key': 'value'} ) + org2_device_group = self._create_device_group( + organization=org2, context={'key': 'org2 device group'} + ) url = ( - reverse('admin:get_template_default_values') + reverse('admin:get_default_values') + f'?pks={org1_template.pk},{org2_template.pk},{shared_template.pk}' ) @@ -281,8 +288,9 @@ def get_template_default_values_authorization(self): org1_user.is_staff = True org1_user.save() self.client.force_login(org1_user) + response = self.client.get(url + f'&group={org2_device_group.pk}') + # Do not override default value from device group of another org expected_response = {'default_values': {'org1': 'secret1', 'key': 'value'}} - response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertJSONEqual(response.content, expected_response) @@ -295,7 +303,7 @@ def get_template_default_values_authorization(self): self.assertEqual(response.status_code, 200) self.assertJSONEqual(response.content, expected_response) - def get_template_default_values_same_keys(self): + def test_get_default_values_same_keys(self): self._login() # Atleast 4 templates are required to create enough entropy in database # to make the test fail consistently without patch @@ -312,7 +320,7 @@ def get_template_default_values_same_keys(self): template5 = self._create_template( name='VNI 5', default_values={'vn1': '5', 'vn2': '50', 'vn3': '500'} ) - url = reverse('admin:get_template_default_values') + url = reverse('admin:get_default_values') templates = [template5, template4, template3, template2, template1] template_pks = ','.join([str(template.pk) for template in templates]) response = self.client.get(url, {'pks': template_pks}) diff --git a/openwisp_controller/config/tests/utils.py b/openwisp_controller/config/tests/utils.py index 581e049f0..d09b5cc33 100644 --- a/openwisp_controller/config/tests/utils.py +++ b/openwisp_controller/config/tests/utils.py @@ -233,7 +233,7 @@ def _create_device_group(self, **kwargs): options = { 'name': 'Routers', 'description': 'Group for all routers', - 'meta_data': {}, + 'context': {}, } options.update(kwargs) if 'organization' not in options: diff --git a/openwisp_controller/config/views.py b/openwisp_controller/config/views.py index e0cf25c11..a98cd8734 100644 --- a/openwisp_controller/config/views.py +++ b/openwisp_controller/config/views.py @@ -15,6 +15,7 @@ Organization = load_model('openwisp_users', 'Organization') Template = load_model('config', 'Template') +DeviceGroup = load_model('config', 'DeviceGroup') def get_relevant_templates(request, organization_id): @@ -92,36 +93,73 @@ def schema(request): return HttpResponse(c, status=status, content_type='application/json') -def get_template_default_values(request): +def get_default_values(request): """ - returns default_values for one or more templates + The view returns default values from the templates + and group specified in the URL's query parameters. + + URL query parameters: + pks (required): Comma separated primary keys of Template + group (optional): Primary key of the DeviceGroup + + The conflicting keys between the templates' default_values + and DeviceGroup's context are overridden by the value present + in DeviceGroup's context, i.e. DeviceGroup takes precedence. + + NOTE: Not all key-value pair of DeviceGroup.context are added + in the response. Only the conflicting keys are altered. """ + + def _clean_pk(pks): + pk_list = [] + for pk in pks: + UUID(pk, version=4) + pk_list.append(pk) + return pk_list + user = request.user - pk_list = [] - for pk in request.GET.get('pks', '').split(','): + try: + templates_pk_list = _clean_pk(request.GET.get('pks', '').split(',')) + except ValueError: + return JsonResponse({'error': 'invalid template pks were received'}, status=400) + group_pk = request.GET.get('group', None) + if group_pk: try: - UUID(pk, version=4) + group_pk = _clean_pk([group_pk])[0] except ValueError: - return JsonResponse( - {'error': 'invalid template pks were received'}, status=400 - ) + return JsonResponse({'error': 'invalid group pk were received'}, status=400) else: - pk_list.append(pk) - where = Q(pk__in=pk_list) + group_where = Q(pk=group_pk) + if not request.user.is_superuser: + group_where &= Q(organization__in=user.organizations_managed) + + templates_where = Q(pk__in=templates_pk_list) if not user.is_superuser: - where = where & ( + templates_where = templates_where & ( Q(organization=None) | Q(organization__in=user.organizations_managed) ) - qs = Template.objects.filter(where).values('id', 'default_values') - qs_dict = {} - # Create a mapping of UUID to default values of the templates in qs_dict. - # Iterate over received pk_list and retrieve default_values for corresponding - # template from qs_dict. + templates_qs = Template.objects.filter(templates_where).values( + 'id', 'default_values' + ) + templates_qs_dict = {} + # Create a mapping of UUID to default values of the templates in templates_ + # qs_dict. Iterate over received templates_pk_list and retrieve default_values for + # corresponding template from templates_qs_dict. # This ensures that default_values of templates that come later in the order # will override default_values of any previous template if same keys are present. - for template in qs: - qs_dict[str(template['id'])] = template['default_values'] + for template in templates_qs: + templates_qs_dict[str(template['id'])] = template['default_values'] default_values = {} - for pk in pk_list: - default_values.update(qs_dict[pk]) + for pk in templates_pk_list: + default_values.update(templates_qs_dict.get(pk, {})) + # Check for conflicting key's in DeviceGroup.context + if group_pk: + try: + group = DeviceGroup.objects.only('context').get(group_where) + except DeviceGroup.DoesNotExist: + pass + else: + for key, value in group.get_context().items(): + if key in default_values: + default_values[key] = value return JsonResponse({'default_values': default_values}) diff --git a/tests/openwisp2/sample_config/migrations/0001_initial.py b/tests/openwisp2/sample_config/migrations/0001_initial.py index 8bb0ea91d..77ca53f35 100644 --- a/tests/openwisp2/sample_config/migrations/0001_initial.py +++ b/tests/openwisp2/sample_config/migrations/0001_initial.py @@ -734,6 +734,26 @@ class Migration(migrations.Migration): default=dict, dump_kwargs={'ensure_ascii': False, 'indent': 4}, load_kwargs={'object_pairs_hook': collections.OrderedDict}, + help_text=( + 'Group meta data, use this field to store data which is' + ' related to this group and can be retrieved via the' + ' REST API.' + ), + verbose_name='Metadata', + ), + ), + ( + 'context', + jsonfield.fields.JSONField( + blank=True, + default=dict, + dump_kwargs={'ensure_ascii': False, 'indent': 4}, + help_text=( + 'This field can be used to add meta data for the group' + ' or to add "Configuration Variables" to the devices.' + ), + load_kwargs={'object_pairs_hook': collections.OrderedDict}, + verbose_name='Configuration Variables', ), ), ( From 5adc3ec5986a334b609f5b7694c8f22af4986252 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 28 Jun 2023 19:09:44 +0530 Subject: [PATCH 02/10] [req-changes] Use device group variables in configuration preview --- openwisp_controller/config/admin.py | 1 + openwisp_controller/config/base/config.py | 8 +++----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 8803aac3d..7482910fa 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -662,6 +662,7 @@ def _get_preview_instance(self, request): c.device.name = request.POST.get('name') c.device.mac_address = request.POST.get('mac_address') c.device.key = request.POST.get('key') + c.device.group_id = request.POST.get('group') or None if 'hardware_id' in request.POST: c.device.hardware_id = request.POST.get('hardware_id') return c diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index ce2b0f773..b1120965c 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -611,6 +611,8 @@ def get_context(self, system=False): ) if self.context and not system: extra.update(self.context) + if hasattr(self, 'device') and self.device._get_group(): + extra.update(self.device._get_group().get_context()) extra.update(self.get_vpn_context()) for func in self._config_context_functions: extra.update(func(config=self)) @@ -620,11 +622,7 @@ def get_context(self, system=False): return c def get_system_context(self): - context = {} - context.update(self.get_context(system=True)) - if hasattr(self, 'device') and self.device._get_group(): - context.update(self.device._get_group().get_context()) - return context + return self.get_context(system=True) def manage_group_templates( self, templates, old_templates, ignore_backend_filter=False From e9c17e5b0eed4c8d61a7dc78d313201efde51f68 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 28 Jun 2023 22:44:41 +0530 Subject: [PATCH 03/10] [fix] Fixed bug in frontend --- openwisp_controller/config/base/config.py | 14 +++++++------- .../config/static/config/js/widget.js | 11 ++++++++++- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index b1120965c..a535ad13e 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -600,6 +600,13 @@ def get_context(self, system=False): """ c = collections.OrderedDict() extra = {} + if hasattr(self, 'device') and self.device._get_group(): + extra.update(self.device._get_group().get_context()) + extra.update(self.get_vpn_context()) + for func in self._config_context_functions: + extra.update(func(config=self)) + if app_settings.HARDWARE_ID_ENABLED and self._has_device(): + extra.update({'hardware_id': str(self.device.hardware_id)}) if self._has_device(): c.update( [ @@ -611,13 +618,6 @@ def get_context(self, system=False): ) if self.context and not system: extra.update(self.context) - if hasattr(self, 'device') and self.device._get_group(): - extra.update(self.device._get_group().get_context()) - extra.update(self.get_vpn_context()) - for func in self._config_context_functions: - extra.update(func(config=self)) - if app_settings.HARDWARE_ID_ENABLED and self._has_device(): - extra.update({'hardware_id': str(self.device.hardware_id)}) c.update(sorted(extra.items())) return c diff --git a/openwisp_controller/config/static/config/js/widget.js b/openwisp_controller/config/static/config/js/widget.js index a57db88fb..42da6056e 100644 --- a/openwisp_controller/config/static/config/js/widget.js +++ b/openwisp_controller/config/static/config/js/widget.js @@ -32,7 +32,13 @@ systemContextValue = JSON.parse(systemContextField.text()); // add default values to contextValue Object.keys(defaultValues).forEach(function (key) { - if (!contextValue.hasOwnProperty(key) && !systemContextValue.hasOwnProperty(key)) { + if ( + // Handles the case when different templates and group contains the keys. + // If the contextValue was set by a template or group, then + // override the value. + (prevDefaultValues.hasOwnProperty(key) && prevDefaultValues[key] !== defaultValues[key]) + // Gives precedence to device's context (saved in database) and SystemContextValue + || (!contextValue.hasOwnProperty(key) && !systemContextValue.hasOwnProperty(key))) { contextValue[key] = defaultValues[key]; } }); @@ -435,6 +441,9 @@ $('.sortedm2m-items').on('change', function() { getDefaultValues(); }); + $('.sortedm2m-items').on('sortstop', function() { + getDefaultValues(); + }); $('#id_group').on('change', function() { getDefaultValues(); }); From 2c067e323c8be0ba652008f5eb2e0e76761811f9 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 28 Jun 2023 23:33:01 +0530 Subject: [PATCH 04/10] [docs] Updated README --- README.rst | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/README.rst b/README.rst index cc4dc070c..f692ba062 100644 --- a/README.rst +++ b/README.rst @@ -591,6 +591,9 @@ in read-only mode. .. image:: https://raw.githubusercontent.com/openwisp/openwisp-controller/docs/docs/system-defined-variables.png :alt: system defined variables +**Note:** `Group configuration variables <#group-configuration-variables>` +are also added to the **System Defined Variables** of the device. + Example usage of variables ~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -941,6 +944,7 @@ for the devices of an organization: information across all groups using `"OPENWISP_CONTROLLER_DEVICE_GROUP_SCHEMA" <#openwisp-controller-device-group-schema>`_ setting. - Define `group configuration templates <#group-templates>`_. +- Define `group configuration variables <#group-configuration-variables>`_. .. image:: https://raw.githubusercontent.com/openwisp/openwisp-controller/docs/docs/1.1/device-groups.png :alt: Device Group example @@ -983,6 +987,15 @@ to new devices. This feature works also when editing group templates or the group assigned to a device via the `REST API <#change-device-group-detail>`__. +Group Configuration Variables +############################# + +Groups allow to define configuration variables which are automatically +added to the device's context in the **System Defined Variables**. + +This feature works also when editing group templates or the group assigned +to a device via the `REST API <#change-device-group-detail>`__. + Export/Import Device data ~~~~~~~~~~~~~~~~~~~~~~~~~ From 1e52e635a7c1b473af947ed6cf6162d2a0439aad Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 28 Jun 2023 23:35:29 +0530 Subject: [PATCH 05/10] [qa] Fixed JS formatting --- openwisp_controller/config/static/config/js/widget.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/config/static/config/js/widget.js b/openwisp_controller/config/static/config/js/widget.js index 42da6056e..1d2c932a0 100644 --- a/openwisp_controller/config/static/config/js/widget.js +++ b/openwisp_controller/config/static/config/js/widget.js @@ -36,9 +36,10 @@ // Handles the case when different templates and group contains the keys. // If the contextValue was set by a template or group, then // override the value. - (prevDefaultValues.hasOwnProperty(key) && prevDefaultValues[key] !== defaultValues[key]) + (prevDefaultValues.hasOwnProperty(key) && prevDefaultValues[key] !== defaultValues[key]) || // Gives precedence to device's context (saved in database) and SystemContextValue - || (!contextValue.hasOwnProperty(key) && !systemContextValue.hasOwnProperty(key))) { + (!contextValue.hasOwnProperty(key) && !systemContextValue.hasOwnProperty(key)) + ) { contextValue[key] = defaultValues[key]; } }); From 9f62c7dff1986dc72de149a77cbac7c138675795 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 30 Jun 2023 22:17:37 +0530 Subject: [PATCH 06/10] [req-changes] Updated docs --- README.rst | 47 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/README.rst b/README.rst index f692ba062..569a2c89b 100644 --- a/README.rst +++ b/README.rst @@ -531,7 +531,14 @@ with templates, this feature is also known as *configuration context*, think of it like a dictionary which is passed to the function which renders the configuration, so that it can fill variables according to the passed context. -The different ways in which variables are defined are described below. +The different ways in which variables are defined are described below in +the order (high to low) of their precedence: + +1. `User defined device variables <#user-defined-device-variables>`_ +2. `Predefined device variables <#predefined-device-variables>`_ +3. `Global variables <#global-variables>`_ +4. `Group variables <#group-variables>`_ +5. `Template default values <#template-default-values>`_ Predefined device variables ~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -574,6 +581,14 @@ The default values of variables can be manipulated from the section .. image:: https://raw.githubusercontent.com/openwisp/openwisp-controller/docs/docs/template-default-values.png :alt: default values +Group variables +~~~~~~~~~~~~~~~ + +Variables can also be defined in `Device groups <#device-groups>`__. + +Refer the `Group configuration variables `_ +section for detailed information. + Global variables ~~~~~~~~~~~~~~~~ @@ -591,7 +606,7 @@ in read-only mode. .. image:: https://raw.githubusercontent.com/openwisp/openwisp-controller/docs/docs/system-defined-variables.png :alt: system defined variables -**Note:** `Group configuration variables <#group-configuration-variables>` +**Note:** `Group configuration variables <#group-configuration-variables>`__ are also added to the **System Defined Variables** of the device. Example usage of variables @@ -938,13 +953,9 @@ Device Groups provide features aimed at adding specific management rules for the devices of an organization: - Group similar devices by having dedicated groups for access points, routers, etc. -- Store additional information regarding a group in the structured metadata field - (which can be accessed via the REST API). -- Customize structure and validation of metadata field of DeviceGroup to standardize - information across all groups using `"OPENWISP_CONTROLLER_DEVICE_GROUP_SCHEMA" <#openwisp-controller-device-group-schema>`_ - setting. +- Define `group metadata <#group-metadata>`_. - Define `group configuration templates <#group-templates>`_. -- Define `group configuration variables <#group-configuration-variables>`_. +- Define `group configuration variables <#group-configuration-variables>`__. .. image:: https://raw.githubusercontent.com/openwisp/openwisp-controller/docs/docs/1.1/device-groups.png :alt: Device Group example @@ -992,10 +1003,28 @@ Group Configuration Variables Groups allow to define configuration variables which are automatically added to the device's context in the **System Defined Variables**. +Check the `"How to use configuration variables" section <#how-to-use-configuration-variables>`_ +to learn about precedence of different configuration variables. This feature works also when editing group templates or the group assigned to a device via the `REST API <#change-device-group-detail>`__. +Group Metadata +############## + +Groups allow to store additional information regarding a group in the +structured metadata field (which can be accessed via the REST API). + +The metadata field allows custom structure and validation to standardize +information across all groups using the +`"OPENWISP_CONTROLLER_DEVICE_GROUP_SCHEMA" <#openwisp-controller-device-group-schema>`_ +setting. + +**Note:** *Group configuration variables* and *Group metadata* serves different purposes. +The group configuration variables should be used when the device configuration is required +to be changed for particular group of devices. Group metadata should be used to store +additional data for the devices. Group metadata is not used for configuration generation. + Export/Import Device data ~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -3076,7 +3105,7 @@ while all the other organizations will have all command types enabled. | **default**: | ``{'type': 'object', 'properties': {}}`` | +--------------+------------------------------------------+ -Allows specifying JSONSchema used for validating meta-data of `Device Group <#device-groups>`_. +Allows specifying JSONSchema used for validating meta-data of `Device Group <#device-groups>`__. ``OPENWISP_CONTROLLER_SHARED_MANAGEMENT_IP_ADDRESS_SPACE`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ From 8ba464fd6ac935db81c9aa172a9b6a29ed5648a1 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Mon, 10 Jul 2023 15:13:50 +0530 Subject: [PATCH 07/10] [fix] Fixed Device._has_group method --- openwisp_controller/config/base/device.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openwisp_controller/config/base/device.py b/openwisp_controller/config/base/device.py index 671b4970e..cfe0ef7a1 100644 --- a/openwisp_controller/config/base/device.py +++ b/openwisp_controller/config/base/device.py @@ -128,7 +128,7 @@ def _has_config(self): return hasattr(self, 'config') def _has_group(self): - return hasattr(self, 'config') + return hasattr(self, 'group') def _get_config_attr(self, attr): """ From d8563c33ebbefddd94761be446d53358c1fbc80b Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Mon, 10 Jul 2023 23:53:12 +0530 Subject: [PATCH 08/10] [req-changes] Fixed JS issue --- openwisp_controller/config/static/config/js/widget.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/config/static/config/js/widget.js b/openwisp_controller/config/static/config/js/widget.js index 1d2c932a0..66fd155a2 100644 --- a/openwisp_controller/config/static/config/js/widget.js +++ b/openwisp_controller/config/static/config/js/widget.js @@ -37,8 +37,14 @@ // If the contextValue was set by a template or group, then // override the value. (prevDefaultValues.hasOwnProperty(key) && prevDefaultValues[key] !== defaultValues[key]) || - // Gives precedence to device's context (saved in database) and SystemContextValue - (!contextValue.hasOwnProperty(key) && !systemContextValue.hasOwnProperty(key)) + // Gives precedence to device's context (saved in database) + (!contextValue.hasOwnProperty(key) && + // Gives precedence to systemContextValue. + // But if the default value is equal to the system context value, + // then add the variable in the contextValue. This allows users + // to override the value. + (!systemContextValue.hasOwnProperty(key) || systemContextValue[key] === defaultValues[key]) + ) ) { contextValue[key] = defaultValues[key]; } From 6c67145427a6b12acb4048ba19473a3b94a2bda8 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 13 Jul 2023 23:36:23 +0530 Subject: [PATCH 09/10] [req-changes] Updated README and fixed precedence of variables --- README.rst | 46 +++++++++---------- openwisp_controller/config/base/config.py | 31 ++++++++----- ...context.py => 0049_devicegroup_context.py} | 2 +- .../config/tests/test_admin.py | 7 ++- .../config/tests/test_device.py | 34 ++++++++++++++ 5 files changed, 80 insertions(+), 40 deletions(-) rename openwisp_controller/config/migrations/{0048_devicegroup_context.py => 0049_devicegroup_context.py} (93%) diff --git a/README.rst b/README.rst index 569a2c89b..0be7fe2ff 100644 --- a/README.rst +++ b/README.rst @@ -536,10 +536,20 @@ the order (high to low) of their precedence: 1. `User defined device variables <#user-defined-device-variables>`_ 2. `Predefined device variables <#predefined-device-variables>`_ -3. `Global variables <#global-variables>`_ -4. `Group variables <#group-variables>`_ +3. `Group variables <#group-variables>`_ +4. `Global variables <#global-variables>`_ 5. `Template default values <#template-default-values>`_ +User defined device variables +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In the device configuration section you can find a section named +"Configuration variables" where it is possible to define the configuration +variables and their values, as shown in the example below: + +.. image:: https://raw.githubusercontent.com/openwisp/openwisp-controller/docs/docs/device-context.png + :alt: context + Predefined device variables ~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -550,15 +560,19 @@ Each device gets the following attributes passed as configuration variables: * ``name`` * ``mac_address`` -User defined device variables -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +Group variables +~~~~~~~~~~~~~~~ -In the device configuration section you can find a section named -"Configuration variables" where it is possible to define the configuration -variables and their values, as shown in the example below: +Variables can also be defined in `Device groups <#device-groups>`__. -.. image:: https://raw.githubusercontent.com/openwisp/openwisp-controller/docs/docs/device-context.png - :alt: context +Refer the `Group configuration variables `_ +section for detailed information. + +Global variables +~~~~~~~~~~~~~~~~ + +Variables can also be defined globally using the +`OPENWISP_CONTROLLER_CONTEXT <#openwisp-controller-context>`_ setting. Template default values ~~~~~~~~~~~~~~~~~~~~~~~ @@ -581,20 +595,6 @@ The default values of variables can be manipulated from the section .. image:: https://raw.githubusercontent.com/openwisp/openwisp-controller/docs/docs/template-default-values.png :alt: default values -Group variables -~~~~~~~~~~~~~~~ - -Variables can also be defined in `Device groups <#device-groups>`__. - -Refer the `Group configuration variables `_ -section for detailed information. - -Global variables -~~~~~~~~~~~~~~~~ - -Variables can also be defined globally using the -`OPENWISP_CONTROLLER_CONTEXT <#openwisp-controller-context>`_ setting. - System defined variables ~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index a535ad13e..d3f149915 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -558,7 +558,7 @@ def _has_device(self): return hasattr(self, 'device') def get_vpn_context(self): - context = super().get_context() + context = {} for vpnclient in self.vpnclient_set.all().select_related('vpn', 'cert'): vpn = vpnclient.vpn vpn_id = vpn.pk.hex @@ -599,15 +599,11 @@ def get_context(self, system=False): additional context passed to netjsonconfig """ c = collections.OrderedDict() - extra = {} - if hasattr(self, 'device') and self.device._get_group(): - extra.update(self.device._get_group().get_context()) - extra.update(self.get_vpn_context()) - for func in self._config_context_functions: - extra.update(func(config=self)) - if app_settings.HARDWARE_ID_ENABLED and self._has_device(): - extra.update({'hardware_id': str(self.device.hardware_id)}) + # Add global variables + context = super().get_context() if self._has_device(): + # These pre-defined variables are needed at the start of OrderedDict. + # Hence, they are added separately. c.update( [ ('name', self.name), @@ -616,9 +612,20 @@ def get_context(self, system=False): ('key', self.key), ] ) - if self.context and not system: - extra.update(self.context) - c.update(sorted(extra.items())) + if self.device._get_group(): + # Add device group variables + context.update(self.device._get_group().get_context()) + # Add predefined variables + context.update(self.get_vpn_context()) + for func in self._config_context_functions: + context.update(func(config=self)) + if app_settings.HARDWARE_ID_ENABLED: + context.update({'hardware_id': str(self.device.hardware_id)}) + + if self.context and not system: + context.update(self.context) + + c.update(sorted(context.items())) return c def get_system_context(self): diff --git a/openwisp_controller/config/migrations/0048_devicegroup_context.py b/openwisp_controller/config/migrations/0049_devicegroup_context.py similarity index 93% rename from openwisp_controller/config/migrations/0048_devicegroup_context.py rename to openwisp_controller/config/migrations/0049_devicegroup_context.py index 6dcb6af03..c54c41024 100644 --- a/openwisp_controller/config/migrations/0048_devicegroup_context.py +++ b/openwisp_controller/config/migrations/0049_devicegroup_context.py @@ -9,7 +9,7 @@ class Migration(migrations.Migration): dependencies = [ - ('config', '0047_add_organizationlimits'), + ('config', '0048_wifi_radio_band_migration'), ] operations = [ diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index cd2552154..e50e143a3 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -1519,16 +1519,15 @@ def test_system_context(self): path = reverse(f'admin:{self.app_label}_device_change', args=[d.pk]) self._test_system_context_field_helper(path) - def test_no_system_context(self): + @patch.dict(app_settings.CONTEXT, {}, clear=True) + def test_no_system_context(self, *args): self._create_template() - old_context = app_settings.CONTEXT.copy() - app_settings.CONTEXT = {} path = reverse(f'admin:{self.app_label}_template_add') r = self.client.get(path) + print(app_settings.CONTEXT) self.assertContains( r, 'There are no system defined variables available right now' ) - app_settings.CONTEXT = old_context def test_config_form_old_templates(self): config = self._create_config(organization=self._get_org()) diff --git a/openwisp_controller/config/tests/test_device.py b/openwisp_controller/config/tests/test_device.py index d7d5498d3..5b5222c5b 100644 --- a/openwisp_controller/config/tests/test_device.py +++ b/openwisp_controller/config/tests/test_device.py @@ -321,6 +321,40 @@ def test_device_get_system_context(self): self.assertEqual(system_context['SSID'], 'OpenWISP') self.assertNotIn('test', system_context.keys()) + @mock.patch.dict(app_settings.CONTEXT, {'variable_type': 'global-context-variable'}) + def test_configuration_variable_priority(self, *args): + org = self._get_org() + device_group = self._create_device_group( + organization=org, context={'variable_type': 'device-group-variable'} + ) + device = self._create_device(organization=org, group=device_group) + config = self._create_config( + device=device, + context={ + 'id': 'user-defined-variable', + 'variable_type': 'user-defined-variable', + }, + ) + + # User defined variable has highest precedence + self.assertEqual(config.get_context()['id'], 'user-defined-variable') + self.assertEqual(config.get_context()['variable_type'], 'user-defined-variable') + # Second precedence is given to predefined device variables + config.context = {} + self.assertEqual( + config.get_context()['id'], + str(device.pk), + ) + # It is not possible to overwrite the pre-defined device variables. + # Therefore, for further tests, "tesT" variable is used. + # Third precedence is given to the device group variable' + self.assertEqual(config.get_context()['variable_type'], 'device-group-variable') + # Fourth precedence is given to global variables + device.group = None + self.assertEqual( + config.get_context()['variable_type'], 'global-context-variable' + ) + def test_management_ip_changed_not_emitted_on_creation(self): with catch_signal(management_ip_changed) as handler: self._create_device(organization=self._get_org()) From 41348005e6e4df74d7f548f029ad67b7f34a9af6 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 14 Jul 2023 23:08:21 +0530 Subject: [PATCH 10/10] [req-changes] Fixed search fields for DeviceGroupAdmin --- openwisp_controller/config/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 7482910fa..df3f6e4af 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -1022,7 +1022,7 @@ class DeviceGroupAdmin(MultitenantAdminMixin, BaseAdmin): 'created', 'modified', ] - search_fields = ['name', 'description', 'meta_data', 'context'] + search_fields = ['name', 'description', 'meta_data'] list_filter = [MultitenantOrgFilter, DeviceGroupFilter] multitenant_shared_relations = ('templates',)