Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

Fix the type hint of get_current_user #59

Merged
merged 2 commits into from
Jul 23, 2022

Conversation

DaGuich
Copy link
Contributor

@DaGuich DaGuich commented Jul 11, 2022

FastAPIs Depends function expects a callable. get_current_user is returning a callable but its type hint did not reflect this.

It's just a small change but PyCharms type checker annoyed me...

FastAPIs `Depends` function expects a callable. get_current_user is
returning a callable but its type hint did not reflect this
It's just a small change but PyCharm annoyed me...
Copy link
Collaborator

@JonasScholl JonasScholl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! I have just a small suggestion, since the type hint gets corrected anyways we can also make it as explicit as possible :D

@@ -221,7 +221,7 @@ def user_auth_scheme(self) -> OAuth2PasswordBearer:
"""
return OAuth2PasswordBearer(tokenUrl=self.token_uri)

def get_current_user(self, required_roles: List[str] = None, extra_fields: List[str] = None) -> OIDCUser:
def get_current_user(self, required_roles: List[str] = None, extra_fields: List[str] = None) -> Callable[..., OIDCUser]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type hint can be even more specific :D

Suggested change
def get_current_user(self, required_roles: List[str] = None, extra_fields: List[str] = None) -> Callable[..., OIDCUser]:
def get_current_user(self, required_roles: List[str] = None, extra_fields: List[str] = None) -> Callable[[OAuth2PasswordBearer], OIDCUser]:

@@ -230,7 +230,7 @@ def get_current_user(self, required_roles: List[str] = None, extra_fields: List[
extra_fields List[str]: The names of the additional fields you need that are encoded in JWT

Returns:
OIDCUser: Decoded JWT content
Callable[..., OIDCUser]: Dependency method which returns the decoded JWT content
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Callable[..., OIDCUser]: Dependency method which returns the decoded JWT content
Callable[[OAuth2PasswordBearer], OIDCUser]: Dependency method which returns the decoded JWT content

Copy link
Collaborator

@yannicschroeer yannicschroeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DaGuich
Copy link
Contributor Author

DaGuich commented Jul 18, 2022

Right now we are only allowing OAuth2PasswordBearer. Maybe we want to be a little bit more flexible there and use the base class of OAuth2PasswordBearer (which is OAuth2 I think...)

@JonasScholl
Copy link
Collaborator

Right now we are only allowing OAuth2PasswordBearer. Maybe we want to be a little bit more flexible there and use the base class of OAuth2PasswordBearer (which is OAuth2 I think...)

We can think of supporting this, then we would need to somehow make the user_auth_scheme method extendable 6 customizable so that one can use different auth schemes too. I will just open an issue, anyone interested in supporting this can then pick this up.

@JonasScholl JonasScholl merged commit d3a5e72 into code-specialist:master Jul 23, 2022
@DaGuich DaGuich deleted the cr-fix-type-hint branch July 24, 2022 16:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants