Initial support for Websocket Denial Response#1535
Initial support for Websocket Denial Response#1535paulo-raca wants to merge 10 commits intoKludex:masterfrom
Conversation
|
Curious - what's needed to move forward and merge this in? |
I have no clue, let me know if you have any idea |
Sure - I'll see if we can get this merged in - but, for clarity - from your POV, this is complete and ready to merge? |
|
This is a huge PR, we are on feature freeze, and there was no discussion about it. I understand the discussion on GH was created, but that's not enough. We need to discuss it first. In any case, I didn't check this PR yet, and to proceed a member needs to step in. I'm not sure when I'll have the time. |
I'll take a look at the test failure, but yes, it should be complete and ready. However it is mostly useless as-is: I'll follow up with updates to several middlewares to take advantage of it. |
Yes, it is big, but it is not as complex as it looks at first glance 😬 IMO the biggest/scariest changes happen in testclient.py, where I move code from Maybe I can move this change in a separate commit/PR to simplify the review process? 🤔 I believe that most of the remaining changes are reasonably straightforward.
Sure, just didn't have any clue how to get people to actually engaged in it. I'm glad it is finally happening! Thank for taking a look 🙏 |
|
Unfortunately, nothing will get in on the test client until a decision is made about #1376. |
I for one would love to see removing/decoupling Requests. That PR is awesome work. If I should ask these questions there, @Kludex , let me know
|
That is neat PR, I'd love to see it merged :) From what I can tell it should be easy to apply my changes on top of it. Would it help if I rebased my PR? |
|
I've rebased on top of master and fixed a few minor typing annotations. The test suite should pass now 👍 |
|
I'm going to get back to this PR after 0.19.1 and 0.20.0 are released. See our milestones. |
|
@paulo-raca You should be able to run the pipeline as you wish now. I'll take a look at this in the following weeks (this is not a promise, it's more a self-remind). |
It was merged today. Would you like to proceed with your work here @paulo-raca ? |
…asses, `_HTTPRequestSender` and `_HTTPResponseReceiver`
When this flag is enabled, the Websocket Denial Response extension will be enabled in the ASGI Websocket scope during the test.
The extension must be explicitly enabled on a connection using `websocket_connect(..., denial_response=True)`, If a denial response is returned, a WebSocketDenied exception is thrown with the decoded httpx.Response
…nse into a websocket denial response. This class automatically maps ASGI message between `websocket.*` and `http.*` as necessary. if the WebSocket Denial Response ASGI extension is not supported, it just closes the websocket resulting and HTTP 403 like before.
The test ensures that:
- The denial response is only returned if enabled with `websocket_connect("/", denial_response=True)`
- `receive()` returns disconnected events are received after the HTTP response is sent
- When the extension is unavailable, WebsocketDenialResponse results in a regular `WebSocketDisconnect` exception
47c4705 to
c98c8fb
Compare
Neat! I have rebased this PR, this should be ready to go :) While at it, I also split it into several commits to make reviewing easier (Particularly the TestClient bits) |
|
@paulo-raca Thanks for the PR, and sorry for the long waiting time. I think it's best if we postpone the decision on this feature until we have Starlette 1.0 released. For that reason, I'll be closing this PR. |
Hello, @Kludex The way I see it, this is a straightforward PR, already written and tested, doesn't break APIs, etc. I understand this feature may look useless as-is, since it isn't used anywhere: Otherwise I think this has been postponed enough. If there is not interest in merging it, lets forget 1.0 milestone and just drop it |
When a websocket connection is rejected (e.g., wrong URL, missing permissions, etc),
starlette always returns a generic 403 response (triggered by "websocket.close" message)
or a generic HTTP500 response (Produced by ASGI gateway if there is an unhandled error)
Instead it is better to return useful HTTP error responses using the Websocket Denial Response ASGI extension, if it is available
This PR makes the following changes:
Added WebsocketDenialResponse class to return a HTTP Response to a websocket request, mapping the ASGI message as appropriate.
if the extension is not supported, it just closes the websocket resulting and HTTP 403 like before
Updated testclient to support denial responses.
The extension must be explicitly enabled on a connection using
websocket_connect(..., denial_response=True),and if a denial response is returned a WebSocketDenied exception is thrown
Refactored testclient code so that the code to parse HTTP Responses can be reused on websocket denial responses
This PR superseeds #1478 with better documentation and testing support. Unlike the previous PR,
it does not update the various middlewares.
Instead, I'll create separate PRs for those later, in order to keep this PR smaller