-
Notifications
You must be signed in to change notification settings - Fork 340
Implementing App Default and Refresh Token Credential Types #6
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
Conversation
…ce it does not work on python 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good stuff! Handful of nits but otherwise LGTM. I assume you will make the Application Default Credential be initialized when no credential is provided to firebase.App
in a follow-up PR?
firebase_admin/credentials.py
Outdated
|
||
|
||
class ApplicationDefault(Base): | ||
"""A Google application default credential.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: capitalize "Application" and "Default" (multiple places in this file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
firebase_admin/credentials.py
Outdated
"""A credential initialized from an existing refresh token.""" | ||
|
||
def __init__(self, file_path): | ||
"""Initialized a refresh token credential from the specified JSON file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Initialized/Initializes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tests/test_credentials.py
Outdated
credentials._http = testutils.HttpMock(200, json.dumps(mock_response)) | ||
access_token = credential.get_access_token() | ||
assert access_token.access_token == 'mock_access_token' | ||
assert access_token.expires_in <= 1234 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this <=
instead of ==
? If it's non-obvious, please leave a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because GoogleCredentials class recalculates the expires_in property before returning. Added a comment to mention that.
Made the suggested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.