Remove ohttp_relay from SessionContext #470
Conversation
There was a problem hiding this comment.
ACK. I like how this reduces some boilerplate kinda for free as well.
I'm still unsure about what should be recommended, maybe worth spinning a discussion off from the issue this closes, while this change addresses the possibility of doing that, documenting the tradeoffs of this new degree of freedom seems important but at least speaking for myself I haven't fully thought them through.
There was a problem hiding this comment.
shortly after ACK-ing, which should have been utACK (my bad) i realized the test failures are real and replicate locally, with a 415 ("unsupported media type") response code, so retracting my ACK, and investigating. couldn't post this yesterday due to github being down
The context may use multiple OHTTP Relays, so pass it in the extract method instead. Our integration tests currently fail to relay messages through the OHTTP Relay to the Directory, so they need to pass messages directly to the directory. The reason they're failing otherwise is because the OHTTP Relay only forwards requests using rustls with_webpki_roots, the tests' directory Urls use HTTPS, and the webpki_roots do not contain or allow self-signed certificates. I.e. we have not equivalent _danger-local-https feature available in ohttp_relay and the request isn't downgraded to plain HTTP, so it fails to relay. We may consider having extract_req functions downgrade OHTTP requests to directory targets to HTTP, because they are protected with e2ee as part of OHTTP outside of HTTPS. This change continues to use mock_ohttp_relay (directory) Urls until such a change is made.
DanGould
force-pushed
the
465-rm-relay-from-session-ctx
branch
from
994f2db to
ce9f56d
Compare
|
Added some color to why things were failing with my past change in the commit message. I think this brings a new error to light wrt properly testing OHTTP relay, and perhaps our approach to OHTTP in general that we need to address before 1.0
|
Pull Request Test Coverage Report for Build 12694272135Details
💛 - Coveralls |
| @@ -502,8 +504,7 @@ impl PayjoinProposal { | |||
| target_resource.as_str(), | |||
| Some(&body), | |||
| )?; | |||
| let url = self.context.ohttp_relay.clone(); | |||
| let req = Request::new_v2(url, body); | |||
| let req = Request::new_v2(ohttp_relay.clone(), body); | |||
There was a problem hiding this comment.
For slightly better readability we could update Request::new_v* to take &Url and do the clone in there, then you can do Request::new_v2(ohttp_relay, body); here and in other callsites.
There was a problem hiding this comment.
Seems like really new_v2 is only used after an ohttp_encapsulate so perhaps there should be a shared function as well
The context may use multiple OHTTP Relays, so pass it in the extract method instead.
fix #465