-
Notifications
You must be signed in to change notification settings - Fork 516
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 all 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 |
---|---|---|
@@ -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(): string | ||
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?