Fixed #26470 -- Converted auth permission validation to system checks.

Thanks Tim for the review.

django/contrib/auth/apps.py

@@ -1,9 +1,9 @@
 from django.apps import AppConfig
-from django.contrib.auth.checks import check_user_model
 from django.core import checks
 from django.db.models.signals import post_migrate
 from django.utils.translation import ugettext_lazy as _
 
+from .checks import check_models_permissions, check_user_model
 from .management import create_permissions
 
 
@@ -15,3 +15,4 @@ class AuthConfig(AppConfig):
         post_migrate.connect(create_permissions,
             dispatch_uid="django.contrib.auth.management.create_permissions")
         checks.register(check_user_model, checks.Tags.models)
+        checks.register(check_models_permissions, checks.Tags.models)

django/contrib/auth/checks.py

@@ -1,10 +1,14 @@
 # -*- coding: utf-8 -*-
 from __future__ import unicode_literals
 
+from itertools import chain
+
 from django.apps import apps
 from django.conf import settings
 from django.core import checks
 
+from .management import _get_builtin_permissions
+
 
 def check_user_model(app_configs=None, **kwargs):
     if app_configs is None:
@@ -70,3 +74,72 @@ def check_user_model(app_configs=None, **kwargs):
             )
 
     return errors
+
+
+def check_models_permissions(app_configs=None, **kwargs):
+    if app_configs is None:
+        models = apps.get_models()
+    else:
+        models = chain.from_iterable(app_config.get_models() for app_config in app_configs)
+
+    Permission = apps.get_model('auth', 'Permission')
+    permission_name_max_length = Permission._meta.get_field('name').max_length
+    errors = []
+
+    for model in models:
+        opts = model._meta
+        builtin_permissions = dict(_get_builtin_permissions(opts))
+        # Check builtin permission name length.
+        max_builtin_permission_name_length = max(len(name) for name in builtin_permissions.values())
+        if max_builtin_permission_name_length > permission_name_max_length:
+            verbose_name_max_length = (
+                permission_name_max_length - (max_builtin_permission_name_length - len(opts.verbose_name_raw))
+            )
+            errors.append(
+                checks.Error(
+                    "The verbose_name of model '%s.%s' must be at most %d characters "
+                    "for its builtin permission names to be at most %d characters." % (
+                        opts.app_label, opts.object_name, verbose_name_max_length, permission_name_max_length
+                    ),
+                    obj=model,
+                    id='auth.E007',
+                )
+            )
+        codenames = set()
+        for codename, name in opts.permissions:
+            # Check custom permission name length.
+            if len(name) > permission_name_max_length:
+                errors.append(
+                    checks.Error(
+                        "The permission named '%s' of model '%s.%s' is longer than %d characters." % (
+                            name, opts.app_label, opts.object_name, permission_name_max_length
+                        ),
+                        obj=model,
+                        id='auth.E008',
+                    )
+                )
+            # Check custom permissions codename clashing.
+            if codename in builtin_permissions:
+                errors.append(
+                    checks.Error(
+                        "The permission codenamed '%s' clashes with a builtin permission "
+                        "for model '%s.%s'." % (
+                            codename, opts.app_label, opts.object_name
+                        ),
+                        obj=model,
+                        id='auth.E005',
+                    )
+                )
+            elif codename in codenames:
+                errors.append(
+                    checks.Error(
+                        "The permission codenamed '%s' is duplicated for model '%s.%s'." % (
+                            codename, opts.app_label, opts.object_name
+                        ),
+                        obj=model,
+                        id='auth.E006',
+                    )
+                )
+            codenames.add(codename)
+
+    return errors

django/contrib/auth/management/__init__.py

@@ -9,19 +9,17 @@ import unicodedata
 from django.apps import apps
 from django.contrib.auth import get_permission_codename
 from django.core import exceptions
-from django.core.management.base import CommandError
 from django.db import DEFAULT_DB_ALIAS, router
 from django.utils import six
 from django.utils.encoding import DEFAULT_LOCALE_ENCODING
 
 
-def _get_all_permissions(opts, ctype):
+def _get_all_permissions(opts):
     """
     Returns (codename, name) for all permissions in the given opts.
     """
     builtin = _get_builtin_permissions(opts)
     custom = list(opts.permissions)
-    _check_permission_clashing(custom, builtin, ctype)
     return builtin + custom
 
 
@@ -37,26 +35,6 @@ def _get_builtin_permissions(opts):
     return perms
 
 
-def _check_permission_clashing(custom, builtin, ctype):
-    """
-    Check that permissions for a model do not clash. Raises CommandError if
-    there are duplicate permissions.
-    """
-    pool = set()
-    builtin_codenames = set(p[0] for p in builtin)
-    for codename, _name in custom:
-        if codename in pool:
-            raise CommandError(
-                "The permission codename '%s' is duplicated for model '%s.%s'." %
-                (codename, ctype.app_label, ctype.model_class().__name__))
-        elif codename in builtin_codenames:
-            raise CommandError(
-                "The permission codename '%s' clashes with a builtin permission "
-                "for model '%s.%s'." %
-                (codename, ctype.app_label, ctype.model_class().__name__))
-        pool.add(codename)
-
-
 def create_permissions(app_config, verbosity=2, interactive=True, using=DEFAULT_DB_ALIAS, **kwargs):
     if not app_config.models_module:
         return
