From 18ec70241c496af7c73cfc1a49cfd823bdab7ce9 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Tue, 20 Jul 2021 17:42:06 -0700 Subject: [PATCH 01/12] fix: move to using insecure grpc channels with emulator --- google/cloud/firestore_v1/base_client.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/google/cloud/firestore_v1/base_client.py b/google/cloud/firestore_v1/base_client.py index b2af21e3f..51a67299f 100644 --- a/google/cloud/firestore_v1/base_client.py +++ b/google/cloud/firestore_v1/base_client.py @@ -177,15 +177,20 @@ def _emulator_channel(self, transport): # TODO: Implement a special credentials type for emulator and use # "transport.create_channel" to create gRPC channels once google-auth # extends it's allowed credentials types. + # if "GrpcAsyncIOTransport" in str(transport.__name__): + # return grpc.aio.secure_channel( + # self._emulator_host, self._local_composite_credentials() + # ) + # else: + # return grpc.secure_channel( + # self._emulator_host, self._local_composite_credentials() + # ) if "GrpcAsyncIOTransport" in str(transport.__name__): - return grpc.aio.secure_channel( - self._emulator_host, self._local_composite_credentials() - ) + return grpc.aio.insecure_channel( + self._emulator_host) else: - return grpc.secure_channel( - self._emulator_host, self._local_composite_credentials() - ) - + return grpc.insecure_channel( + self._emulator_host) def _local_composite_credentials(self): """ Creates the credentials for the local emulator channel From 5a703f3703a32b3f9143b5b5b88c5b7550905d32 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Wed, 21 Jul 2021 09:30:06 -0700 Subject: [PATCH 02/12] chore: format --- google/cloud/firestore_v1/base_client.py | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/google/cloud/firestore_v1/base_client.py b/google/cloud/firestore_v1/base_client.py index 51a67299f..6cc813f3c 100644 --- a/google/cloud/firestore_v1/base_client.py +++ b/google/cloud/firestore_v1/base_client.py @@ -177,20 +177,11 @@ def _emulator_channel(self, transport): # TODO: Implement a special credentials type for emulator and use # "transport.create_channel" to create gRPC channels once google-auth # extends it's allowed credentials types. - # if "GrpcAsyncIOTransport" in str(transport.__name__): - # return grpc.aio.secure_channel( - # self._emulator_host, self._local_composite_credentials() - # ) - # else: - # return grpc.secure_channel( - # self._emulator_host, self._local_composite_credentials() - # ) if "GrpcAsyncIOTransport" in str(transport.__name__): - return grpc.aio.insecure_channel( - self._emulator_host) + return grpc.aio.insecure_channel(self._emulator_host) else: - return grpc.insecure_channel( - self._emulator_host) + return grpc.insecure_channel(self._emulator_host) + def _local_composite_credentials(self): """ Creates the credentials for the local emulator channel From 6140ba5552293c938c66c0b416c92b1e1f4f02c7 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Wed, 21 Jul 2021 11:54:27 -0700 Subject: [PATCH 03/12] fix: add code to manually inject the id token on an insecure channel --- google/cloud/firestore_v1/base_client.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/google/cloud/firestore_v1/base_client.py b/google/cloud/firestore_v1/base_client.py index 6cc813f3c..fb4a19bb1 100644 --- a/google/cloud/firestore_v1/base_client.py +++ b/google/cloud/firestore_v1/base_client.py @@ -167,20 +167,24 @@ def _firestore_api_helper(self, transport, client_class, client_module) -> Any: def _emulator_channel(self, transport): """ - Creates a channel using self._credentials in a similar way to grpc.secure_channel but - using grpc.local_channel_credentials() rather than grpc.ssh_channel_credentials() to allow easy connection - to a local firestore emulator. This allows local testing of firestore rules if the credentials have been - created from a signed custom token. + Creates an insecure channel to communicate with the local emulator. + If credentials are provided the token is extracted and added to the + headers. this supports local testing of firestore rules if the credentials + have been created from a signed custom token. :return: grpc.Channel or grpc.aio.Channel """ - # TODO: Implement a special credentials type for emulator and use - # "transport.create_channel" to create gRPC channels once google-auth - # extends it's allowed credentials types. + # Insecure channels are used for the emulator as secure channels + # cannot be used to communicate on some environments. https://github.com/googleapis/python-firestore/issues/359 + options = [] + if self._credentials is not None and self._credentials.id_token is not None: + options.append(("Authorization", f"Bearer {self._credentials.id_token}")) + + channel = None if "GrpcAsyncIOTransport" in str(transport.__name__): - return grpc.aio.insecure_channel(self._emulator_host) + return grpc.aio.insecure_channel(self._emulator_host, options=options) else: - return grpc.insecure_channel(self._emulator_host) + return grpc.insecure_channel(self._emulator_host, options=options) def _local_composite_credentials(self): """ From 935bfeeed2d92da50651affd34e5eaa74d6d7d3e Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Wed, 21 Jul 2021 11:54:50 -0700 Subject: [PATCH 04/12] chore: add line for comment --- google/cloud/firestore_v1/base_client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/google/cloud/firestore_v1/base_client.py b/google/cloud/firestore_v1/base_client.py index fb4a19bb1..4dba9f25b 100644 --- a/google/cloud/firestore_v1/base_client.py +++ b/google/cloud/firestore_v1/base_client.py @@ -175,7 +175,8 @@ def _emulator_channel(self, transport): :return: grpc.Channel or grpc.aio.Channel """ # Insecure channels are used for the emulator as secure channels - # cannot be used to communicate on some environments. https://github.com/googleapis/python-firestore/issues/359 + # cannot be used to communicate on some environments. + # https://github.com/googleapis/python-firestore/issues/359 options = [] if self._credentials is not None and self._credentials.id_token is not None: options.append(("Authorization", f"Bearer {self._credentials.id_token}")) From e0cdd188899fab1df3b743732f11507f5280b0c8 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Wed, 21 Jul 2021 13:06:08 -0700 Subject: [PATCH 05/12] test: use the correct credentials object in mock --- tests/unit/v1/test_base_client.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/unit/v1/test_base_client.py b/tests/unit/v1/test_base_client.py index fd176d760..b3838e192 100644 --- a/tests/unit/v1/test_base_client.py +++ b/tests/unit/v1/test_base_client.py @@ -392,10 +392,9 @@ def test_paths(self): def _make_credentials(): - import google.auth.credentials - - return mock.Mock(spec=google.auth.credentials.Credentials) + import google.oauth2.credentials + return mock.Mock(spec=google.oauth2.credentials.Credentials) def _make_batch_response(**kwargs): from google.cloud.firestore_v1.types import firestore From e29fda8fd974386b7c02c7b08ba2b7e1f2cf500e Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Wed, 21 Jul 2021 13:29:02 -0700 Subject: [PATCH 06/12] chore: black --- tests/unit/v1/test_base_client.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/v1/test_base_client.py b/tests/unit/v1/test_base_client.py index b3838e192..8a4fa5f63 100644 --- a/tests/unit/v1/test_base_client.py +++ b/tests/unit/v1/test_base_client.py @@ -396,6 +396,7 @@ def _make_credentials(): return mock.Mock(spec=google.oauth2.credentials.Credentials) + def _make_batch_response(**kwargs): from google.cloud.firestore_v1.types import firestore From 83fc86b0c37380ba01584781c97855a12be04c11 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Wed, 21 Jul 2021 14:26:21 -0700 Subject: [PATCH 07/12] chore: unused var --- google/cloud/firestore_v1/base_client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/google/cloud/firestore_v1/base_client.py b/google/cloud/firestore_v1/base_client.py index 4dba9f25b..417a56336 100644 --- a/google/cloud/firestore_v1/base_client.py +++ b/google/cloud/firestore_v1/base_client.py @@ -181,7 +181,6 @@ def _emulator_channel(self, transport): if self._credentials is not None and self._credentials.id_token is not None: options.append(("Authorization", f"Bearer {self._credentials.id_token}")) - channel = None if "GrpcAsyncIOTransport" in str(transport.__name__): return grpc.aio.insecure_channel(self._emulator_host, options=options) else: From bf334ba540499fa8b378b14222ba5af31b7c385f Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Wed, 21 Jul 2021 15:05:11 -0700 Subject: [PATCH 08/12] always configure the bearer token, even if not available --- google/cloud/firestore_v1/base_client.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/google/cloud/firestore_v1/base_client.py b/google/cloud/firestore_v1/base_client.py index 417a56336..959805486 100644 --- a/google/cloud/firestore_v1/base_client.py +++ b/google/cloud/firestore_v1/base_client.py @@ -177,9 +177,10 @@ def _emulator_channel(self, transport): # Insecure channels are used for the emulator as secure channels # cannot be used to communicate on some environments. # https://github.com/googleapis/python-firestore/issues/359 - options = [] + token = "owner" if self._credentials is not None and self._credentials.id_token is not None: - options.append(("Authorization", f"Bearer {self._credentials.id_token}")) + token = self._credentials.id_token + options = [("Authorization", f"Bearer {token}")] if "GrpcAsyncIOTransport" in str(transport.__name__): return grpc.aio.insecure_channel(self._emulator_host, options=options) From bec5c82b42f5187555c54eacced899214c443944 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Wed, 21 Jul 2021 15:34:53 -0700 Subject: [PATCH 09/12] test: test the path populating an id token --- google/cloud/firestore_v1/base_client.py | 2 +- tests/unit/v1/test_base_client.py | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/google/cloud/firestore_v1/base_client.py b/google/cloud/firestore_v1/base_client.py index 959805486..2e7802f22 100644 --- a/google/cloud/firestore_v1/base_client.py +++ b/google/cloud/firestore_v1/base_client.py @@ -170,7 +170,7 @@ def _emulator_channel(self, transport): Creates an insecure channel to communicate with the local emulator. If credentials are provided the token is extracted and added to the headers. this supports local testing of firestore rules if the credentials - have been created from a signed custom token. + have been created from a signed custom token. :return: grpc.Channel or grpc.aio.Channel """ diff --git a/tests/unit/v1/test_base_client.py b/tests/unit/v1/test_base_client.py index 8a4fa5f63..12a7ac37e 100644 --- a/tests/unit/v1/test_base_client.py +++ b/tests/unit/v1/test_base_client.py @@ -176,6 +176,27 @@ def test_emulator_channel(self): ) ) + # Verify that when credentials are provided with an id token it is used + # for channel construction + # NOTE: On windows, emulation requires an insecure channel. If this is + # altered to use a secure channel, start by verifying that it still + # works as expected on windows. + emulator_host = "localhost:8081" + with mock.patch("os.getenv") as getenv: + getenv.return_value = emulator_host + + credentials = _make_credentials() + credentials.id_token = "test" + database = "quanta" + client = self._make_one( + project=self.PROJECT, credentials=credentials, database=database + ) + with mock.patch("grpc.insecure_channel") as insecure_channel: + channel = client._emulator_channel(FirestoreGrpcTransport) + insecure_channel.assert_called_once_with( + emulator_host, options=[("Authorization", "Bearer test")] + ) + def test_field_path(self): klass = self._get_target_class() self.assertEqual(klass.field_path("a", "b", "c"), "a.b.c") From 4699a1bdfcc8eff907af650609935c502045bfd8 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Wed, 21 Jul 2021 15:38:28 -0700 Subject: [PATCH 10/12] chore: remove unused code and testing of unused code --- google/cloud/firestore_v1/base_client.py | 26 ------------------------ tests/unit/v1/test_base_client.py | 15 -------------- 2 files changed, 41 deletions(-) diff --git a/google/cloud/firestore_v1/base_client.py b/google/cloud/firestore_v1/base_client.py index 2e7802f22..56e67caf6 100644 --- a/google/cloud/firestore_v1/base_client.py +++ b/google/cloud/firestore_v1/base_client.py @@ -187,32 +187,6 @@ def _emulator_channel(self, transport): else: return grpc.insecure_channel(self._emulator_host, options=options) - def _local_composite_credentials(self): - """ - Creates the credentials for the local emulator channel - :return: grpc.ChannelCredentials - """ - credentials = google.auth.credentials.with_scopes_if_required( - self._credentials, None - ) - request = google.auth.transport.requests.Request() - - # Create the metadata plugin for inserting the authorization header. - metadata_plugin = google.auth.transport.grpc.AuthMetadataPlugin( - credentials, request - ) - - # Create a set of grpc.CallCredentials using the metadata plugin. - google_auth_credentials = grpc.metadata_call_credentials(metadata_plugin) - - # Using the local_credentials to allow connection to emulator - local_credentials = grpc.local_channel_credentials() - - # Combine the local credentials and the authorization credentials. - return grpc.composite_channel_credentials( - local_credentials, google_auth_credentials - ) - def _target_helper(self, client_class) -> str: """Return the target (where the API is). Eg. "firestore.googleapis.com" diff --git a/tests/unit/v1/test_base_client.py b/tests/unit/v1/test_base_client.py index 12a7ac37e..3dd414354 100644 --- a/tests/unit/v1/test_base_client.py +++ b/tests/unit/v1/test_base_client.py @@ -160,21 +160,6 @@ def test_emulator_channel(self): self.assertTrue(isinstance(channel, grpc.Channel)) channel = client._emulator_channel(FirestoreGrpcAsyncIOTransport) self.assertTrue(isinstance(channel, grpc.aio.Channel)) - # checks that the credentials are composite ones using a local channel from grpc - composite_credentials = client._local_composite_credentials() - self.assertTrue(isinstance(composite_credentials, grpc.ChannelCredentials)) - self.assertTrue( - isinstance( - composite_credentials._credentials._call_credentialses[0], - grpc._cython.cygrpc.MetadataPluginCallCredentials, - ) - ) - self.assertTrue( - isinstance( - composite_credentials._credentials._channel_credentials, - grpc._cython.cygrpc.LocalChannelCredentials, - ) - ) # Verify that when credentials are provided with an id token it is used # for channel construction From b2af8b2d2d4988a9419f94fab376f49901172047 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Wed, 21 Jul 2021 15:39:55 -0700 Subject: [PATCH 11/12] chore: remove some code repetition --- tests/unit/v1/test_base_client.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/unit/v1/test_base_client.py b/tests/unit/v1/test_base_client.py index 3dd414354..5de0e4962 100644 --- a/tests/unit/v1/test_base_client.py +++ b/tests/unit/v1/test_base_client.py @@ -146,11 +146,11 @@ def test_emulator_channel(self): ) emulator_host = "localhost:8081" + credentials = _make_credentials() + database = "quanta" with mock.patch("os.getenv") as getenv: getenv.return_value = emulator_host - - credentials = _make_credentials() - database = "quanta" + credentials.id_token = None client = self._make_one( project=self.PROJECT, credentials=credentials, database=database ) @@ -166,13 +166,9 @@ def test_emulator_channel(self): # NOTE: On windows, emulation requires an insecure channel. If this is # altered to use a secure channel, start by verifying that it still # works as expected on windows. - emulator_host = "localhost:8081" with mock.patch("os.getenv") as getenv: getenv.return_value = emulator_host - - credentials = _make_credentials() credentials.id_token = "test" - database = "quanta" client = self._make_one( project=self.PROJECT, credentials=credentials, database=database ) From ae185e139b99980ed1865efda88deddd387aa300 Mon Sep 17 00:00:00 2001 From: Chris Wilcox Date: Wed, 21 Jul 2021 16:21:24 -0700 Subject: [PATCH 12/12] chore: feedback --- google/cloud/firestore_v1/base_client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/google/cloud/firestore_v1/base_client.py b/google/cloud/firestore_v1/base_client.py index 56e67caf6..7eb5c26b0 100644 --- a/google/cloud/firestore_v1/base_client.py +++ b/google/cloud/firestore_v1/base_client.py @@ -169,7 +169,7 @@ def _emulator_channel(self, transport): """ Creates an insecure channel to communicate with the local emulator. If credentials are provided the token is extracted and added to the - headers. this supports local testing of firestore rules if the credentials + headers. This supports local testing of firestore rules if the credentials have been created from a signed custom token. :return: grpc.Channel or grpc.aio.Channel @@ -177,6 +177,7 @@ def _emulator_channel(self, transport): # Insecure channels are used for the emulator as secure channels # cannot be used to communicate on some environments. # https://github.com/googleapis/python-firestore/issues/359 + # Default the token to a non-empty string, in this case "owner". token = "owner" if self._credentials is not None and self._credentials.id_token is not None: token = self._credentials.id_token