-
Notifications
You must be signed in to change notification settings - Fork 16
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
Passing extra query parameters to the request #63
Comments
@pedropregueiro you can inherit from from aioauth.requests import Post, Query as _Query, Request
from dataclasses import dataclass
@dataclass
class Query(_Query):
team: str | None = None
async def to_oauth2_request(
request: Request, settings: Settings = Settings()
) -> OAuth2Request:
"""Converts :py:class:`fastapi.Request` instance to :py:class:`aioauth.requests.Request` instance"""
form = await request.form()
post = dict(form)
query_params = dict(request.query_params)
method = request.method
headers = HTTPHeaderDict(**request.headers)
url = str(request.url)
user = None
if request.user.is_authenticated:
user = request.user
return OAuth2Request(
settings=settings,
method=RequestMethod[method],
headers=headers,
post=Post(**post),
query=Query(**query_params),
url=url,
user=user,
) |
Perfect, this works! |
@aliev using custom dataclasses is creating some type validation issues (using mypy==0.931). Here's an example with custom
When running
Tbh, I haven't used dataclasses that often so not sure what's the best way to handle type inheritance. |
hmm that seems to be expected. I think I can find possible solutions, give me some time. |
see the issue #63 while inheriting from models (Token, Client, AuthorizationCode), and using them in aioauth, mypy throws an error like: ``` error: Argument 1 of "create_authorization_code" is incompatible with supertype "BaseStorage"; supertype defines the argument type as "Request" ``` this PR fixes the above bug by adding additional `TToken`, `TClient` and `TAuthorizationCode` parameters to the `BaseModel` generic. usage example: ```python from dataclasses import dataclass from typing import Optional from aioauth.models import AuthorizationCode, Token, Client from aioauth.requests import Post as _Post from aioauth.requests import Request as _Request from aioauth.storage import BaseStorage from aioauth.types import RequestMethod, GrantType @DataClass class CustomPost(_Post): my_field: str = "" @DataClass class CustomRequest(_Request): post: CustomPost = CustomPost() class Storage(BaseStorage[Token, Client, AuthorizationCode, CustomRequest]): async def create_authorization_code( self, request: CustomRequest, client_id: str, scope: str, response_type: str, redirect_uri: str, code_challenge_method: Optional[str], code_challenge: Optional[str], code: str, ) -> AuthorizationCode: pass ```
@pedropregueiro Created PR. not sure if it won't break anything further. needs to be tested |
thanks @aliev! that solves the error with the Storage methods, but unfortunately not the custom Query or Post one:
I tried to fix this locally with the same idea you applied (using generic types) but bumped into an issue when trying to do:
mypy didn't like having a different type as the default value. thoughts? |
well, something like this should work: TQuery = TypeVar("TQuery", bound=Query)
TPost = TypeVar("TPost", bound=Post)
TUser = TypeVar("TUser")
@dataclass
class BaseRequest(Generic[TQuery, TPost, TUser]):
method: RequestMethod
query: TQuery
post: TPost
headers: HTTPHeaderDict = HTTPHeaderDict()
url: str = ""
user: Optional[TUser] = None
settings: Settings = Settings()
@dataclass
class Request(BaseRequest[Query, Post, Any]):
"""Object that contains a client's complete request."""
headers: HTTPHeaderDict = HTTPHeaderDict()
query: Query = Query()
post: Post = Post()
url: str = ""
user: Optional[Any] = None
settings: Settings = Settings()
method: RequestMethod thus, to make a custom class MyRequest(BaseRequest[MyQuery, MyPost, MyUser]):
... I'm checking now. |
@pedropregueiro could you check what i just pushed? |
sorry for the delay, this seems to be working as expected now, mypy is happy! I had to change the
result
I'm using the |
try like this? from aioauth.types import GrantType
test_form_dict = ImmutableMultiDict(
{"grant_type": GrantType.TYPE_AUTHORIZATION_CODE, "channel": "123"}
) |
yeah, of course, but I meant when parsing a framework's Request (eg: FastAPI) into aioauth's structure. I was using the async def to_oauth2_request(
request: Request, settings: Settings = Settings()
) -> MyRequest:
form = await request.form() # type: ImmutableMultiDict
post = dict(form) # type: dict
# ... some more code hidden
return MyRequest(
post=MyPost(**post), # this gives the incompatible type error
) A simple change, fixed this for me: async def to_oauth2_request(
request: Request, settings: Settings = Settings()
) -> MyRequest:
form = await request.form()
post: MyPost = MyPost(**form)
# ... some more code hidden
return MyRequest(
post=post, # ok types
) Anyway, this feels unrelated now. Thanks for fixing the custom types! |
maybe we should use Literals instead of enums. I'll take a look later |
I can add these changes to the current PR, but I'm not sure how soon I can merge it, since the documentation needs to be edited, a lot has changed. |
Can maybe open a diff issue for the literal work, if that's what you think will delay a release? The way I have it now works well and is valid with mypy, not in a rush to change the enums. |
@pedropregueiro I think that I will make a release on the weekend, as this requires updating the documentation as well. give me some time |
Of course, no problems! |
* fix: mypy errors on custom models see the issue #63 while inheriting from models (Token, Client, AuthorizationCode), and using them in aioauth, mypy throws an error like: ``` error: Argument 1 of "create_authorization_code" is incompatible with supertype "BaseStorage"; supertype defines the argument type as "Request" ``` this PR fixes the above bug by adding additional `TToken`, `TClient` and `TAuthorizationCode` parameters to the `BaseModel` generic. usage example: ```python from dataclasses import dataclass from aioauth_fastapi.router import get_oauth2_router from aioauth.storage import BaseStorage from aioauth.requests import BaseRequest from aioauth.models import AuthorizationCode, Client, Token from aioauth.config import Settings from aioauth.server import AuthorizationServer from fastapi import FastAPI app = FastAPI() @DataClass class User: """Custom user model""" first_name: str last_name: str @DataClass class Request(BaseRequest[Query, Post, User]): """"Custom request""" class Storage(BaseStorage[Token, Client, AuthorizationCode, Request]): """ Storage methods must be implemented here. """ storage = Storage() authorization_server = AuthorizationServer[Request, Storage](storage) # NOTE: Redefinition of the default aioauth settings # INSECURE_TRANSPORT must be enabled for local development only! settings = Settings( INSECURE_TRANSPORT=True, ) # Include FastAPI router with oauth2 endpoints. app.include_router( get_oauth2_router(authorization_server, settings), prefix="/oauth2", tags=["oauth2"], ) ``` * enums were replaced to literals
@pedropregueiro v1.5.0 has been released, including the fixes with literals (enums) |
With the latest changes, I have to either maintain my own enums or have some hardcoded strings in the code:
Rather than what was possible to do before:
Is there a better way to do this? |
I see several issues with Enum that I come up in this project:
string literals are typed, so the mypy will complain to the following code: from enum import Enum
from typing import Literal
GrantType = Literal["authorization_code"]
def check_grant_type(grant_type: GrantType):
...
check_grant_type("refresh_token")
replacing so string literals are very useful, they add typing, and at the same time you can enjoy all the benefits of strings without additional type casting. in your case, I think the first option is quite appropriate. |
I disagree with this. It differs because explicitly having to write the string (e.g.) "authorization_code" might generate odd errors on the developer end. Someone might write "authorisation_code" or a myriad of other typos. Even further, someone might confuse In both cases, unless the dev is using a type checker (and continuously re-checking their code on each change), the errors will go unnoticed as the interpreter won't raise them on start. This will lead to unnecessary time trying to figure out why things are failing down the line. Especially with OAuth2, where errors tend to be slightly generalized for obfuscation purposes. |
(.venv) ➜ ~ cat test_enum.py
from enum import Enum
from typing import Literal
GrantType = Literal["authorization_code"]
def check_grant_type(grant_type: GrantType):
...
check_grant_type("authorisation_code")
(.venv) ➜ ~ mypy test_enum.py
test_enum.py:12: error: Argument 1 to "check_grant_type" has incompatible type "Literal['authorisation_code']"; expected "Literal['authorization_code']"
Found 1 error in 1 file (checked 1 source file)
(.venv) ➜ ~
that's why we have to rely on CI with mypy and tests :) |
Indeed, but if you take the same example and do
100% agree with having mypy and tests (+more) on CI, and I also use In any case, I made the changes to make this work for me, so mostly discussing this for the future of the library and upcoming users hehe |
What's the best way to pass custom query parameters to the Storage methods? For example, slack supports sending a
team=
query parameter for narrowing down oauth2 flows. I'd love to do something similar (with param 'channel'), but getting errors when trying to pass any extra query params:I understand the Query dataclass doesn't have support for it today, but is there any other good way to handle this? Or am I overcomplicating things?
p.s. – Thanks for the great work on this lib btw! 🙏
The text was updated successfully, but these errors were encountered: