feat: workforce credentials #485
Conversation
bshaffer
force-pushed
the
workforce-credentials
branch
from
642ebb3 to
3bec514
Compare
|
@aeitzman I'd love to get your review on this one as well! One thing I'd like to get clarity on - in the python implementation, there's a check that bypasses setting the "additionalOptions" when a |
| // using the IAM API. This saves a call to fetch an access token when a | ||
| // cached token exists. | ||
| if ($this->fetcher instanceof Credentials\GCECredentials | ||
| || $this->fetcher instanceof Credentials\ImpersonatedServiceAccountCredentials |
There was a problem hiding this comment.
Hmm, no. I am not sure why this would be in here. I probably just realized that it was something that needed fixing, when adding similar logic below for getProjectId.
| // Pass the access token from cache for credentials that require an | ||
| // access token to fetch the project ID. This saves a call to fetch an | ||
| // access token when a cached token exists. | ||
| if ($this->fetcher instanceof Credentials\ExternalAccountCredentials) { |
There was a problem hiding this comment.
Just double checking since I don't fully understand the caching flow in this library, this call is only used if getProjectId is explicitly called by a user/some other library right? Just want to make sure that the default flow for getting an access token doesn't break if cloudresourcemanager is down.
There was a problem hiding this comment.
Yes that's correct - this is ONLY for calls to $credentials->getProjectId()
| $httpHandler = function (RequestInterface $request) { | ||
| $this->assertEquals( | ||
| 'https://cloudresourcemanager.googleapis.com/v1/projects/1234', | ||
| (string) $request->getUri() | ||
| ); | ||
| $this->assertEquals('Bearer some-token', $request->getHeaderLine('authorization')); | ||
| $body = $this->prophesize(StreamInterface::class); | ||
| $body->__toString()->willReturn(json_encode(['projectId' => 'test-project-id'])); | ||
|
|
||
| $response = $this->prophesize(ResponseInterface::class); | ||
| $response->getBody()->willReturn($body->reveal()); | ||
| $response->hasHeader('Content-Type')->willReturn(false); | ||
|
|
||
| return $response->reveal(); | ||
| }; |
There was a problem hiding this comment.
Can you help me understand why are we defining this httphandler if the token and eventually the project id would be fetched from the mocked cache?
There was a problem hiding this comment.
Sure. This test is testing the getProjectId method, which makes an API call using the project number in order to retrieve the project ID. It's also testing that a cached access token is used when that API call is made, instead of retrieving a new one.
So the cache is for the access token, which is needed for the API request to get the project ID. The httpHandler is the HTTP request made when getProjectId is called, which is not cached.
There was a problem hiding this comment.
Oh, now this makes sense.
I feel adding a small explanation there in comment would go a long way to quickly understand it.
addresses #483
(note: relevant testing requested in phpspec/prophecy#611)