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

change firebase authentication to class #593

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dejima-shikou
Copy link
Collaborator

@dejima-shikou dejima-shikou commented Feb 7, 2025

PR の目的

  • APIの認証機能を、firebaseとsupabaseで切り替え可能とする
    • 未実装
  • まずはsupabse無しでfirebaseのみクラス化する修正を行い、設計レビューする

経緯・意図・意思決定

  • HTTPAuthorizationCredentials.credentialsを取得する際、
    token: HTTPAuthorizationCredentials = Depends(HTTPBearer))
    のようにして取得するが、このDependsはルーター関数のDepends、またはそのDependsから呼び出されるDepends、でないとエラーとなる

  • auth.pyはauthディレクトリと名前競合でエラーでたので、一旦account.pyとしましたが、置き場所含めて再検討したいです。

  • 現状、下記テストでエラーが出る。別途検討予定
    FAILED app/tests/medium/routers/test_users.py::test_create_user_without_auth - AttributeError: 'NoneType' object has no attribute 'credentials'
    FAILED app/tests/requests/test_pteams.py::test_get_pteams__without_auth - AttributeError: 'NoneType' object has no attribute 'credentials'
    FAILED app/tests/requests/test_pteams.py::test_get_pteam__without_auth - AttributeError: 'NoneType' object has no attribute 'credentials'
    FAILED app/tests/requests/test_pteams.py::test_create_pteam__without_auth - AttributeError: 'NoneType' object has no attribute 'credentials'

参考文献

Copy link
Collaborator

@mshim03 mshim03 left a comment

Choose a reason for hiding this comment

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

大筋は問題ないと思います。細かい点だけコメントしました

Comment on lines 7 to 8
def get_credentials(token: HTTPAuthorizationCredentials = Depends(token_scheme)):
return token.credentials
Copy link
Collaborator

Choose a reason for hiding this comment

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

利用していないので、この処理は不要かも?



def get_current_user(
token: HTTPAuthorizationCredentials = Depends(token_scheme),
Copy link
Collaborator

Choose a reason for hiding this comment

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

HTTPBearer 以外のtoken scheme を使う予定もなさそうなので、DependsでなくてHTTP Bearer決め内でもいいかもしれません

api/app/main.py Outdated

# Dependency injection as needed
app.dependency_overrides[get_firebase_credentials] = lambda: cred
app.dependency_overrides[get_auth_module] = override_get_auth_module
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
app.dependency_overrides[get_auth_module] = override_get_auth_module
app.dependency_overrides[get_auth_module] = lambda: auth_module

@dejima-shikou
Copy link
Collaborator Author

@mshim03
指摘の通り修正しました。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants