[DISCUSS] Remove duplicate authorisation check

[janl]

While working on #1245 I noticed fabric:get_security() is being called twice for each database request.

I tracked it down to chttpd_auth_request:do_authorization_check() and chttpd:do_db_request().

Removing the second one passes the test suite fine. I’d like a thorough review of this, because we might not be testing an invariant that this provides for good reason.

[davisp]

This looks fine to me. The only issue I see is that if someone wants to use a custom request authorization module they now have the burden of ensuring that fabric:get_security/2 is called. Although using a non-default authorization module means you're already in non-default behavior, and allowing for all authorization to happen in that configurable model actually seems more right than "Do all your own authorization! Except not really you also have to have this hard coded authorization check as well!"

[chewbranca]

I'm kind of -0.5 to this, at least without someone taking a very close look at this. The removed check - fabric:get_security(DbName, [{user_ctx,Ctx}]), % calls check_is_reader is the primary check for reader access. I forget why the duplicate call to fabric:get_security was added to the auth check modules, but it was a relatively new addition in 2.x, whereas the check in do_db_req has been the primary check for a long time.

That said, the duplication of fabric:get_security is a bit brutal given how much of a hog that request is; this would be a non issue if we had cassim caching these values. If we can legitimately remove the duplication then great, but I'm concerned as this is removing the primary db reader check that has been used for many years, so let's make sure we get it right.

[janl]

@chewbranca good feedback, thanks.

0. I agree that this should be reviewed VERY thoroughly, hence the controversially short PR, to solicit said review :)

1. I care less about which duplicate we remove and more about making sure we only do it once. I chose to keep the one higher up the chain so we can avoid doing any more work as soon as we know we don’t have an auth match.

2. I’d love to get cassim in. Did you have a chance to resolve the issues we found in the running up to 2.0?

[chewbranca]

@janl:

0. sounds good, 👍

1. agreed. In principle the check

   couchdb/src/chttpd/src/chttpd_auth_request.erl

   Lines 94 to 96 in 884db89

   	db_authorization_check(#httpd{path_parts=[DbName|_],user_ctx=Ctx}=Req) ->
   	{_} = fabric:get_security(DbName, [{user_ctx, Ctx}]),
   	Req.

   seems like it should do the right things and obviate the need for the check in chttp_db:do_db_Req, so this seems like the right approach, but we'll also want to keep in mind @davisp's point in [DISCUSS] Remove duplicate authorisation check #1246 (comment).

2. the blocker for cassim is the current security dichotomy where we have clustered level security in chttpd, and shard local security in couch_db.erl. We need to remove the shard local security model and then cassim will be rocking. There's been some discussion around this a few times, and the general consensus seems to be folks are on board with dropping the shard local security model. Now we just need to make it happen.