From 49799440a4f820a5316406f23349aa6b22e4637e Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Sun, 7 Feb 2016 18:59:38 -0800 Subject: [PATCH] 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 | 2 +- zerver/lib/actions.py | 25 ++-- .../commands/initialize_voyager_db.py | 5 + zerver/migrations/0011_remove_guardian.py | 110 ++++++++++++++++++ zerver/models.py | 18 +-- zerver/views/__init__.py | 10 +- zerver/views/messages.py | 2 +- zerver/views/streams.py | 2 +- zilencer/management/commands/populate_db.py | 5 + zproject/settings.py | 4 - 10 files changed, 147 insertions(+), 36 deletions(-) create mode 100644 zerver/migrations/0011_remove_guardian.py diff --git a/zerver/decorator.py b/zerver/decorator.py index f46c64f5a6..8a6791a76a 100644 --- a/zerver/decorator.py +++ b/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 diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 74b5a3cfc6..2653c0e382 100644 --- a/zerver/lib/actions.py +++ b/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 - forwarder_user_profile.is_api_super_user()): + elif sender.is_api_super_user or (forwarder_user_profile is not None and + 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'): - if is_admin: - assign_perm(permission, user_profile, user_profile.realm) +def do_change_is_admin(user_profile, value, permission='administer'): + if permission == "administer": + 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. diff --git a/zerver/management/commands/initialize_voyager_db.py b/zerver/management/commands/initialize_voyager_db.py index 7cfc61ac53..4e82ffcea8 100644 --- a/zerver/management/commands/initialize_voyager_db.py +++ b/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) diff --git a/zerver/migrations/0011_remove_guardian.py b/zerver/migrations/0011_remove_guardian.py new file mode 100644 index 0000000000..b0754e32f4 --- /dev/null +++ b/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), + ] diff --git a/zerver/models.py b/zerver/models.py index d0bf6e52c7..576297a474 100644 --- a/zerver/models.py +++ b/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. - candidates = get_users_with_perms(self, only_with_perms=['administer']) - return candidates + return UserProfile.objects.filter(realm=self, is_realm_admin=True, + is_active=True).select_related() 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" diff --git a/zerver/views/__init__.py b/zerver/views/__init__.py index 91c1c0d758..00a00ca6f8 100644 --- a/zerver/views/__init__.py +++ b/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_users'] = [u.email for u in users if not 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_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, diff --git a/zerver/views/messages.py b/zerver/views/messages.py index 0aaaef18df..7a7cd58898 100644 --- a/zerver/views/messages.py +++ b/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") diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 9bdcbdd15a..77e15f3610 100644 --- a/zerver/views/streams.py +++ b/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") diff --git a/zilencer/management/commands/populate_db.py b/zilencer/management/commands/populate_db.py index 1f73c5d449..fc6a55d084 100644 --- a/zilencer/management/commands/populate_db.py +++ b/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 diff --git a/zproject/settings.py b/zproject/settings.py index f25ecd0f16..697e28842f 100644 --- a/zproject/settings.py +++ b/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)