Do a policy check at the resource level before any other callbacks

[chvp]

Expect for the auth callback, of course. This fixes a leak in our policy/resource handling, where a user could know whether a record with a certain ID existed based on the HTTP response.

A similar issue existed for logged-in users for other user's secret records (auth tokens and playlists, in practice). By policy scoping the find calls, authorized users will always get a 404 for records they don't have access to, whether that record exists or not.

• I've added tests relevant to my changes. (Well, I've mostly updated the existing tests, to be more clear.)

[chvp]

The coverage going down is because we don't check the scope.none case for a lot of the policies (in practice this is now mostly already handled by the new early resource check, but I think we should still keep it for defense-in-depth). We should probably introduce unit tests for our policies, but to me that doesn't feel like something for this PR.

[robbevp]

I'm not sure about this one, using check_resource_access feels a bit weird to me:

• I find it more natural if the public methods of policies correspond to the actions in the controller
• I think it solves the same problem as you do with policy_scope: I don't see a lot of cases where the user would get passed policy_scope (and the policy that matches the action) but would currently get stopped by check_resource_access

Maybe we should split this problem in two:

1. A user who is not logged in, should have no way to find out details about what's in the database
2. A user who is logged in, should not know about the ids of objects it can't see.

For the first problem, I think we might be better of with an early return of unauthorized. Rather than having to check this in each policy, we know that signing in is the only action that doesn't require a user. I guess either by:

• Updating our authenticate_user to require the user to be present or returning unauthorized (and then using skip_before_action for the sign in action)
• Adding something like require_user! and adding this as a before action (either per controller, or with skip_before_action).
• We could make our policies a closed system, and also make some exception for creating auth tokens

For the second problem (which I don't worry about as much, I'm not sure what I could do by knowing the id of a secret playlist someone else made 🤷 ), the policy_scope should be enough?

[chvp]

The problem with just using policy_scope is that the HTTP status unauthorized users get will still be a 404, which doesn't necessarily match the truth, since they might be able to view the resource if they were logged in.

In my view, this is essentially an early unauthorized return. It's just set up a bit more flexibly, allowing to also use that early unauthorized return to also deny regular users any access to moderator-only routes (like locations and cover filenames).

The closed system solution might also be good though (I think we can also make that work for cover filenames and locations by expanding that check a bit in a custom constructor in those policy classes.)

WDYT?

[chvp]

Reworked it to be a closed system, where the AuthTokensController#create action now doesn't use pundit.

[robbevp]

👍 Just noticed one method that should be removed

[robbevp]

👍