[IMPROVEMENT] Implement Django permissions for Rule editor #96

To avoid issues with common rules accidentally being disabled, require
rule_change permissions and _change permission on the content_object when
editing or deleting rules.

This also sets the foundation for enforcing permissions for more objects
in the future

* Adds a Default group controlled by PROMGEN_DEFAULT_GROUP
* Migration adds all existing users to Default group
* post_save signal to add newly created users to Default group

promgen/migrations/0003_default-group.py

@@ -0,0 +1,53 @@
+# Generated by Django 2.0.7 on 2018-07-31 08:09
+
+from django.db import migrations
+from django.conf import settings
+
+
+def create_group(apps, schema_editor):
+    if not settings.PROMGEN_DEFAULT_GROUP:
+        return
+
+    # Create Default Group
+    group, created = apps.get_model('auth', 'Group').objects.get_or_create(
+        name=settings.PROMGEN_DEFAULT_GROUP
+    )
+
+    # Create default permissions. We skip the permissions that are
+    # generally for admin rules (Shards, Prometheus, Audit) and skip
+    # custom permissions for label/annotations but list everything else
+    Permission = apps.get_model('auth', 'Permission')
+    group.permissions.set(
+        Permission.objects.filter(
+            content_type__app_label='promgen',
+            content_type__model__in=[
+                'exporter',
+                'farm',
+                'host',
+                'project',
+                'rule',
+                'sender',
+                'service',
+                'url',
+            ],
+        )
+    )
+
+    # Add users to default group
+    User = apps.get_model('auth', 'User')
+    for user in User.objects.all():
+        user.groups.add(group)
+
+
+def remove_group(apps, schema_editor):
+    apps.get_model('auth', 'Group').objects.filter(
+        name=settings.PROMGEN_DEFAULT_GROUP
+    ).delete()
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [('promgen', '0002_auto_20180316_0525')]
+
+    operations = [migrations.RunPython(create_group, remove_group)]
+

promgen/settings.py

@@ -48,6 +48,8 @@
 else:
     PROMGEN = {}
 
+PROMGEN_DEFAULT_GROUP = 'Default'
+
 ALLOWED_HOSTS = ['*']
 
 

promgen/signals.py

@@ -4,12 +4,13 @@
 import logging
 from functools import wraps
 
+from django.conf import settings
 from django.contrib import messages
+from django.contrib.auth.models import Group, User
 from django.core.cache import cache
 from django.db.models.signals import (post_delete, post_save, pre_delete,
                                       pre_save)
 from django.dispatch import Signal, receiver
-
 from promgen import models, prometheus
 
 logger = logging.getLogger(__name__)
@@ -223,3 +224,16 @@ def save_service(sender, instance, **kwargs):
             # If any of our save_project returns True, then we do not need to
             # check any others
             return True
+
+
+@receiver(post_save, sender=User)
+def add_user_to_default_group(sender, instance, created, **kwargs):
+    # If we enabled our default group, then we want to ensure that all newly
+    # created users are also added to our default group so they inherit the
+    # default permissions
+    if not settings.PROMGEN_DEFAULT_GROUP:
+        return
+    if not created:
+        return
+
+    instance.groups.add(Group.objects.get(name=settings.PROMGEN_DEFAULT_GROUP))

promgen/tests/test_routes.py

@@ -4,7 +4,7 @@
 from unittest import mock
 
 import factory.django
-from django.contrib.auth.models import User
+from django.contrib.auth.models import User, Permission
 from django.db.models.signals import post_save, pre_save
 from django.test import override_settings
 from django.urls import reverse
@@ -23,7 +23,8 @@ class RouteTests(PromgenTest):
 
     @factory.django.mute_signals(pre_save, post_save)
     def setUp(self):
