Fix password saving for the Credential Store when savePassword is ena…

…bled (#471) * updated removeProfile function to optionally remove password from the credential store * made removeProfile parameters easier to understand * adding test to cover new functionality * minor code fixes and added test for false keepCredentialStore
microsoft · Dec 7, 2016 · 2fa5704 · 2fa5704
1 parent 9b6fad9
commit 2fa5704
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 3 deletions.
diff --git a/src/models/connectionCredentials.ts b/src/models/connectionCredentials.ts
@@ -118,7 +118,8 @@ export class ConnectionCredentials implements IConnectionCredentials {
                     // If this is a profile, and the user has set save password to true and stored the password in the config file,
                     // then transfer the password to the credential store
                     if (profile.savePassword && !wasPasswordEmptyInConfigFile) {
-                        connectionStore.removeProfile(profile).then(() => {
+                        // Remove profile, but keep credential store if savePassword was enabled
+                        connectionStore.removeProfile(profile, true).then(() => {
                             connectionStore.saveProfile(profile);
                         });
                     // Or, if the user answered any additional questions for the profile, be sure to save it

diff --git a/src/models/connectionStore.ts b/src/models/connectionStore.ts
@@ -308,9 +308,10 @@ export class ConnectionStore {
      * from the credential store
      *
      * @param {IConnectionProfile} profile the profile to be removed
+     * @param {Boolean} keepCredentialStore optional value to keep the credential store after a profile removal
      * @returns {Promise<boolean>} true if successful
      */
-    public removeProfile(profile: IConnectionProfile): Promise<boolean> {
+    public removeProfile(profile: IConnectionProfile, keepCredentialStore: boolean = false): Promise<boolean> {
         const self = this;
         return new Promise<boolean>((resolve, reject) => {
             self._connectionConfig.removeConnection(profile).then(profileFound => {
@@ -329,12 +330,13 @@ export class ConnectionStore {
             });
         }).then(profileFound => {
             // Now remove password from credential store. Currently do not care about status unless an error occurred
-            if (profile.savePassword === true) {
+            if (profile.savePassword === true && !keepCredentialStore) {
                 let credentialId = ConnectionStore.formatCredentialId(profile.server, profile.database, profile.user, ConnectionStore.CRED_PROFILE_USER);
                 self._credentialStore.deleteCredential(credentialId).then(undefined, rejected => {
                     throw new Error(rejected);
                 });
             }
+
             return profileFound;
         });
     }

diff --git a/test/connectionStore.test.ts b/test/connectionStore.test.ts
@@ -215,6 +215,43 @@ suite('ConnectionStore tests', () => {
             }).catch(err => done(new Error(err)));
     });
 
+    test('RemoveProfile should not remove password from CredentialStore if keepCredentialStore is enabled', (done) => {
+        testRemoveProfileWithKeepCredential(true, done);
+    });
+
+    test('RemoveProfile should remove password from CredentialStore if keepCredentialStore is disabled', (done) => {
+        testRemoveProfileWithKeepCredential(false, done);
+    });
+
+    function testRemoveProfileWithKeepCredential(keepCredentialStore: boolean,  done: Function): void {
+        // Given have 2 profiles
+        let profile = Object.assign(new ConnectionProfile(), defaultNamedProfile, {
+            profileName: 'otherServer-bcd-cde',
+            server: 'otherServer',
+            savePassword: true
+        });
+        connectionConfig.setup(x => x.removeConnection(TypeMoq.It.isAny())).returns(p => Promise.resolve(p));
+        credentialStore.setup(x => x.deleteCredential(TypeMoq.It.isAny())).returns(() => Promise.resolve(true));
+
+        globalstate.setup(x => x.update(TypeMoq.It.isAnyString(), TypeMoq.It.isAny())).returns(() => Promise.resolve());
+        let connectionStore = new ConnectionStore(context.object, credentialStore.object, connectionConfig.object);
+
+        // deleteCredential should never be called when keepCredentialStore is set to true
+        connectionStore.removeProfile(profile, keepCredentialStore)
+            .then(success => {
+                // Then expect that profile's password to be removed from connectionConfig but kept in the credential store
+                assert.ok(success);
+                connectionConfig.verify(x => x.removeConnection(TypeMoq.It.isAny()), TypeMoq.Times.once());
+                if (keepCredentialStore) {
+                    credentialStore.verify(x => x.deleteCredential(TypeMoq.It.isAny()), TypeMoq.Times.never());
+                } else {
+                    credentialStore.verify(x => x.deleteCredential(TypeMoq.It.isAny()), TypeMoq.Times.once());
+                }
+                done();
+            }).catch(err => done(new Error(err)));
+    }
+
+
     test('RemoveProfile finds and removes profile with no profile name', (done) => {
         // Given have 2 profiles
         let unnamedProfile = Object.assign(new ConnectionProfile(), defaultNamedProfile, {