-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Add getId to enhanced authflow #433
fix: Add getId to enhanced authflow #433
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance, the unit test when getCredentialsForIdentity called then credentials are returned
is failing. I reran to approval workflow and it failed a second time. Will take a closer look
question: Should we update the documentation here (specifically for the Guest Role ARN), to reflect that Enhanced auth flow will be used if its omitted? |
@@ -169,29 +154,19 @@ describe('EnhancedAuthentication tests', () => { | |||
test('when credential is retrieved from basic auth then next credential is retrieved from localStorage', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, however I don't think fixing these fall within the scope of this change.
Yes, and probably update the examples to show the enhanced authflow instead of the basic authflow. I think we should do make these doc changes in a separate PR, as the purpose of this change is to patch a bug. |
Oops, I didn't wait for the CI checks to finish 😬 -- fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
We can do future PRs with:
- New docs and doc updates
- Update the test names
@@ -31,18 +28,6 @@ describe('EnhancedAuthentication tests', () => { | |||
sessionToken: 'z', | |||
expiration: new Date(Date.now() + 3600 * 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is GetCredentialsForIdentity supposed to return secretKey
instead of secretAccessKey
?
https://docs.aws.amazon.com/cognitoidentity/latest/APIReference/API_GetCredentialsForIdentity.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CognitoIdentityClient
, which is implemented by the RUM web client (recall we don't use the AWS SDK here), returns secretAccessKey
. This is by design, because it returns a Credential
object, which has the property secretAccessKey
.
One could make the argument that the CognitoIdentityClient
should have behavior which is consistent with the actual Cognito client. However, I've altered this behavior for the sake of maintainability and reducing the size of the web client bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Double checked by reviewing the enhanced auth flow and looking at the documentation for GetCredentialsForIdentity.
Q: What was the impact of this bug? And how long has GetCredentialsForIdentity required identityId instead of identityPoolId?
Cognito's enhanced authflow doesn't support RUM, so it has never been integration tested and no one is using it. That does raise the question of "How do we integration test unauthenicated authorization?". I think the answer to this question is that we test during smoke tests. We should update the smoke tests accordingly before we "release" enhanced authflow by updating docs. |
Cognito recently onboarded CloudWatch RUM to its enhanced authflow. However, the enhanced authflow implemented in the web client is currently broken because it uses the identity pool Id, instead of the identity Id, as the argument for
getCredentialsForIdentity
.This change modifies the enhanced authflow to fetch the identity Id before calling
getCredentialsForIdentity
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.