search

aws-observability / aws-rum-web

Amazon CloudWatch RUM Web Client
Apache License 2.0
114 stars 65 forks source link

fix: Add getId to enhanced authflow #433

Closed qhanam closed 10 months ago

qhanam commented 10 months ago

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.

ps863 commented 10 months ago

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?

qhanam commented 10 months ago

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?

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.

qhanam commented 10 months ago

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

Oops, I didn't wait for the CI checks to finish 😬 -- fixed

qhanam commented 10 months ago

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.