adds support for SSO credentials #1519
Conversation
|
@jderusse Would you accept this functionality? The official SDK supports it. In case yes I would make this PR ready to be merged. |
|
I'm on the fence here.. From one side: You have a use case and we state that we accept all PR that add new operation if people have a use case. On the other side: This adds a new Client inside the I think we should create a "normal" (not inside core) client, and use |
|
I tend to agree with @jderusse here |
smoench
force-pushed
the
sso-credentials
branch
2 times, most recently
from
39b1252 to
c2e8247
Compare
|
Thanks for the feedback. Sounds great. I already covered the move to its own client and the |
smoench
force-pushed
the
sso-credentials
branch
3 times, most recently
from
23e39f4 to
f141edd
Compare
|
I tested it locally and it worked :-) How should I deal with the psalm failure? |
|
@jderusse here is the return of this dreaded Psalm caching issue... |
|
Is there anything left I can do or support you with? I would love to get this merged and released :-) |
src/Integration/Symfony/Bundle/src/DependencyInjection/AwsPackagesProvider.php
Show resolved
Hide resolved
smoench
force-pushed
the
sso-credentials
branch
2 times, most recently
from
0f229d0 to
3d7498b
Compare
| $filepath = sprintf('%s/.aws/sso/cache/%s.json', $this->getHomeDir(), sha1($ssoStartUrl)); | ||
|
|
||
| if (!@is_readable($filepath)) { | ||
| $this->logger->warning('The sso cache file {path} is not readable.', ['path' => $filepath]); |
There was a problem hiding this comment.
I don't think this is the right place for this check, and opens up a race condition between here and line 42. file_get_contents could still fail, but we don't properly handle that.
There was a problem hiding this comment.
Example robust ways to open the file, then read it's contents can be found in the Guzzle Utils class (tryFopen, tryGetContents): https://github.com/guzzle/psr7/blob/2.5.0/src/Utils.php#L340-L434.
There was a problem hiding this comment.
What do you think about this? Would it be enough?
if (!@is_readable($filepath) || false === ($content = file_get_contents($filepath))) {There was a problem hiding this comment.
file_get_contents can also raise errors. The linked code handles them and surfaces they back to the user as an exception.
There was a problem hiding this comment.
Checking is_readable is largely not useful, because the file's readability could change in between that call and then the file_get_contents call.
There was a problem hiding this comment.
Great - this means we are justified to add a little internal class in the Core namespace to get the contents of a file or throw an exception.
There was a problem hiding this comment.
@GrahamCampbell Since the referenced code is written by yourself, would you consider to contribute it here?
There was a problem hiding this comment.
Since I didn't get an answer, I will go with following:
if (false === ($contents = @file_get_contents($filepath))) {I don't feel well to copy the referenced code when the original author is involved here 😉
There was a problem hiding this comment.
are you OK with this change @GrahamCampbell
There was a problem hiding this comment.
Yeh, we could also just "fix it in post", since this is an existing bug, and nobody seems to mind it so far. Are you OK if we make an issue for it, assign me to it, then I'll get to it when I have some time?
smoench
force-pushed
the
sso-credentials
branch
2 times, most recently
from
8972136 to
6892109
Compare
smoench
force-pushed
the
sso-credentials
branch
from
6892109 to
670f415
Compare
This PR adds SSO (legacy) credentials support. The official SDK supports it and we are using this.