Fix #923 - add allow_admin to user_has_privilege decorator

5 files changed, 35 insertions, 33 deletions

diff --git a/mediagoblin/db/models.py b/mediagoblin/db/models.py
index c2d101ac..c6424e71 100644
--- a/mediagoblin/db/models.py
+++ b/mediagoblin/db/models.py
@@ -106,25 +106,26 @@ class User(Base, UserMixin):
         super(User, self).delete(**kwargs)
         _log.info('Deleted user "{0}" account'.format(self.username))
 
-    def has_privilege(self,*priv_names):
+    def has_privilege(self, privilege, allow_admin=True):
         """
         This method checks to make sure a user has all the correct privileges
         to access a piece of content.
 
-        :param  priv_names      A variable number of unicode objects which rep-
-                                -resent the different privileges which may give
-                                the user access to this content. If you pass
-                                multiple arguments, the user will be granted
-                                access if they have ANY of the privileges
-                                passed.
+        :param  privilege       A unicode object which represent the different
+                                privileges which may give the user access to
+                                content.
+
+        :param  allow_admin     If this is set to True the then if the user is
+                                an admin, then this will always return True
+                                even if the user hasn't been given the
+                                privilege. (defaults to True)
         """
-        if len(priv_names) == 1:
-            priv = Privilege.query.filter(
-                Privilege.privilege_name==priv_names[0]).one()
-            return (priv in self.all_privileges)
-        elif len(priv_names) > 1:
-            return self.has_privilege(priv_names[0]) or \
-                self.has_privilege(*priv_names[1:])
+        priv = Privilege.query.filter_by(privilege_name=privilege).one()
+        if priv in self.all_privileges:
+            return True
+        elif allow_admin and self.has_privilege(u'admin', allow_admin=False):
+            return True
+
         return False
 
     def is_banned(self):
diff --git a/mediagoblin/decorators.py b/mediagoblin/decorators.py
index 90edf96b..5bf60048 100644
--- a/mediagoblin/decorators.py
+++ b/mediagoblin/decorators.py
@@ -74,7 +74,7 @@ def require_active_login(controller):
     return new_controller_func
 
 
-def user_has_privilege(privilege_name):
+def user_has_privilege(privilege_name, allow_admin=True):
     """
     Requires that a user have a particular privilege in order to access a page.
     In order to require that a user have multiple privileges, use this
@@ -85,14 +85,17 @@ def user_has_privilege(privilege_name):
                                         the privilege object. This object is
                                         the name of the privilege, as assigned
                                         in the Privilege.privilege_name column
+
+        :param allow_admin          If this is true then if the user is an admin
+                                    it will allow the user even if the user doesn't
+                                    have the privilage given in privilage_name.
     """
 
     def user_has_privilege_decorator(controller):
         @wraps(controller)
         @require_active_login
         def wrapper(request, *args, **kwargs):
-            user_id = request.user.id
-            if not request.user.has_privilege(privilege_name):
+            if not request.user.has_privilege(privilege_name, allow_admin):
                 raise Forbidden()
 
             return controller(request, *args, **kwargs)
@@ -369,7 +372,8 @@ def require_admin_or_moderator_login(controller):
     @wraps(controller)
     def new_controller_func(request, *args, **kwargs):
         if request.user and \
-            not request.user.has_privilege(u'admin',u'moderator'):
+            not (request.user.has_privilege(u'admin')
+                or request.user.has_privilege(u'moderator')):
 
             raise Forbidden()
         elif not request.user:
diff --git a/mediagoblin/templates/mediagoblin/base.html b/mediagoblin/templates/mediagoblin/base.html
index 63a2a6ff..13cfb47b 100644
--- a/mediagoblin/templates/mediagoblin/base.html
+++ b/mediagoblin/templates/mediagoblin/base.html
@@ -72,8 +72,8 @@
             </div>
             <div class="header_right">
               {%- if request.user %}
-                {% if request.user and 
-                      request.user.has_privilege('active') and 
+                {% if request.user and
+                      request.user.has_privilege('active') and
                       not request.user.is_banned() %}
 
                   {% set notification_count = get_notification_count(request.user.id) %}
@@ -158,7 +158,7 @@
                   {%- trans %}Create new collection{% endtrans -%}
                 </a>
                 {% template_hook("header_dropdown_buttons") %}
-                {% if request.user.has_privilege('admin','moderator') %}
+                {% if request.user.has_privilege('moderator') %}
                   <p>
                     <span class="dropdown_title">{% trans %}Moderation powers:{% endtrans %}</span>
                     <a href="{{ request.urlgen('mediagoblin.moderation.media_panel') }}">
diff --git a/mediagoblin/templates/mediagoblin/moderation/user.html b/mediagoblin/templates/mediagoblin/moderation/user.html
index 37e7eee9..594f845d 100644
--- a/mediagoblin/templates/mediagoblin/moderation/user.html
+++ b/mediagoblin/templates/mediagoblin/moderation/user.html
@@ -175,7 +175,7 @@
         {% for privilege in privileges %}
           <tr>
             <td>{{ privilege.privilege_name }}</td>
-            {% if privilege in user.all_privileges  %}
+            {% if user.has_privilege(privilege.privilege_name) %}
               <td class="user_with_privilege">
                 {% trans %}Yes{% endtrans %}{% else %}
               <td class="user_without_privilege">
@@ -183,7 +183,7 @@
             </td>
             {% if request.user.has_privilege('admin') %}
               <td>
-                {% if privilege in user.all_privileges  %}
+                {% if user.has_privilege(privilege.privilege_name) %}
                 <input type=submit id="{{ privilege.privilege_name }}"
                        class="submit_button button_action"
                        value =" -" />
diff --git a/mediagoblin/tests/test_modelmethods.py b/mediagoblin/tests/test_modelmethods.py
index ca436c76..32d5dce0 100644
--- a/mediagoblin/tests/test_modelmethods.py
+++ b/mediagoblin/tests/test_modelmethods.py
@@ -179,20 +179,17 @@ class TestUserHasPrivilege:
         self._setup()
 
         # then test out the user.has_privilege method for one privilege
-        assert not self.natalie_user.has_privilege(u'commenter')
-        assert self.aeva_user.has_privilege(u'active')
+        assert not self.aeva_user.has_privilege(u'admin')
+        assert self.natalie_user.has_privilege(u'active')
 
-
-    def test_user_has_privileges_multiple(self, test_app):
+    def test_allow_admin(self, test_app):
         self._setup()
 
-        # when multiple args are passed to has_privilege,  the method returns
-        # True if the user has ANY of the privileges
-        assert self.natalie_user.has_privilege(u'admin',u'commenter')
-        assert self.aeva_user.has_privilege(u'moderator',u'active')
-        assert not self.natalie_user.has_privilege(u'commenter',u'uploader')
-
+        # This should work because she is an admin.
+        assert self.natalie_user.has_privilege(u'commenter')
 
+        # Test that we can look this out ignoring that she's an admin
+        assert not self.natalie_user.has_privilege(u'commenter', allow_admin=False)
 
 def test_media_data_init(test_app):
     Session.rollback()