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
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
12 changes: 10 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,18 @@ For API v1, it expects:
- the `project_id` parameter to contain the `Project ID`,
which can be acquired from Firebase Console at:
`https://console.cloud.google.com/project/<PROJECT NAME>/settings/general/`
- the `service_account_file` parameter to contain the path to the service account file,
which can be acquired from Firebase Console at:

API v1 also expects one of the following to be configured:
- the [Application Default Credentials](https://cloud.google.com/docs/authentication/application-default-credentials) configured on the local system.
This can point to a service account file which can be acquired from Firebase Console at:
`https://console.firebase.google.com/project/<PROJECT NAME>/settings/serviceaccounts/adminsdk`

**OR..**

- the `service_account_file` config parameter to contain the path to the service account file

It is recommended to use the application default credentials method to acquire google credentials.
devonh marked this conversation as resolved.
Show resolved Hide resolved

Using an HTTP Proxy for outbound traffic
----------------------------------------

Expand Down
1 change: 1 addition & 0 deletions changelog.d/383.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Switch to google application-default-credentials for firebase auth.
2 changes: 2 additions & 0 deletions scripts-dev/proxy-test/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ services:
no-internet:
ipv4_address: 172.28.0.2
container_name: sygnal
environment:
GOOGLE_APPLICATION_CREDENTIALS: "/service_account.json"
volumes:
- ./sygnal.yaml:/sygnal.yaml
- ./service_account.json:/service_account.json:ro
Expand Down
1 change: 0 additions & 1 deletion scripts-dev/proxy-test/sygnal.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,3 @@ apps:
type: gcm
api_version: v1
project_id: <PROJECT_ID>
service_account_file: /service_account.json
1 change: 0 additions & 1 deletion sygnal.yaml.sample
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ apps:
# #api_key:
# api_version: v1
# project_id: project-id
# service_account_file: /path/to/service_account.json
#
# # This is the maximum number of connections to GCM servers at any one time
# # the default is 20.
Expand Down
53 changes: 30 additions & 23 deletions sygnal/gcmpushkin.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
# https://github.com/googleapis/google-auth-library-python/issues/613
import aiohttp
import google.auth.transport._aiohttp_requests
from google.auth._default_async import load_credentials_from_file
from google.auth._default_async import default_async, load_credentials_from_file
from google.oauth2._credentials_async import Credentials
from opentracing import Span, logs, tags
from prometheus_client import Counter, Gauge, Histogram
Expand Down Expand Up @@ -186,23 +186,39 @@ def __init__(self, name: str, sygnal: "Sygnal", config: Dict[str, Any]) -> None:
"Must configure `project_id` when using FCM api v1",
)

self._load_credentials(proxy_url)

# Use the fcm_options config dictionary as a foundation for the body;
# this lets the Sygnal admin choose custom FCM options
# (e.g. content_available).
self.base_request_body = self.get_config("fcm_options", dict, {})
if not isinstance(self.base_request_body, dict):
raise PushkinSetupException(
"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

if not self.service_account_file:
raise PushkinSetupException(
"Must configure `service_account_file` when using FCM api v1",
)
try:
self.credentials, _ = load_credentials_from_file(
str(self.service_account_file),
scopes=AUTH_SCOPES,
)
except google.auth.exceptions.DefaultCredentialsError as e:
raise PushkinSetupException(
f"`service_account_file` must be valid: {str(e)}",
)
if self.service_account_file:
try:
self.credentials, _ = load_credentials_from_file(
str(self.service_account_file),
scopes=AUTH_SCOPES,
)
except google.auth.exceptions.DefaultCredentialsError as e:
raise PushkinSetupException(
f"`service_account_file` must be valid: {str(e)}",
)
else:
try:
self.credentials, _ = default_async(scopes=AUTH_SCOPES)
except google.auth.exceptions.DefaultCredentialsError as e:
raise PushkinSetupException(
f"Failed loading google credentials: {str(e)}",
)

session = None
if proxy_url:
Expand All @@ -215,15 +231,6 @@ def __init__(self, name: str, sygnal: "Sygnal", config: Dict[str, Any]) -> None:
session=session
)

