Replace use of django-guardian with fields on UserProfile.

As documented in https://github.com/zulip/zulip/issues/441, Guardian
has quite poor performance, and in fact almost 50% of the time spent
running the Zulip backend test suite on my laptop was inside Guardian.

As part of this migration, we also clean up the old API_SUPER_USERS
variable used to mark EMAIL_GATEWAY_BOT as an API super user; now that
permission is managed entirely via the database.

When rebasing past this commit, developers will need to do a
`manage.py migrate` in order to apply the migration changes before the
server will run again.

We can't yet remove Guardian from INSTALLED_APPS, requirements.txt,
etc. in this release, because otherwise the reverse migration won't
work.

Fixes #441.

zerver/decorator.py

@@ -89,7 +89,7 @@ def require_post(func):
 def require_realm_admin(func):
     @wraps(func)
     def wrapper(request, user_profile, *args, **kwargs):
-        if not user_profile.has_perm('administer', user_profile.realm):
+        if not user_profile.is_realm_admin:
             raise JsonableError("Must be a realm administrator")
         return func(request, user_profile, *args, **kwargs)
     return wrapper

zerver/lib/actions.py

@@ -20,7 +20,6 @@ from zerver.models import Realm, RealmEmoji, Stream, UserProfile, UserActivity,
     ScheduledJob, realm_filters_for_domain, get_active_bot_dicts_in_realm
 
 from zerver.lib.avatar import get_avatar_url, avatar_url
-from guardian.shortcuts import assign_perm, remove_perm
 
 from django.db import transaction, IntegrityError
 from django.db.models import F, Q
@@ -224,7 +223,7 @@ def process_new_human_user(user_profile, prereg_user=None, newsletter_data=None)
 def notify_created_user(user_profile):
     event = dict(type="realm_user", op="add",
                  person=dict(email=user_profile.email,
-                             is_admin=user_profile.is_admin(),
+                             is_admin=user_profile.is_realm_admin,
                              full_name=user_profile.full_name,
                              is_bot=user_profile.is_bot))
     send_event(event, active_user_ids(user_profile.realm))
@@ -883,8 +882,8 @@ def check_message(sender, client, message_type_name, message_to,
         elif subscribed_to_stream(sender, stream):
             # Or it is private, but your are subscribed
             pass
-        elif sender.is_api_super_user() or (forwarder_user_profile is not None and
+        elif sender.is_api_super_user or (forwarder_user_profile is not None and
-                                            forwarder_user_profile.is_api_super_user()):
+                                          forwarder_user_profile.is_api_super_user):
             # Or this request is being done on behalf of a super user
             pass
         elif sender.is_bot and subscribed_to_stream(sender.bot_owner, stream):
@@ -1582,16 +1581,20 @@ def do_change_default_all_public_streams(user_profile, value, log=True):
                                 default_all_public_streams=user_profile.default_all_public_streams,)),
                     bot_owner_userids(user_profile))
 
-def do_change_is_admin(user_profile, is_admin, permission='administer'):
+def do_change_is_admin(user_profile, value, permission='administer'):
-    if is_admin:
+    if permission == "administer":
-        assign_perm(permission, user_profile, user_profile.realm)
+        user_profile.is_realm_admin = value
+        user_profile.save(update_fields=["is_realm_admin"])
+    elif permission == "api_super_user":
+        user_profile.is_api_super_user = value
+        user_profile.save(update_fields=["is_api_super_user"])
     else:
-        remove_perm(permission, user_profile, user_profile.realm)
+        raise Exception("Unknown permission")
 
     if permission == 'administer':
         event = dict(type="realm_user", op="update",
                      person=dict(email=user_profile.email,
-                                 is_admin=is_admin))
+                                 is_admin=value))
         send_event(event, active_user_ids(user_profile.realm))
 
 def do_make_stream_public(user_profile, realm, stream_name):
@@ -2165,7 +2168,7 @@ def do_update_message(user_profile, message_id, subject, propagate_mode, content
     if message.sender == user_profile:
         pass
     elif (content is None) and ((message.subject == "(no topic)") or
-                                user_profile.is_admin()):
+                                user_profile.is_realm_admin):
         pass
     else:
         raise JsonableError("You don't have permission to edit this message")
@@ -2950,7 +2953,7 @@ def get_occupied_streams(realm):
 
 def do_get_streams(user_profile, include_public=True, include_subscribed=True,
                    include_all_active=False):
