fix: make expiration_time optional in response schema (#1091)

* fix: allowing missing expire time in response * fix: adding sanity check for expiration time when using output file * update exception content * addressing comments * Update exception info * Update user doc * fix indents of rst * udpate user doc Co-authored-by: Jin Qin <qinjin@google.com>
googleapis · Aug 5, 2022 · 032fb8d · 032fb8d
1 parent b62c25c
commit 032fb8d
Show file tree

Hide file tree

Showing 3 changed files with 87 additions and 23 deletions.
diff --git a/docs/user-guide.rst b/docs/user-guide.rst
@@ -429,24 +429,28 @@ 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``.
+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

diff --git a/google/auth/pluggable.py b/google/auth/pluggable.py
@@ -262,11 +262,14 @@ def _parse_subject_token(self, response):
                     response["code"], response["message"]
                 )
             )
-        if "expiration_time" not in response:
+        if (
+            "expiration_time" not in response
+            and self._credential_source_executable_output_file
+        ):
             raise ValueError(
-                "The executable response is missing the expiration_time field."
+                "The executable response must contain an expiration_time for successful responses when an output_file has been specified in the configuration."
             )
-        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
@@ -636,7 +636,9 @@ 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_without_expiration_time_should_fail_when_output_file_specified(
+        self
+    ):
         EXECUTABLE_SUCCESSFUL_OIDC_RESPONSE = {
             "version": 1,
             "success": True,
@@ -658,9 +660,64 @@ def test_retrieve_subject_token_missing_expiration_time(self):
                 _ = credentials.retrieve_subject_token(None)
 
             assert excinfo.match(
-                r"The executable response is missing the expiration_time field."
+                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"})
+    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 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)
+
+    @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 = {
+            "version": 1,
+            "success": True,
+            "token_type": "urn:ietf:params:oauth:token-type:id_token",
+            "id_token": self.EXECUTABLE_OIDC_TOKEN,
+        }
+
+        CREDENTIAL_SOURCE = {
+            "executable": {"command": "command", "timeout_millis": 30000}
+        }
+
+        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=CREDENTIAL_SOURCE)
+            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):
         EXECUTABLE_SUCCESSFUL_OIDC_RESPONSE = {