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

[Feature request] Support WebSocket connections. #191

Closed
epelaez1 opened this issue Apr 4, 2024 · 4 comments · Fixed by #200
Closed

[Feature request] Support WebSocket connections. #191

epelaez1 opened this issue Apr 4, 2024 · 4 comments · Fixed by #200
Labels
enhancement New feature or request

Comments

@epelaez1
Copy link

epelaez1 commented Apr 4, 2024

Hello!
First of all thank you for this repo! It has saved me a lot of time.

I have tried to use SingleTenantAzureAuthorizationCodeBearer with a web socket connection and FastAPI throws this error:

TypeError: AzureAuthorizationCodeBearerBase.__call__() missing 1 required positional argument: 'request'

I think it would be very easy to add support to WebSockets, just replacing the dependency type from starlette.requests.Request to starlette.requests.HTTPConnection.

As described in documentation:

When you want to define dependencies that should be compatible with both HTTP and WebSockets, you can define a parameter that takes an HTTPConnection instead of a Request or a WebSocket.

I tried a quick patch replacing it in AzureAuthorizationCodeBearerBase.__call__ and in OAuth2AuthorizationCodeBearer.__call__, and it worked perfectly.

HTTPConnection is the base class for both WebSocket and Request, and it has the headers and state properties, so it would probably be backwards compatible.

Let me know if you think it is feasible! If you need hands on, I can try to do a PR.

@epelaez1 epelaez1 added the enhancement New feature or request label Apr 4, 2024
@JonasKs
Copy link
Member

JonasKs commented Apr 4, 2024

Hi!
I'd love to get a PR for this. Let me know if I can help.

@sreenivasulureddysura
Copy link

@JonasKs any update on this?

@JonasKs
Copy link
Member

JonasKs commented Jun 7, 2024

No, not really.

Any contribution would help, whether it's a test, reproducible example or a full implementation.

@JonasKs JonasKs mentioned this issue Jul 12, 2024
Merged
@JonasKs
Copy link
Member

JonasKs commented Jul 13, 2024

Implemented in #200, please check it out.

@JonasKs JonasKs mentioned this issue Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants