Settings now removes password when a connection is added manually (#1512

)

* settings now removes password from as plaintext

* review comments

* remove password for all connections

* code review comments

* remove delete password logic

* prepopulate password if it's saved

* use vscode wrapper to set config

* fix tests

* fix tests

* fix connection profile tests

* fix tests

src/connectionconfig/connectionconfig.ts

@@ -3,7 +3,7 @@
 *  Licensed under the MIT License. See License.txt in the project root for license information.
 *--------------------------------------------------------------------------------------------*/
 'use strict';
-
+import * as vscode from 'vscode';
 import * as Constants from '../constants/constants';
 import * as LocalizedConstants from '../constants/localizedConstants';
 import * as Utils from '../models/utils';
@@ -150,7 +150,7 @@ export class ConnectionConfig implements IConnectionConfig {
         // Save the file
         const self = this;
         return new Promise<void>((resolve, reject) => {
-            self._vscodeWrapper.getConfiguration(Constants.extensionName).update(Constants.connectionsArrayName, profiles, true).then(() => {
+            self._vscodeWrapper.setConfiguration(Constants.extensionName, Constants.connectionsArrayName, profiles).then(() => {
                 resolve();
             }, err => {
                 reject(err);

src/controllers/mainController.ts

@@ -931,14 +931,27 @@ export default class MainController implements vscode.Disposable {
             for (let conn of newConnections) {
                 // if a connection is not connected
                 // that means it was added manually
-                const uri = ObjectExplorerUtils.getNodeUriFromProfile(<IConnectionProfile>conn);
+                const newConnectionProfile = <IConnectionProfile>conn;
+                const uri = ObjectExplorerUtils.getNodeUriFromProfile(newConnectionProfile);
                 if (!this.connectionManager.isActiveConnection(conn) &&
                     !this.connectionManager.isConnecting(uri)) {
-                    // add a disconnected node for it
-                    needsRefresh = true;
+                    // add a disconnected node for the connection
                     this._objectExplorerProvider.addDisconnectedNode(conn);
+                    needsRefresh = true;
                 }
             }
+
+            // Get rid of all passwords in the settings file
+            for (let conn of userConnections) {
+                if (!Utils.isEmpty(conn.password)) {
+                    // save the password in the credential store if save password is true
+                    await this.connectionManager.connectionStore.saveProfilePasswordIfNeeded(conn);
+                }
+                conn.password = '';
+            }
+
+            this._vscodeWrapper.setConfiguration(Constants.extensionName, Constants.connectionsArrayName, userConnections);
+
             if (needsRefresh) {
                 this._objectExplorerProvider.refresh(undefined);
             }

src/controllers/vscodeWrapper.ts

@@ -339,6 +339,13 @@ export default class VscodeWrapper {
     }
 
     /**
+     * Change a configuration setting
+     */
+    public setConfiguration(extensionName: string, resource: string, value: any): Thenable<void> {
+        return this.getConfiguration(extensionName).update(resource, value, vscode.ConfigurationTarget.Global);
+    }
+
+    /*
      * Called when there's a change in the extensions
      */
     public get onDidChangeExtensions(): vscode.Event<void> {

src/models/connectionCredentials.ts

@@ -85,7 +85,7 @@ export class ConnectionCredentials implements IConnectionCredentials {
         return details;
     }
 
-    public static ensureRequiredPropertiesSet(
+    public static async ensureRequiredPropertiesSet(
         credentials: IConnectionCredentials,
         isProfile: boolean,
         isPasswordRequired: boolean,
@@ -94,7 +94,7 @@ export class ConnectionCredentials implements IConnectionCredentials {
         connectionStore: ConnectionStore,
         defaultProfileValues?: IConnectionCredentials): Promise<IConnectionCredentials> {
 
-        let questions: IQuestion[] = ConnectionCredentials.getRequiredCredentialValuesQuestions(credentials, false, isPasswordRequired, defaultProfileValues);
+        let questions: IQuestion[] = await ConnectionCredentials.getRequiredCredentialValuesQuestions(credentials, false, isPasswordRequired, connectionStore, defaultProfileValues);
         let unprocessedCredentials: IConnectionCredentials = Object.assign({}, credentials);
 
         // Potentially ask to save password
@@ -153,11 +153,12 @@ export class ConnectionCredentials implements IConnectionCredentials {
     }
 
     // gets a set of questions that ensure all required and core values are set
-    protected static getRequiredCredentialValuesQuestions(
+    protected static async getRequiredCredentialValuesQuestions(
         credentials: IConnectionCredentials,
         promptForDbName: boolean,
         isPasswordRequired: boolean,
-        defaultProfileValues?: IConnectionCredentials): IQuestion[] {
+        connectionStore: ConnectionStore,
+        defaultProfileValues?: IConnectionCredentials): Promise<IQuestion[]> {
 
         let authenticationChoices: INameValueChoice[] = ConnectionCredentials.getAuthenticationTypesChoice();
 
@@ -235,7 +236,8 @@ export class ConnectionCredentials implements IConnectionCredentials {
                             (<IConnectionProfile>credentials).emptyPasswordInput = utils.isEmpty(credentials.password);
                         }
                     }
-                }
+                },
+                default: defaultProfileValues ? await connectionStore.lookupPassword(defaultProfileValues) : undefined
             }
         ];
         return questions;

src/models/connectionProfile.ts

@@ -9,6 +9,7 @@ import { IConnectionProfile, AuthenticationTypes } from './interfaces';
 import { ConnectionCredentials } from './connectionCredentials';
 import { QuestionTypes, IQuestion, IPrompter, INameValueChoice } from '../prompts/question';
 import * as utils from './utils';
+import { ConnectionStore } from './connectionStore';
 
 // Concrete implementation of the IConnectionProfile interface
 
@@ -26,7 +27,7 @@ export class ConnectionProfile extends ConnectionCredentials implements IConnect
      * @param  {IConnectionProfile} (optional) default profile values that will be prefilled for questions, if any
      * @returns Promise - resolves to undefined if profile creation was not completed, or IConnectionProfile if completed
      */
-    public static createProfile(prompter: IPrompter, defaultProfileValues?: IConnectionProfile): Promise<IConnectionProfile> {
+    public static async createProfile(prompter: IPrompter, connectionStore: ConnectionStore, defaultProfileValues?: IConnectionProfile): Promise<IConnectionProfile> {
         let profile: ConnectionProfile = new ConnectionProfile();
         // Ensure all core properties are entered
         let authOptions: INameValueChoice[] = ConnectionCredentials.getAuthenticationTypesChoice();
@@ -35,7 +36,7 @@ export class ConnectionProfile extends ConnectionCredentials implements IConnect
             profile.authenticationType = authOptions[0].value;
         }
 
-        let questions: IQuestion[] = ConnectionCredentials.getRequiredCredentialValuesQuestions(profile, true, false, defaultProfileValues);
+        let questions: IQuestion[] = await ConnectionCredentials.getRequiredCredentialValuesQuestions(profile, true, false, connectionStore, defaultProfileValues);
         // Check if password needs to be saved
         questions.push(
             {

src/models/connectionStore.ts

@@ -317,7 +317,7 @@ export class ConnectionStore {
         });
     }
 
-    private saveProfilePasswordIfNeeded(profile: IConnectionProfile): Promise<boolean> {
+    public saveProfilePasswordIfNeeded(profile: IConnectionProfile): Promise<boolean> {
         if (!profile.savePassword) {
             return Promise.resolve(true);
         }
@@ -401,6 +401,17 @@ export class ConnectionStore {
         return result;
     }
 
+
+    /**
+     * Removes password from a saved profile and credential store
+     */
+    public async removeProfilePassword(connection: IConnectionCredentials): Promise<void> {
+        // if the password is saved in the credential store, remove it
+        let profile = connection as IConnectionProfile;
+        profile.password = '';
+        await this.saveProfile(profile);
+    }
+
     // Load connections from user preferences
     public loadAllConnections(addRecentConnections: boolean = false): IConnectionCredentialsQuickPickItem[] {
         let quickPickItems: IConnectionCredentialsQuickPickItem[] = [];

src/objectExplorer/objectExplorerService.ts

@@ -253,6 +253,26 @@ export class ObjectExplorerService {
         return [new AddConnectionTreeNode()];
     }
 
+    /**
+     * Handles a generic OE create session failure by creating a
+     * sign in node
+     */
+    private createSignInNode(element: TreeNodeInfo): AccountSignInTreeNode[] {
+        const signInNode = new AccountSignInTreeNode(element);
+        this._treeNodeToChildrenMap.set(element, [signInNode]);
+        return [signInNode];
+    }
+
+    /**
+     * Handles a connection error after an OE session is
+     * sucessfully created by creating a connect node
+     */
+    private createConnectTreeNode(element: TreeNodeInfo): ConnectTreeNode[] {
+        const connectNode = new ConnectTreeNode(element);
+        this._treeNodeToChildrenMap.set(element, [connectNode]);
+        return [connectNode];
+    }
+
     async getChildren(element?: TreeNodeInfo): Promise<vscode.TreeItem[]> {
         if (element) {
             if (element !== this._currentNode) {
@@ -284,17 +304,19 @@ export class ObjectExplorerService {
                     const sessionId = await this.createSession(promise, element.connectionCredentials);
                     if (sessionId) {
                         let node = await promise;
-                        // if password failed
+                        // if the server was found but connection failed
                         if (!node) {
-                            const connectNode = new ConnectTreeNode(element);
-                            this._treeNodeToChildrenMap.set(element, [connectNode]);
-                            return [connectNode];
+                            let profile = element.connectionCredentials as IConnectionProfile;
+                            let password = await this._connectionManager.connectionStore.lookupPassword(profile);
+                            if (password) {
+                                return this.createSignInNode(element);
+                            } else {
+                                return this.createConnectTreeNode(element);
+                            }
                         }
                     } else {
-                        // If node create session failed
-                        const signInNode = new AccountSignInTreeNode(element);
-                        this._treeNodeToChildrenMap.set(element, [signInNode]);
-                        return [signInNode];
+                        // If node create session failed (server wasn't found)
+                        return this.createSignInNode(element);
                     }
                     // otherwise expand the node by refreshing the root
                     // to add connected context key

src/prompts/input.ts

@@ -37,6 +37,9 @@ export default class InputPrompt extends Prompt {
         }
 
         this._options.placeHolder = placeHolder;
+        if (this._question.default) {
+            this._options.value = this._question.default;
+        }
 
         return this._vscodeWrapper.showInputBox(this._options)
             .then(result => {

src/views/connectionUI.ts

@@ -545,7 +545,7 @@ export class ConnectionUI {
     }
 
     private promptForCreateProfile(): Promise<IConnectionProfile> {
-        return ConnectionProfile.createProfile(this._prompter);
+        return ConnectionProfile.createProfile(this._prompter, this._connectionStore);
     }
 
     private async promptToRetryAndSaveProfile(profile: IConnectionProfile, isFirewallError: boolean = false): Promise<IConnectionProfile> {
@@ -562,7 +562,7 @@ export class ConnectionUI {
         let errorMessage = isFirewallError ? LocalizedConstants.msgPromptRetryFirewallRuleAdded : LocalizedConstants.msgPromptRetryCreateProfile;
         return this._vscodeWrapper.showErrorMessage(errorMessage, LocalizedConstants.retryLabel).then(result => {
             if (result === LocalizedConstants.retryLabel) {
-                return ConnectionProfile.createProfile(this._prompter, profile);
+                return ConnectionProfile.createProfile(this._prompter, this._connectionStore, profile);
             } else {
                 return undefined;
             }

test/connectionCredentials.test.ts

@@ -209,9 +209,9 @@ suite('ConnectionCredentials Tests', () => {
         assert.equal(connectionDetails.options.database, credentials.database);
     });
 
-    test('Subsequent connection credential questions are skipped if a connection string is given', () => {
+    test('Subsequent connection credential questions are skipped if a connection string is given', async () => {
         let credentials = new ConnectionCredentials();
-        let questions = ConnectionCredentials['getRequiredCredentialValuesQuestions'](credentials, false, false);
+        let questions = await ConnectionCredentials['getRequiredCredentialValuesQuestions'](credentials, false, false, undefined);
         let serverQuestion = questions.filter(question => question.name === LocalizedConstants.serverPrompt)[0];
 
         let connectionString = 'server=some-server';
@@ -222,9 +222,9 @@ suite('ConnectionCredentials Tests', () => {
         otherQuestions.forEach(question => assert.equal(question.shouldPrompt({}), false));
     });
 
-    test('Server question properly handles connection strings', () => {
+    test('Server question properly handles connection strings', async () => {
         let credentials = new ConnectionCredentials();
-        let questions = ConnectionCredentials['getRequiredCredentialValuesQuestions'](credentials, false, false);
+        let questions = await ConnectionCredentials['getRequiredCredentialValuesQuestions'](credentials, false, false, undefined);
         let serverQuestion = questions.filter(question => question.name === LocalizedConstants.serverPrompt)[0];
 
         let connectionString = 'server=some-server';

test/connectionProfile.test.ts

@@ -59,7 +59,7 @@ suite('Connection Profile tests', () => {
         // No setup currently needed
     });
 
-    test('CreateProfile should ask questions in correct order', done => {
+    test('CreateProfile should ask questions in correct order', async () => {
         // Given
         let prompter: TypeMoq.IMock<IPrompter> = TypeMoq.Mock.ofType(TestPrompter);
         let answers: {[key: string]: string} = {};
@@ -77,7 +77,7 @@ suite('Connection Profile tests', () => {
                     return Promise.resolve(answers);
                 });
 
-        ConnectionProfile.createProfile(prompter.object)
+        await ConnectionProfile.createProfile(prompter.object, undefined)
             .then(profile => profileReturned = profile);
 
         // Then expect the following flow:
@@ -97,7 +97,6 @@ suite('Connection Profile tests', () => {
         }
         // And expect result to be undefined as questions were not answered
         assert.strictEqual(profileReturned, undefined);
-        done();
     });
 
 
@@ -118,7 +117,7 @@ suite('Connection Profile tests', () => {
                     return answers;
                 });
 
-        await ConnectionProfile.createProfile(prompter.object);
+        await ConnectionProfile.createProfile(prompter.object, undefined);
 
         // Then expect SqlAuth to be the only default type
         let authChoices = <INameValueChoice[]>profileQuestions[authTypeQuestionIndex].choices;
@@ -141,7 +140,7 @@ suite('Connection Profile tests', () => {
                 });
 
         // When createProfile is called on an OS
-        await ConnectionProfile.createProfile(prompter.object);
+        await ConnectionProfile.createProfile(prompter.object, undefined);
 
         // Then integrated auth should/should not be supported
         // TODO if possible the test should mock out the OS dependency but it's not clear
@@ -307,7 +306,7 @@ suite('Connection Profile tests', () => {
         });
 
         // Verify that a profile was created
-        ConnectionProfile.createProfile(prompter.object).then( profile => {
+        ConnectionProfile.createProfile(prompter.object, undefined).then( profile => {
             assert.equal(Boolean(profile), true);
             done();
         });

test/connectionStore.test.ts

@@ -280,6 +280,11 @@ suite('ConnectionStore tests', () => {
             return workspaceConfiguration;
         });
 
+        vscodeWrapper.setup(x => x.setConfiguration(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+        .returns(() => {
+            return workspaceConfiguration.update('connections', [defaultNamedProfile, namedProfile]);
+        });
+
         let config = new ConnectionConfig(vscodeWrapper.object);
 
         credentialStore.setup(x => x.deleteCredential(TypeMoq.It.isAny()))
@@ -320,6 +325,11 @@ suite('ConnectionStore tests', () => {
             return workspaceConfiguration;
         });
 
+        vscodeWrapper.setup(x => x.setConfiguration(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+        .returns(() => {
+            return workspaceConfiguration.update('connections', [defaultNamedProfile, unnamedProfile]);
+        });
+
         let config = new ConnectionConfig(vscodeWrapper.object);
 
         credentialStore.setup(x => x.deleteCredential(TypeMoq.It.isAny()))