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: Implement authentication for API v2 based on OAuth2 potocol. #388

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

alowave223
Copy link
Member

@alowave223 alowave223 commented Jan 15, 2023

Describe your changes

Created authorization for third-party clients based on the OAuth2 protocol standard.

Related Issues / Projects

https://github.com/orgs/osuAkatsuki/projects/2

Checklist

  • The changes pass pre-commit checks (make lint)
  • The changes follow coding style

@alowave223 alowave223 self-assigned this Jan 15, 2023
@alowave223 alowave223 added the enhancement New feature or request label Jan 15, 2023
@@ -0,0 +1,85 @@
from __future__ import annotations
Copy link
Member Author

@alowave223 alowave223 Jan 15, 2023

Choose a reason for hiding this comment

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

This import breaks FastApi's Depends. Related issue: fastapi/fastapi#1654

Copy link
Member

Choose a reason for hiding this comment

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

yeah i ran into this a long while ago at work, nice work actually tracking it down lol, that's not an easy error

@cmyui
Copy link
Member

cmyui commented Apr 8, 2023

hey, there are quite a few typing errors still, i suspect your vscode is using type_checking_mode: off in your preferences

you can open preferences with ctrl+, and set type checking mode to basic to have vs code redline any of these errors

image

e.g.
image

image

auth_credentials: Optional[dict[str, Union[str, int]]] = Depends(
auth_credentials: Optional[dict[str, Any]] = Depends(
Copy link
Member

Choose a reason for hiding this comment

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

@alowave223 typing the possible values of a dict is usually a bad idea imo - if you're going to try, consider using a typing.TypedDict

@cmyui
Copy link
Member

cmyui commented Apr 9, 2023

there are also quite a few cases like this state: str = Query(default=None), where the variable is typed as T but at runtime can actually be of type Optional[T] due to the default case

Comment on lines 63 to 66
# https://developer.zendesk.com/api-reference/sales-crm/authentication/requests/#client-authentication
def get_credentials_from_basic_auth(
request: Request,
) -> Optional[dict[str, Union[str, int]]]:
Copy link
Member

@cmyui cmyui Apr 9, 2023

Choose a reason for hiding this comment

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

better to use a TypedDict here, something like this so that the individual keys are statically typed

Suggested change
# https://developer.zendesk.com/api-reference/sales-crm/authentication/requests/#client-authentication
def get_credentials_from_basic_auth(
request: Request,
) -> Optional[dict[str, Union[str, int]]]:
from typing import TypedDict
class BasicAuthCredentials(TypedDict):
client_id: int
client_secret: str
# https://developer.zendesk.com/api-reference/sales-crm/authentication/requests/#client-authentication
def get_credentials_from_basic_auth(
request: Request,
) -> Optional[BasicAuthCredentials]:

@@ -0,0 +1,207 @@
""" bancho.py's v2 apis for interacting with clans """
Copy link
Member

Choose a reason for hiding this comment

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

some docstrings still out of date

@cmyui cmyui marked this pull request as draft April 9, 2023 00:16
Copy link
Member

@cmyui cmyui left a comment

Choose a reason for hiding this comment

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

left a review with some changes requested

overall i'd like to also reduce usage of typing.Union - better to constrain the variance of types when they come into our ecosystem so that we do not have to deal with this complexity ourselves

nice work so far! & sorry for the very late pr review lol

@cmyui
Copy link
Member

cmyui commented Jun 27, 2023

need to remember to bump version @ release

app/settings.py Outdated Show resolved Hide resolved
@cmyui cmyui self-assigned this Jul 5, 2023
@cmyui
Copy link
Member

cmyui commented Feb 13, 2024

started work, a couple of things stand out:

  1. since this is meant to be an api for custom web apps (other than the osu! client), i think we should use jwt for it's symmetric key verification mechanism to avoid storing raw access tokens in our application. we can include verifiable claims in the token such as a token_id, which can be stored server-side to enable token revocation by server staff without the storage of a signed access token.
  2. i'm not certain whether it's best to persist access tokens vs. refresh tokens vs. both -- i'll have to look a bit deeper into the oauth 2.0 standard to see if there's a reason to prefer one over the other.
  3. there hasn't yet been much thought yet put towards the mechanisms which should be made available to server admins w.r.t. access controls -- i think two main ones that come to mind are:
    • ability for server staff to revoke all sessions for a given user
    • ability server staff to temporarily disable a user's account
  4. there isn't much client-identifying information being collecting alongside the authorization grants -- i think it would be wise to store simple identification information such as the client ip address, and user agent along with grants or tokens.
  5. there are some implementation-specifics that need adjustments -- shouldn't be very major; the previously mentioned issues are more important/fundamental than these implementation specifics.

overall i think i'll continue to work on top of this pr, as it's quite decent at matching the spec so far -- nothing particularly wrong.

@cmyui
Copy link
Member

cmyui commented Feb 13, 2024

  1. i'm not certain whether it's best to persist access tokens vs. refresh tokens vs. both -- i'll have to look a bit deeper into the oauth 2.0 standard to see if there's a reason to prefer one over the other.

It may also be worth storing other things, such as authorization grants and failed authorization attempts.

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 this pull request may close these issues.

2 participants