Merge from master

auslib/admin/base.py

@@ -31,6 +31,13 @@ def isa(error):
     log.debug("Request headers are: %s", request.headers)
     return error
 
+# bug 887790: add necessary security headers
+@app.after_request
+def add_security_headers(response):
+    response.headers['X-Frame-Options'] = 'DENY'
+    response.headers['X-Content-Type-Options'] = 'nosniff'
+    return response
+
 app.add_url_rule('/csrf_token', view_func=CSRFView.as_view('csrf'))
 app.add_url_rule('/users', view_func=UsersView.as_view('users'))
 app.add_url_rule('/users/<username>/permissions', view_func=PermissionsView.as_view('permissions'))

auslib/admin/views/releases.py

@@ -4,7 +4,7 @@
 
 from auslib.blob import createBlob, CURRENT_SCHEMA_VERSION
 from auslib.db import OutdatedDataError
-from auslib.log import cef_event, CEF_WARN
+from auslib.log import cef_event, CEF_WARN, CEF_ALERT
 from auslib.util import getPagination
 from auslib.admin.base import db
 from auslib.admin.views.base import (
@@ -175,7 +175,7 @@ def get(self, release, platform, locale):
         return Response(response=json.dumps(locale), mimetype='application/json', headers=headers)
 
     @requirelogin
-    @requirepermission('/releases/:name/builds/:platform/:locale', options=[])
+    @requirepermission('/releases/:name/builds/:platform/:locale')
     def _put(self, release, platform, locale, changed_by, transaction):
         """Something important to note about this method is that using the
            "copyTo" field of the form, updates can be made to more than just
@@ -221,7 +221,7 @@ def get(self, release):
         return Response(response=render_template('fragments/release_row.html', row=release[0]), headers=headers)
 
     @requirelogin
-    @requirepermission('/releases/:name', options=[])
+    @requirepermission('/releases/:name')
     def _put(self, release, changed_by, transaction):
         form = NewReleaseForm()
         if not form.validate():
@@ -309,7 +309,6 @@ def get(self, release):
         )
 
     @requirelogin
-    @requirepermission('/releases', options=[])
     def _post(self, release, transaction, changed_by):
         change_id = request.form.get('change_id')
         if not change_id:
@@ -324,6 +323,10 @@ def _post(self, release, transaction, changed_by):
         if releases is None:
             return Response(status=404, response='bad release')
         release = releases[0]
+        if not db.permissions.hasUrlPermission(changed_by, '/releases/:name', 'POST', urlOptions={'product': release['product']}):
+            msg = "%s is not allowed to alter %s releases" % (changed_by, release['product'])
+            cef_event('Unauthorized access attempt', CEF_ALERT, msg=msg)
+            return Response(status=401, response=msg)
         old_data_version = release['data_version']
 
         # now we're going to make a new update based on this change

auslib/admin/views/rules.py

@@ -6,8 +6,9 @@
 from auslib.admin.views.base import (
     requirelogin, requirepermission, AdminView, HistoryAdminView
 )
+from auslib.admin.views.csrf import get_csrf_headers
 from auslib.admin.views.forms import EditRuleForm, RuleForm
-from auslib.log import cef_event, CEF_WARN
+from auslib.log import cef_event, CEF_WARN, CEF_ALERT
 from auslib.util import getPagination
 
 class RulesPageView(AdminView):
@@ -54,7 +55,7 @@ class RulesAPIView(AdminView):
     """/rules"""
     # changed_by is available via the requirelogin decorator
     @requirelogin
-    @requirepermission('/rules', options=[])
+    @requirepermission('/rules')
     def _post(self, transaction, changed_by):
         # a Post here creates a new rule
         form = RuleForm()
@@ -116,17 +117,25 @@ def get(self, rule_id):
                 releaseNames]
         form.mapping.choices.insert(0, ('', 'NULL' ) )
 
-        return render_template('fragments/single_rule.html', rule=rule, form=form)
+        headers = {'X-Data-Version': rule['data_version']}
+        headers.update(get_csrf_headers())
+        return Response(response=render_template('fragments/single_rule.html', rule=rule, form=form), mimetype='text/html', headers=headers)
 
     # changed_by is available via the requirelogin decorator
     @requirelogin
-    @requirepermission('/rules/:id', options=[])
     def _post(self, rule_id, transaction, changed_by):
         # Verify that the rule_id exists.
-        if not db.rules.getRuleById(rule_id, transaction=transaction):
+        rule = db.rules.getRuleById(rule_id, transaction=transaction)
+        if not rule:
             return Response(status=404)
         form = EditRuleForm()
 
+        # Verify that the user has permission for the existing rule _and_ what the rule would become.
+        for product in (rule['product'], form['product']):
+            if not db.permissions.hasUrlPermission(changed_by, '/rules/:id', 'POST', urlOptions={'product': product}):
+                msg = "%s is not allowed to alter rules that affect %s" % (changed_by, product)
+                cef_event('Unauthorized access attempt', CEF_ALERT, msg=msg)
+                return Response(status=401, response=msg)
         releaseNames = db.releases.getReleaseNames()
 
         form.mapping.choices = [(item['name'],item['name']) for item in releaseNames]
@@ -231,7 +240,6 @@ def get(self, rule_id):
         )
 
     @requirelogin
-    @requirepermission('/rules', options=[])
     def _post(self, rule_id, transaction, changed_by):
         rule_id = int(rule_id)
 
@@ -247,6 +255,12 @@ def _post(self, rule_id, transaction, changed_by):
         rule = db.rules.getRuleById(rule_id=rule_id)
         if rule is None:
             return Response(status=404, response='bad rule_id')
+        # Verify that the user has permission for the existing rule _and_ what the rule would become.
+        for product in (rule['product'], change['product']):
+            if not db.permissions.hasUrlPermission(changed_by, '/rules/:id', 'POST', urlOptions={'product': product}):
+                msg = "%s is not allowed to alter rules that affect %s" % (changed_by, product)
+                cef_event('Unauthorized access attempt', CEF_ALERT, msg=msg)
+                return Response(status=401, response=msg)
         old_data_version = rule['data_version']
 
         # now we're going to make a new insert based on this

auslib/db.py

@@ -1047,15 +1047,32 @@ def hasUrlPermission(self, username, url, method, urlOptions={}, transaction=Non
         # so we can put them in the same loop
         allOptions = urlOptions.copy()
         allOptions['method'] = method
-        ret = True
+
         for opt in allOptions:
-            allowedOpts = options.get(opt, None)
-            # When a permission doesn't have the option specified, we treat that
-            # it as allowed. When it does have it specified, the incoming option
-            # must match the one in the permission.
-            if allowedOpts and allOptions[opt] not in allowedOpts:
-                ret = False
-        return ret
+            allowedOpt = options.get(opt, None)
+            incomingOpt = allOptions[opt]
+
+            # If no option is specified in the request and the user has
+            # restrictions on what they can modify, they are denied.
+            # An example of this could be trying to modify a rule that doesn't
+            # have a product specified. Changing it could affect more products
+            # than the user is allowed to modify.
+            if not incomingOpt and allowedOpt:
+                return False
+
+            # If the request has this option specified, the user has
+            # restrictions for this option, and they don't match - they are denied.
+            # An example of this could be a user who only has permissions for
+            # the "Firefox" product trying to modify a "Thunderbird" release.
+            if incomingOpt and allowedOpt and incomingOpt != allowedOpt:
+                return False
+
+        # Any other combinations of incoming options/user restrictions are acceptable.
+        # Could be any of the following:
+        # * No incoming option nor user restriction.
+        # * Incoming option specified, user has no restrictions.
+        # * Incoming option specified, user has a restriction, and they match.
+        return True
 
 
 def getHumanModificationMonitors(systemAccounts):

auslib/test/admin/views/base.py

@@ -30,6 +30,7 @@ def setUp(self):
         db.permissions.t.insert().execute(permission='admin', username='bill', data_version=1)
         db.permissions.t.insert().execute(permission='/users/:id/permissions/:permission', username='bob', data_version=1)
         db.permissions.t.insert().execute(permission='/releases/:name', username='bob', options=json.dumps(dict(product='fake')), data_version=1)
+        db.permissions.t.insert().execute(permission='/rules/:id', username='bob', options=json.dumps(dict(product='fake')), data_version=1)
         db.releases.t.insert().execute(name='a', product='a', version='a', data=json.dumps(dict(name='a', schema_version=1)), data_version=1)
         db.releases.t.insert().execute(name='ab', product='a', version='a', data=json.dumps(dict(name='ab', schema_version=1)), data_version=1)
         db.releases.t.insert().execute(name='b', product='b', version='b', data=json.dumps(dict(name='b', schema_version=1)), data_version=1)
@@ -54,7 +55,7 @@ def setUp(self):
         db.rules.t.insert().execute(id=1, priority=100, version='3.5', buildTarget='d', backgroundRate=100, mapping='c', update_type='minor', data_version=1)
         db.rules.t.insert().execute(id=2, priority=100, version='3.3', buildTarget='d', backgroundRate=100, mapping='b', update_type='minor', data_version=1)
         db.rules.t.insert().execute(id=3, priority=100, version='3.5', buildTarget='a', backgroundRate=100, mapping='a', update_type='minor', data_version=1)
-        db.rules.t.insert().execute(id=4, priority=80, buildTarget='d', backgroundRate=100, mapping='a', update_type='minor', data_version=1)
+        db.rules.t.insert().execute(id=4, product='fake', priority=80, buildTarget='d', backgroundRate=100, mapping='a', update_type='minor', data_version=1)
         db.rules.t.insert().execute(id=5, priority=80, buildTarget='d', version='3.3', backgroundRate=0, mapping='c', update_type='minor', data_version=1)
         self.client = app.test_client()
 

auslib/test/admin/views/test_index.py

@@ -6,6 +6,10 @@
 
 
 class TestIndexPage(ViewTest):
+    def testSecurityHeaders(self):
+        ret = self.client.get('/')
+        self.assertEquals(ret.headers['X-Frame-Options'], 'DENY')
+        self.assertEquals(ret.headers['X-Content-Type-Options'], 'nosniff')
 
     def testLandingPage(self):
         ret = self.client.get('/')

auslib/test/admin/views/test_releases.py

@@ -84,6 +84,11 @@ def testLocalePut(self):
 }
 """))
 