# Use the fcm_options config dictionary as a foundation for the body;
# this lets the Sygnal admin choose custom FCM options
# (e.g. content_available).
self.base_request_body = self.get_config("fcm_options", dict, {})
if not isinstance(self.base_request_body, dict):
raise PushkinSetupException(
"Config field fcm_options, if set, must be a dictionary of options"
)

@classmethod
async def create(
cls, name: str, sygnal: "Sygnal", config: Dict[str, Any]
Expand Down
35 changes: 7 additions & 28 deletions tests/test_gcm.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import json
import tempfile
from typing import TYPE_CHECKING, Any, AnyStr, Dict, List, Tuple
from typing import TYPE_CHECKING, Any, AnyStr, Dict, List, Optional, Tuple
from unittest.mock import MagicMock

from sygnal.gcmpushkin import APIVersion, GcmPushkin
Expand Down Expand Up @@ -108,8 +107,6 @@ def __init__(self, name: str, sygnal: "Sygnal", config: Dict[str, Any]):
self.last_request_body: Dict[str, Any] = {}
self.last_request_headers: Dict[AnyStr, List[AnyStr]] = {} # type: ignore[valid-type]
self.num_requests = 0
if self.api_version is APIVersion.V1:
self.credentials = TestCredentials() # type: ignore[assignment]

def preload_with_response(
self, code: int, response_payload: Dict[str, Any]
Expand All @@ -120,6 +117,12 @@ def preload_with_response(
self.preloaded_response = DummyResponse(code)
self.preloaded_response_payload = response_payload

def _load_credentials(self, proxy_url: Optional[str]) -> None:
if self.api_version is APIVersion.V1:
self.credentials = TestCredentials() # type: ignore[assignment]
self.google_auth_request = None # type: ignore[assignment]
pass
devonh marked this conversation as resolved.
Show resolved Hide resolved

async def _perform_http_request( # type: ignore[override]
self, body: Dict[str, Any], headers: Dict[AnyStr, List[AnyStr]]
) -> Tuple[DummyResponse, str]:
Expand All @@ -134,23 +137,6 @@ async def _refresh_credentials(self) -> None:
await self.credentials.refresh(self.google_auth_request)


FAKE_SERVICE_ACCOUNT_FILE = b"""
{
"type": "service_account",
"project_id": "project_id",
"private_key_id": "private_key_id",
"private_key": "-----BEGIN PRIVATE KEY-----\\nMIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQC0PwE6TeTHjD5R\\nY2nOw1rsTgQZ38LCR2CLtx36n+LUkgej/9b+fwC88oKIqJKjUwn43JEOhf4rbA/a\\nqo4jVoLgv754G5+7Glfarr3/rqg+AVT75x6J5DRvhIYpDXwMIUqLAAbfk3TTFNJn\\n2ctrkBF2ZP9p3mzZ3NRjU63Wbf3LBpRqs8jdFEQu8JAecG8VKV1mboJIXG3hwqFN\\nJmcpC/+sWaxB5iMgSqy0w/rGFs6ZbZF6D10XYvf40lEEk9jQIovT+QD4+6GTlroT\\nbOk8uIwxFQcwMFpXj4MktqVNSNyiuuttptIvBWcMWHlaabXrR89vqUFe1g1Jx4GL\\nCF89RrcLAgMBAAECggEAPUYZ3b8zId78JGDeTEq+8wwGeuFFbRQkrvpeN5/41Xib\\nHlZPuQ5lqtXqKBjeWKVXA4G/0icc45gFv7kxPrQfI9YrItuJLmrjKNU0g+HVEdcU\\nE9pa2Fd6t9peXUBXRixfEee9bm3LTiKK8IDqlTNRrGTjKxNQ/7MBhI6izv1vRH/x\\n8i0o1xxNdqstHZ9wBFKYO9w8UQjtfzckkBNDLkaJ/WN0BoRubmUiV1+KwAyyBr6O\\nRnnZ9Tvy8VraSNSdJhX36ai36y18/sT6PWOp99zHYuDyz89KIz1la/fT9eSoR0Jy\\nYePmTEi+9pWhvtpAkqJkRxe5IDz71JVsQ07KoVfzaQKBgQDzKKUd/0ujhv/B9MQf\\nHcwSeWu/XnQ4hlcwz8dTWQjBV8gv9l4yBj9Pra62rg/tQ7b5XKMt6lv/tWs1IpdA\\neMsySY4972VPrmggKXgCnyKckDUYydNtHAIj9buo6AV8rONaneYnGv5wpSsf3q2c\\nOZrkamRgbBkI+B2mZ2obH1oVlQKBgQC9w9HkrDMvZ5L/ilZmpsvoHNFlQwmDgNlN\\n0ej5QGID5rljRM3CcLNHdyQiKqvLA9MCpPEXb2vVJPdmquD12A7a9s0OwxB/dtOD\\nykofcTY0ZHEM1HEyYJGmdK4FvZuNU4o2/D268dePjtj1Xw3c5fs0bcDiGQMtjWlz\\n5hjBzMsyHwKBgGjrIsPcwlBfEcAo0u7yNnnKNnmuUcuJ+9kt7j3Cbwqty80WKvK+\\ny1agBIECfhDMZQkXtbk8JFIjf4y/zi+db1/VaTDEORy2jmtCOWw4KgEQIDj/7OBp\\nc2r8vupUovl2x+rzsrkw5pTIT+FCffqoyHLCjWkle2/pTzHb8Waekoo5AoGAbELk\\nYy5uwTO45Hr60fOEzzZpq/iz28dNshz4agL2KD2gNGcTcEO1tCbfgXKQsfDLmG2b\\ncgBKJ77AOl1wnDEYQIme8TYOGnojL8Pfx9Jh10AaUvR8Y/49+hYFFhdXQCiR6M69\\nNQM2NJuNYWdKVGUMjJu0+AjHDFzp9YonQ6Ffp4cCgYEAmVALALCjU9GjJymgJ0lx\\nD9LccVHMwf9NmR/sMg0XNePRbCEcMDHKdtVJ1zPGS5txuxY3sRb/tDpv7TfuitrU\\nAw0/2ooMzunaoF/HXo+C/+t+pfuqPqLK4sCCyezUlMfCcaPdwXN2FmbgsaFHfe7I\\n7sGEnS/d8wEgydMiptJEf9s=\\n-----END PRIVATE KEY-----\\n",
"client_email": "firebase-adminsdk@project_id.iam.gserviceaccount.com",
"client_id": "client_id",
"auth_uri": "https://accounts.google.com/o/oauth2/auth",
"token_uri": "https://oauth2.googleapis.com/token",
"auth_provider_x509_cert_url": "https://www.googleapis.com/oauth2/v1/certs",
"client_x509_cert_url": "https://www.googleapis.com/robot/v1/metadata/x509/firebase-adminsdk%40project_id.iam.gserviceaccount.com",
"universe_domain": "googleapis.com"
}
"""


class GcmTestCase(testutils.TestCase):
maxDiff = None

Expand All @@ -165,14 +151,10 @@ def config_setup(self, config: Dict[str, Any]) -> None:
"api_key": "kii",
"fcm_options": {"content_available": True, "mutable_content": True},
}
self.service_account_file = tempfile.NamedTemporaryFile()
self.service_account_file.write(FAKE_SERVICE_ACCOUNT_FILE)
self.service_account_file.flush()
config["apps"]["com.example.gcm.apiv1"] = {
"type": "tests.test_gcm.TestGcmPushkin",
"api_version": "v1",
"project_id": "example_project",
"service_account_file": self.service_account_file.name,
devonh marked this conversation as resolved.
Show resolved Hide resolved
"fcm_options": {
"android": {
"notification": {
Expand All @@ -193,9 +175,6 @@ def config_setup(self, config: Dict[str, Any]) -> None:
},
}

def tearDown(self) -> None:
self.service_account_file.close()

def get_test_pushkin(self, name: str) -> TestGcmPushkin:
pushkin = self.sygnal.pushkins[name]
assert isinstance(pushkin, TestGcmPushkin)
Expand Down
Loading