-
Notifications
You must be signed in to change notification settings - Fork 69
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
feat: support unique credential cookie names #560
feat: support unique credential cookie names #560
Conversation
question: will we update existing doc/provide examples in separate PR? |
question: if there are multiple web clients running for different app monitors, user would have to make sure the unique configuration is enabled in each one for this to work correctly right? |
Thanks for the reminder, updated existing doc in latest commit. |
Technically if they had 2 app monitors, only one would need to configure unique cookies because they would read from different cookies. But as best practice, users should configure each app monitor, if using multiple, to configure using unique cookies. |
Co-authored-by: Quinn Hanam <[email protected]>
Co-authored-by: Quinn Hanam <[email protected]>
Co-authored-by: Quinn Hanam <[email protected]>
I think this change needs to be integration tested somehow. That is, when unique keys are enabled, and multiple web clients are used concurrently, then authentication succeeds. For example:
I don't know if mocking the requests to Cognito and STS makes sense here. Mocking might be OK if we at least manually verify authentication succeeds for case (2). Otherwise, these might need to be run in the smoke test environment, which makes remote requests. |
Added smoke tests for applications using npm. The second web client is created after a 10 second wait from the first one to ensure credentials are retrieved and loaded separately. During smoke test dev, we discovered that we currently don't support multiple web clients for applications using CDN. Smoke tests successfully pass: https://github.com/limhjgrace/aws-rum-web/actions/runs/9487042028/job/26142730870 |
src/dispatch/Authentication.ts
Outdated
this.config.cookieAttributes.unique, | ||
applicationId |
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(blocking): Pass the entire config, and put these inside the CognitoIdentityClientConfig
object.
Smoke tests passed: https://github.com/limhjgrace/aws-rum-web/actions/runs/9847231882/job/27186743456. Need to update secret keys for main repo. |
Multiple AppMonitors from different AWS accounts cannot currently be used on the same page. Because the credential cookie name
cwr_c
is not unique to an AppMonitor, multiple web clients running on the same page attempt to read and store credential info to the same cookie, which would produce incorrect data.This change uses the existing
cookieAttributes.unique
configuration to add the AppMonitorID as a postfix to the credential cookiecwr_c
. When used, the credential cookie name will have the formatcwr_c_[AppMonitor Id]
, and multiple web clients sending data to app monitors in different accounts can run on the same page.Related FR: #557
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.