-    if include_all_active and not user_profile.is_api_super_user():
+    if include_all_active and not user_profile.is_api_super_user:
         raise JsonableError("User not authorized for this query")
 
     # Listing public streams are disabled for the mit.edu realm.

zerver/management/commands/initialize_voyager_db.py

@@ -53,6 +53,11 @@ class Command(BaseCommand):
             bot.bot_owner = bot
             bot.save()
 
+        # Initialize the email gateway bot as an API Super User
+        email_gateway_bot = UserProfile.objects.get(email__iexact=settings.EMAIL_GATEWAY_BOT)
+        email_gateway_bot.is_api_super_user = True
+        email_gateway_bot.save()
+
         (admin_realm, _) = do_create_realm(settings.ADMIN_DOMAIN,
                                            settings.ADMIN_DOMAIN, True)
 

zerver/migrations/0011_remove_guardian.py

@@ -0,0 +1,110 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import models, migrations, connection
+from django.conf import settings
+
+# Translate the UserProfile fields back to the old Guardian model
+def unmigrate_guardian_data(apps, schema_editor):
+    Permission = apps.get_model('auth', 'Permission')
+    ContentType = apps.get_model('contenttypes', 'ContentType')
+    UserProfile = apps.get_model('zerver', 'UserProfile')
+
+    try:
+        administer = Permission.objects.get(codename="administer")
+    except Permission.DoesNotExist:
+        administer = Permission.objects.create(codename="administer")
+    try:
+        api_super_user = Permission.objects.get(codename="api_super_user")
+    except Permission.DoesNotExist:
+        api_super_user = Permission.objects.create(codename="api_super_user")
+    realm_content_type = ContentType.objects.get(app_label="zerver",
+                                                 model="realm")
+
+    # assign_perm isn't usable inside a migration, so we just directly
+    # create the UserObjectPermission objects
+    UserObjectPermission = apps.get_model('guardian', 'UserObjectPermission')
+    for user_profile in UserProfile.objects.filter(is_realm_admin=True):
+        UserObjectPermission(permission=administer,
+                             user_id=user_profile.id,
+                             object_pk=user_profile.realm_id,
+                             content_type=realm_content_type).save()
+    for user_profile in UserProfile.objects.filter(is_api_super_user=True):
+        UserObjectPermission(permission=api_super_user,
+                             user_id=user_profile.id,
+                             object_pk=user_profile.realm_id,
+                             content_type=realm_content_type).save()
+
+# Migrate all the guardian data for which users are realm admins or
+# API super users to the new fields on the UserProfile model
+def migrate_guardian_data(apps, schema_editor):
+    Permission = apps.get_model('auth', 'Permission')
+    ContentType = apps.get_model('contenttypes', 'ContentType')
+    UserProfile = apps.get_model('zerver', 'UserProfile')
+
+    try:
+        administer_id = Permission.objects.get(codename="administer").id
+    except Permission.DoesNotExist:
+        administer_id = None
+    try:
+        api_super_user_id = Permission.objects.get(codename="api_super_user").id
+    except Permission.DoesNotExist:
+        api_super_user_id = None
+
+    # If ContentType hasn't been initialized yet, we have a new, clean
+    # database and the below is not needed
+    if ContentType.objects.count() == 0:
+        return
+
+    realm_content_type = ContentType.objects.get(app_label="zerver",
+                                                 model="realm")
+
+    cursor = connection.cursor()
+    cursor.execute("SELECT id, object_pk, content_type_id, permission_id, user_id FROM guardian_userobjectpermission")
+    for row in cursor.fetchall():
+        (row_id, object_pk, content_type_id, permission_id, user_id) = row
+        if content_type_id != realm_content_type.id:
+            raise Exception("Unexected non-realm content type")
+        user_profile = UserProfile.objects.get(id=user_id)
+        if permission_id == administer_id:
+            user_profile.is_realm_admin = True
+        elif permission_id == api_super_user_id:
+            user_profile.is_api_super_user = True
+        else:
+            raise Exception("Unexpected Django permission")
+        user_profile.save()
+    # Set the email gateway bot as an API super user so we can clean
+    # up the old API_SUPER_USERS hack.
+    if settings.EMAIL_GATEWAY_BOT is not None:
+        try:
+            email_gateway_bot = UserProfile.objects.get(email__iexact=settings.EMAIL_GATEWAY_BOT)
+            email_gateway_bot.is_api_super_user = True
+            email_gateway_bot.save()
+        except UserProfile.DoesNotExist:
+            pass
+
+    # Delete the old permissions data in the Guardian tables; this
+    # makes the reverse-migration work safely (otherwise we'd have to
+    # worry about the migrate/unmigrate process racing with a user's
+    # admin permissions being revoked).
+    cursor.execute("DELETE FROM guardian_userobjectpermission")
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('zerver', '0010_delete_streamcolor'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='userprofile',
+            name='is_api_super_user',
+            field=models.BooleanField(default=False, db_index=True),
+        ),
+        migrations.AddField(
+            model_name='userprofile',
+            name='is_realm_admin',
+            field=models.BooleanField(default=False, db_index=True),
+        ),
+        migrations.RunPython(migrate_guardian_data, unmigrate_guardian_data),
+    ]

