feat: Ensure service account file exists before initializing credentials#386
feat: Ensure service account file exists before initializing credentials#386cacheflow wants to merge 3 commits into
Conversation
|
Hey @olucurious or @Niccari when y'all have a moment, can you review this PR? |
Niccari
left a comment
There was a problem hiding this comment.
Thanks for the fix! I've reviewed it and here are my comments below. I've also prepared a suggestion branch (https://github.com/Niccari/PyFCM/tree/fix-suggestion-ensure-service-account-file-exists) — feel free to pull it in if it looks good.
| missing_account_file = not path.isfile(self._service_account_file) | ||
|
|
||
| if missing_account_file: | ||
| raise InvalidDataError(f"The service account file you passed does not exist at '{path}'. " |
There was a problem hiding this comment.
{path} here refers to the os.path module imported above, not the file path string. This will render as something like <module 'posixpath' from '...'> in the error message. It should be {self._service_account_file}.
| raise InvalidDataError(f"The service account file you passed does not exist at '{path}'. " | |
| if not path.isfile(self._service_account_file): | |
| raise InvalidDataError( | |
| f"The service account file you passed does not exist at '{self._service_account_file}'. " | |
| "Ensure it does not have any typos and exists." | |
| ) |
There was a problem hiding this comment.
Thanks for the feedback. I've incorporated the change.
|
|
||
| def test_push_service_with_incorrect_service_account_file(): | ||
| try: | ||
| # figure out why this goddamn test is not running |
There was a problem hiding this comment.
This looks like a leftover note. Could you remove it before merging? (FYI, the test does run — notify() calls send_request → request_headers → _get_access_token → _initialize_credentials.)
| # figure out why this goddamn test is not running | |
| fcm = FCMNotification(service_account_file='./foo.json', project_id=None, credentials=None) |
There was a problem hiding this comment.
Thanks for the updates — the error message and the comment are both addressed now. However, this revision introduces a runtime regression I'd like to flag before merge. See the inline comment.
I've updated my suggestion branch (https://github.com/Niccari/PyFCM/tree/fix-suggestion-ensure-service-account-file-exists), rebased on this PR's latest commit (see: 08041af), which restores the correct class and adds a regression test. Feel free to pull it in.
| raise InvalidDataError(f"The service account file you passed does not exist at '{self._service_account_file}'. " | ||
| "Ensure it does not have any typos and exists." | ||
| ) | ||
| self.credentials = Credentials.from_service_account_file( |
There was a problem hiding this comment.
This is a runtime regression. Credentials here is google.oauth2.credentials.Credentials (the user/OAuth2 access-token class), which has no from_service_account_file classmethod. With a valid service_account_file, this raises:
AttributeError: type object 'Credentials' has no attribute 'from_service_account_file'
Only google.oauth2.service_account.Credentials provides from_service_account_file, which is why the original code imported and used service_account. The removal of from google.oauth2 import service_account (line 11) needs to be reverted as well.
| self.credentials = Credentials.from_service_account_file( | |
| self.credentials = service_account.Credentials.from_service_account_file( |
There was a problem hiding this comment.
You're right. I saw two credential files in the upstream Google lib and pulled in the wrong one.
There was a problem hiding this comment.
Thanks again. I've brought in the changes from your branch.
There was a problem hiding this comment.
Thanks, LGTM! 🎉
note - As a non-maintainer reviewer I don't have permission to trigger it.
so @olucurious would need to approve the workflow run to get CI to execute.
This PR adds a validation check to ensure that the service_account_file path passed to FCM exists before attempting to initialize credentials using Google OAuth.
References #385.