fix: purge invalid credentials on 403#740
Conversation
williazz
changed the title
|
Also resolves #699 |
|
question: Unique cookies configuration should help with the scenario wherein there are multiple app monitor setup for monitoring one web app. Would enabling that by default address the first problem? If yes, then I would assume this patch would be applicable to scenarios wherein app monitor is being swapped out? |
Yeah this helps with
Does not help with To address 4, we should enabled unique cookies by default. But I did notice we accidentally also made session cookies unique #560, which was probably a mistake. We should revert that if we want to enable unique cookies by default. I can make an issue for it. |
|
Created issue #742 |
| 'Rebuilding client with fresh cognito credentials' | ||
| ); | ||
| localStorage.removeItem(this.identityStorageKey); | ||
| this.setCognitoCredentials( |
There was a problem hiding this comment.
question: will this retrigger credential fetching?
There was a problem hiding this comment.
Yep
public setCognitoCredentials(
identityPoolId: string,
guestRoleArn?: string
) {
if (identityPoolId && guestRoleArn) {
this.setAwsCredentials(
new BasicAuthentication(this.config, this.applicationId)
.ChainAnonymousCredentialsProvider
);
} else {
this.setAwsCredentials(
new EnhancedAuthentication(this.config, this.applicationId)
.ChainAnonymousCredentialsProvider
);
}
}
| ) { | ||
| // If auth fails and a credentialProvider has been configured, | ||
| // then we need to make sure that the cached credentials are for | ||
| // the intended RUM app monitor. Otherwise, the irrelevant cached |
There was a problem hiding this comment.
question: Does this cause a risk of infinite cycle of purge and creation of new client if say the user actually just happens to have configuration issues on their IAM permissions?
There was a problem hiding this comment.
ok i think this isn't an issue with the shouldPurge flag you added
There was a problem hiding this comment.
yeah we only purge once, and let the normal retries/backoff do their thing
3 participants
Summary
When signing is enabled, then RUM stores two credentials in local storage: (1) cognito identity pool under
cwr_i(2) sigv4 credentials undercwr_c. However, these credentials are only updated if they become expired, not if the RUM user intends to set a new app monitor.Without this change, the web client will never call PutRumEvents with 200 status if invalid credentials are stored in local storage. Resolves #583
Solution
If signing is enabled and we encounter a 403, then we will purge
cwr_cand rebuild the dataplane client with the previously set credentials provider. If cognito is enabled, then we will also purgecwr_i.Testing
appmonitor1andappmonitor2with cognito enabledappmonitor1appmonitor2. This should now crash the web client and it will never recover.But with this change, (3) will instead fail once for
appmointor2, then retry successfully with the correct credentials.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.