Skip to content
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.

Sign up for GitHub

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

chore: added migration step to migrate keytar secrets to vscode SecretStorage - VSCODE-435 #552

Merged
merged 17 commits into from
Jul 6, 2023
Merged

chore: added migration step to migrate keytar secrets to vscode SecretStorage - VSCODE-435 #552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5f85bb7
chore: added migration step to migrate keytar secrets to vscode secre…
himanshusinghs Jul 4, 2023
7e0e4c7
prettier reformatting
himanshusinghs Jul 4, 2023
f624554
PR feedback - better variable
himanshusinghs Jul 4, 2023
dcd2a11
PR feedback - replaces usage of assert with assert.strictEqual
himanshusinghs Jul 4, 2023
6c67419
PR feedback - reposition removing secrets from vscode.SecretStorage a…
himanshusinghs Jul 4, 2023
eea1fcc
PR feedback - replace {} with Object.create(null)
himanshusinghs Jul 4, 2023
2970158
PR feedback - renamed multiple migration method to make them more des…
himanshusinghs Jul 4, 2023
b83d44b
PR feedback - remove unnecessary type aliasing
himanshusinghs Jul 4, 2023
0742e4b
PR feedback - better error description:
himanshusinghs Jul 4, 2023
cdd9018
PR feedback - remove unnecessary variable to check for connection cha…
himanshusinghs Jul 4, 2023
e6ea5bf
PR feedback - use type unions instead of enums
himanshusinghs Jul 4, 2023
1249543
PR feedback - parallelise connection secrets fetch and track / notify…
himanshusinghs Jul 5, 2023
36e17c0
PR feedback - changed misleading comment, added a test for verifying …
himanshusinghs Jul 5, 2023
808e60e
PR feedback - catch possible errors from keytar.deletePassword
himanshusinghs Jul 5, 2023
4250dd6
ensures that failure of keytar migration do not affect loading of oth…
himanshusinghs Jul 5, 2023
ee19bb1
lint fixes
himanshusinghs Jul 5, 2023
8cca2be
PR feedback - updated the copy of the message for failed migration
himanshusinghs Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
363 changes: 219 additions & 144 deletions src/connectionController.ts
View file
Open in desktop

Large diffs are not rendered by default.

31 changes: 28 additions & 3 deletions src/storage/storageController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,15 @@ export type ConnectionsFromStorage = {
[connectionId: string]: StoreConnectionInfo;
};

export const SecretStorageLocation = {
Keytar: 'vscode.Keytar',
SecretStorage: 'vscode.SecretStorage',
} as const;

export type SecretStorageLocationType =
| typeof SecretStorageLocation.Keytar
| typeof SecretStorageLocation.SecretStorage;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be just export type SecretStorageLocationType = 'vscode.Keytar' | 'vscode.SecretStorage';? Do we need at all SecretStorageLocation? cc @addaleax

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I think it could be. I probably did it this way just so I also have a string const to use in places where the types are not available from the context (for example tests and a couple other places in the code as well where we declare a connection info like object with secret storage).

interface StorageVariableContents {
[StorageVariables.GLOBAL_USER_ID]: string;
[StorageVariables.GLOBAL_ANONYMOUS_ID]: string;
Expand All @@ -48,11 +57,14 @@ export default class StorageController {
[StorageLocation.WORKSPACE]: vscode.Memento;
};

_secretStorage: vscode.SecretStorage;

constructor(context: vscode.ExtensionContext) {
this._storage = {
[StorageLocation.GLOBAL]: context.globalState,
[StorageLocation.WORKSPACE]: context.workspaceState,
};
this._secretStorage = context.secrets;
}

get<T extends StoredVariableName>(
Expand Down Expand Up @@ -123,9 +135,9 @@ export default class StorageController {
);
}

async saveConnection(
storeConnectionInfo: StoreConnectionInfo
): Promise<StoreConnectionInfo> {
async saveConnection<T extends StoreConnectionInfo>(
storeConnectionInfo: T
): Promise<T> {
const dontShowSaveLocationPrompt = vscode.workspace
.getConfiguration('mdb.connectionSaving')
.get('hideOptionToChooseWhereToSaveNewConnections');
Expand Down Expand Up @@ -243,4 +255,17 @@ export default class StorageController {

return StorageLocation.NONE;
}

async getSecret(key: string): Promise<string | null> {
return (await this._secretStorage.get(key)) ?? null;
}

async deleteSecret(key: string): Promise<boolean> {
await this._secretStorage.delete(key);
return true;
}

async setSecret(key: string, value: string): Promise<void> {
await this._secretStorage.store(key, value);
}
}
9 changes: 8 additions & 1 deletion src/telemetry/telemetryService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ type PlaygroundLoadedTelemetryEventProperties = {
file_type?: string;
};

type KeytarSecretsMigrationFailedProperties = {
totalConnections: number;
connectionsWithFailedKeytarMigration: number;
};

export type TelemetryEventProperties =
| PlaygroundTelemetryEventProperties
| LinkClickedTelemetryEventProperties
Expand All @@ -78,7 +83,8 @@ export type TelemetryEventProperties =
| QueryExportedTelemetryEventProperties
| PlaygroundCreatedTelemetryEventProperties
| PlaygroundSavedTelemetryEventProperties
| PlaygroundLoadedTelemetryEventProperties;
| PlaygroundLoadedTelemetryEventProperties
| KeytarSecretsMigrationFailedProperties;

export enum TelemetryEventTypes {
PLAYGROUND_CODE_EXECUTED = 'Playground Code Executed',
Expand All @@ -92,6 +98,7 @@ export enum TelemetryEventTypes {
QUERY_EXPORTED = 'Query Exported',
AGGREGATION_EXPORTED = 'Aggregation Exported',
PLAYGROUND_CREATED = 'Playground Created',
KEYTAR_SECRETS_MIGRATION_FAILED = 'Keytar Secrets Migration Failed',
}

/**
Expand Down
Loading