do not let admins/reviewers install premium apps for free from detail pages no matter how badly they want to (bug 748854)

apps/access/acl.py

@@ -33,18 +33,25 @@ def action_allowed(request, app, action):
 
 def action_allowed_user(user, app, action):
     """Similar to action_allowed, but takes user instead of request."""
-    allowed =  any(match_rules(group.rules, app, action) for group in
-                   user.groups.all())
+    allowed = any(match_rules(group.rules, app, action) for group in
+                  user.groups.all())
     return allowed
 
 
-def check_ownership(request, obj, require_owner=False, ignore_disabled=False):
+def check_ownership(request, obj, require_owner=False, require_author=False,
+                    ignore_disabled=False, admin=True):
     """
     A convenience function.  Check if request.user has permissions
     for the object.
     """
     if isinstance(obj, Addon):
+        # This checks if user has correct permissions but is NOT an admin.
+        if require_author:
+            require_owner = False
+            ignore_disabled = True
+            admin = False
         return check_addon_ownership(request, obj, viewer=not require_owner,
+                                     admin=admin,
                                      ignore_disabled=ignore_disabled)
     elif isinstance(obj, Collection):
         return check_collection_ownership(request, obj, require_owner)
@@ -69,7 +76,7 @@ def check_collection_ownership(request, collection, require_owner=False):
 
 
 def check_addon_ownership(request, addon, viewer=False, dev=False,
-                          support=False, ignore_disabled=False):
+                          support=False, admin=True, ignore_disabled=False):
     """
     Check request.amo_user's permissions for the addon.
 
@@ -86,7 +93,7 @@ def check_addon_ownership(request, addon, viewer=False, dev=False,
     if addon.is_deleted:
         return False
     # Users with 'Addons:Edit' can do anything.
-    if action_allowed(request, 'Addons', 'Edit'):
+    if admin and action_allowed(request, 'Addons', 'Edit'):
         return True
     # Only admins can edit admin-disabled addons.
     if addon.status == amo.STATUS_DISABLED and not ignore_disabled:

apps/access/helpers.py

@@ -6,9 +6,11 @@ import acl
 
 @register.function
 @jinja2.contextfunction
-def check_ownership(context, object, require_owner=False):
+def check_ownership(context, object, require_owner=False,
+                    require_author=False):
     return acl.check_ownership(context['request'], object,
-                               require_owner=require_owner)
+                               require_owner=require_owner,
+                               require_author=require_author)
 
 
 @register.function

apps/access/tests.py

@@ -11,7 +11,8 @@ from addons.models import Addon, AddonUser
 from cake.models import Session
 from users.models import UserProfile
 
-from .acl import match_rules, action_allowed, check_addon_ownership
+from .acl import (action_allowed, check_addon_ownership, check_ownership,
+                  match_rules)
 
 
 def test_match_rules():
@@ -117,6 +118,19 @@ class TestHasPerm(TestCase):
         self.request.amo_user = self.login_admin()
         self.request.groups = self.request.amo_user.groups.all()
         assert check_addon_ownership(self.request, self.addon)
+        assert check_addon_ownership(self.request, self.addon, admin=True)
+        assert not check_addon_ownership(self.request, self.addon, admin=False)
+
+    def test_require_author(self):
+        assert check_ownership(self.request, self.addon, require_author=True)
+
+    def test_require_author_when_admin(self):
+        self.request.amo_user = self.login_admin()
+        self.request.groups = self.request.amo_user.groups.all()
+        assert check_ownership(self.request, self.addon, require_author=False)
+
+        assert not check_ownership(self.request, self.addon,
+                                   require_author=True)
 
     def test_disabled(self):
         self.addon.update(status=amo.STATUS_DISABLED)

media/css/mkt/detail.less

@@ -55,6 +55,7 @@ h1 .num {
     .faked-purchase,
     .approval {
         .border-radius(0 0 5px 5px);
+        color: @black;
         display: block;
         margin-top: -5px;
         padding: 15px 15px 10px;
@@ -69,33 +70,21 @@ h1 .num {
             padding: 0 5px 0 30px;
         }
     }
-    .faked-purchase {
-        .border-radius(0 0 5px 5px);
+    .faked-purchase.vitals {
+        .border-radius(0 0 0 0);
         background: fadeOut(@red, 90%);
-        font-size: 12px;
+        font-size: 11px;
+        font-weight: 600;
+        float: none;
+        margin-left: 0;
         text-align: center;
-        b {
-            color: @maroon;
-            display: block;
-            text-transform: uppercase;
-        }
     }
     .approval {
         background: fadeOut(@green, 90%);
     }
     .faked-purchase + .approval {
-        margin-top: -15px;
-    }
-    .approval-pitch {
-        display: block;
-        color: @dark-gray;
-        font-size: 12px;
-        font-weight: bold;
-        padding-top: 8px;
-        text-align: center;
-        &:hover {
-            color: @medium-gray;
-        }
+        margin-top: -10px;
+        padding-top: 10px;
     }
 }
 
@@ -191,9 +180,7 @@ h1 .num {
             font-size: 14px;
             margin-top: 7px;
         }
-        .cats {
-            display: inline-block;
-            margin: 5px 0 5px;
+        .pricetag, .cats {
             &:after {
                 color: @note-gray;
                 content: "\00B7";
@@ -201,9 +188,15 @@ h1 .num {
                 padding-right: 3px;
             }
         }
-        .downloads {
+        .price {
+            text-shadow: 0 1px 0 @black;
+            &:after {
+                text-shadow: 0 1px 0 @white;
+            }
+        }
+        p {
             display: inline-block;
-            margin: 5px 0 0;
+            margin: 10px 0 5px;
         }
     }
     .visual {

mkt/detail/templates/detail/app.html

@@ -46,6 +46,11 @@
       </h1>
       <h2 class="authors">{{ users_list(product.listed_authors, size=3) }}</h2>
       {% set categories = product.all_categories %}
+      {% if is_dev and product.is_premium() and product.premium %}
+        <p class="pricetag faked-purchase">
+          <span class="price">{{ product.premium.get_price_locale() }}</span>
+        </p>
+      {% endif %}
       {% if categories %}
         <p class="cats">
           {% for category in categories %}

mkt/detail/templates/detail/protected_app.html

@@ -6,8 +6,7 @@
   {% endif %}
 {% endblock %}
 
-{% set is_dev = request.check_ownership(product, require_owner=False,
-                                        ignore_disabled=True) %}
+{% set is_dev = check_ownership(product, require_author=True) %}
 
 {# Reviewers should be able to see pending apps. #}
 {% if not is_admin and is_reviewer and product.is_pending() %}

mkt/detail/tests/test_views.py

@@ -117,7 +117,8 @@ class TestDetail(DetailBase):
         eq_(doc('.faked-purchase').length, 1)
         eq_(doc('.manage').length, 1)
 
-    def test_paid_public_install_button_for_reviewer(self):
+    def test_no_paid_public_install_button_for_reviewer(self):
+        # Too bad. Reviewers can review the app from the Reviewer Tools.
         self.make_premium(self.webapp)
         assert self.client.login(username='editor@mozilla.com',
                                  password='password')
@@ -126,14 +127,15 @@ class TestDetail(DetailBase):
         eq_(doc('.faked-purchase').length, 0)
         eq_(doc('.manage').length, 0)
 
-    def test_paid_pending_install_button_for_reviewer(self):
+    def test_no_paid_pending_install_button_for_reviewer(self):
+        # Too bad. Reviewers can review the app from the Reviewer Tools.
         self.webapp.update(status=amo.STATUS_PENDING)
         self.make_premium(self.webapp)
         assert self.client.login(username='editor@mozilla.com',
                                  password='password')
         doc = self.get_pq()
-        eq_(doc('.product.install.premium').length, 1)
-        eq_(doc('.faked-purchase').length, 1)
+        eq_(doc('.product.premium').length, 1)
+        eq_(doc('.faked-purchase').length, 0)
         eq_(doc('.manage').length, 0)
 
     def test_manage_button_for_owner(self):

mkt/site/helpers.py

@@ -32,10 +32,9 @@ def market_button(context, product):
         faked_purchase = False
         purchased = (request.amo_user and
                      product.pk in request.amo_user.purchase_ids())
-        is_dev = request.check_ownership(product, require_owner=False,
-                                         ignore_disabled=True)
-        if (not purchased and is_dev or product.is_pending() and
-            (context['is_reviewer'] or context['is_admin'])):
+        # App authors are able to install their apps free of charge.
+        if (not purchased and
+            request.check_ownership(product, require_author=True)):
             purchased = faked_purchase = True
         classes = ['button', 'product']
         label = product.get_price()

mkt/site/templates/site/helpers/webapp_button.html

@@ -4,9 +4,3 @@
     class="{{ classes }}">
         {{ label }}
 </a>
-{% if faked_purchase and product.is_premium() and product.premium %}
-  <p class="faked-purchase">
-    This would've cost you
-    <span class="price">{{ product.premium.get_price_locale() }}</span>
-  </p>
-{% endif %}