Skip to content

Commit 2fa5704

Browse files
authored
Fix password saving for the Credential Store when savePassword is enabled (#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
1 parent 9b6fad9 commit 2fa5704

File tree

3 files changed

+43
-3
lines changed

3 files changed

+43
-3
lines changed

src/models/connectionCredentials.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,8 @@ export class ConnectionCredentials implements IConnectionCredentials {
118118
// If this is a profile, and the user has set save password to true and stored the password in the config file,
119119
// then transfer the password to the credential store
120120
if (profile.savePassword && !wasPasswordEmptyInConfigFile) {
121-
connectionStore.removeProfile(profile).then(() => {
121+
// Remove profile, but keep credential store if savePassword was enabled
122+
connectionStore.removeProfile(profile, true).then(() => {
122123
connectionStore.saveProfile(profile);
123124
});
124125
// Or, if the user answered any additional questions for the profile, be sure to save it

src/models/connectionStore.ts

+4-2
Original file line numberDiff line numberDiff line change
@@ -308,9 +308,10 @@ export class ConnectionStore {
308308
* from the credential store
309309
*
310310
* @param {IConnectionProfile} profile the profile to be removed
311+
* @param {Boolean} keepCredentialStore optional value to keep the credential store after a profile removal
311312
* @returns {Promise<boolean>} true if successful
312313
*/
313-
public removeProfile(profile: IConnectionProfile): Promise<boolean> {
314+
public removeProfile(profile: IConnectionProfile, keepCredentialStore: boolean = false): Promise<boolean> {
314315
const self = this;
315316
return new Promise<boolean>((resolve, reject) => {
316317
self._connectionConfig.removeConnection(profile).then(profileFound => {
@@ -329,12 +330,13 @@ export class ConnectionStore {
329330
});
330331
}).then(profileFound => {
331332
// Now remove password from credential store. Currently do not care about status unless an error occurred
332-
if (profile.savePassword === true) {
333+
if (profile.savePassword === true && !keepCredentialStore) {
333334
let credentialId = ConnectionStore.formatCredentialId(profile.server, profile.database, profile.user, ConnectionStore.CRED_PROFILE_USER);
334335
self._credentialStore.deleteCredential(credentialId).then(undefined, rejected => {
335336
throw new Error(rejected);
336337
});
337338
}
339+
338340
return profileFound;
339341
});
340342
}

test/connectionStore.test.ts

+37
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,43 @@ suite('ConnectionStore tests', () => {
215215
}).catch(err => done(new Error(err)));
216216
});
217217

218+
test('RemoveProfile should not remove password from CredentialStore if keepCredentialStore is enabled', (done) => {
219+
testRemoveProfileWithKeepCredential(true, done);
220+
});
221+
222+
test('RemoveProfile should remove password from CredentialStore if keepCredentialStore is disabled', (done) => {
223+
testRemoveProfileWithKeepCredential(false, done);
224+
});
225+
226+
function testRemoveProfileWithKeepCredential(keepCredentialStore: boolean, done: Function): void {
227+
// Given have 2 profiles
228+
let profile = Object.assign(new ConnectionProfile(), defaultNamedProfile, {
229+
profileName: 'otherServer-bcd-cde',
230+
server: 'otherServer',
231+
savePassword: true
232+
});
233+
connectionConfig.setup(x => x.removeConnection(TypeMoq.It.isAny())).returns(p => Promise.resolve(p));
234+
credentialStore.setup(x => x.deleteCredential(TypeMoq.It.isAny())).returns(() => Promise.resolve(true));
235+
236+
globalstate.setup(x => x.update(TypeMoq.It.isAnyString(), TypeMoq.It.isAny())).returns(() => Promise.resolve());
237+
let connectionStore = new ConnectionStore(context.object, credentialStore.object, connectionConfig.object);
238+
239+
// deleteCredential should never be called when keepCredentialStore is set to true
240+
connectionStore.removeProfile(profile, keepCredentialStore)
241+
.then(success => {
242+
// Then expect that profile's password to be removed from connectionConfig but kept in the credential store
243+
assert.ok(success);
244+
connectionConfig.verify(x => x.removeConnection(TypeMoq.It.isAny()), TypeMoq.Times.once());
245+
if (keepCredentialStore) {
246+
credentialStore.verify(x => x.deleteCredential(TypeMoq.It.isAny()), TypeMoq.Times.never());
247+
} else {
248+
credentialStore.verify(x => x.deleteCredential(TypeMoq.It.isAny()), TypeMoq.Times.once());
249+
}
250+
done();
251+
}).catch(err => done(new Error(err)));
252+
}
253+
254+
218255
test('RemoveProfile finds and removes profile with no profile name', (done) => {
219256
// Given have 2 profiles
220257
let unnamedProfile = Object.assign(new ConnectionProfile(), defaultNamedProfile, {

0 commit comments

Comments
 (0)