rsc: Add route to check for valid api key #1549
Conversation
| @@ -497,6 +504,36 @@ mod tests { | |||
|
|
|||
| assert_eq!(res.status(), StatusCode::BAD_REQUEST); | |||
|
|
|||
| // Authorization check with invalid auth should 401 | |||
There was a problem hiding this comment.
MDN suggests this often means "unauthenticated" in practice rather than a literal "unauthorized", and while there's definitely no reason not to follow the literal meaning since we're using a very specific client software, I might have gone with a 403 Forbidden as an unambiguous response.
There was a problem hiding this comment.
Yep, I agree that 401 means "unauthenticated" which imo is correct here since the point is to check if the provided auth key grants authentication :)
There was a problem hiding this comment.
Makes sense -- I was reading it as the auth key basically being the credentials themselves, in which case you've got valid authentication and just not authorization (i.e. this would be the gate for a restricted-push store), but sounds like my model of what's going on was slightly off.
|
|
||
| assert_eq!(res.status(), StatusCode::UNAUTHORIZED); | ||
|
|
||
| // Authorization check with valid auth should 200 |
There was a problem hiding this comment.
This one is probably the best code. There's potential in 204 No Content, but I'm not sure the connotations there exactly match up.
| # ``` | ||
| export def rscApiCheckAuthorization (api: RemoteCacheApi): Result Unit Error = | ||
| require Some auth = api.getRemoteCacheApiAuthorization | ||
| else failWithError "rsc: Checking authorization requires authorization but none was provided" |
There was a problem hiding this comment.
I don't know what sort of logic chokepoints you have closer to the surface and with this being export it might indeed be the best way to handle it,1 but don't forget that if everything ultimately funnels through a small number of narrow necks, you can collect all the rsc: prefixes through addErrorContext and save a bit of typing/typo risk.
Footnotes
-
Actually, why is it exported? Doesn't seem like anything which would be helpful for general audiences, but I don't know what sort of package organization you're planning. ↩
There was a problem hiding this comment.
why is it exported
I consider any HTTP web route as a public part of the RemoteCacheApi interface. Anyone with an instance of the apii should be able to call any function on the remote server.
Since POST auth/check is a function made available on the server, anyone should be able to call it.
If everything ultimately funnels through a small number of narrow necks, you can collect all the rsc: prefixes
I'll need you to give an example of what you mean, but maybe given the above comment this doesn't make sense anymore? The important thing to keep in mind is that this library is just a wrapper around what is available as the public interface on the server. What the consumer does with it (in this case remote_cache_runner) is entirely up to them
There was a problem hiding this comment.
Yeah, part 1 makes sense and with that clarification, part 2 is indeed invalid. I basically just meant that if you had a lot of potential errors which were not in directly-exported functions but instead were only accessible by going through a small number of entry points, you wouldn't have to add the rsc: prefix to each of the errors at the place you throw them, and could instead call addErrorContext "rsc" at those chokepoints, for what would likely be fewer times than you have errors.
|
|
||
| RemoteCacheApi domain port auth | ||
| | Pass | ||
| # TODO: check for compatable wake version |
There was a problem hiding this comment.
I thought this version check would be on the server side? What could we check for here?
There was a problem hiding this comment.
see followup PR :)
but tl;dr we send our version to the server and it tells us if it thinks we are compatible
| require Some auth = api.getRemoteCacheApiAuthorization | ||
| else failWithError "rsc: Checking authorization requires authorization but none was provided" | ||
|
|
||
| require Pass response = |
There was a problem hiding this comment.
How frequently is this function called?
The db should cache the HTTPRequest job but I'm curious if the wake function should be a target?
There was a problem hiding this comment.
How frequently is this function called?
Two answers here
- As much as the consuming client calls it
- In the case of RemoteCacheRunner/default wake implementation, only once per wake invocation so a target doesn't change anything here
Aside:
The db should cache the HTTPRequest job
This isn't the case. Web requests are never safe to cache as the result can change at any time. Because of that web request jobs are never stored in the db
Add the route
POST /auth/checkwhich returns200on valid auth key and401on invalid auth key. It will be used as an early warning to actors they they may have incorrectly specified their auth key when connecting to the rsc