zerver/models.py

@@ -19,7 +19,6 @@ from django.utils import timezone
 from django.contrib.sessions.models import Session
 from zerver.lib.timestamp import datetime_to_timestamp
 from django.db.models.signals import pre_save, post_save, post_delete
-from guardian.shortcuts import get_users_with_perms
 import zlib
 
 from bitfield import BitField
@@ -149,9 +148,8 @@ class Realm(models.Model):
         self._deployments = [value] # type: Any
 
     def get_admin_users(self):
-        # This method is kind of expensive, due to our complex permissions model.
+        return UserProfile.objects.filter(realm=self, is_realm_admin=True,
-        candidates = get_users_with_perms(self, only_with_perms=['administer'])
+                                          is_active=True).select_related()
-        return candidates
 
     def get_active_users(self):
         return UserProfile.objects.filter(realm=self, is_active=True).select_related()
@@ -295,7 +293,9 @@ class UserProfile(AbstractBaseUser, PermissionsMixin):
     email = models.EmailField(blank=False, db_index=True, unique=True)
     is_staff = models.BooleanField(default=False)
     is_active = models.BooleanField(default=True, db_index=True)
+    is_realm_admin = models.BooleanField(default=False, db_index=True)
     is_bot = models.BooleanField(default=False, db_index=True)
+    is_api_super_user = models.BooleanField(default=False, db_index=True)
     date_joined = models.DateTimeField(default=timezone.now)
     is_mirror_dummy = models.BooleanField(default=False)
     bot_owner = models.ForeignKey('self', null=True, on_delete=models.SET_NULL)
@@ -393,19 +393,11 @@ class UserProfile(AbstractBaseUser, PermissionsMixin):
         """Returns whether this user has permission to modify target_user"""
         if target_user.bot_owner == self:
             return True
-        elif self.has_perm('administer', target_user.realm):
+        elif self.is_realm_admin and self.realm == target_user.realm:
             return True
         else:
             return False
 