+    def testLocalePutWithoutPermissionForProduct(self):
+        data = json.dumps(dict(complete=dict(filesize='435')))
+        ret = self._put('/releases/a/builds/p/l', username='bob', data=dict(data=data, product='a', version='a', data_version=1))
+        self.assertStatusCode(ret, 401)
+
     def testLocalePutForNewRelease(self):
         data = json.dumps(dict(complete=dict(filesize='678')))
         # setting schema_version in the incoming blob is a hack for testing

auslib/test/admin/views/test_rules.py

@@ -17,7 +17,9 @@ def testNewRulePost(self):
 
     # A POST without the required fields shouldn't be valid
     def testMissingFields(self):
-        ret = self._post('/rules', data=dict( ))
+        # But we still need to pass product, because permission checking
+        # is done before what we're testing
+        ret = self._post('/rules', data=dict({'product': 'a'}))
         self.assertEquals(ret.status_code, 400, "Status Code: %d, Data: %s" % (ret.status_code, ret.data))
         self.assertTrue('backgroundRate' in  ret.data, msg=ret.data)
         self.assertTrue('priority' in  ret.data, msg=ret.data)
@@ -46,12 +48,22 @@ def testPost(self):
     def testBadAuthPost(self):
         ret = self._badAuthPost('/rules/1', data=dict(backgroundRate=100, mapping='c', priority=100, data_version=1))
         self.assertEquals(ret.status_code, 401, "Status Code: %d, Data: %s" % (ret.status_code, ret.data))
-        self.assertTrue("not allowed to access" in ret.data, msg=ret.data)
+        self.assertTrue("not allowed to alter" in ret.data, msg=ret.data)
+
+    def testNoPermissionToAlterExistingProduct(self):
+        ret = self._post('/rules/1', data=dict(backgroundRate=71, data_version=1), username='bob')
+        self.assertEquals(ret.status_code, 401)
+
+    def testNoPermissionToAlterNewProduct(self):
+        ret = self._post('/rules/4', data=dict(product='protected', mapping='a', backgroundRate=71, priority=50, update_type='minor', data_version=1), username='bob')
+        self.assertEquals(ret.status_code, 401)
 
     def testGetSingleRule(self):
         ret = self._get('/rules/1')
         self.assertEquals(ret.status_code, 200)
         self.assertTrue("c" in ret.data, msg=ret.data)
+        for h in ("X-CSRF-Token", "X-Data-Version"):
+            self.assertTrue(h in ret.headers, msg=ret.headers)
 
 
 class TestRulesView_HTML(ViewTest, HTMLTestMixin):
@@ -194,6 +206,46 @@ def testPostRevisionRollback(self):
         self.assertEqual(row['distVersion'], '19')
         self.assertEqual(row['buildTarget'], 'MAC')
 
+    def testRollbackWithoutPermission(self):
+        ret = self._post(
+            '/rules/1',
+            data=dict(
+                backgroundRate=71,
+                mapping='d',
+                priority=73,
+                data_version=1,
+                product='',
+                update_type='minor',
+                channel='nightly',
+                build_id='1234',
+                os_version='10.5',
+                header_arch='INTEL',
+                dist_version='19',
+                build_target='MAC',
+            )
+        )
+        ret = self._post(
+            '/rules/1',
+            data=dict(
+                backgroundRate=72,
+                mapping='d',
+                priority=73,
+                product='',
+                data_version=2,
+                update_type='minor',
+                channel='nightly',
+            )
+        )
+        row, = db.rules.history.select(
+            where=[db.rules.history.backgroundRate == 72],
+            limit=1
+        )
+        change_id = row['change_id']
+
+        url = '/rules/1/revisions/'
+        ret = self._post(url, {'change_id': change_id}, username='bob')
+        self.assertEquals(ret.status_code, 401)
+
     def testPostRevisionRollbackBadRequests(self):
         # when posting you need both the rule_id and the change_id
         wrong_url = '/rules/999/revisions/'

auslib/test/test_db.py

@@ -1214,20 +1214,27 @@ def testGetOptionsPermissionDoesntExist(self):
     def testGetOptionsNoOptions(self):
         self.assertEquals(self.permissions.getOptions('cathy', '/rules'), {})
 
-    def testHasUrlPermission(self):
-        self.assertTrue(self.permissions.hasUrlPermission('cathy', '/rules', 'PUT', dict(product='fake')))
+    def testHasUrlPermissionAdmin(self):
+        self.assertTrue(self.permissions.hasUrlPermission('bill', '/rules', 'FOO'))
 
-    def testHasUrlPermissionWithOption(self):
-        self.assertTrue(self.permissions.hasUrlPermission('bob', '/rules/:id', 'POST', dict(product='fake')))
+    def testHasUrlPermissionGranular(self):
+        self.assertTrue(self.permissions.hasUrlPermission('cathy', '/rules', 'FOO'))
+
+    def testHasUrlPermissionWithDbOption(self):
+        self.assertTrue(self.permissions.hasUrlPermission('bob', '/rules/:id', 'POST'))
+
+    def testHasUrlPermissionWithUrlOption(self):
+        self.assertTrue(self.permissions.hasUrlPermission('bob', '/releases/:name', 'FOO', dict(product='fake')))
 
     def testHasUrlPermissionNotAllowed(self):
-        self.assertFalse(self.permissions.hasUrlPermission('cathy', '/rules/:id', 'DELETE', dict(product='fake')))
+        self.assertFalse(self.permissions.hasUrlPermission('cathy', '/rules/:id', 'FOO'))
+
+    def testHasUrlPermissionNotAllowedWithDbOption(self):
+        self.assertFalse(self.permissions.hasUrlPermission('bob', '/rules/:id', 'NOTPOST'))
 
-    def testHasUrlPermissionNotAllowedWithOption(self):
-        self.assertFalse(self.permissions.hasUrlPermission('bob', '/rules/:id', 'DELETE', dict(product='fake')))
+    def testHasUrlPermissionNotAllowedWithUrlOption(self):
+        self.assertFalse(self.permissions.hasUrlPermission('bob', '/releases/:name', 'FOO', dict(product='reallyfake')))
 
-    def testHasUrlPermissionWithProduct(self):
-        self.assertTrue(self.permissions.hasUrlPermission('bob', '/releases/:name', 'DELETE', dict(product='fake')))
 
 class TestDB(unittest.TestCase):
     def testSetDburiAlreadySetup(self):