use pool_authorized() for all lookups #873
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davepacheco
commented
Apr 4, 2022
| } else { | ||
| // If the user did not authenticate successfully, this will | ||
| // become a 401 rather than a 403. | ||
| Err(if !is_authn { |
There was a problem hiding this comment.
This is a small but important change.
Prior to this, if you failed an authz check, the logic went like this:
- Pick a "fallback" error to signify "you didn't have permissions to do this". This would be 401 if you were unauthenticated and 403 if you're authenticated.
- Check with the resource impl (via the call to
on_unauthorizedat L109). The only interesting impl is this one, which is used for most API resources. Pass the fallback error, but return whatever the resource says to return.
The intent of the on_unauthorized call is to determine if you're allowed to even know this resource exists. If so, the "fallback error" (401 or 403) is fine. If not, return a 404 instead. The on_unauthorized impl I linked above is what actually does this.
After this change, the 401 always takes precedence. If you fail the authz check and you're not authenticated, you always get a 401 -- we don't invoke on_unauthorized() at all. If you're authenticated, then we do the same as before: 403 if you're allowed to read the resource, 404 otherwise.
This change isn't strictly necessary. I did this for consistency of the public API. If we just change lookups to use pool_authorized(), then all the endpoints that do a lookup behave this way without this change here. But the endpoints that don't do lookups for whatever reason (see #845) would still return 404s in some cases for unauthenticated users that fail an authz check. I thought it better to put this logic here for consistency. If you ever add a new endpoint (whether it hits the database or not), and the user fails an authz check, and they're not authenticated, they'll get a 401, instead of having it be dependent on how the endpoint was implemented.
|
In case it's helpful, here's the output for the "unauthorized" test showing the new behavior: and here's the diff -- unfortunately it's a column that changed, so nearly every line changed, and I'm not sure how to most usefully show that: The best I know how to do is show
|
This was referenced Apr 4, 2022
david-crespo
approved these changes
Apr 6, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recall that the
DataStorehas two methods for getting a database connection:pool()andpool_authorized(opctx: &OpContext). The latter checks whether the caller is even allowed to query the database. All authenticated users have this permission, so it's kind of silly. But this seemed useful as a belt-and-suspenders way to ensure that if we accidentally add some unauthenticated code path, an attacker can't use this to trivially DoS the database. We should eventually be usingpool_authorized()everywhere and rip outpool()altogether. Right now, lookups usepool(), only for historical reasons. It's time to switch these to usepool_authorized(opctx).This changes some failure modes for unauthenticated users! Previously, if an unauthenticated user tried to fetch, say,
/organizations/foo, they would have gotten a 404. Same if they tried to update or delete it. That's because we'd look up the resource, find they didn't have read access, and send a 404. To be clear, I think that behavior remains reasonable. But with this change, we're kicking them out before even doing the lookup. I think that's also reasonable. From some casual research, it seems that people often advocate for using 401 when there's any authz error and no credentials were provided, so we're consistent with expectations there. This is also what our own docs/http-status-codes.adoc says.This is especially relevant when we get to Silos. Once we do #850, it won't even be possible to do the lookup without credentials, so it makes sense to kick the request out quickly in that case -- and with a 401.