starlette
starlette copied to clipboard
encode
Initial support for Websocket Denial Response
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
Curious - what's needed to move forward and merge this in?
I have no clue, let me know if you have any idea
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.
Sure - I'll see if we can get this merged in - but, for clarity - from your POV, this is complete and ready to merge?
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.
This is a huge PR
Yes, it is big, but it is not as complex as it looks at first glance :grimacing:
IMO the biggest/scariest changes happen in testclient.py, where I move code from _ASGIAdapter.send() into HTTPRequestSender and HTTPResponseReceiver (so that they can be reused in _raise_on_denial()).
Maybe I can move this change in a separate commit/PR to simplify the review process? :thinking:
I believe that most of the remaining changes are reasonably straightforward.
I understand the discussion on GH was created, but that's not enough. We need to discuss it first.
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 :pray:
Unfortunately, nothing will get in on the test client until a decision is made about https://github.com/encode/starlette/pull/1376.
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
- What's the top 3 blockers for that PR?
- Do you have bandwidth to address them? If not, how can I help? Happy to connect offline as well. I would love to see that PR and this one go in.
Unfortunately, nothing will get in on the test client until a decision is made about #1376.
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 :+1:
@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).
Unfortunately, nothing will get in on the test client until a decision is made about #1376.
@Kludex, any news on this? I would be happy to update this PR on top of httpx if that is still happening
Unfortunately, nothing will get in on the test client until a decision is made about #1376.
@Kludex, any news on this? I would be happy to update this PR on top of httpx if that is still happening
It was merged today. Would you like to proceed with your work here @paulo-raca ?
It was merged today. Would you like to proceed with your work here @paulo-raca ?
Neat! Thank you for the notice :smiley:
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)
@Kludex, @adriangb, can I get a look on this? I'm eager to use these changes on my current project
(I'm also eager to write a bunch of follow-up PRs for some of the middlewares -- auth, error, redirect, etc)
@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.
I think it's best if we postpone the decision on this feature until we have Starlette 1.0 released.
Hello, @Kludex
The way I see it, this is a straightforward PR, already written and tested, doesn't break APIs, etc. Can we just get it merged, pretty please? :pray:
I understand this feature may look useless as-is, since it isn't used anywhere: I commit to write updates to each middleware once we get this merged (Or now, if you think this would unblock this PR - -- As I have already done in the previous version of this PR)
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