Passing extra query parameters to the request

[pedropregueiro]

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:

...     query=Query(**query_params),
TypeError: __init__() got an unexpected keyword argument 'channel'

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! 🙏

[aliev]

@pedropregueiro you can inherit from Query and add your own fields, like this:

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,
    )

[pedropregueiro]

Perfect, this works!

[pedropregueiro]

@aliev using custom dataclasses is creating some type validation issues (using mypy==0.931).

Here's an example with custom Post and Request classes:

from dataclasses import dataclass
from typing import Optional

from aioauth.models import AuthorizationCode
from aioauth.requests import Post as _Post
from aioauth.requests import Request as _Request
from aioauth.storage import BaseStorage
from aioauth.types import RequestMethod

@dataclass
class CustomPost(_Post):
    my_field: str = ""

@dataclass
class CustomRequest(_Request):
    post: CustomPost = CustomPost()

class Storage(BaseStorage):
    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

oauth2_request = CustomRequest(
    method=RequestMethod.POST,
    post=CustomPost(**{"grant_type": "authorization_code", "my_field": "test"}),
)

When running mypy I get the following errors:

test.py:22: error: Argument 1 of "create_authorization_code" is incompatible with supertype "BaseStorage"; supertype defines the argument type as "Request"
test.py:22: note: This violates the Liskov substitution principle
test.py:22: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
test.py:38: error: Argument 1 to "CustomPost" has incompatible type "**Dict[str, str]"; expected "Optional[GrantType]"
Found 2 errors in 1 file (checked 1 source file)

Tbh, I haven't used dataclasses that often so not sure what's the best way to handle type inheritance.

[aliev]

hmm that seems to be expected. I think I can find possible solutions, give me some time.

[aliev]

@pedropregueiro Created PR. not sure if it won't break anything further. needs to be tested

[pedropregueiro]

thanks @aliev! that solves the error with the Storage methods, but unfortunately not the custom Query or Post one:

test.py:38: error: Argument 1 to "CustomPost" has incompatible type "**Dict[str, str]"; expected "Optional[GrantType]"

I tried to fix this locally with the same idea you applied (using generic types) but bumped into an issue when trying to do:

@dataclass
class Request(Generic[TQuery, TPost]):
  ...
  query: TQuery = Query()
  post: TPost = Post()
  ...

mypy didn't like having a different type as the default value. thoughts?

[aliev]

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 Request, you will have to inherit from BaseRequest:

class MyRequest(BaseRequest[MyQuery, MyPost, MyUser]):
    ...

I'm checking now.

[aliev]

@pedropregueiro could you check what i just pushed?

[pedropregueiro]

sorry for the delay, this seems to be working as expected now, mypy is happy!

I had to change the CustomPost initialization, but that seems to be a diff issue. here's the latest working sample code for reference:

from dataclasses import dataclass
from typing import Any, Optional

from aioauth.requests import Query, Post, BaseRequest
from aioauth.types import RequestMethod
from starlette.datastructures import ImmutableMultiDict

@dataclass
class MyPost(Post):
    channel: Optional[str] = None

@dataclass
class MyRequest(BaseRequest[Query, MyPost, Any]):
    post: MyPost = MyPost()
    query: Query = Query()

test_form_dict = ImmutableMultiDict(
    {"grant_type": "authorization_code", "channel": "123"}
)
post: MyPost = MyPost(**test_form_dict)

oauth2_request = MyRequest(
    method=RequestMethod.POST,
    post=post,
)

print("request", oauth2_request)
print("channel", oauth2_request.post.channel)

result

request MyRequest(method=<RequestMethod.POST: 'POST'>, query=Query(client_id=None, redirect_uri='', response_type=None, state='', scope='', nonce=None, code_challenge_method=None, code_challenge=None, response_mode=None), post=MyPost(grant_type='authorization_code', client_id=None, client_secret=None, redirect_uri=None, scope='', username=None, password=None, refresh_token=None, code=None, token=None, token_type_hint=None, code_verifier=None, channel='123'), headers={}, url='', user=None, settings=Settings(TOKEN_EXPIRES_IN=86400, REFRESH_TOKEN_EXPIRES_IN=172800, AUTHORIZATION_CODE_EXPIRES_IN=300, INSECURE_TRANSPORT=False, ERROR_URI='', AVAILABLE=True))
channel 123

I'm using the ImmutableMultiDict because that's the type I'll get from FastAPI requests form data. That said, if I try to instantiate the MyPost() just using a normal dictionary, mypy will keep failing with the error I described before: error: Argument 1 to "MyPost" has incompatible type "**Dict[str, str]"; expected "Optional[GrantType]"

[aliev]

try like this?

from aioauth.types import GrantType

test_form_dict = ImmutableMultiDict(
    {"grant_type": GrantType.TYPE_AUTHORIZATION_CODE, "channel": "123"}
)

[pedropregueiro]

yeah, of course, but I meant when parsing a framework's Request (eg: FastAPI) into aioauth's structure. I was using the to_oauth2_request method from your fastapi-extension as inspiration, but that's where the type issues were coming from:

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!

[aliev]

maybe we should use Literals instead of enums. I'll take a look later

[aliev]

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.

[pedropregueiro]

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.

[aliev]

@pedropregueiro I think that I will make a release on the weekend, as this requires updating the documentation as well. give me some time

[pedropregueiro]

Of course, no problems!

[aliev]

@pedropregueiro v1.5.0 has been released, including the fixes with literals (enums)

[pedropregueiro]

With the latest changes, I have to either maintain my own enums or have some hardcoded strings in the code:

if request.post.grant_type == "authorization_code":

Rather than what was possible to do before:

if request.post.grant_type == GrantType.TYPE_AUTHORIZATION_CODE:

Is there a better way to do this?

[aliev]

I see several issues with Enum that I come up in this project:

1) they must be explicitly converted to a string, otherwise mypy will complain

2) duplicates in the code. how does GrantType.TYPE_AUTHORIZATION_CODE actually differ from the string literal "authorization_code"? Actually, there is no difference. It only adds complication in the code and unnecessary imports.

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")

test_enum.py:12: error: Argument 1 to "check_grant_type" has incompatible type "Literal['refresh_token']"; expected "Literal['authorization_code']"

replacing check_grant_type("refresh_token") with check_grant_type("authorization_code") will fix the error.

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.

[pedropregueiro]

duplicates in the code. how does GrantType.TYPE_AUTHORIZATION_CODE actually differ from the string literal "authorization_code"? Actually, there is no difference. It only adds complication in the code and unnecessary imports.

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 grant_type with reponse_type and just use the word "code" wrongfully.

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.

[aliev]

Someone might write "authorisation_code" or a myriad of other typos. Even further, someone might confuse grant_type with reponse_type and just use the word "code" wrongfully.

Literal won't let you do that, mypy will complain if the passed string doesn't match what it expects:

(.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) ➜  ~ 

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.

that's why we have to rely on CI with mypy and tests :) aioauth using pre-commit, before I commit changes, it checks for typing issues.

[pedropregueiro]

mypy will complain if the passed string doesn't match what it expects

Indeed, but if you take the same example and do python test_enum.py you won't get any errors calling check_grant_type("authorisation_code")

that's why we have to rely on CI with mypy and tests :)

100% agree with having mypy and tests (+more) on CI, and I also use pre-commit locally as an additional check as you do on aioauth. That said, I think it's risky to assume that all devs using the library will be using mypy or any form of type checking. Still early days for typed Python after all :)

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