generated from amazon-archives/__template_Apache-2.0
-
Notifications
You must be signed in to change notification settings - Fork 85
fix: purge invalid credentials on 403 #740
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
df9148f
fix: purge cookies when app monitor changes
williazz 3ed3752
fix: update keys for unique cookies
williazz c63e35a
add tests
williazz 0130857
fix orchestration
williazz 24c13c2
add unit tests
williazz 3951aca
ensure signing
williazz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,9 @@ import { Config } from '../orchestration/Orchestration'; | |
| import { v4 } from 'uuid'; | ||
| import { RetryHttpHandler } from './RetryHttpHandler'; | ||
| import { InternalLogger } from '../utils/InternalLogger'; | ||
| import { CRED_KEY, IDENTITY_KEY } from '../utils/constants'; | ||
| import { BasicAuthentication } from '../dispatch/BasicAuthentication'; | ||
| import { EnhancedAuthentication } from '../dispatch/EnhancedAuthentication'; | ||
|
|
||
| type SendFunction = ( | ||
| putRumEventsRequest: PutRumEventsRequest | ||
|
|
@@ -31,6 +34,7 @@ export type ClientBuilder = ( | |
| ) => DataPlaneClient; | ||
|
|
||
| export class Dispatch { | ||
| private applicationId: string; | ||
| private region: string; | ||
| private endpoint: URL; | ||
| private eventCache: EventCache; | ||
|
|
@@ -41,13 +45,23 @@ export class Dispatch { | |
| private config: Config; | ||
| private disableCodes = ['403', '404']; | ||
| private headers: any; | ||
| private credentialProvider: | ||
| | AwsCredentialIdentity | ||
| | AwsCredentialIdentityProvider | ||
| | undefined; | ||
|
|
||
| private shouldPurgeCredentials = true; | ||
| private credentialStorageKey: string; | ||
| private identityStorageKey: string; | ||
|
|
||
| constructor( | ||
| applicationId: string, | ||
| region: string, | ||
| endpoint: URL, | ||
| eventCache: EventCache, | ||
| config: Config | ||
| ) { | ||
| this.applicationId = applicationId; | ||
| this.region = region; | ||
| this.endpoint = endpoint; | ||
| this.eventCache = eventCache; | ||
|
|
@@ -68,6 +82,14 @@ export class Dispatch { | |
| } else { | ||
| this.rum = this.buildClient(this.endpoint, this.region, undefined); | ||
| } | ||
|
|
||
| this.credentialStorageKey = this.config.cookieAttributes.unique | ||
| ? `${CRED_KEY}_${applicationId}` | ||
| : CRED_KEY; | ||
|
|
||
| this.identityStorageKey = this.config.cookieAttributes.unique | ||
| ? `${IDENTITY_KEY}_${applicationId}` | ||
| : IDENTITY_KEY; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -100,6 +122,7 @@ export class Dispatch { | |
| | AwsCredentialIdentity | ||
| | AwsCredentialIdentityProvider | ||
| ): void { | ||
| this.credentialProvider = credentialProvider; | ||
| this.rum = this.buildClient( | ||
| this.endpoint, | ||
| this.region, | ||
|
|
@@ -112,6 +135,23 @@ export class Dispatch { | |
| } | ||
| } | ||
|
|
||
| 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 | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Send meta data and events to the AWS RUM data plane service via fetch. | ||
| */ | ||
|
|
@@ -273,17 +313,32 @@ export class Dispatch { | |
| } | ||
|
|
||
| private handleReject = (e: any): { response: HttpResponse } => { | ||
| if (e instanceof Error && this.disableCodes.includes(e.message)) { | ||
| // RUM disables only when dispatch fails and we are certain | ||
| // that subsequent attempts will not succeed, such as when | ||
| // credentials are invalid or the app monitor does not exist. | ||
| if (this.config.debug) { | ||
| InternalLogger.error( | ||
| 'Dispatch failed with status code:', | ||
| e.message | ||
| ); | ||
| if (e instanceof Error) { | ||
| if ( | ||
| e.message === '403' && | ||
| this.config.signing && | ||
| this.shouldPurgeCredentials | ||
| ) { | ||
| // 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 | ||
| // credentials will never get evicted. | ||
| // | ||
| // The next retry or request will be made with fresh credentials. | ||
| this.shouldPurgeCredentials = false; | ||
| this.forceRebuildClient(); | ||
| } else if (this.disableCodes.includes(e.message)) { | ||
| // RUM disables only when dispatch fails and we are certain | ||
| // that subsequent attempts will not succeed, such as when | ||
| // credentials are invalid or the app monitor does not exist. | ||
| if (this.config.debug) { | ||
| InternalLogger.error( | ||
| 'Dispatch failed with status code:', | ||
| e.message | ||
| ); | ||
| } | ||
| this.disable(); | ||
| } | ||
| this.disable(); | ||
| } | ||
| throw e; | ||
| }; | ||
|
|
@@ -314,4 +369,31 @@ export class Dispatch { | |
| headers: this.headers | ||
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * Purges the cached credentials and rebuilds the dataplane client. This is only necessary | ||
| * if signing is enabled and the cached credentials are for the wrong app monitor. | ||
| * | ||
| * @param credentialProvider - The credential or provider use to sign requests | ||
| */ | ||
| private forceRebuildClient() { | ||
| InternalLogger.warn('Removing credentials from local storage'); | ||
| localStorage.removeItem(this.credentialStorageKey); | ||
|
|
||
| if (this.config.identityPoolId) { | ||
| InternalLogger.info( | ||
| 'Rebuilding client with fresh cognito credentials' | ||
| ); | ||
| localStorage.removeItem(this.identityStorageKey); | ||
| this.setCognitoCredentials( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: will this retrigger credential fetching?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep |
||
| this.config.identityPoolId, | ||
| this.config.guestRoleArn | ||
| ); | ||
| } else if (this.credentialProvider) { | ||
| InternalLogger.info( | ||
| 'Rebuilding client with most recently passed provider' | ||
| ); | ||
| this.setAwsCredentials(this.credentialProvider); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i think this isn't an issue with the shouldPurge flag you added
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.
yeah we only purge once, and let the normal retries/backoff do their thing