-        self.client.force_login(User.objects.create_user(id=999, username="Foo"), 'django.contrib.auth.backends.ModelBackend')
+        self.user = User.objects.create_user(id=999, username="Foo")
+        self.client.force_login(self.user, 'django.contrib.auth.backends.ModelBackend')
 
     @override_settings(PROMGEN=TEST_SETTINGS)
     @override_settings(CELERY_TASK_ALWAYS_EAGER=True)
@@ -36,9 +37,12 @@ def test_alert(self):
     @mock.patch('promgen.signals._trigger_write_config')
     @mock.patch('promgen.prometheus.reload_prometheus')
     def test_import(self, mock_write, mock_reload):
-        response = self.client.post(reverse('import'), {
-            'config': TEST_IMPORT
-        })
+        self.user.user_permissions.add(
+            Permission.objects.get(codename='change_rule'),
+            Permission.objects.get(codename='change_site'),
+            Permission.objects.get(codename='change_exporter'),
+        )
+        response = self.client.post(reverse('import'), {'config': TEST_IMPORT})
 
         self.assertEqual(response.status_code, 302, 'Redirect to imported object')
         self.assertEqual(models.Service.objects.count(), 1, 'Import one service')
@@ -53,14 +57,17 @@ def test_import(self, mock_write, mock_reload):
     @mock.patch('promgen.signals._trigger_write_config')
     @mock.patch('promgen.prometheus.reload_prometheus')
     def test_replace(self, mock_write, mock_reload):
-        response = self.client.post(reverse('import'), {
-            'config': TEST_IMPORT
-        })
+        # Set the required permissions
+        self.user.user_permissions.add(
+            Permission.objects.get(codename='change_rule'),
+            Permission.objects.get(codename='change_site'),
+            Permission.objects.get(codename='change_exporter'),
+        )
+
+        response = self.client.post(reverse('import'), {'config': TEST_IMPORT})
         self.assertEqual(response.status_code, 302, 'Redirect to imported object')
 
-        response = self.client.post(reverse('import'), {
-            'config': TEST_REPLACE
-        })
+        response = self.client.post(reverse('import'), {'config': TEST_REPLACE})
         self.assertEqual(response.status_code, 302, 'Redirect to imported object (2)')
 
         self.assertEqual(models.Service.objects.count(), 1, 'Import one service')
@@ -121,9 +128,18 @@ def test_scrape(self, mock_get):
             )
             self.assertEqual(mock_get.call_args[0][0], url)
 
+    def test_failed_permission(self):
+        # Test for redirect
+        for request in [{'viewname': 'rule-new', 'args': ('site', 1)}]:
+            response = self.client.get(reverse(**request))
+            self.assertEqual(response.status_code, 302)
+            self.assertTrue(response.url.startswith('/login'))
+
     def test_other_routes(self):
-        for request in [
-            {'viewname': 'rule-new', 'args': ('site', 1)},
-        ]:
+        self.user.user_permissions.add(
+            Permission.objects.get(codename='add_rule'),
+            Permission.objects.get(codename='change_site'),
+        )
+        for request in [{'viewname': 'rule-new', 'args': ('site', 1)}]:
             response = self.client.get(reverse(**request))
             self.assertEqual(response.status_code, 200)

promgen/tests/test_rules.py

@@ -3,7 +3,7 @@
 
 from unittest import mock
 
-from django.contrib.auth.models import User
+from django.contrib.auth.models import User, Permission
 from django.urls import reverse
 
 import promgen.templatetags.promgen as macro
@@ -76,6 +76,10 @@ def test_copy(self, mock_post):
 
     @mock.patch('django.dispatch.dispatcher.Signal.send')
     def test_import_v1(self, mock_post):
+        self.user.user_permissions.add(
+            Permission.objects.get(codename='change_rule'),
+            Permission.objects.get(codename='change_site'),
+        )
         self.client.post(reverse('rule-import'), {
             'rules': PromgenTest.data('examples', 'import.rule')
         })
