Conversation
jan-auer
left a comment
There was a problem hiding this comment.
Thanks, this looks great!
There's one request to add serde annotations to the outcome payload, and a few nitpicks.
|
|
||
| fn send_outcomes(state: CurrentServiceState, body: SignedJson<SendOutcomes>) -> HttpResponse { | ||
| if !body.relay.internal { | ||
| return HttpResponse::Unauthorized().finish(); |
There was a problem hiding this comment.
nit: Shouldn't this return with Forbidden?
This status is similar to
401, but in this case, re-authenticating will make no difference. The access is permanently forbidden and tied to the application logic, such as insufficient rights to a resource.
Similarly, I'm wondering about the other status codes:
- We can be more semantic in the success case and respond with
202 Accepted. However,200is also perfectly fine here. - If outcomes are not enabled, should we maybe respond with
501 Not Implementedor404 Not Found? It's not really an authentication issue, so403seems wrong.
There was a problem hiding this comment.
401 vs 403... I read a bit more about it and you are right. I initially interpreted Unauthorized to mean you are Authenticated but not Authorized (so exactly our case for external relays). From what I read it is actually used as a signal that you are not Authenticated, so yes Forbidden would be the best HTTP code for both the cases when you are authenticated but don't have the required permissions and when the system is configured not to send outcomes regardless of your permissions.
I would not go with 501 since it is not necessarily a server error. It might be a server miss-configuration ( in which case it should be a 501) or the server might be rightly configured and the client request be unreasonable (in which case it should be a 4xx). So let's err. on the safe side and blame the client :-) .
So I think we should answer with HTTP 403 Forbidden for both cases when requests come from external relays or the current relay has the flag emit_outcomes=false.
For the success case I have no real preference, I think that generally 200 is safer that 202 for badly written clients that don't check for 2xx & 3xx but check specifically for 200. In our case the client is another Relay so we can go with whatever you prefer.
There was a problem hiding this comment.
I think that generally 200 is safer that 202 for badly written clients
That's a good argument. I was already thinking about making /envelope/ respond with 202, but potentially that might just complicate things. In the case of outcomes, we know that the client is always another Relay. Since we always respond with 200 at the moment, let's stick with that. We can still re-evaluate that in the future.
Edit: I saw that you're doing 202 now. Either is fine, so let's just stick with what's checked in.
Thanks!
There was a problem hiding this comment.
Tests are failing now since optional outcome attributes are missing. For instance:
relay/tests/integration/test_outcome.py
Lines 40 to 41 in 4a7e898
After those have been updated, this is good to go 👍 . Please merge master to resolve the conflict in endpoints/mod.rs (note that the endpoints are now ordered into categories).
Edit: Tests were just flakey once and you updated them already. Sorry about that.
untitaker
left a comment
There was a problem hiding this comment.
this is good but I would add an integration test for it. we already have plenty of infra to test outcome messages so this should not take long
Absolutely, there is a subtask for unit testing the outcomes endpoint |
Adds the outcomes endpoint for outcomes ingestion