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

Use google default for auth when no service_account_json file specified #383

Closed
wants to merge 9 commits into from

Conversation

devonh
Copy link
Contributor

@devonh devonh commented Jun 10, 2024

This allows the use of various other mechanisms to acquire google application credentials.
See here for more info:
https://cloud.google.com/docs/authentication#auth-decision-tree
https://cloud.google.com/docs/authentication/application-default-credentials

To achieve the same behaviour as specifying service_account_json in the config, set the env var GOOGLE_APPLICATION_CREDENTIALS="/path/to/service_account.json"

Note: This has the downside of applying the same google credentials for all GCM pushers.
If you desire different google credentials for different pushkins running on the same Sygnal instance, setting the service_account_json is the only way to achieve that.

@devonh devonh requested a review from a team as a code owner June 10, 2024 20:13
@devonh devonh changed the title Switch to google default for auth Use google default for auth when no service_account_json file specified Jun 10, 2024
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

A few questions, but nothing major.

"Config field fcm_options, if set, must be a dictionary of options"
)

def _load_credentials(self, proxy_url: Optional[str]) -> None:
self.credentials: Optional[Credentials] = None

if self.api_version is APIVersion.V1:
self.service_account_file = self.get_config("service_account_file", str)
Copy link
Member

Choose a reason for hiding this comment

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

We seem to be trying the service_account_file config option first, before attempting the Application Default Credentials route. This seems counter-intuitive to me, given that environment variables typically trump config options when defined. And we recommend Application Default Credentials over the service_account_file config option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only try that option first if the user has explicitly configured it.
I wanted to keep this option to prevent breakages and forcing anyone who is using the existing setup with the v1 api to have to change their deployment method.

The recommendation of using ADC is because google recommends it for security reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could try the Application Default Credentials route first, but that leads to it trying for ~5 seconds before timing out. I didn't want to make startup longer if you had explicitly configured the service_account_file

tests/test_gcm.py Outdated Show resolved Hide resolved
tests/test_gcm.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@devonh
Copy link
Contributor Author

devonh commented Jun 20, 2024

After thinking about this more, we need to be able to specify the service account file per pusher. Which means a global environment variable does not fit the bill.
Closing this PR.

@devonh devonh closed this Jun 20, 2024
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