From 08e344b56bb6c66b018dc010908aa140bdd159eb Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Fri, 22 Jul 2022 22:28:08 +0000 Subject: [PATCH 1/8] fix: allowing missing expire time in response --- google/auth/pluggable.py | 6 +----- tests/test_pluggable.py | 11 ++--------- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/google/auth/pluggable.py b/google/auth/pluggable.py index ca583744a..d7709b47f 100644 --- a/google/auth/pluggable.py +++ b/google/auth/pluggable.py @@ -262,11 +262,7 @@ def _parse_subject_token(self, response): response["code"], response["message"] ) ) - if "expiration_time" not in response: - raise ValueError( - "The executable response is missing the expiration_time field." - ) - if response["expiration_time"] < time.time(): + if "expiration_time" in response and response["expiration_time"] < time.time(): raise exceptions.RefreshError( "The token returned by the executable is expired." ) diff --git a/tests/test_pluggable.py b/tests/test_pluggable.py index 383b7a866..e2696df40 100644 --- a/tests/test_pluggable.py +++ b/tests/test_pluggable.py @@ -636,7 +636,7 @@ def test_retrieve_subject_token_missing_error_code_message(self): ) @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) - def test_retrieve_subject_token_missing_expiration_time(self): + def test_retrieve_subject_token_missing_expiration_time_should_pass(self): EXECUTABLE_SUCCESSFUL_OIDC_RESPONSE = { "version": 1, "success": True, @@ -652,14 +652,7 @@ def test_retrieve_subject_token_missing_expiration_time(self): returncode=0, ), ): - credentials = self.make_pluggable(credential_source=self.CREDENTIAL_SOURCE) - - with pytest.raises(ValueError) as excinfo: - _ = credentials.retrieve_subject_token(None) - - assert excinfo.match( - r"The executable response is missing the expiration_time field." - ) + self.make_pluggable(credential_source=self.CREDENTIAL_SOURCE) @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) def test_retrieve_subject_token_missing_token_type(self): From 96745cd2c1ae2076066b7c85a2171d70f6ef569f Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Wed, 27 Jul 2022 22:46:02 +0000 Subject: [PATCH 2/8] fix: adding sanity check for expiration time when using output file --- google/auth/pluggable.py | 7 +++++++ tests/test_pluggable.py | 39 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/google/auth/pluggable.py b/google/auth/pluggable.py index d7709b47f..b40e9690d 100644 --- a/google/auth/pluggable.py +++ b/google/auth/pluggable.py @@ -262,6 +262,13 @@ def _parse_subject_token(self, response): response["code"], response["message"] ) ) + if ( + "expiration_time" not in response + and self._credential_source_executable_output_file + ): + raise ValueError( + "Expiration_time must be specified while using output file" + ) if "expiration_time" in response and response["expiration_time"] < time.time(): raise exceptions.RefreshError( "The token returned by the executable is expired." diff --git a/tests/test_pluggable.py b/tests/test_pluggable.py index e2696df40..df0c627c6 100644 --- a/tests/test_pluggable.py +++ b/tests/test_pluggable.py @@ -636,7 +636,37 @@ def test_retrieve_subject_token_missing_error_code_message(self): ) @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) - def test_retrieve_subject_token_missing_expiration_time_should_pass(self): + def test_retrieve_subject_token_without_expiration_time_should_fail_when_using_output_file( + self + ): + EXECUTABLE_SUCCESSFUL_OIDC_RESPONSE = { + "version": 1, + "success": True, + "token_type": "urn:ietf:params:oauth:token-type:id_token", + "id_token": self.EXECUTABLE_OIDC_TOKEN, + } + + with mock.patch( + "subprocess.run", + return_value=subprocess.CompletedProcess( + args=[], + stdout=json.dumps(EXECUTABLE_SUCCESSFUL_OIDC_RESPONSE).encode("UTF-8"), + returncode=0, + ), + ): + credentials = self.make_pluggable(credential_source=self.CREDENTIAL_SOURCE) + + with pytest.raises(ValueError) as excinfo: + _ = credentials.retrieve_subject_token(None) + + assert excinfo.match( + r"Expiration_time must be specified while using output file" + ) + + @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) + def test_retrieve_subject_token_without_expiration_time_should_pass_when_not_using_output_file( + self + ): EXECUTABLE_SUCCESSFUL_OIDC_RESPONSE = { "version": 1, "success": True, @@ -644,6 +674,10 @@ def test_retrieve_subject_token_missing_expiration_time_should_pass(self): "id_token": self.EXECUTABLE_OIDC_TOKEN, } + CREDENTIAL_SOURCE = { + "executable": {"command": "command", "timeout_millis": 30000} + } + with mock.patch( "subprocess.run", return_value=subprocess.CompletedProcess( @@ -652,7 +686,8 @@ def test_retrieve_subject_token_missing_expiration_time_should_pass(self): returncode=0, ), ): - self.make_pluggable(credential_source=self.CREDENTIAL_SOURCE) + credentials = self.make_pluggable(credential_source=CREDENTIAL_SOURCE) + credentials.retrieve_subject_token(None) @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) def test_retrieve_subject_token_missing_token_type(self): From 3e76748219408c9d136b2580d91372e4c7ddafa8 Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Wed, 27 Jul 2022 22:51:02 +0000 Subject: [PATCH 3/8] update exception content --- google/auth/pluggable.py | 2 +- tests/test_pluggable.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/google/auth/pluggable.py b/google/auth/pluggable.py index b40e9690d..cd88ce1f9 100644 --- a/google/auth/pluggable.py +++ b/google/auth/pluggable.py @@ -267,7 +267,7 @@ def _parse_subject_token(self, response): and self._credential_source_executable_output_file ): raise ValueError( - "Expiration_time must be specified while using output file" + "The output file response must contain the expiration_time field when success=true." ) if "expiration_time" in response and response["expiration_time"] < time.time(): raise exceptions.RefreshError( diff --git a/tests/test_pluggable.py b/tests/test_pluggable.py index df0c627c6..fd7f684ba 100644 --- a/tests/test_pluggable.py +++ b/tests/test_pluggable.py @@ -660,7 +660,7 @@ def test_retrieve_subject_token_without_expiration_time_should_fail_when_using_o _ = credentials.retrieve_subject_token(None) assert excinfo.match( - r"Expiration_time must be specified while using output file" + r"The output file response must contain the expiration_time field when success=true." ) @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) From 2d324d07752e3f17adeb975faa8d80324069ce7d Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Thu, 28 Jul 2022 00:15:58 +0000 Subject: [PATCH 4/8] addressing comments --- tests/test_pluggable.py | 35 ++++++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/tests/test_pluggable.py b/tests/test_pluggable.py index fd7f684ba..d326659a9 100644 --- a/tests/test_pluggable.py +++ b/tests/test_pluggable.py @@ -636,7 +636,7 @@ def test_retrieve_subject_token_missing_error_code_message(self): ) @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) - def test_retrieve_subject_token_without_expiration_time_should_fail_when_using_output_file( + def test_retrieve_subject_token_without_expiration_time_should_fail_when_output_file_specified( self ): EXECUTABLE_SUCCESSFUL_OIDC_RESPONSE = { @@ -664,7 +664,34 @@ def test_retrieve_subject_token_without_expiration_time_should_fail_when_using_o ) @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) - def test_retrieve_subject_token_without_expiration_time_should_pass_when_not_using_output_file( + def test_retrieve_subject_token_without_expiration_time_should_fail_when_retrieving_from_output_file( + self + ): + ACTUAL_CREDENTIAL_SOURCE_EXECUTABLE_OUTPUT_FILE = "actual_output_file" + ACTUAL_CREDENTIAL_SOURCE_EXECUTABLE = { + "command": "command", + "timeout_millis": 30000, + "output_file": ACTUAL_CREDENTIAL_SOURCE_EXECUTABLE_OUTPUT_FILE, + } + ACTUAL_CREDENTIAL_SOURCE = {"executable": ACTUAL_CREDENTIAL_SOURCE_EXECUTABLE} + data = self.EXECUTABLE_SUCCESSFUL_OIDC_RESPONSE_ID_TOKEN.copy() + data.pop("expiration_time") + + with open(ACTUAL_CREDENTIAL_SOURCE_EXECUTABLE_OUTPUT_FILE, "w") as output_file: + json.dump(data, output_file) + + credentials = self.make_pluggable(credential_source=ACTUAL_CREDENTIAL_SOURCE) + + with pytest.raises(ValueError) as excinfo: + _ = credentials.retrieve_subject_token(None) + + assert excinfo.match( + r"The output file response must contain the expiration_time field when success=true." + ) + os.remove(ACTUAL_CREDENTIAL_SOURCE_EXECUTABLE_OUTPUT_FILE) + + @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) + def test_retrieve_subject_token_without_expiration_time_should_pass_when_output_file_not_specified( self ): EXECUTABLE_SUCCESSFUL_OIDC_RESPONSE = { @@ -687,7 +714,9 @@ def test_retrieve_subject_token_without_expiration_time_should_pass_when_not_usi ), ): credentials = self.make_pluggable(credential_source=CREDENTIAL_SOURCE) - credentials.retrieve_subject_token(None) + subject_token = credentials.retrieve_subject_token(None) + + assert subject_token == self.EXECUTABLE_OIDC_TOKEN @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) def test_retrieve_subject_token_missing_token_type(self): From cf8c6572ac5329a14896d6cc6cdc97250d8d6604 Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Thu, 28 Jul 2022 17:32:20 +0000 Subject: [PATCH 5/8] Update exception info --- google/auth/pluggable.py | 2 +- tests/test_pluggable.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/google/auth/pluggable.py b/google/auth/pluggable.py index cd88ce1f9..42f6bcd81 100644 --- a/google/auth/pluggable.py +++ b/google/auth/pluggable.py @@ -267,7 +267,7 @@ def _parse_subject_token(self, response): and self._credential_source_executable_output_file ): raise ValueError( - "The output file response must contain the expiration_time field when success=true." + "The executable response must contain an expiration_time for successful responses when an output_file has been specified in the configuration." ) if "expiration_time" in response and response["expiration_time"] < time.time(): raise exceptions.RefreshError( diff --git a/tests/test_pluggable.py b/tests/test_pluggable.py index d326659a9..b90c86c3a 100644 --- a/tests/test_pluggable.py +++ b/tests/test_pluggable.py @@ -660,7 +660,7 @@ def test_retrieve_subject_token_without_expiration_time_should_fail_when_output_ _ = credentials.retrieve_subject_token(None) assert excinfo.match( - r"The output file response must contain the expiration_time field when success=true." + r"The executable response must contain an expiration_time for successful responses when an output_file has been specified in the configuration." ) @mock.patch.dict(os.environ, {"GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES": "1"}) @@ -686,7 +686,7 @@ def test_retrieve_subject_token_without_expiration_time_should_fail_when_retriev _ = credentials.retrieve_subject_token(None) assert excinfo.match( - r"The output file response must contain the expiration_time field when success=true." + r"The executable response must contain an expiration_time for successful responses when an output_file has been specified in the configuration." ) os.remove(ACTUAL_CREDENTIAL_SOURCE_EXECUTABLE_OUTPUT_FILE) From 3321d9da5f23c7e8ece0feaa7be2ea82057cd676 Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Fri, 29 Jul 2022 21:40:52 +0000 Subject: [PATCH 6/8] Update user doc --- docs/user-guide.rst | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/docs/user-guide.rst b/docs/user-guide.rst index a09247897..cbbfe942c 100644 --- a/docs/user-guide.rst +++ b/docs/user-guide.rst @@ -429,24 +429,31 @@ These are all required fields for an error response. The code and message fields will be used by the library as part of the thrown exception. -Response format fields summary: ``version``: The version of the JSON -output. Currently only version 1 is supported. ``success``: The -status of the response. When true, the response must contain the 3rd -party token, token type, and expiration. The executable must also exit -with exit code 0. When false, the response must contain the error code -and message fields and exit with a non-zero value. ``token_type``: -The 3rd party subject token type. Must be -*urn:ietf:params:oauth:token-type:jwt*, -*urn:ietf:params:oauth:token-type:id_token*, or -*urn:ietf:params:oauth:token-type:saml2*. ``id_token``: The 3rd party -OIDC token. ``saml_response``: The 3rd party SAML response. -``expiration_time``: The 3rd party subject token expiration time in -seconds (unix epoch time). ``code``: The error code string. -``message``: The error message. - -All response types must include both the ``version`` and ``success`` -fields. Successful responses must include the ``token_type``, -``expiration_time``, and one of ``id_token`` or ``saml_response``. +Response format fields summary: + +- ``version``: The version of the JSON output. Currently only version 1 is + supported. +- ``success``: The status of the response. + - When true, the response must contain the 3rd party token, token type, + and expiration. The executable must also exit with exit code 0. + - When false, the response must contain the error code and message + fields and exit with a non-zero value. +- ``token_type``: The 3rd party subject token type. Must be + - *urn:ietf:params:oauth:token-type:jwt* + - *urn:ietf:params:oauth:token-type:id_token* + - *urn:ietf:params:oauth:token-type:saml2* +- ``id_token``: The 3rd party OIDC token. +- ``saml_response``: The 3rd party SAML response. +- ``expiration_time``: The 3rd party subject token expiration time in seconds + (unix epoch time). +- ``code``: The error code string. +- ``message``: The error message. + +All response types must include both the ``version`` and ``success`` fields. +Successful responses must include the ``token_type``, and one of ``id_token`` +or ``saml_response``. +``expiration_time`` is only optional when ``output_file`` is not specified. +Otherwise we will treat the response as malformed. Error responses must include both the ``code`` and ``message`` fields. The library will populate the following environment variables when the From d6a5af2ed6dcafa2384a03b49a2f557fe6092ca7 Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Thu, 4 Aug 2022 00:06:20 +0000 Subject: [PATCH 7/8] fix indents of rst --- docs/user-guide.rst | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/user-guide.rst b/docs/user-guide.rst index cbbfe942c..f673e20a1 100644 --- a/docs/user-guide.rst +++ b/docs/user-guide.rst @@ -434,10 +434,8 @@ Response format fields summary: - ``version``: The version of the JSON output. Currently only version 1 is supported. - ``success``: The status of the response. - - When true, the response must contain the 3rd party token, token type, - and expiration. The executable must also exit with exit code 0. - - When false, the response must contain the error code and message - fields and exit with a non-zero value. + - When true, the response must contain the 3rd party token, token type, and expiration. The executable must also exit with exit code 0. + - When false, the response must contain the error code and message fields and exit with a non-zero value. - ``token_type``: The 3rd party subject token type. Must be - *urn:ietf:params:oauth:token-type:jwt* - *urn:ietf:params:oauth:token-type:id_token* From 3265f99ef4c3bc8956069c08c4bd6eb7ebdacc90 Mon Sep 17 00:00:00 2001 From: Jin Qin Date: Fri, 5 Aug 2022 16:48:04 +0000 Subject: [PATCH 8/8] udpate user doc --- docs/user-guide.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/user-guide.rst b/docs/user-guide.rst index f673e20a1..16de58d9d 100644 --- a/docs/user-guide.rst +++ b/docs/user-guide.rst @@ -450,8 +450,7 @@ Response format fields summary: All response types must include both the ``version`` and ``success`` fields. Successful responses must include the ``token_type``, and one of ``id_token`` or ``saml_response``. -``expiration_time`` is only optional when ``output_file`` is not specified. -Otherwise we will treat the response as malformed. +If output file is specified, ``expiration_time`` is mandatory. Error responses must include both the ``code`` and ``message`` fields. The library will populate the following environment variables when the