-
Notifications
You must be signed in to change notification settings - Fork 407
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
Credentials Management #888
Changes from 36 commits
409af50
51c094f
74ae1c3
1989d51
aadf828
cdfb8a9
102655c
b7b6445
5ed5f73
9fd898f
6a83b2e
b5abf17
2ee351e
dcb2d4c
f14b125
f47fad3
29c760f
ef8e981
2440bbf
004d6b9
7380cce
c848df8
23f6962
c34820f
150f199
4094ee4
4a0e321
676ae7b
3a7afa6
f7491f3
7712177
5025c19
6f33ac4
bc32e0d
b262bc7
6c76476
5f38544
ab3807e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"type": "Bug Fix", | ||
"description": "Fixed an issue where invalid credentials were reused until VS Code was closed and re-opened, even if the credentials source was updated. It is no longer necessary to restart VS Code. (#705)" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"type": "Feature", | ||
"description": "When credentials are invalid a notification is shown. To help diagnose these situations, a button was added to the notification that can open the logs." | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"type": "Feature", | ||
"description": "When changes are made to Shared Credentials files, they will be picked up by the Toolkit the next time credentials are selected during the 'Connect to AWS' command." | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"type": "Feature", | ||
"description": "Credentials were previously shown by their Shared Credentials profile names. They are now displayed in a \"type:name\" format, to better indicate the type of Credentials being used, and to support additional Credentials types in the future. Shared Credentials are shown with the type \"profile\"." | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,8 @@ import { profileSettingKey } from '../shared/constants' | |
import { CredentialsProfileMru } from '../shared/credentials/credentialsProfileMru' | ||
import { SettingsConfiguration } from '../shared/settingsConfiguration' | ||
import { LoginManager } from './loginManager' | ||
import { CredentialsProviderId, fromString } from './providers/credentialsProviderId' | ||
import { SharedCredentialsProvider } from './providers/sharedCredentialsProvider' | ||
|
||
export interface CredentialsInitializeParameters { | ||
extensionContext: vscode.ExtensionContext | ||
|
@@ -29,9 +31,16 @@ export async function loginWithMostRecentCredentials( | |
toolkitSettings: SettingsConfiguration, | ||
loginManager: LoginManager | ||
): Promise<void> { | ||
const previousCredentialsId = toolkitSettings.readSetting(profileSettingKey, '') | ||
const previousCredentialsId = toolkitSettings.readSetting<string>(profileSettingKey, '') | ||
if (previousCredentialsId) { | ||
await loginManager.login(previousCredentialsId) | ||
// Migrate from older Toolkits - If the last providerId isn't in the new CredentialProviderId format, | ||
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. I get that this will make it more seamless for a user but I don't care to have this lying around if it's solely for that purpose. I'd say just invalidate the existing credentials and have the user log back in...it's fast enough that it shouldn't be an issue. At most, give them a one-version grace period and mark this for deletion in the next release afterwards. 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. This can be removed after a few versions. It makes for a better experience than seeing an "invalid credentials" notification, or a phantom log-out. It's low cost, the comment explains what is going on, and git history will tell us (if necessary) when this was added. |
||
// treat it like a Shared Crdentials Provider. | ||
const loginCredentialsId = tryMakeCredentialsProviderId(previousCredentialsId) ?? { | ||
credentialType: SharedCredentialsProvider.getCredentialsType(), | ||
credentialTypeId: previousCredentialsId | ||
} | ||
|
||
await loginManager.login(loginCredentialsId) | ||
} else { | ||
await loginManager.logout() | ||
} | ||
|
@@ -68,3 +77,11 @@ function updateConfigurationWhenAwsContextChanges( | |
}) | ||
) | ||
} | ||
|
||
function tryMakeCredentialsProviderId(credentialsProviderId: string): CredentialsProviderId | undefined { | ||
try { | ||
return fromString(credentialsProviderId) | ||
} catch (err) { | ||
return undefined | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,22 @@ | ||
/*! | ||
* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* Copyright 2019-2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import * as AWS from 'aws-sdk' | ||
import { CredentialsProvider } from './providers/credentialsProvider' | ||
import { asString, CredentialsProviderId } from './providers/credentialsProviderId' | ||
|
||
export interface CachedCredentials { | ||
credentials: AWS.Credentials | ||
credentialsHashCode: number | ||
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. what is this for? 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. It is a hash of the contents that were used to produce the credentials. |
||
} | ||
|
||
/** | ||
* Simple cache for credentials | ||
*/ | ||
export class CredentialsStore { | ||
private readonly credentialsCache: { [key: string]: AWS.Credentials } | ||
private readonly credentialsCache: { [key: string]: CachedCredentials } | ||
|
||
public constructor() { | ||
this.credentialsCache = {} | ||
|
@@ -18,34 +25,36 @@ export class CredentialsStore { | |
/** | ||
* Returns undefined if credentials are not stored for given ID | ||
*/ | ||
public async getCredentials(credentialsId: string): Promise<AWS.Credentials | undefined> { | ||
return this.credentialsCache[credentialsId] | ||
public async getCredentials(credentialsProviderId: CredentialsProviderId): Promise<CachedCredentials | undefined> { | ||
return this.credentialsCache[asString(credentialsProviderId)] | ||
} | ||
|
||
/** | ||
* If credentials are not stored, the provided create function is called. Created credentials are then stored. | ||
* If credentials are not stored, the credentialsProvider is used to produce credentials (which are then stored). | ||
*/ | ||
public async getCredentialsOrCreate( | ||
credentialsId: string, | ||
createCredentialsFn: (credentialsId: string) => Promise<AWS.Credentials> | ||
): Promise<AWS.Credentials> { | ||
let credentials = await this.getCredentials(credentialsId) | ||
|
||
if (credentials) { | ||
return credentials | ||
public async getOrCreateCredentials( | ||
credentialsProviderId: CredentialsProviderId, | ||
credentialsProvider: CredentialsProvider | ||
): Promise<CachedCredentials> { | ||
let credentials = await this.getCredentials(credentialsProviderId) | ||
|
||
if (!credentials) { | ||
credentials = { | ||
credentials: await credentialsProvider.getCredentials(), | ||
credentialsHashCode: credentialsProvider.getHashCode() | ||
} | ||
|
||
this.credentialsCache[asString(credentialsProviderId)] = credentials | ||
} | ||
|
||
credentials = await createCredentialsFn(credentialsId) | ||
this.credentialsCache[credentialsId] = credentials | ||
|
||
return credentials | ||
} | ||
|
||
/** | ||
* Evicts credentials from storage | ||
*/ | ||
public invalidateCredentials(credentialsId: string) { | ||
public invalidateCredentials(credentialsProviderId: CredentialsProviderId) { | ||
// tslint:disable-next-line:no-dynamic-delete | ||
delete this.credentialsCache[credentialsId] | ||
delete this.credentialsCache[asString(credentialsProviderId)] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/*! | ||
* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { CredentialsProviderId } from './credentialsProviderId' | ||
|
||
export interface CredentialsProvider { | ||
getCredentialsProviderId(): CredentialsProviderId | ||
getDefaultRegion(): string | undefined | ||
getHashCode(): number | ||
getCredentials(): Promise<AWS.Credentials> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
/*! | ||
* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { CredentialsProvider } from './credentialsProvider' | ||
import { CredentialsProviderId, isEqual } from './credentialsProviderId' | ||
|
||
/** | ||
* Responsible for producing CredentialsProvider objects for a Credential Type | ||
*/ | ||
export interface CredentialsProviderFactory { | ||
getCredentialType(): string | ||
listProviders(): CredentialsProvider[] | ||
getProvider(credentialsProviderId: CredentialsProviderId): CredentialsProvider | undefined | ||
refresh(): Promise<void> | ||
} | ||
|
||
export abstract class BaseCredentialsProviderFactory<T extends CredentialsProvider> | ||
implements CredentialsProviderFactory { | ||
protected providers: T[] = [] | ||
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. Is an array / list really the right data-structure here? We seem to be doing lookup-style operations mostly that point to a dictionary / map being a better choice. 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. The top access is a mix of "list all providers" and "get provider x" (the doc is currently open in #889 ). Collection still seems reasonable to me -- the lookups don't seem like they would have a heavy performance. |
||
public abstract getCredentialType(): string | ||
|
||
public listProviders(): T[] { | ||
return [...this.providers] | ||
hunterwerlla marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
public getProvider(credentialsProviderId: CredentialsProviderId): CredentialsProvider | undefined { | ||
for (const provider of this.providers) { | ||
if (isEqual(provider.getCredentialsProviderId(), credentialsProviderId)) { | ||
return provider | ||
} | ||
} | ||
|
||
return undefined | ||
} | ||
|
||
public abstract async refresh(): Promise<void> | ||
|
||
protected addProvider(provider: T) { | ||
this.providers.push(provider) | ||
} | ||
|
||
protected removeProvider(provider: T) { | ||
this.providers = this.providers.filter(x => x !== provider) | ||
} | ||
|
||
protected resetProviders() { | ||
this.providers = [] | ||
} | ||
} |
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.
is this something that should be attached to the credential itself rather than a top-level on
awsContext
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.
awsContext contains an AwsContextCredentials object, which then backs these calls. Would it make more sense to have a single getter for that object instead of getters for each property?