xp-forge / aws

AWS Core for the XP Framework
0 stars 0 forks source link

Undefined array key "accessToken" when refreshing SSO tokens fails #17

Closed thekid closed 1 month ago

thekid commented 1 month ago

The refresh method is missing handling of refresh errors:

$res= $this->refresh->post(new RequestData(Json::of($payload)), [
  'X-Amz-User-Agent' => $this->userAgent,
  'Content-Type'     => self::JSON,
]);
$refresh= Json::read(new StreamInput($res->in()));

In this case, the HTTP response has a 400 status code and the body consists of the following:

{
  "error" : "invalid_grant",
  "error_description" : "Invalid refresh token provided"
}

We should check for this.

thekid commented 1 month ago

Potential fix:

diff --git a/src/main/php/com/amazon/aws/credentials/FromSSO.class.php b/src/main/php/com/amazon/aws/credentials/FromSSO.class.php
index 41ba261..8bcaff8 100644
--- a/src/main/php/com/amazon/aws/credentials/FromSSO.class.php
+++ b/src/main/php/com/amazon/aws/credentials/FromSSO.class.php
@@ -3,6 +3,7 @@
 use com\amazon\aws\Credentials;
 use io\{File, Path};
 use lang\{Environment, IllegalStateException, Throwable};
+use peer\AuthenticationException;
 use peer\http\{HttpConnection, RequestData};
 use text\json\{Json, FileInput, FileOutput, StreamInput};

@@ -69,9 +70,14 @@ class FromSSO extends Provider {
         'X-Amz-User-Agent' => $this->userAgent,
         'Content-Type'     => self::JSON,
       ]);
+      if (200 !== $res->statusCode()) {
+        throw new AuthenticationException($res->readData(), $cache['refreshToken']);
+      }
       $refresh= Json::read(new StreamInput($res->in()));
     } catch (Throwable $t) {
       throw new IllegalStateException("OOIDC refreshing via {$this->refresh->getUrl()->getURL()} failed", $t);
+    } finally {
+      $res?->closeStream();
     }

     $cache['accessToken']= $refresh['accessToken'];

The ?-> operator is not supported by all PHP versions we support, and I don't like throwing, catching and rethrowing, so a bit of rewriting is necessary before this can be merged.