@@ -71,9 +49,6 @@ def create_permissions(app_config, verbosity=2, interactive=True, using=DEFAULT_
 
     from django.contrib.contenttypes.models import ContentType
 
-    permission_name_max_length = Permission._meta.get_field('name').max_length
-    verbose_name_max_length = permission_name_max_length - 11  # len('Can change ') prefix
-
     # This will hold the permissions we're looking for as
     # (content_type, (codename, name))
     searched_perms = list()
@@ -84,17 +59,8 @@ def create_permissions(app_config, verbosity=2, interactive=True, using=DEFAULT_
         # before creating foreign keys to them.
         ctype = ContentType.objects.db_manager(using).get_for_model(klass)
 
-        if len(klass._meta.verbose_name) > verbose_name_max_length:
-            raise exceptions.ValidationError(
-                "The verbose_name of %s.%s is longer than %s characters" % (
-                    ctype.app_label,
-                    ctype.model,
-                    verbose_name_max_length,
-                )
-            )
-
         ctypes.add(ctype)
-        for perm in _get_all_permissions(klass._meta, ctype):
+        for perm in _get_all_permissions(klass._meta):
             searched_perms.append((ctype, perm))
 
     # Find all the Permissions that have a content_type for a model we're
@@ -111,18 +77,6 @@ def create_permissions(app_config, verbosity=2, interactive=True, using=DEFAULT_
         for ct, (codename, name) in searched_perms
         if (ct.pk, codename) not in all_perms
     ]
-    # Validate the permissions before bulk_creation to avoid cryptic database
-    # error when the name is longer than 255 characters
-    for perm in perms:
-        if len(perm.name) > permission_name_max_length:
-            raise exceptions.ValidationError(
-                "The permission name %s of %s.%s is longer than %s characters" % (
-                    perm.name,
-                    perm.content_type.app_label,
-                    perm.content_type.model,
-                    permission_name_max_length,
-                )
-            )
     Permission.objects.using(using).bulk_create(perms)
     if verbosity >= 2:
         for perm in perms:

docs/ref/checks.txt

@@ -432,6 +432,16 @@ Auth
   ``USERNAME_FIELD``.
 * **auth.W004**: ``<field>`` is named as the ``USERNAME_FIELD``, but it is not
   unique.
+* **auth.E005**: The permission codenamed ``<codename>`` clashes with a builtin
+  permission for model ``<model>``.
+* **auth.E006**: The permission codenamed ``<codename>`` is duplicated for model
+  ``<model>``.
+* **auth.E007**: The :attr:`verbose_name
+  <django.db.models.Options.verbose_name>` of model ``<model>`` must be at most
+  244 characters for its builtin permission names
+  to be at most 255 characters.
+* **auth.E008**: The permission named ``<name>`` of model ``<model>`` is longer
+  than 255 characters.
 
 
 Content Types

tests/auth_tests/test_checks.py

@@ -1,6 +1,8 @@
 from __future__ import unicode_literals
 
-from django.contrib.auth.checks import check_user_model
+from django.contrib.auth.checks import (
+    check_models_permissions, check_user_model,
+)
 from django.contrib.auth.models import AbstractBaseUser
 from django.core import checks
 from django.db import models
@@ -80,3 +82,83 @@ class UserModelChecksTests(SimpleTestCase):
                     id='auth.W004',
                 ),
             ])
+
+
+@isolate_apps('auth_tests', attr_name='apps')
+@override_system_checks([check_models_permissions])
+class ModelsPermissionsChecksTests(SimpleTestCase):
+    def test_clashing_default_permissions(self):
+        class Checked(models.Model):
+            class Meta:
+                permissions = [
+                    ('change_checked', 'Can edit permission (duplicate)')
+                ]
+        errors = checks.run_checks(self.apps.get_app_configs())
+        self.assertEqual(errors, [
+            checks.Error(
+                "The permission codenamed 'change_checked' clashes with a builtin "
+                "permission for model 'auth_tests.Checked'.",
+                obj=Checked,
+                id='auth.E005',
+            ),
+        ])
+
+    def test_non_clashing_custom_permissions(self):
+        class Checked(models.Model):
+            class Meta:
+                permissions = [
+                    ('my_custom_permission', 'Some permission'),
+                    ('other_one', 'Some other permission'),
+                ]
+        errors = checks.run_checks(self.apps.get_app_configs())
+        self.assertEqual(errors, [])
+
+    def test_clashing_custom_permissions(self):
+        class Checked(models.Model):
+            class Meta:
+                permissions = [
+                    ('my_custom_permission', 'Some permission'),
+                    ('other_one', 'Some other permission'),
+                    ('my_custom_permission', 'Some permission with duplicate permission code'),
+                ]
+        errors = checks.run_checks(self.apps.get_app_configs())
+        self.assertEqual(errors, [
+            checks.Error(
+                "The permission codenamed 'my_custom_permission' is duplicated for "
+                "model 'auth_tests.Checked'.",
+                obj=Checked,
+                id='auth.E006',
+            ),
+        ])
+
+    def test_verbose_name_max_length(self):
+        class Checked(models.Model):
+            class Meta:
+                verbose_name = 'some ridiculously long verbose name that is out of control' * 5
+        errors = checks.run_checks(self.apps.get_app_configs())
+        self.assertEqual(errors, [
+            checks.Error(
+                "The verbose_name of model 'auth_tests.Checked' must be at most 244 "
+                "characters for its builtin permission names to be at most 255 characters.",
+                obj=Checked,
+                id='auth.E007',
+            ),
+        ])
+
+    def test_custom_permission_name_max_length(self):
+        custom_permission_name = 'some ridiculously long verbose name that is out of control' * 5
+
+        class Checked(models.Model):
+            class Meta:
+                permissions = [
+                    ('my_custom_permission', custom_permission_name),
+                ]
+        errors = checks.run_checks(self.apps.get_app_configs())
+        self.assertEqual(errors, [
+            checks.Error(
+                "The permission named '%s' of model 'auth_tests.Checked' is longer "
+                "than 255 characters." % custom_permission_name,
+                obj=Checked,
+                id='auth.E008',
+            ),
+        ])