-    def is_admin(self):
-        return self.has_perm('administer', self.realm)
-
-    def is_api_super_user(self):
-        # TODO: Remove API_SUPER_USERS hack; fixing this will require
-        # setting the email bot as a super user in the provision process.
-        return self.has_perm('api_super_user', self.realm) or self.email in settings.API_SUPER_USERS
-
     def last_reminder_tzaware(self):
         if self.last_reminder is not None and timezone.is_naive(self.last_reminder):
             logging.warning("Loaded a user_profile.last_reminder for user %s that's not tz-aware: %s"

zerver/views/__init__.py

@@ -543,8 +543,8 @@ def login_page(request, **kwargs):
         MAX_DEV_BACKEND_USERS = 100
         users_query = UserProfile.objects.select_related().filter(is_bot=False, is_active=True)
         users = users_query.order_by('email')[0:MAX_DEV_BACKEND_USERS]
-        extra_context['direct_admins'] = [u.email for u in users if u.is_admin()]
+        extra_context['direct_admins'] = [u.email for u in users if u.is_realm_admin]
-        extra_context['direct_users'] = [u.email for u in users if not u.is_admin()]
+        extra_context['direct_users'] = [u.email for u in users if not u.is_realm_admin]
     template_response = django_login_page(
         request, authentication_form=OurAuthenticationForm,
         extra_context=extra_context, **kwargs)
@@ -830,7 +830,7 @@ def home(request):
         alert_words           = register_ret['alert_words'],
         muted_topics          = register_ret['muted_topics'],
         realm_filters         = register_ret['realm_filters'],
-        is_admin              = user_profile.is_admin(),
+        is_admin              = user_profile.is_realm_admin,
         can_create_streams    = user_profile.can_create_streams(),
         name_changes_disabled = name_changes_disabled(user_profile.realm),
         has_mobile_devices    = num_push_devices_for_user(user_profile) > 0,
@@ -859,7 +859,7 @@ def home(request):
     show_invites = True
 
     # Some realms only allow admins to invite users
-    if user_profile.realm.invite_by_admins_only and not user_profile.is_admin():
+    if user_profile.realm.invite_by_admins_only and not user_profile.is_realm_admin:
         show_invites = False
 
     product_name = "Zulip"
@@ -874,7 +874,7 @@ def home(request):
                                        settings.DEBUG and ('show_debug' in request.GET),
                                    'pipeline': settings.PIPELINE,
                                    'show_invites': show_invites,
-                                   'is_admin': user_profile.is_admin(),
+                                   'is_admin': user_profile.is_realm_admin,
                                    'show_webathena': user_profile.realm.domain == "mit.edu",
                                    'enable_feedback': settings.ENABLE_FEEDBACK,
                                    'embedded': narrow_stream is not None,

zerver/views/messages.py

@@ -708,7 +708,7 @@ def send_message_backend(request, user_profile,
                          local_id = REQ(default=None),
                          queue_id = REQ(default=None)):
     client = request.client
-    is_super_user = request.user.is_api_super_user()
+    is_super_user = request.user.is_api_super_user
     if forged and not is_super_user:
         return json_error("User not authorized for this query")
 

zerver/views/streams.py

@@ -184,7 +184,7 @@ def remove_subscriptions_backend(request, user_profile,
 
     removing_someone_else = principals and \
         set(principals) != set((user_profile.email,))
-    if removing_someone_else and not user_profile.is_admin():
+    if removing_someone_else and not user_profile.is_realm_admin:
         # You can only unsubscribe other people from a stream if you are a realm
         # admin.
         return json_error("This action requires administrative rights")

zilencer/management/commands/populate_db.py

@@ -209,6 +209,11 @@ class Command(BaseCommand):
             create_users(realms, zulip_realm_bots, bot=True)
 
             if not options["test_suite"]:
+                # Initialize the email gateway bot as an API Super User
+                email_gateway_bot = UserProfile.objects.get(email__iexact=settings.EMAIL_GATEWAY_BOT)
+                email_gateway_bot.is_api_super_user = True
+                email_gateway_bot.save()
+
                 # To keep the messages.json fixtures file for the test
                 # suite fast, don't add these users and subscriptions
                 # when running populate_db for the test suite

zproject/settings.py

@@ -144,7 +144,6 @@ DEFAULT_SETTINGS = {'TWITTER_CONSUMER_KEY': '',
                     'INITIAL_PASSWORD_SALT': None,
                     'FEEDBACK_BOT': 'feedback@zulip.com',
                     'FEEDBACK_BOT_NAME': 'Zulip Feedback Bot',
-                    'API_SUPER_USERS': set(),
                     'ADMINS': '',
                     'INLINE_IMAGE_PREVIEW': True,
                     'CAMO_URI': '',
@@ -466,8 +465,6 @@ for bot in INTERNAL_BOTS:
         bot_email = bot['email_template'] % (INTERNAL_BOT_DOMAIN,)
         vars()[bot['var_name'] ] = bot_email
 
-if EMAIL_GATEWAY_BOT not in API_SUPER_USERS:
-    API_SUPER_USERS.add(EMAIL_GATEWAY_BOT)
 if EMAIL_GATEWAY_PATTERN != "":
     EMAIL_GATEWAY_EXAMPLE = EMAIL_GATEWAY_PATTERN % ("support+abcdefg",)
 
@@ -916,7 +913,6 @@ if (len(AUTHENTICATION_BACKENDS) == 1 and
 else:
     HOME_NOT_LOGGED_IN = '/login'
     ONLY_SSO = False
-AUTHENTICATION_BACKENDS += ('guardian.backends.ObjectPermissionBackend',)
 AUTHENTICATION_BACKENDS += ('zproject.backends.ZulipDummyBackend',)
 
 POPULATE_PROFILE_VIA_LDAP = bool(AUTH_LDAP_SERVER_URI)