Skip to content

feat(server): Add outcomes endpoint#589

Merged
RaduW merged 4 commits intomasterfrom
outcome-endpoint
Jun 2, 2020
Merged

feat(server): Add outcomes endpoint#589
RaduW merged 4 commits intomasterfrom
outcome-endpoint

Conversation

@RaduW
Copy link
Contributor

@RaduW RaduW commented May 27, 2020

Adds the outcomes endpoint for outcomes ingestion

@RaduW RaduW force-pushed the outcome-endpoint branch from f8e04af to b08584e Compare May 27, 2020 16:00
@RaduW RaduW marked this pull request as ready for review May 28, 2020 15:08
@RaduW RaduW requested review from a team and jan-auer May 28, 2020 15:08
@jan-auer jan-auer changed the title feat(server) Add outcomes endpoint feat(server): Add outcomes endpoint May 28, 2020
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, 200 is also perfectly fine here.
  • If outcomes are not enabled, should we maybe respond with 501 Not Implemented or 404 Not Found? It's not really an authentication issue, so 403 seems wrong.

Copy link
Contributor Author

@RaduW RaduW May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jan-auer jan-auer May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@RaduW RaduW force-pushed the outcome-endpoint branch from c92f24d to 495cb46 Compare May 29, 2020 11:39
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are failing now since optional outcome attributes are missing. For instance:

"org_id": None,
"key_id": None,

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.

Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RaduW
Copy link
Contributor Author

RaduW commented Jun 2, 2020

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

@RaduW RaduW force-pushed the outcome-endpoint branch from 495cb46 to 906567a Compare June 2, 2020 09:22
@RaduW RaduW merged commit aad40e7 into master Jun 2, 2020
@RaduW RaduW deleted the outcome-endpoint branch June 2, 2020 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants