database: add authorisation checks to `UserCredentialsStore`
Created by: LawnGnome
Since this doesn't link back to repo
, we can't reuse AuthzQueryConds
. Instead, a new method is added that provides the user_credentials
equivalent based on the current context, and that is used in everything except Create
. (Create
has its own bespoke check entirely in Go, since we don't have a database query to append a WHERE
clause to at that point.)
At the same time, I migrated the UserCredentialsStore
tests to use assert
and require
, mostly because updating them was annoying me. They were verbose.
Otherwise, there's a small amount of unit test noise because we have a bunch of Batch Changes unit tests that were playing fast and loose with using background contexts, and now they need real actors attached. This is probably good.1
The one thing I do need feedback on is whether the authz.enforceForSiteAdmins
site setting should apply for user credentials, since it's currently documented as only affecting repositories. For now, I've implemented it (so, if it's enabled, site admins cannot access user credentials of other users), but it may be out of scope here.2
I've chosen not to remove the existing security checks from the GraphQL resolver. They're not doing any harm.
Fixes #15770.
Test plan
I've exercised the various user credential paths in the UI, and our test coverage is good here — we even had pre-existing permission tests for the relevant resolver, and they actually threw up an interesting quirk in the existing behaviour (that user credentials that the user doesn't have access to would return an empty response instead of a not found error3).
-
I did double check that the migrators do get invoked with an internal actor, since I had to update their tests. This was a case of the test suite being pretty lax, but the actual running code doing the right thing. ↩
-
Practically, I don't believe this decision really matters very much — there's no UI right now for site admins to actually do anything with user credentials owned by other users anyway. It would only affect a site admin using the GraphQL API directly. ↩
-
Lest anyone read this and start panicking about security implications, user credentials use sequential IDs, so knowing about the existence of an ID is hardly privileged information. There was no path to actually access the user credential. Nevertheless, closing this issue at the database level probably validates why we should have done this work in the first place.4 ↩
-
Yes, I'm aware I was the person who said it was unnecessary. I was wrong. All hail the sustaining sprint! ↩