-
Notifications
You must be signed in to change notification settings - Fork 512
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 26 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 { CREDENTIALS_PROVIDER_ID_SEPARATOR, makeCredentialsProviderId } from './providers/credentialsProviderId' | ||
import { SharedCredentialsProvider } from './providers/sharedCredentialsProvider' | ||
|
||
export interface CredentialsInitializeParameters { | ||
extensionContext: vscode.ExtensionContext | ||
|
@@ -29,8 +31,17 @@ export async function loginWithMostRecentCredentials( | |
toolkitSettings: SettingsConfiguration, | ||
loginManager: LoginManager | ||
): Promise<void> { | ||
const previousCredentialsId = toolkitSettings.readSetting(profileSettingKey, '') | ||
let previousCredentialsId = toolkitSettings.readSetting<string>(profileSettingKey, '') | ||
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. nit: I really don't like the "create a mutable variable, apply a conditional and maybe re-assign it's value cos it's convenient" pattern - I know it's tightly scoped but I really like to avoid mutability where possible. It's a smell. I think you're better off changing this to something like : const previousCredentialsId = toolkitSettings.readSetting<string>(profileSettingKey, '')
if (previousCredentialsId) {
// Migrate from older Toolkits - If the last providerId isn't in the new CredentialProviderId format,
// treat it like a Shared Crdentials Provider.
const loginCredential = previousCredentialsId.indexOf(CREDENTIALS_PROVIDER_ID_SEPARATOR) === -1 ?
previousCredentialsId = makeCredentialsProviderId({
credentialType: SharedCredentialsProviderFactory.CREDENTIAL_TYPE,
credentialTypeId: previousCredentialsId
}) : previousCredentialsId
await loginManager.login(loginCredential)
} else {
await loginManager.logout()
} 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 completely agree. Prefer immutable where possible. Will change |
||
if (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. | ||
if (previousCredentialsId.indexOf(CREDENTIALS_PROVIDER_ID_SEPARATOR) === -1) { | ||
previousCredentialsId = makeCredentialsProviderId({ | ||
credentialType: SharedCredentialsProvider.getCredentialsType(), | ||
credentialTypeId: previousCredentialsId | ||
}) | ||
} | ||
|
||
await loginManager.login(previousCredentialsId) | ||
} else { | ||
await loginManager.logout() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,16 @@ | |
|
||
import * as AWS from 'aws-sdk' | ||
|
||
interface CredentialsData { | ||
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]: CredentialsData } | ||
|
||
public constructor() { | ||
this.credentialsCache = {} | ||
|
@@ -19,26 +24,33 @@ 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] | ||
return this.credentialsCache[credentialsId]?.credentials | ||
} | ||
|
||
/** | ||
* If credentials are not stored, the provided create function is called. Created credentials are then stored. | ||
*/ | ||
public async getCredentialsOrCreate( | ||
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. Nit: change the name to 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. Good catch 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. Unfortunately, this was added in a separate PR, so the rename belongs in a separate PR. 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 cache has been updated to treat Credentials + Hash as one object, so I added the rename as well. |
||
credentialsId: string, | ||
createCredentialsFn: (credentialsId: string) => Promise<AWS.Credentials> | ||
createCredentialsFn: (credentialsId: string) => Promise<CredentialsData> | ||
): Promise<AWS.Credentials> { | ||
let credentials = await this.getCredentials(credentialsId) | ||
const credentials = await this.getCredentials(credentialsId) | ||
|
||
if (credentials) { | ||
return credentials | ||
} | ||
|
||
credentials = await createCredentialsFn(credentialsId) | ||
this.credentialsCache[credentialsId] = credentials | ||
const newCredentials = await createCredentialsFn(credentialsId) | ||
this.credentialsCache[credentialsId] = newCredentials | ||
|
||
return newCredentials.credentials | ||
} | ||
|
||
return credentials | ||
/** | ||
* Returns undefined if credentials are not stored for given ID | ||
*/ | ||
public getCredentialsHashCode(credentialsId: string): number | undefined { | ||
return this.credentialsCache[credentialsId]?.credentialsHashCode | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,19 @@ | ||
/*! | ||
* Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
bryceitoc9 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import * as nls from 'vscode-nls' | ||
const localize = nls.loadMessageBundle() | ||
|
||
import * as vscode from 'vscode' | ||
import { AwsContext } from '../shared/awsContext' | ||
import { credentialHelpUrl } from '../shared/constants' | ||
import { getAccountId } from '../shared/credentials/accountId' | ||
import { UserCredentialsUtils } from '../shared/credentials/userCredentialsUtils' | ||
import { getLogger } from '../shared/logger' | ||
import { createCredentials } from './credentialsCreator' | ||
import { CredentialsStore } from './credentialsStore' | ||
import { CredentialsProvider } from './providers/credentialsProvider' | ||
import { CredentialsProviderManager } from './providers/credentialsProviderManager' | ||
|
||
export class LoginManager { | ||
private readonly credentialsStore: CredentialsStore = new CredentialsStore() | ||
|
@@ -21,31 +26,40 @@ export class LoginManager { | |
*/ | ||
public async login(credentialsId: string): Promise<void> { | ||
try { | ||
const credentials = await this.credentialsStore.getCredentialsOrCreate(credentialsId, createCredentials) | ||
const provider = await CredentialsProviderManager.getInstance().getCredentialsProvider(credentialsId) | ||
if (!provider) { | ||
throw new Error(`Could not find Credentials Provider for ${credentialsId}`) | ||
} | ||
|
||
await this.updateCredentialsStore(credentialsId, provider) | ||
|
||
const credentials = await this.credentialsStore.getCredentials(credentialsId) | ||
if (!credentials) { | ||
throw new Error(`No credentials found for id ${credentialsId}`) | ||
} | ||
|
||
// TODO : Get a region relevant to the partition for these credentials -- https://github.com/aws/aws-toolkit-vscode/issues/188 | ||
const accountId = await getAccountId(credentials, 'us-east-1') | ||
|
||
if (!accountId) { | ||
throw new Error('Could not determine Account Id for credentials') | ||
} | ||
|
||
await this.awsContext.setCredentials({ | ||
credentials: credentials, | ||
credentialsId: credentialsId, | ||
accountId: accountId | ||
accountId: accountId, | ||
defaultRegion: provider.getDefaultRegion() | ||
}) | ||
} catch (err) { | ||
getLogger().error('Error logging in', err as Error) | ||
getLogger().error( | ||
`Error trying to connect to AWS with Credentials Provider ${credentialsId}. Toolkit will now disconnect from AWS.`, | ||
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. Are these bubbled to the user? Should they be localized? 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. They're only surfaced to the user in the form of logs. Might be a good idea to surface the error to the user through the interface, but we definitely don't want to localize the logs. 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. They are only logged (not placed in a notification). The current approach has been to only localize contents that are sent to the UI, and not logged materials. 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. Take a look at the revamped notification in the PR description. The intent was to steer people to the logs if they were interested, without having to route specific details about the error into the UI. 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. Nit: the toolkit wasn't connected to AWS in the first place. |
||
err as Error | ||
) | ||
this.credentialsStore.invalidateCredentials(credentialsId) | ||
|
||
await this.logout() | ||
|
||
// tslint:disable-next-line: no-floating-promises | ||
UserCredentialsUtils.notifyUserCredentialsAreBad(credentialsId) | ||
this.notifyUserInvalidCredentials(credentialsId) | ||
} | ||
} | ||
|
||
|
@@ -55,4 +69,45 @@ export class LoginManager { | |
public async logout(): Promise<void> { | ||
await this.awsContext.setCredentials(undefined) | ||
} | ||
|
||
/** | ||
* Updates the CredentialsStore if the credentials are considered different | ||
*/ | ||
private async updateCredentialsStore(credentialsId: string, provider: CredentialsProvider): Promise<void> { | ||
const currentHash = this.credentialsStore.getCredentialsHashCode(credentialsId) | ||
if (provider.getHashCode() !== currentHash) { | ||
hunterwerlla marked this conversation as resolved.
Show resolved
Hide resolved
|
||
getLogger().verbose(`Credentials for ${credentialsId} have changed, using updated credentials.`) | ||
this.credentialsStore.invalidateCredentials(credentialsId) | ||
} | ||
|
||
await this.credentialsStore.getCredentialsOrCreate(credentialsId, async () => { | ||
hunterwerlla marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return { | ||
credentials: await provider.getCredentials(), | ||
credentialsHashCode: provider.getHashCode() | ||
} | ||
}) | ||
} | ||
|
||
private notifyUserInvalidCredentials(credentialProviderId: string) { | ||
const getHelp = localize('AWS.message.credentials.invalid.help', 'Get Help...') | ||
const viewLogs = localize('AWS.message.credentials.invalid.logs', 'View logs...') | ||
|
||
vscode.window | ||
.showErrorMessage( | ||
localize( | ||
'AWS.message.credentials.invalid', | ||
'Invalid Credentials {0}, see logs for more information.', | ||
credentialProviderId | ||
), | ||
getHelp, | ||
viewLogs | ||
) | ||
.then((selection: string | undefined) => { | ||
if (selection === getHelp) { | ||
vscode.env.openExternal(vscode.Uri.parse(credentialHelpUrl)) | ||
} else if (selection === viewLogs) { | ||
vscode.commands.executeCommand('aws.viewLogs') | ||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/*! | ||
* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
export interface CredentialsProvider { | ||
getCredentialsProviderId(): string | ||
getDefaultRegion(): string | undefined | ||
getHashCode(): number | ||
getCredentials(): Promise<AWS.Credentials> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/*! | ||
* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { CredentialsProvider } from './credentialsProvider' | ||
|
||
/** | ||
* Responsible for producing CredentialsProvider objects for a Credential Type | ||
*/ | ||
export interface CredentialsProviderFactory { | ||
getCredentialType(): string | ||
listProviders(): CredentialsProvider[] | ||
getProvider(credentialsProviderId: string): 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: string): CredentialsProvider | undefined { | ||
for (const provider of this.providers) { | ||
if (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 = [] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
/*! | ||
* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
export const CREDENTIALS_PROVIDER_ID_SEPARATOR = ':' | ||
hunterwerlla marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export interface CredentialsProviderIdComponents { | ||
credentialType: string | ||
credentialTypeId: string | ||
} | ||
|
||
export function makeCredentialsProviderIdComponents(credentialsProviderId: string): CredentialsProviderIdComponents { | ||
const chunks = credentialsProviderId.split(CREDENTIALS_PROVIDER_ID_SEPARATOR) | ||
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 "splitting by 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 think it is only explicit in the tests, for readability. Anywhere in the toolkit code goes through the make/decode methods. I initially stopped short of creating a credential provider id object. Since the intent was to support additional data in the future (using the separator), its probably saving future pain to make and pass around an object now instead of a string. I'll give this a go. |
||
|
||
if (chunks.length !== 2) { | ||
throw new Error(`Unexpected credentialsProviderId format: ${credentialsProviderId}`) | ||
} | ||
|
||
return { | ||
credentialType: chunks[0], | ||
credentialTypeId: chunks[1] | ||
} | ||
} | ||
|
||
export function makeCredentialsProviderId(credentialsProviderIdComponents: CredentialsProviderIdComponents): string { | ||
return [credentialsProviderIdComponents.credentialType, credentialsProviderIdComponents.credentialTypeId].join( | ||
CREDENTIALS_PROVIDER_ID_SEPARATOR | ||
) | ||
} |
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?