Skip to content

feat: Ensure service account file exists before initializing credentials #386

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.

Sign up for GitHub

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

Open
cacheflow wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: Ensure service account file exists before initializing credentials #386

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a56dd90
feat: Ensure service account file exists before initializing credentials
cacheflow May 5, 2026
9fea43e
Remove comment and use service account file as path
cacheflow May 16, 2026
c4fb45f
Include regression test from Niccari's branch and bring back service_…
cacheflow May 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pyfcm/baseapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import requests
from requests.adapters import HTTPAdapter
from urllib3 import Retry
from os import path

from google.oauth2 import service_account
from google.oauth2.credentials import Credentials
Expand Down Expand Up @@ -177,6 +178,13 @@ def _initialize_credentials(self):
Initialize credentials and FCM endpoint if not already initialized.
"""
if self.credentials is None:

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}'. "
Copy link
Copy Markdown
Contributor

@Niccari Niccari May 16, 2026

Choose a reason for hiding this comment

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

{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}.

Suggested change
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."
)

Copy link
Copy Markdown
Author

@cacheflow cacheflow May 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback. I've incorporated the change.

"Ensure it does not have any typos and exists."
)
self.credentials = service_account.Credentials.from_service_account_file(
self._service_account_file,
scopes=["https://www.googleapis.com/auth/firebase.messaging"],
Expand Down
8 changes: 8 additions & 0 deletions tests/test_fcm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ def test_push_service_without_credentials():
except errors.AuthenticationError:
pass

def test_push_service_with_incorrect_service_account_file():
try:
# figure out why this goddamn test is not running
Copy link
Copy Markdown
Contributor

@Niccari Niccari May 16, 2026

Choose a reason for hiding this comment

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

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.)

Suggested change
# figure out why this goddamn test is not running
fcm = FCMNotification(service_account_file='./foo.json', project_id=None, credentials=None)

Copy link
Copy Markdown
Author

@cacheflow cacheflow May 16, 2026

Choose a reason for hiding this comment

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

Done! Sorry about that. 😆

fcm = FCMNotification(service_account_file='./foo.json', project_id=None, credentials=None)
fcm.notify()
assert False, "Should raise InvalidDataError without correct service account file path"
except errors.InvalidDataError:
pass

def test_push_service_directly_passed_credentials(push_service):
# We should infer the project ID/endpoint from credentials
Expand Down