tests/auth_tests/test_management.py

@@ -11,7 +11,6 @@ from django.contrib.auth.management.commands import (
 )
 from django.contrib.auth.models import Group, Permission, User
 from django.contrib.contenttypes.models import ContentType
-from django.core import exceptions
 from django.core.management import call_command
 from django.core.management.base import CommandError
 from django.test import TestCase, mock, override_settings
@@ -567,48 +566,12 @@ class PermissionTestCase(TestCase):
     def setUp(self):
         self._original_permissions = Permission._meta.permissions[:]
         self._original_default_permissions = Permission._meta.default_permissions
-        self._original_verbose_name = Permission._meta.verbose_name
 
     def tearDown(self):
         Permission._meta.permissions = self._original_permissions
         Permission._meta.default_permissions = self._original_default_permissions
-        Permission._meta.verbose_name = self._original_verbose_name
         ContentType.objects.clear_cache()
 
-    def test_duplicated_permissions(self):
-        """
-        Test that we show proper error message if we are trying to create
-        duplicate permissions.
-        """
-        auth_app_config = apps.get_app_config('auth')
-
-        # check duplicated default permission
-        Permission._meta.permissions = [
-            ('change_permission', 'Can edit permission (duplicate)')]
-        msg = (
-            "The permission codename 'change_permission' clashes with a "
-            "builtin permission for model 'auth.Permission'."
-        )
-        with self.assertRaisesMessage(CommandError, msg):
-            create_permissions(auth_app_config, verbosity=0)
-
-        # check duplicated custom permissions
-        Permission._meta.permissions = [
-            ('my_custom_permission', 'Some permission'),
-            ('other_one', 'Some other permission'),
-            ('my_custom_permission', 'Some permission with duplicate permission code'),
-        ]
-        msg = "The permission codename 'my_custom_permission' is duplicated for model 'auth.Permission'."
-        with self.assertRaisesMessage(CommandError, msg):
-            create_permissions(auth_app_config, verbosity=0)
-
-        # should not raise anything
-        Permission._meta.permissions = [
-            ('my_custom_permission', 'Some permission'),
-            ('other_one', 'Some other permission'),
-        ]
-        create_permissions(auth_app_config, verbosity=0)
-
     def test_default_permissions(self):
         auth_app_config = apps.get_app_config('auth')
 
@@ -631,32 +594,3 @@ class PermissionTestCase(TestCase):
         self.assertEqual(Permission.objects.filter(
             content_type=permission_content_type,
         ).count(), 1)
-
-    def test_verbose_name_length(self):
-        auth_app_config = apps.get_app_config('auth')
-
-        permission_content_type = ContentType.objects.get_by_natural_key('auth', 'permission')
-        Permission.objects.filter(content_type=permission_content_type).delete()
-        Permission._meta.verbose_name = "some ridiculously long verbose name that is out of control" * 5
-
-        msg = "The verbose_name of auth.permission is longer than 244 characters"
-        with self.assertRaisesMessage(exceptions.ValidationError, msg):
-            create_permissions(auth_app_config, verbosity=0)
-
-    def test_custom_permission_name_length(self):
-        auth_app_config = apps.get_app_config('auth')
-
-        ContentType.objects.get_by_natural_key('auth', 'permission')
-        custom_perm_name = 'a' * 256
-        Permission._meta.permissions = [
-            ('my_custom_permission', custom_perm_name),
-        ]
-        try:
-            msg = (
-                "The permission name %s of auth.permission is longer than "
-                "255 characters" % custom_perm_name
-            )
-            with self.assertRaisesMessage(exceptions.ValidationError, msg):
-                create_permissions(auth_app_config, verbosity=0)
-        finally:
-            Permission._meta.permissions = []