@@ -87,6 +91,10 @@ def test_import_v1(self, mock_post):
 
     @mock.patch('django.dispatch.dispatcher.Signal.send')
     def test_import_v2(self, mock_post):
+        self.user.user_permissions.add(
+            Permission.objects.get(codename='change_rule'),
+            Permission.objects.get(codename='change_site'),
+        )
         self.client.post(reverse('rule-import'), {
             'rules': PromgenTest.data('examples', 'import.rule.yml')
         })
@@ -96,6 +104,15 @@ def test_import_v2(self, mock_post):
         self.assertEqual(models.RuleLabel.objects.count(), 4, 'Missing labels')
         self.assertEqual(models.RuleAnnotation.objects.count(), 9, 'Missing annotations')
 
+    @mock.patch('django.dispatch.dispatcher.Signal.send')
+    def test_missing_permission(self, mock_post):
+        self.client.post(reverse('rule-import'), {
+            'rules': PromgenTest.data('examples', 'import.rule.yml')
+        })
+
+        # Should only be a single rule from our initial setup
+        self.assertEqual(models.Rule.objects.count(), 1, 'Missing Rule')
+
     @mock.patch('django.dispatch.dispatcher.Signal.send')
     def test_macro(self, mock_post):
         self.project = models.Project.objects.create(name='Project 1', service=self.service)

promgen/views.py

@@ -12,12 +12,15 @@
 from itertools import chain
 from urllib.parse import urljoin
 
+import promgen.templatetags.promgen as macro
 import requests
 from dateutil import parser
 from django import forms as django_forms
 from django.conf import settings
 from django.contrib import messages
-from django.contrib.auth.mixins import LoginRequiredMixin
+from django.contrib.auth.mixins import (LoginRequiredMixin,
+                                        PermissionRequiredMixin)
+from django.contrib.auth.views import redirect_to_login
 from django.contrib.contenttypes.models import ContentType
 from django.db.models import Count, Q
 from django.db.utils import IntegrityError
@@ -33,15 +36,23 @@
 from django.views.generic.base import ContextMixin, RedirectView, TemplateView
 from django.views.generic.edit import DeleteView, FormView
 from prometheus_client import Gauge, generate_latest
-
-import promgen.templatetags.promgen as macro
 from promgen import (celery, discovery, forms, models, plugins, prometheus,
                      signals, util, version)
 from promgen.shortcuts import resolve_domain
 
 logger = logging.getLogger(__name__)
 
 
+class PromgenPermissionMixin(PermissionRequiredMixin):
+    def handle_no_permission(self):
+        messages.warning(self.request, self.get_permission_denied_message())
+        return redirect_to_login(
+            self.request.get_full_path(),
+            self.get_login_url(),
+            self.get_redirect_field_name(),
+        )
+
+
 class ShardMixin(ContextMixin):
     def get_context_data(self, **kwargs):
         context = super(ShardMixin, self).get_context_data(**kwargs)
@@ -288,14 +299,40 @@ def post(self, request, pk):
         return JsonResponse({'redirect': exporter.project.get_absolute_url()})
 
 
-class RuleDelete(LoginRequiredMixin, DeleteView):
+class RuleDelete(PromgenPermissionMixin, DeleteView):
     model = models.Rule
 
+    def get_permission_denied_message(self):
+        return 'Unable to delete rule %s. User lacks permission' % self.object
+
+    def get_permission_required(self):
+        # In the case of rules, we want to make sure the user has permission
+        # to delete the rule itself, but also permission to change the linked object
+        self.object = self.get_object()
+        obj = self.object._meta
+        tgt = self.object.content_object._meta
+
+        yield '{}.delete_{}'.format(obj.app_label, obj.model_name)
+        yield '{}.change_{}'.format(tgt.app_label, tgt.model_name)
+
     def get_success_url(self):
         return self.object.content_object.get_absolute_url()
 
 
