Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add support for dependencies in WebSocket routes #4534

Merged
merged 12 commits into from Jun 11, 2023

Conversation

paulo-raca
Copy link
Contributor

@paulo-raca paulo-raca commented Feb 8, 2022

I've been using dependencies to handle authentication.
But imagine my surprise when I realized my websocket endpoint wasn't authenticated at all?

This commit cherry-picks the dependencies chunks from APIRoute into APIWebSocketRoute

I also made a few minor style nit-picks.

Here is a much-simplified code that demonstrates the issue:

import uvicorn
from fastapi import Depends, FastAPI, HTTPException,  WebSocket
from starlette.requests import Request, HTTPConnection
from starlette.websockets import WebSocket
from starlette.responses import PlainTextResponse, Response

def check_permission():
    print("check_permission")
    raise HTTPException(status_code=401, detail="Unauthorized")

app = FastAPI(dependencies=[Depends(check_permission)])

@app.get("/get")
def get_endpoint() -> Response:
    return PlainTextResponse("GET")

@app.websocket("/ws")
async def websocket_endpoint(ws: WebSocket):
    await ws.accept()
    await ws.send_text("Hello")

if __name__ == "__main__":
    uvicorn.run(app, host="0.0.0.0", port=8000)

As expected, accessing "/get" results in 401 error:

$ curl "http://localhost:8000/get"
{"detail":"Unauthorized"}

However the websocket endpoint remains accessible:

$ python -m websockets "ws://localhost:8000/ws"
Connected to ws://localhost:8000/ws.
< Hello
Connection closed: 1000 (OK).

After applying the patch, the dependencies execute successfully:

$ python -m websockets "ws://localhost:8000/ws"
Failed to connect to ws://localhost:8000/ws: server rejected WebSocket connection: HTTP 500.

Interestingly the websocket connection fails with code 500 instead of 401. I have addressed it into a separate PR to starlette

@paulo-raca paulo-raca force-pushed the websocket-dependencies branch 2 times, most recently from 10dc6e9 to 0ac85d4 Compare February 10, 2022 18:07
@paulo-raca
Copy link
Contributor Author

Hello!

Is there anything missing on this PR?

@paulo-raca
Copy link
Contributor Author

Thank you for the reviews, @BilalAlpaslan @cikay.

I just fixed the merge conflicts, it should be ready for merging

I've been using dependencies to handle authentication.
But imagine my surprise when I realized my websocket endpoint wasn't authenticated at all?

This commit cherry-picks the `dependencies` chunks from `APIRoute` into `APIWebSocketRoute`

I also made a few minor style nit-picks
@paulo-raca
Copy link
Contributor Author

@tiangolo, sorry to call you directly, but can I get your eyes on this?

This is a straightforward issue, but it has been bugging me a lot in the last month 😅

Thank you 🙏

@paulo-raca
Copy link
Contributor Author

paulo-raca commented Jun 15, 2022

Anything missing from this change? How can I get it merged?

@github-actions
Copy link
Contributor

📝 Docs preview for commit 6453788 at: https://62a95f500fa552369a8a9c11--fastapi.netlify.app

@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (cf73051) 100.00% compared to head (ccc1987) 100.00%.

❗ Current head ccc1987 differs from pull request most recent head 931b492. Consider uploading reports for the commit 931b492 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #4534   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          540       541    +1     
  Lines        13969     14001   +32     
=========================================
+ Hits         13969     14001   +32     
Impacted Files Coverage Δ
fastapi/applications.py 100.00% <100.00%> (ø)
fastapi/routing.py 100.00% <100.00%> (ø)
tests/test_ws_dependencies.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 73249da at: https://62a96d422ba68a456ea1be02--fastapi.netlify.app

@paulo-raca
Copy link
Contributor Author

Merged master branch again 👍

@tiangolo, @Kludex, can I get eyes on this one?

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2022

📝 Docs preview for commit 3945a32 at: https://6337d198c51f172ca119d1f5--fastapi.netlify.app

fastapi/routing.py Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

📝 Docs preview for commit e4ddcb8 at: https://633b8d39f6ae0b0073aa6272--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 5943bae at: https://634eff6b41393725675bf3ea--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit ccc1987 at: https://6355f671a4e4cf7923aace28--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit f2541d2 at: https://639ce37dca15d5006470360d--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2023

📝 Docs preview for commit 275208e at: https://63dfcc686a21a87de30b11e3--fastapi.netlify.app

@paulo-raca
Copy link
Contributor Author

ping?

@lclbm
Copy link

lclbm commented Feb 26, 2023

Is there any progress about this issue?
have the same problem

@tiangolo
Copy link
Owner

📝 Docs preview for commit a463414 at: https://64861d40ff845b421d97876c--fastapi.netlify.app

@tiangolo tiangolo changed the title Support dependencies in websocket routes ✨ Add support for dependencies in WebSocket routes Jun 11, 2023
@tiangolo
Copy link
Owner

Great, thank you @paulo-raca! 🚀 🙇

This will be available in the next version, FastAPI 0.97.0, released in the next hours (or tomorrow). 🎉

@tiangolo tiangolo enabled auto-merge (squash) June 11, 2023 20:33
@paulo-raca
Copy link
Contributor Author

Neat, thank you

@tiangolo tiangolo merged commit d8b8f21 into tiangolo:master Jun 11, 2023
10 checks passed
@bastianh
Copy link

bastianh commented Jun 22, 2023

For me this was a breaking change because some dependencies I have on the router itself had Request as a dependency which is not available on the websocket endpoint... and those were suddenly executed for the websocket endpoint as well.

@gjanezic
Copy link

I have the same problem as @bastianh. How can we resolve this? Maybe related to #8503?

@tiangolo
Copy link
Owner

@bastianh and/or @gjanezic, please create a new discussion with a simple example I can copy paste and replicate your issue: https://github.com/tiangolo/fastapi/discussions/categories/questions 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants