Feat add firebase auth kath 070224 #2
Conversation
… but not tested. Sign Up not yet implemented
Missing: tests, some error handling, clean-up
| @@ -0,0 +1,32 @@ | |||
| """added firebase_id column to User table | |||
There was a problem hiding this comment.
next time, use a shorter name because this becomes the file name.
| alembic | ||
| email-validator | ||
| fastapi | ||
| firebase_admin |
There was a problem hiding this comment.
you need to execute ./update-deps.sh when you make changes to the dependencies. that will change the lock files, which need to be a part of this PR.
There was a problem hiding this comment.
Did you do that or do I still need to do that?
|
|
||
|
|
||
| def verify_firebase_token(authorization: str): | ||
| # TODO: adding delay is a hack fix for clock skew issue with firebase - find better solution? |
There was a problem hiding this comment.
i thought you were not gonna do this approach?
There was a problem hiding this comment.
I moved away from using firebase to authenticate each API request. However, I still need to authenticate with firebase when they log in which requires this hack.
| if ENV == "test": | ||
| log.debug("Bypass token verification") | ||
| return 'fakeid' | ||
| try: |
There was a problem hiding this comment.
what is the point of this exception handling? i think you can take it out.
There was a problem hiding this comment.
catches errors like if the token is invalid or expired
| class UserFirebase(User): | ||
| firebase_id: str | ||
|
|
||
| class UserAuth(User): |
There was a problem hiding this comment.
I don't like having so many variants of the User class. I wonder if we should separate the access-token from the User. Instead, make it an Auth object. If needed, we can have endpoints that return both a User and an Auth.
There was a problem hiding this comment.
I'll leave this one to you.
| email: EmailStr | ||
| username: str = Field(..., min_length=3, max_length=15) | ||
| # TODO: need to inform user of min/max user name constraint | ||
| # or create allert in the event that credentials don't meet specifications |
There was a problem hiding this comment.
| # or create allert in the event that credentials don't meet specifications | |
| # or create alert in the event that credentials don't meet specifications |
| @@ -1 +1,3 @@ | |||
| export ENV=dev | |||
There was a problem hiding this comment.
I don't like this. The default is already dev. If you put this here, you can't run the prod-debug env, or the test env, with this script.
There was a problem hiding this comment.
so...just delete the line?
Co-authored-by: Nico <[email protected]>
add nico's comment Co-authored-by: Nico <[email protected]>
explicitly make firebase_id column nullable Co-authored-by: Nico <[email protected]>
firebase authentication + jwt access tokens. Working (yay!) but still needs clean up and testing.