-class RuleToggle(LoginRequiredMixin, View):
+class RuleToggle(PromgenPermissionMixin, View):
+    def get_permission_denied_message(self):
+        return 'Unable to toggle rule %s. User lacks permission' % self.object
+
+    def get_permission_required(self):
+        # In the case of rules, we want to make sure the user has permission
+        # to delete the rule itself, but also permission to change the linked object
+        self.object = self.get_object()
+        obj = self.object._meta
+        tgt = self.object.content_object._meta
+
+        yield '{}.change_{}'.format(obj.app_label, obj.model_name)
+        yield '{}.change_{}'.format(tgt.app_label, tgt.model_name)
+
     def post(self, request, pk):
         rule = get_object_or_404(models.Rule, id=pk)
         rule.enabled = not rule.enabled
@@ -610,7 +647,20 @@ class ServiceUpdate(LoginRequiredMixin, UpdateView):
     template_name = 'promgen/service_form.html'
 
 
-class RuleUpdate(LoginRequiredMixin, UpdateView):
+class RuleUpdate(PromgenPermissionMixin, UpdateView):
+    def get_permission_denied_message(self):
+        return 'Unable to edit rule %s. User lacks permission' % self.object
+
+    def get_permission_required(self):
+        # In the case of rules, we want to make sure the user has permission
+        # to change the rule itself, but also permission to change the linked object
+        self.object = self.get_object()
+        obj = self.object._meta
+        tgt = self.object.content_object._meta
+
+        yield '{}.change_{}'.format(obj.app_label, obj.model_name)
+        yield '{}.change_{}'.format(tgt.app_label, tgt.model_name)
+
     queryset = models.Rule.objects.prefetch_related(
         'content_object',
         'overrides',
@@ -670,11 +720,20 @@ def post(self, request, *args, **kwargs):
         return self.form_valid(form)
 
 
-class RuleRegister(LoginRequiredMixin, FormView, ServiceMixin):
+class RuleRegister(PromgenPermissionMixin, FormView, ServiceMixin):
     model = models.Rule
     template_name = 'promgen/rule_register.html'
     form_class = forms.NewRuleForm
 
+    def get_permission_required(self):
+        # In the case of rules, we want to make sure the user has permission
+        # to add the rule itself, but also permission to change the linked object
+        yield 'promgen.add_rule'
+        if self.kwargs['content_type'] == 'site':
+            yield 'sites.change_site'
+        else:
+            yield 'promgen.change_' + self.kwargs['content_type']
+
     def get_context_data(self, **kwargs):
         context = super(RuleRegister, self).get_context_data(**kwargs)
         # Set a dummy rule, so that our header/breadcrumbs render correctly
@@ -1006,10 +1065,16 @@ def get(self, request):
         return render(request, 'promgen/search.html', context)
 
 
-class RuleImport(LoginRequiredMixin, FormView):
+class RuleImport(PromgenPermissionMixin, FormView):
     form_class = forms.ImportRuleForm
     template_name = 'promgen/rule_import.html'
 
+    # Since rule imports can change a lot of site wide stuff we
+    # require site edit permission here
+    permission_required = ('sites.change_site', 'promgen.change_rule')
+    permisison_denied_message = 'User lacks permission to import'
+
+
     def form_valid(self, form):
         data = form.clean()
         if data.get('file_field'):
@@ -1029,10 +1094,18 @@ def form_valid(self, form):
             return self.form_invalid(form)
 
 
-class Import(LoginRequiredMixin, FormView):
+class Import(PromgenPermissionMixin, FormView):
     template_name = 'promgen/import_form.html'
     form_class = forms.ImportConfigForm
 
+    # Since imports can change a lot of site wide stuff we
+    # require site edit permission here
+    permission_required = (
+        'sites.change_site', 'promgen.change_rule', 'promgen.change_exporter'
+    )
+
+    permission_denied_message = 'User lacks permission to import'
+
     def form_valid(self, form):
         data = form.clean()
         if data.get('file_field'):