From bc4868caa394736f7525a86f0bf191952c1c1b00 Mon Sep 17 00:00:00 2001 From: Michael Bausor Date: Mon, 10 Sep 2018 13:58:21 -0700 Subject: [PATCH 1/3] Explicitly handle null access token --- src/ApiCore/CredentialsWrapper.php | 14 ++++++++-- src/ApiCore/GapicClientTrait.php | 28 +++++++++---------- .../Transport/HttpUnaryTransportTrait.php | 5 +++- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/src/ApiCore/CredentialsWrapper.php b/src/ApiCore/CredentialsWrapper.php index ce146a155..7d6f8fb4f 100644 --- a/src/ApiCore/CredentialsWrapper.php +++ b/src/ApiCore/CredentialsWrapper.php @@ -141,7 +141,12 @@ public static function build(array $args = []) */ public function getBearerString() { - return 'Bearer ' . self::getToken($this->credentialsFetcher, $this->authHttpHandler); + $token = self::getToken($this->credentialsFetcher, $this->authHttpHandler); + if (isset($token)) { + return "Bearer $token"; + } else { + return ''; + } } /** @@ -156,7 +161,12 @@ public function getAuthorizationHeaderCallback() // be passed into the gRPC c extension, and changes have the potential to trigger very // difficult-to-diagnose segmentation faults. return function () use ($credentialsFetcher, $authHttpHandler) { - return ['authorization' => ['Bearer ' . self::getToken($credentialsFetcher, $authHttpHandler)]]; + $token = self::getToken($credentialsFetcher, $authHttpHandler); + if (isset($token)) { + return ['authorization' => ["Bearer $token"]]; + } else { + return []; + } }; } diff --git a/src/ApiCore/GapicClientTrait.php b/src/ApiCore/GapicClientTrait.php index 7b7f2c514..1a48333b9 100644 --- a/src/ApiCore/GapicClientTrait.php +++ b/src/ApiCore/GapicClientTrait.php @@ -315,28 +315,28 @@ private function setClientOptions(array $options) } /** - * @param mixed $auth - * @param array $authConfig + * @param mixed $credentials + * @param array $credentialsConfig * @return CredentialsWrapper * @throws ValidationException */ - private function createCredentialsWrapper($auth, array $authConfig) + private function createCredentialsWrapper($credentials, array $credentialsConfig) { - if (is_null($auth)) { - return CredentialsWrapper::build($authConfig); - } elseif (is_string($auth) || is_array($auth)) { - return CredentialsWrapper::build(['keyFile' => $auth] + $authConfig); - } elseif ($auth instanceof FetchAuthTokenInterface) { - $authHttpHandler = isset($authConfig['authHttpHandler']) - ? $authConfig['authHttpHandler'] + if (is_null($credentials)) { + return CredentialsWrapper::build($credentialsConfig); + } elseif (is_string($credentials) || is_array($credentials)) { + return CredentialsWrapper::build(['keyFile' => $credentials] + $credentialsConfig); + } elseif ($credentials instanceof FetchAuthTokenInterface) { + $authHttpHandler = isset($credentialsConfig['authHttpHandler']) + ? $credentialsConfig['authHttpHandler'] : null; - return new CredentialsWrapper($auth, $authHttpHandler); - } elseif ($auth instanceof CredentialsWrapper) { - return $auth; + return new CredentialsWrapper($credentials, $authHttpHandler); + } elseif ($credentials instanceof CredentialsWrapper) { + return $credentials; } else { throw new ValidationException( 'Unexpected value in $auth option, got: ' . - print_r($auth, true) + print_r($credentials, true) ); } } diff --git a/src/ApiCore/Transport/HttpUnaryTransportTrait.php b/src/ApiCore/Transport/HttpUnaryTransportTrait.php index 9b0a3d92b..88495f00d 100644 --- a/src/ApiCore/Transport/HttpUnaryTransportTrait.php +++ b/src/ApiCore/Transport/HttpUnaryTransportTrait.php @@ -92,7 +92,10 @@ private static function buildCommonHeaders(array $options) // If not already set, add an auth header to the request if (!isset($headers['Authorization']) && isset($options['credentialsWrapper'])) { - $headers['Authorization'] = $options['credentialsWrapper']->getBearerString(); + $bearerString = $options['credentialsWrapper']->getBearerString(); + if (!empty($bearerString)) { + $headers['Authorization'] = $bearerString; + } } return $headers; From e2f13bc5f893a698f56e14a4f46cc05d49177458 Mon Sep 17 00:00:00 2001 From: Michael Bausor Date: Mon, 10 Sep 2018 14:06:10 -0700 Subject: [PATCH 2/3] Add tests for empty and null access token --- src/ApiCore/CredentialsWrapper.php | 12 ++++---- .../Tests/Unit/CredentialsWrapperTest.php | 29 +++++++++++++++++++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/ApiCore/CredentialsWrapper.php b/src/ApiCore/CredentialsWrapper.php index 7d6f8fb4f..e4bf13b52 100644 --- a/src/ApiCore/CredentialsWrapper.php +++ b/src/ApiCore/CredentialsWrapper.php @@ -142,10 +142,10 @@ public static function build(array $args = []) public function getBearerString() { $token = self::getToken($this->credentialsFetcher, $this->authHttpHandler); - if (isset($token)) { - return "Bearer $token"; - } else { + if (empty($token)) { return ''; + } else { + return "Bearer $token"; } } @@ -162,10 +162,10 @@ public function getAuthorizationHeaderCallback() // difficult-to-diagnose segmentation faults. return function () use ($credentialsFetcher, $authHttpHandler) { $token = self::getToken($credentialsFetcher, $authHttpHandler); - if (isset($token)) { - return ['authorization' => ["Bearer $token"]]; - } else { + if (empty($token)) { return []; + } else { + return ['authorization' => ["Bearer $token"]]; } }; } diff --git a/tests/ApiCore/Tests/Unit/CredentialsWrapperTest.php b/tests/ApiCore/Tests/Unit/CredentialsWrapperTest.php index c936ec903..44edc8bf4 100644 --- a/tests/ApiCore/Tests/Unit/CredentialsWrapperTest.php +++ b/tests/ApiCore/Tests/Unit/CredentialsWrapperTest.php @@ -179,9 +179,23 @@ public function getBearerStringData() 'access_token' => 123, 'expires_at' => time() + 100, ]); + $insecureFetcher = $this->prophesize(FetchAuthTokenInterface::class); + $insecureFetcher->getLastReceivedToken()->willReturn(null); + $insecureFetcher->fetchAuthToken(Argument::any()) + ->willReturn([ + 'access_token' => '', + ]); + $nullFetcher = $this->prophesize(FetchAuthTokenInterface::class); + $nullFetcher->getLastReceivedToken()->willReturn(null); + $nullFetcher->fetchAuthToken(Argument::any()) + ->willReturn([ + 'access_token' => null, + ]); return [ [$expiredFetcher->reveal(), 'Bearer 456'], [$unexpiredFetcher->reveal(), 'Bearer 123'], + [$insecureFetcher->reveal(), ''], + [$nullFetcher->reveal(), ''] ]; } @@ -215,9 +229,24 @@ public function getAuthorizationHeaderCallbackData() 'access_token' => 123, 'expires_at' => time() + 100, ]); + + $insecureFetcher = $this->prophesize(FetchAuthTokenInterface::class); + $insecureFetcher->getLastReceivedToken()->willReturn(null); + $insecureFetcher->fetchAuthToken(Argument::any()) + ->willReturn([ + 'access_token' => '', + ]); + $nullFetcher = $this->prophesize(FetchAuthTokenInterface::class); + $nullFetcher->getLastReceivedToken()->willReturn(null); + $nullFetcher->fetchAuthToken(Argument::any()) + ->willReturn([ + 'access_token' => null, + ]); return [ [$expiredFetcher->reveal(), ['authorization' => ['Bearer 456']]], [$unexpiredFetcher->reveal(), ['authorization' => ['Bearer 123']]], + [$insecureFetcher->reveal(), []], + [$nullFetcher->reveal(), []], ]; } } From 083908217fd4526f3bf028e77323e0eada1fb078 Mon Sep 17 00:00:00 2001 From: Michael Bausor Date: Mon, 10 Sep 2018 14:09:12 -0700 Subject: [PATCH 3/3] Use ternary operator --- src/ApiCore/CredentialsWrapper.php | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/ApiCore/CredentialsWrapper.php b/src/ApiCore/CredentialsWrapper.php index e4bf13b52..f9f31f0c3 100644 --- a/src/ApiCore/CredentialsWrapper.php +++ b/src/ApiCore/CredentialsWrapper.php @@ -142,11 +142,7 @@ public static function build(array $args = []) public function getBearerString() { $token = self::getToken($this->credentialsFetcher, $this->authHttpHandler); - if (empty($token)) { - return ''; - } else { - return "Bearer $token"; - } + return empty($token) ? '' : "Bearer $token"; } /** @@ -162,11 +158,7 @@ public function getAuthorizationHeaderCallback() // difficult-to-diagnose segmentation faults. return function () use ($credentialsFetcher, $authHttpHandler) { $token = self::getToken($credentialsFetcher, $authHttpHandler); - if (empty($token)) { - return []; - } else { - return ['authorization' => ["Bearer $token"]]; - } + return empty($token) ? [] : ['authorization' => ["Bearer $token"]]; }; }