Rename poorly named has_perm to mysteriously unused but well named method

apps/access/acl.py

@@ -30,9 +30,12 @@ def action_allowed(request, app, action):
 
 
 def check_ownership(request, obj, require_owner=False):
-    """Check if request.user has permissions for the object."""
+    """
+    A convenience function.  Check if request.user has permissions
+    for the object.
+    """
     if isinstance(obj, Addon):
-        return has_perm(request, obj, viewer=not require_owner)
+        return check_addon_ownership(request, obj, viewer=not require_owner)
     elif isinstance(obj, Collection):
         return check_collection_ownership(request, obj, require_owner)
     else:
@@ -53,22 +56,7 @@ def check_collection_ownership(request, collection, require_owner=False):
         return False
 
 
-def check_addon_ownership(request, addon, require_owner=False):
-    """Check if request.user has owner permissions for the add-on."""
-    if not request.user.is_authenticated():
-        return False
-    if action_allowed(request, 'Admin', 'EditAnyAddon'):
-        return True
-
-    roles = (amo.AUTHOR_ROLE_OWNER, amo.AUTHOR_ROLE_DEV)
-    if not require_owner:
-        roles += (amo.AUTHOR_ROLE_VIEWER,)
-
-    return bool(addon.authors.filter(addonuser__role__in=roles,
-                                     user=request.amo_user))
-
-
-def has_perm(request, addon, viewer=False, dev=False, ignore_disabled=False):
+def check_addon_ownership(request, addon, viewer=False, dev=False, ignore_disabled=False):
     """
     Check request.amo_user's permissions for the addon.
 

apps/access/tests.py

@@ -10,7 +10,7 @@ from addons.models import Addon, AddonUser
 from cake.models import Session
 from users.models import UserProfile
 
-from .acl import match_rules, action_allowed, has_perm
+from .acl import match_rules, action_allowed, check_addon_ownership
 
 
 def test_match_rules():
@@ -111,51 +111,51 @@ class TestHasPerm(TestCase):
     def test_anonymous(self):
         self.request.user.is_authenticated.return_value = False
         self.client.logout()
-        assert not has_perm(self.request, self.addon)
+        assert not check_addon_ownership(self.request, self.addon)
 
     def test_admin(self):
         self.request.amo_user = self.login_admin()
         self.request.groups = self.request.amo_user.groups.all()
-        assert has_perm(self.request, self.addon)
+        assert check_addon_ownership(self.request, self.addon)
 
     def test_disabled(self):
         self.addon.update(status=amo.STATUS_DISABLED)
-        assert not has_perm(self.request, self.addon)
+        assert not check_addon_ownership(self.request, self.addon)
         self.test_admin()
 
     def test_ignore_disabled(self):
         self.addon.update(status=amo.STATUS_DISABLED)
-        assert has_perm(self.request, self.addon, ignore_disabled=True)
+        assert check_addon_ownership(self.request, self.addon, ignore_disabled=True)
 
     def test_owner(self):
-        assert has_perm(self.request, self.addon)
+        assert check_addon_ownership(self.request, self.addon)
 
         self.au.role = amo.AUTHOR_ROLE_DEV
         self.au.save()
-        assert not has_perm(self.request, self.addon)
+        assert not check_addon_ownership(self.request, self.addon)
 
         self.au.role = amo.AUTHOR_ROLE_VIEWER
         self.au.save()
-        assert not has_perm(self.request, self.addon)
+        assert not check_addon_ownership(self.request, self.addon)
 
     def test_dev(self):
-        assert has_perm(self.request, self.addon, dev=True)
+        assert check_addon_ownership(self.request, self.addon, dev=True)
 
         self.au.role = amo.AUTHOR_ROLE_DEV
         self.au.save()
-        assert has_perm(self.request, self.addon, dev=True)
+        assert check_addon_ownership(self.request, self.addon, dev=True)
 
         self.au.role = amo.AUTHOR_ROLE_VIEWER
         self.au.save()
-        assert not has_perm(self.request, self.addon, dev=True)
+        assert not check_addon_ownership(self.request, self.addon, dev=True)
 
     def test_viewer(self):
-        assert has_perm(self.request, self.addon, viewer=True)
+        assert check_addon_ownership(self.request, self.addon, viewer=True)
 
         self.au.role = amo.AUTHOR_ROLE_DEV
         self.au.save()
-        assert has_perm(self.request, self.addon, viewer=True)
+        assert check_addon_ownership(self.request, self.addon, viewer=True)
 
         self.au.role = amo.AUTHOR_ROLE_VIEWER
         self.au.save()
-        assert has_perm(self.request, self.addon, viewer=True)
+        assert check_addon_ownership(self.request, self.addon, viewer=True)

apps/api/handlers.py

@@ -31,7 +31,7 @@ def check_addon_and_version(f):
             addon = Addon.objects.id_or_slug(addon_id).get()
         except:
             return rc.NOT_HERE
-        if not acl.has_perm(request, addon, viewer=True):
+        if not acl.check_addon_ownership(request, addon, viewer=True):
             return rc.FORBIDDEN
 
         if 'version_id' in kw:

apps/api/tests/test_oauth.py

@@ -571,7 +571,7 @@ class TestAddon(BaseOauth):
                        self.token, data={}, content_type=MULTIPART_CONTENT)
         eq_(r.status_code, 410, r.content)
 
-    @patch('access.acl.has_perm')
+    @patch('access.acl.check_addon_ownership')
     def test_not_my_addon(self, acl):
         data = self.create_addon()
         id = data['id']

apps/devhub/helpers.py

@@ -14,7 +14,7 @@ from access import acl
 from addons.helpers import new_context
 
 
-register.function(acl.has_perm)
+register.function(acl.check_addon_ownership)
 
 
 @register.inclusion_tag('devhub/addons/listing/items.html')

apps/devhub/templates/devhub/addons/listing/item_actions.html

@@ -1,14 +1,14 @@
 <h5>{{ _('Actions') }}</h5>
 <ul>
 {% if addon.is_incomplete() %}
-  {% if has_perm(request, addon, dev=True) %}
+  {% if check_addon_ownership(request, addon, dev=True) %}
     <li>
       <a href="{{ url('devhub.submit.resume', addon.slug) }}" class="tooltip"
          title="{{ _("Resume the submission process for this add-on.")}}">
       {{ _('Resume') }}</a>
     </li>
   {% endif %}
-  {% if has_perm(request, addon) %}
+  {% if check_addon_ownership(request, addon) %}
     <li>
       <a href="#" class="delete-addon tooltip"
          title="{{ _('Delete this add-on.') }}">{{ _('Delete') }}</a>
@@ -18,7 +18,7 @@
     </li>
   {% endif %}
 {% else %}
-  {% if has_perm(request, addon, dev=True) %}
+  {% if check_addon_ownership(request, addon, dev=True) %}
     <li>
       <a href="{{ url('devhub.addons.edit', addon.slug) }}" class="tooltip"
          title="{{ _("Edit details about this add-on's listing.") }}">

apps/devhub/templates/devhub/addons/owner.html

@@ -7,7 +7,7 @@
 
 {% block bodyclass %}
 {{ super() }}
-{% if not has_perm(request, addon) %} no-edit{% endif %}
+{% if not check_addon_ownership(request, addon) %} no-edit{% endif %}
 {% endblock %}
 
 {% block content %}

apps/devhub/templates/devhub/addons/payments.html

@@ -4,7 +4,7 @@
 {% block title %}{{ dev_page_title(title, addon) }}{% endblock %}
 
 {% set can_edit = (addon.status == amo.STATUS_PUBLIC
-                   and has_perm(request, addon)) %}
+                   and check_addon_ownership(request, addon)) %}
 {% block bodyclass %}
   {{ super() }}{% if not can_edit %} no-edit{% endif %}
 {% endblock %}

apps/devhub/templates/devhub/base.html

@@ -1,7 +1,7 @@
 {% extends "base.html" %}
 
 {% if addon %}
-  {% set editable = "no-edit" if not has_perm(request, addon, dev=True) %}
+  {% set editable = "no-edit" if not check_addon_ownership(request, addon, dev=True) %}
 {% endif %}
 {% block bodyclass %}developer-hub {{ editable }}{% endblock %}
 

apps/devhub/templates/devhub/versions/edit.html

@@ -58,7 +58,7 @@
                     {% endfor %}
                   </tbody>
                 </table>
-                {% if has_perm(request, addon, dev=True) %}
+                {% if check_addon_ownership(request, addon, dev=True) %}
                   <p class="add-app{% if not compat_form.extra_forms %} hide{% endif %}">
                     <a href="#">{{ _('Add Another Application&hellip;')|safe }}</a>
                   </p>

apps/devhub/templates/devhub/versions/list.html

@@ -50,7 +50,7 @@
       {% endif %}
 
       <div class="version-status-actions item-actions">
-        {% if has_perm(request, addon, dev=True) %}
+        {% if check_addon_ownership(request, addon, dev=True) %}
           {% set req = {amo.STATUS_PUBLIC: _('Request Full Review'),
                         amo.STATUS_LITE: _('Request Preliminary Review')} %}
           {% for status in addon.can_request_review() %}
@@ -70,7 +70,7 @@
             <a href="#" id="cancel-review">{{ _('Cancel Review Request') }}</a> &middot;
           {% endif %}
         {% endif %}
-        {% if has_perm(request, addon) %}
+        {% if check_addon_ownership(request, addon) %}
           {% if addon.disabled_by_user %}
             <a href="{{ url('devhub.addons.enable', addon.slug) }}" id="enable-addon">{{ _('Enable Add-on') }}</a> &middot;
           {% elif not addon.is_disabled %}

apps/devhub/views.py

@@ -67,11 +67,11 @@ def dev_required(owner_for_post=False, allow_editors=False):
                     return fun()
             # Require an owner or dev for POST requests.
             if request.method == 'POST':
-                if acl.has_perm(request, addon, dev=not owner_for_post):
+                if acl.check_addon_ownership(request, addon, dev=not owner_for_post):
                     return fun()
             # Ignore disabled so they can view their add-on.
-            elif acl.has_perm(request, addon, viewer=True,
-                              ignore_disabled=True):
+            elif acl.check_addon_ownership(request, addon, viewer=True,
+                                           ignore_disabled=True):
                 step = SubmitStep.objects.filter(addon=addon)
                 # Redirect to the submit flow if they're not done.
                 if not getattr(f, 'submitting', False) and step:
@@ -238,8 +238,8 @@ def feed(request, addon_id=None):
             rssurl = urlparams(reverse('devhub.feed', args=[addon_id]),
                                privaterss=key.key)
 
-            if not acl.has_perm(request, addons, viewer=True,
-                                ignore_disabled=True):
+            if not acl.check_addon_ownership(request, addons, viewer=True,
+                                             ignore_disabled=True):
                 return http.HttpResponseForbidden()
         else:
             rssurl = _get_rss_feed(request)

apps/reviews/views.py

@@ -60,7 +60,7 @@ def review_list(request, addon, review_id=None, user_id=None, template=None):
         ctx['review_perms'] = {
             'is_admin': acl.action_allowed(request, 'Admin', 'EditAnyAddon'),
             'is_editor': acl.action_allowed(request, 'Editor', '%'),
-            'is_author': acl.has_perm(request, addon, dev=True),
+            'is_author': acl.check_addon_ownership(request, addon, dev=True),
             'can_delete': acl.action_allowed(request, 'Editors',
                                              'DeleteReview'),
         }
@@ -124,7 +124,7 @@ def _review_details(request, addon, form):
 @login_required
 def reply(request, addon, review_id):
     is_admin = acl.action_allowed(request, 'Admin', 'EditAnyAddon')
-    is_author = acl.has_perm(request, addon, dev=True)
+    is_author = acl.check_addon_ownership(request, addon, dev=True)
     if not (is_admin or is_author):
         return http.HttpResponseForbidden()
 

apps/versions/views.py

@@ -64,7 +64,7 @@ def download_file(request, file_id, type=None):
     addon = get_object_or_404(Addon.objects, pk=file.version.addon_id)
 
     if addon.is_disabled or file.status == amo.STATUS_DISABLED:
-        if acl.has_perm(request, addon, viewer=True, ignore_disabled=True):
+        if acl.check_addon_ownership(request, addon, viewer=True, ignore_disabled=True):
             return HttpResponseSendFile(request, file.guarded_file_path,
                                         content_type='application/xp-install')
         else: