Move permission enforcement to database layer

[bhearsum]

This is in support of #63, but it's a big enough change (and well isolated) that it's probably safer and easier to do in a separate PR. Here are the highlights:

• Change permissions structure to be object/action based instead of URL based.
• Do all permissions in AUSTable subclasses.
• Pass in a reference to the database object, rather than relying on dbo (https://bugzilla.mozilla.org/show_bug.cgi?id=1261061 has some reasoning behind this)
• Fix API methods to return 403 for permission denied instead of 401 (per https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#4xx_Client_Error)
• Adjust test set-up to cope with db-level tests requiring permissions now.
• Minor changes to pytest to speed up and improve output (capturelog has been replaced with catchlog, xdist lets us parallelize).

You'll notice that few new tests were added here, merely adjusted existing ones. I took a TDD approach to this and changed all of the tests first, ensured they failed, and then fixed them with my changes to the non-test code.

Landing this will need to be two stage to avoid getting into a situation where permission checks fail errantly. My thinking is:

• Add the new permissions to allPermissions in a small patch, and deploy.
• Grant the new permissions to all the relevant accounts.
• Land and deploy this patch. At this point, the db level permission checking will be happening, and I can verify that everything looks okay there, no permissions are missing etc.
• Once everything looks good, remove all the old URL-style permissions from the db.

[nthomas-mozilla]

Did you consider 'release_locale' instead of 'build', and 'release_readonly' instead of 'read_only' ? I'm guessing that 'build' might be a bit opaque after some time away from the code. Or perhaps mitigate with some UI docs.

Are we removing only unused permissions, like /releases/:name/revisions ?

[bhearsum]

I like those names - I'll update the branch.

You're right that we're only removing unused permissions. We can always add them back later if we want (though one of the advantages of non-URL based permissions is that we can check the "release" permission instead of a history-specific one!).

[nthomas-mozilla]

This all looks good, although I didn't get out the superfine-tooth comb. Since it's hard to catch everything in refactoring like this I'd like to query the test coverage. Hopefully very close to complete but can coveralls help reveal any gaps ? If we cover all the lines do we test both permissions working and lack-of-perms not working ?

Nice tweaks to the test config, looking forward to that landing.

[bhearsum]

Good eye on the test coverage. Looks like we lost coveralls in the transition to Taskcluster (I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1276289 to get that fixed). I found a few places where we weren't verifying the new permission checking in the db and fixed those up. I think we're covering the negative and positive cases in all places now. Overall code coverage remains at 91%, and the individual files all have comparable coverage (auslib/admin/base.py and auslib/admin/view/base's percent went down, but that's because of the removal of dead code that had tests -- the untested lines remains the same).

[nthomas-mozilla]

I thought I'd commented about this but can't find that now. Does this just work because AUSTransaction.execute() uses rollback() in the try/except ? We may have to check all the places we use transactions.

[bhearsum]

Rereading this file makes me think that you're right, because these exception handlers happen after the context manager exits. It looks like I was confusing this with https://bugzilla.mozilla.org/show_bug.cgi?id=1246993 (which I just re-verified is still an issue). I'll remove this FIXME.

[bhearsum]

Further evidence that this is fine: I created a situation locally where a PermissionDeniedError was raised after an update was done, and I added a log message as part of AUSTransaction.rollback. Output was as follows:

balrogadmin_1 | [pid: 210|app: 0|req: 2/2] 172.17.42.1 () {46 vars in 805 bytes} [Tue May 31 12:51:36 2016] GET /api/csrf_token => generated 20 bytes in 2 msecs (HTTP/1.1 200) 8 headers in 432 bytes (1 switches on core 0)
balrogadmin_1 | 2016-05-31 12:51:36,302 - DEBUG - PID: 210 - Request: 140232923627344 - SingleReleaseView.put#55: processing PUT request to /releases/B2G-gecko-nightly-latest
balrogadmin_1 | 2016-05-31 12:51:36,304 - DEBUG - PID: 210 - Request: 140232923627344 - Releases.getReleases#940: Looking for releases with:
balrogadmin_1 | 2016-05-31 12:51:36,304 - DEBUG - PID: 210 - Request: 140232923627344 - Releases.getReleases#941: name: B2G-gecko-nightly-latest
balrogadmin_1 | 2016-05-31 12:51:36,304 - DEBUG - PID: 210 - Request: 140232923627344 - Releases.getReleases#942: product: None
balrogadmin_1 | 2016-05-31 12:51:36,308 - DEBUG - PID: 210 - Request: 140232923627344 - AUSTransaction.execute#108: Attempting to execute SELECT releases.`read_only` 
balrogadmin_1 | FROM releases 
balrogadmin_1 | WHERE releases.name = %s
balrogadmin_1 | 2016-05-31 12:51:36,309 - DEBUG - PID: 210 - Request: 140232923627344 - AUSTransaction.execute#108: Attempting to execute SELECT releases.product 
balrogadmin_1 | FROM releases 
balrogadmin_1 | WHERE releases.name = %s
balrogadmin_1 | 2016-05-31 12:51:36,309 - DEBUG - PID: 210 - Request: 140232923627344 - auslib.blobs.apprelease.ReleaseBlobV4.validate#70: Validating blob {u'platforms': {u'aries-userdebug': {u'locales': {u'en-US': {u'buildID': u'20151112022457', u'platformVersion': u'45.0a1', u'displayVersion': u'45.0a1', u'completes': [{u'fileUrl': u'https://queue.taskcluster.net/v1/task/IlV-0N-DQhC3PBkctAQtlg/runs/0/artifacts/public/build/b2g-aries-gecko-update.mar', u'from': u'*', u'hashValue': u'94b9b0564418d610853e3b1f422ef08d4efbdc9e5362d606f7617228b007c0aa74009cdc44f7dbf2c0a87c40e15d015a0cb47270a22151901bf3b0cc79e4e1d6', u'filesize': 131059753}], u'appVersion': u'45.0a1'}}}, u'flame-userdebug': {u'locales': {u'en-US': {u'isOSUpdate': True, u'buildID': u'20151110120524', u'appVersion': u'45.0a1', u'displayVersion': u'45.0a1', u'platformVersion': u'45.0a1', u'completes': [{u'fileUrl': u'https://queue.taskcluster.net/v1/task/V1HyfEHJRPG3va_W1MfIJQ/runs/0/artifacts/public/build/fota-flame-update.mar', u'from': u'*', u'hashValue': u'2685288aa4896685e4aa611d0c1be83a06641f585aa11e26a81444707bc8a6efdd65bebb2d6cf9f31a14e56839f416c1361fef2059f1a7ff33a0fd7974a7f634', u'filesize': 108017496}]}}}, u'niiiiiiexus': {u'locales': {u'en-US': {u'isOSUpdate': True, u'buildID': u'20151110113826', u'appVersion': u'44.0a2', u'displayVersion': u'44.0a2', u'platformVersion': u'44.0a2', u'completes': [{u'fileUrl': u'https://queue.taskcluster.net/v1/task/WBQBJAhDSTik2zkk_LY_2A/runs/0/artifacts/public/build/fota-hammerhead-update.mar', u'from': u'*', u'hashValue': u'8b796b1a6240f56e958b236b82a0cfff80aee3c604aae4a4cac7b7e733c7123ee719b7be2d6e855a6958a4ea6fd4306b8e79d322a3f9aba0c6723e01c21715d3', u'filesize': 101589914}]}}}, u'flame': {u'locales': {u'en-US': {u'isOSUpdate': True, u'buildID': u'20151110120640', u'appVersion': u'45.0a1', u'displayVersion': u'45.0a1', u'platformVersion': u'45.0a1', u'completes': [{u'fileUrl': u'https://queue.taskcluster.net/v1/task/U7LxpmZvTUqo2-UqMXYGRQ/runs/0/artifacts/public/build/fota-flame-update.mar', u'from': u'*', u'hashValue': u'1d67ea90413c3a8cc6017dda1210ed90afebbd4525678bd52f1f6c6900cd7c27cf04b16c648c9e4340fa688c00ba1f1a1092e803c8a29b441817fa1a70c709fb', u'filesize': 107923517}]}}}, u'aries': {u'locales': {u'en-US': {u'buildID': u'20151130145625', u'platformVersion': u'45.0a1', u'displayVersion': u'45.0a1', u'appVersion': u'45.0a1', u'completes': [{u'fileUrl': u'https://queue.taskcluster.net/v1/task/Do8_llXIRXCZ6uao3hfVvQ/runs/0/artifacts/public/build/b2g-aries-gecko-update.mar', u'from': u'*', u'hashValue': u'd094e5a7a912aac5fca107958aaf55924114902bc7a33f3cb3056752df760287ba3ee11582366e7880a50278ca4406ab7a1394bdf741e73fa5ba6a18f3b39e4a', u'filesize': 132127809}]}}}}, u'hashFunction': u'sha512', u'name': u'B2G-gecko-nightly-latest', u'schema_version': 4}
balrogadmin_1 | 2016-05-31 12:51:36,353 - DEBUG - PID: 210 - Request: 140232923627344 - AUSTransaction.execute#108: Attempting to execute SELECT releases.name, releases.product, releases.`read_only`, releases.data, releases.data_version 
balrogadmin_1 | FROM releases 
balrogadmin_1 | WHERE releases.name = %s
balrogadmin_1 | 2016-05-31 12:51:36,354 - DEBUG - PID: 210 - Request: 140232923627344 - AUSTransaction.execute#108: Attempting to execute UPDATE releases SET name=%s, product=%s, `read_only`=%s, data=%s, data_version=%s WHERE releases.name = %s AND releases.data_version = %s
balrogadmin_1 | 2016-05-31 12:51:36,355 - DEBUG - PID: 210 - Request: 140232923627344 - AUSTransaction.execute#108: Attempting to execute INSERT INTO releases_history (changed_by, timestamp, name, product, `read_only`, data, data_version) VALUES (%s, %s, %s, %s, %s, %s, %s)
balrogadmin_1 | 2016-05-31 12:51:36,356 - DEBUG - PID: 210 - Request: 140232923627344 - AUSTransaction.__exit__#87: exc is:
balrogadmin_1 | Traceback (most recent call last):
balrogadmin_1 |   File "./auslib/admin/views/base.py", line 57, in put
balrogadmin_1 |     return self._put(*args, transaction=trans, **kwargs)
balrogadmin_1 |   File "./auslib/admin/views/base.py", line 19, in decorated
balrogadmin_1 |     return f(*args, changed_by=username, **kwargs)
balrogadmin_1 |   File "./auslib/admin/views/releases.py", line 236, in _put
balrogadmin_1 |     old_data_version=data_version, transaction=transaction)
balrogadmin_1 |   File "./auslib/db.py", line 1093, in updateRelease
balrogadmin_1 |     raise PermissionDeniedError("wtf!")
balrogadmin_1 | PermissionDeniedError: wtf!
balrogadmin_1 | 2016-05-31 12:51:36,356 - DEBUG - PID: 210 - Request: 140232923627344 - AUSTransaction.rollback#131: Rolling back

...and a manual look at the release showed that its contents were unchanged.

[nthomas-mozilla]

This looks to be blocked by bob's permissions to modify products 'fake' and 'b' only. Could we have a test based on actions too ?

[nthomas-mozilla]

All of those comments can be fixed without me looking at this again, but I'd really like to hear about the rollback situation before signing off on this.

[nthomas-mozilla]

Please address the minor issues on landing, it's an r+ from me

[bhearsum]

Thanks Nick! I'll proceed with that. I may hold off on landing this depending on how comfortable I and others are landing it during a release week.

Also a reminder to myself: this needs to be a two-stage landing. Roughly:

• The new permission definitions must go to production first (without removing the old ones)
• Grant new permissions to necessary users (mostly system accounts)
• Merge this, push to prod
• Delete old permissions

[bhearsum]

I updated https://wiki.mozilla.org/Balrog#Permissions with docs on all the new permissions.

[bhearsum]

I did a full simulation of landing this locally, and discovered that this will need to be a three part landing instead of a two:

1. Add new permissions and a hack to make it possible to add them. Once this is in production we can add new permissions to all of the accounts. I landed this already as a5f96c9
2. Add the bulk of this patch, minus the bits that remove the old permissions. Once this is in production, we can remove the old permissions from the accounts.
3. Remove the last bits of the old permissions.

[bhearsum]

The rest of step 1 is done - I've granted all accounts new-style permissions.

[bhearsum]

Step 2 is mostly done, but unfortunately I removed the code that allowed us to remove URL style permissions...so I'm unable to remove those. I'll have to add that back to remove the old permissions before step 3.

[bhearsum]

The hack to let me remove old style permissions went to prod today, and I've included step 3 with #63, which just merged to master. All done here now.