Skip to content
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

Merged
merged 17 commits into from
Jul 8, 2024

Conversation

limhjgrace
Copy link
Contributor

@limhjgrace limhjgrace commented Jun 6, 2024

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 cookie cwr_c. When used, the credential cookie name will have the format cwr_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.

@limhjgrace limhjgrace requested review from ps863 and qhanam June 6, 2024 22:07
@ps863
Copy link
Member

ps863 commented Jun 7, 2024

question: will we update existing doc/provide examples in separate PR?

@ps863
Copy link
Member

ps863 commented Jun 7, 2024

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?

@limhjgrace
Copy link
Contributor Author

question: will we update existing doc/provide examples in separate PR?

Thanks for the reminder, updated existing doc in latest commit.

@limhjgrace
Copy link
Contributor Author

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?

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.

src/dispatch/Authentication.ts Show resolved Hide resolved
src/dispatch/Authentication.ts Outdated Show resolved Hide resolved
src/dispatch/Authentication.ts Outdated Show resolved Hide resolved
src/sessions/SessionManager.ts Outdated Show resolved Hide resolved
@qhanam
Copy link
Member

qhanam commented Jun 11, 2024

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:

  1. When unique keys are enabled, does authentication succeed?
  2. When there are multiple app monitor instances, does authentication succeed?

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.

@limhjgrace
Copy link
Contributor Author

limhjgrace commented Jun 12, 2024

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:

  1. When unique keys are enabled, does authentication succeed?
  2. When there are multiple app monitor instances, does authentication succeed?

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

Comment on lines 21 to 22
this.config.cookieAttributes.unique,
applicationId
Copy link
Member

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.

@limhjgrace
Copy link
Contributor Author

Smoke tests passed: https://github.com/limhjgrace/aws-rum-web/actions/runs/9847231882/job/27186743456. Need to update secret keys for main repo.

@limhjgrace limhjgrace merged commit a440016 into aws-observability:main Jul 8, 2024
3 checks passed
@limhjgrace limhjgrace deleted the unique-cred-cookie branch July 8, 2024 23:32
@williazz williazz mentioned this pull request Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants