diff --git a/src/sql/parts/objectExplorer/viewlet/treeSelectionHandler.ts b/src/sql/parts/objectExplorer/viewlet/treeSelectionHandler.ts index 9304913e96..786f8684fd 100644 --- a/src/sql/parts/objectExplorer/viewlet/treeSelectionHandler.ts +++ b/src/sql/parts/objectExplorer/viewlet/treeSelectionHandler.ts @@ -81,7 +81,7 @@ export class TreeSelectionHandler { let connectionProfile: ConnectionProfile = undefined; let options: IConnectionCompletionOptions = { params: undefined, - saveTheConnection: false, + saveTheConnection: true, showConnectionDialogOnError: true, showFirewallRuleOnError: true, showDashboard: isDoubleClick // only show the dashboard if the action is double click diff --git a/src/sql/platform/connection/common/connectionManagementService.ts b/src/sql/platform/connection/common/connectionManagementService.ts index 7492fab72e..22ab12db69 100644 --- a/src/sql/platform/connection/common/connectionManagementService.ts +++ b/src/sql/platform/connection/common/connectionManagementService.ts @@ -464,6 +464,12 @@ export class ConnectionManagementService extends Disposable implements IConnecti } this.createNewConnection(uri, connection).then(connectionResult => { if (connectionResult && connectionResult.connected) { + // The connected succeeded so add it to our active connections now, optionally adding it to the MRU based on + // the options.saveTheConnection setting + let connectionMgmtInfo = this._connectionStatusManager.findConnection(uri); + let activeConnection = connectionMgmtInfo.connectionProfile; + this.tryAddActiveConnection(connectionMgmtInfo, activeConnection, options.saveTheConnection); + if (callbacks.onConnectSuccess) { callbacks.onConnectSuccess(options.params); } @@ -860,9 +866,9 @@ export class ConnectionManagementService extends Disposable implements IConnecti /** * Add a connection to the active connections list. */ - private tryAddActiveConnection(connectionManagementInfo: ConnectionManagementInfo, newConnection: IConnectionProfile, isConnectionToDefaultDb: boolean): void { + private tryAddActiveConnection(connectionManagementInfo: ConnectionManagementInfo, newConnection: IConnectionProfile, addToMru: boolean): void { if (newConnection) { - this._connectionStore.addActiveConnection(newConnection, isConnectionToDefaultDb) + this._connectionStore.addActiveConnection(newConnection, addToMru) .then(() => { connectionManagementInfo.connectHandler(true); }, err => { @@ -896,10 +902,6 @@ export class ConnectionManagementService extends Disposable implements IConnecti let connection = this._connectionStatusManager.onConnectionComplete(info); if (info.connectionId) { - let isConnectionToDefaultDb = false; - if (connection.connectionProfile && (!connection.connectionProfile.databaseName || connection.connectionProfile.databaseName.trim() === '')) { - isConnectionToDefaultDb = true; - } if (info.connectionSummary && info.connectionSummary.databaseName) { this._connectionStatusManager.updateDatabaseName(info); } @@ -907,8 +909,6 @@ export class ConnectionManagementService extends Disposable implements IConnecti connection.extensionTimer.stop(); connection.connectHandler(true); - let activeConnection = connection.connectionProfile; - self.tryAddActiveConnection(connection, activeConnection, isConnectionToDefaultDb); self.addTelemetryForConnection(connection); if (self._connectionStatusManager.isDefaultTypeUri(info.ownerUri)) { diff --git a/src/sql/platform/connection/common/connectionProfile.ts b/src/sql/platform/connection/common/connectionProfile.ts index b4b305e9df..f9b6bfa277 100644 --- a/src/sql/platform/connection/common/connectionProfile.ts +++ b/src/sql/platform/connection/common/connectionProfile.ts @@ -181,6 +181,13 @@ export class ConnectionProfile extends ProviderConnectionInfo implements interfa }; } + /** + * Returns whether this profile is connected to the default database (it doesn't specify a database to connect to) + */ + public static isConnectionToDefaultDb(profile: azdata.IConnectionProfile): boolean { + return !profile.databaseName || profile.databaseName.trim() === ''; + } + public static fromIConnectionProfile(capabilitiesService: ICapabilitiesService, profile: azdata.IConnectionProfile) { if (profile) { if (profile instanceof ConnectionProfile) { diff --git a/src/sql/platform/connection/common/connectionStore.ts b/src/sql/platform/connection/common/connectionStore.ts index 3b34ea19b7..8fae8094af 100644 --- a/src/sql/platform/connection/common/connectionStore.ts +++ b/src/sql/platform/connection/common/connectionStore.ts @@ -277,20 +277,30 @@ export class ConnectionStore { * Password values are stored to a separate credential store if the "savePassword" option is true * * @param {IConnectionCredentials} conn the connection to add + * @param {boolean} addToMru Whether to add this connection to the MRU * @returns {Promise} a Promise that returns when the connection was saved */ - public addActiveConnection(conn: IConnectionProfile, isConnectionToDefaultDb: boolean = false): Promise { - if (this.getActiveConnections().some(existingConn => existingConn.id === conn.id)) { - return Promise.resolve(undefined); - } else { - return this.addConnectionToMemento(conn, Constants.activeConnections, undefined, conn.savePassword).then(() => { - let maxConnections = this.getMaxRecentConnectionsCount(); - if (isConnectionToDefaultDb) { - conn.databaseName = ''; - } - return this.addConnectionToMemento(conn, Constants.recentConnections, maxConnections); - }); + public async addActiveConnection(conn: IConnectionProfile, addToMru: boolean): Promise { + if (addToMru) { + await this.addConnectionToMru(conn); } + + // Only add connections we don't already know about + if (!this.getActiveConnections().some(existingConn => existingConn.id === conn.id)) { + await this.addConnectionToMemento(conn, Constants.activeConnections, undefined, conn.savePassword); + } + } + + /** + * Adds the specified connection to the MRU list + * @param conn The connection to add + */ + private async addConnectionToMru(conn: IConnectionProfile): Promise { + let maxConnections = this.getMaxRecentConnectionsCount(); + if (ConnectionProfile.isConnectionToDefaultDb(conn)) { + conn.databaseName = ''; + } + await this.addConnectionToMemento(conn, Constants.recentConnections, maxConnections); } public addConnectionToMemento(conn: IConnectionProfile, mementoKey: string, maxConnections?: number, savePassword?: boolean): Promise { diff --git a/src/sqltest/parts/connection/connectionManagementService.test.ts b/src/sqltest/parts/connection/connectionManagementService.test.ts index d6582f68a8..bd2b8a0fe1 100644 --- a/src/sqltest/parts/connection/connectionManagementService.test.ts +++ b/src/sqltest/parts/connection/connectionManagementService.test.ts @@ -99,7 +99,6 @@ suite('SQL ConnectionManagementService tests', () => { connectionDialogService.setup(x => x.showDialog(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => TPromise.as(none)); connectionDialogService.setup(x => x.showDialog(TypeMoq.It.isAny(), TypeMoq.It.isAny(), undefined, TypeMoq.It.isAny())).returns(() => TPromise.as(none)); - connectionStore.setup(x => x.addActiveConnection(TypeMoq.It.isAny())).returns(() => Promise.resolve()); connectionStore.setup(x => x.addActiveConnection(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve()); connectionStore.setup(x => x.saveProfile(TypeMoq.It.isAny())).returns(() => Promise.resolve(connectionProfile)); workbenchEditorService.setup(x => x.openEditor(undefined, TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => TPromise.as(undefined)); diff --git a/src/sqltest/parts/connection/connectionStore.test.ts b/src/sqltest/parts/connection/connectionStore.test.ts index 4f1b590886..1e5920b02b 100644 --- a/src/sqltest/parts/connection/connectionStore.test.ts +++ b/src/sqltest/parts/connection/connectionStore.test.ts @@ -227,7 +227,7 @@ suite('SQL ConnectionStore tests', () => { defaultNamedConnectionProfile = new ConnectionProfile(capabilitiesService, defaultNamedProfile); }); - test('addActiveConnection should limit recent connection saves to the MaxRecentConnections amount', (done) => { + test('addActiveConnection should limit recent connection saves to the MaxRecentConnections amount', async () => { // Given 5 is the max # creds let numCreds = 6; @@ -239,32 +239,47 @@ suite('SQL ConnectionStore tests', () => { // Expect all of them to be saved even if size is limited to 3 let connectionStore = new ConnectionStore(storageServiceMock.object, context.object, undefined, workspaceConfigurationServiceMock.object, credentialStore.object, capabilitiesService, connectionConfig.object); - let promise = Promise.resolve(); for (let i = 0; i < numCreds; i++) { let cred = Object.assign({}, defaultNamedProfile, { serverName: defaultNamedProfile.serverName + i }); let connectionProfile = new ConnectionProfile(capabilitiesService, cred); - promise = promise.then(() => { - return connectionStore.addActiveConnection(connectionProfile); - }).then(() => { - let current = connectionStore.getRecentlyUsedConnections(); - if (i >= maxRecent) { - assert.equal(current.length, maxRecent, `expect only top ${maxRecent} creds to be saved`); - } else { - assert.equal(current.length, i + 1, `expect all credentials to be saved ${current.length}|${i + 1} `); - } - assert.equal(current[0].serverName, cred.serverName, 'Expect most recently saved item to be first in list'); - assert.ok(!current[0].password); - }); + await connectionStore.addActiveConnection(connectionProfile, true); + + let current = connectionStore.getRecentlyUsedConnections(); + if (i >= maxRecent) { + assert.equal(current.length, maxRecent, `expect only top ${maxRecent} creds to be saved to MRU`); + } else { + assert.equal(current.length, i + 1, `expect all credentials to be saved to MRU ${current.length}|${i + 1} `); + } + assert.equal(current[0].serverName, cred.serverName, 'Expect most recently saved item to be first in list'); + assert.ok(!current[0].password); } - promise.then(() => { - credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.exactly(numCreds)); - let recentConnections = connectionStore.getActiveConnections(); - assert.equal(numCreds, recentConnections.length, `expect number of active connection ${numCreds}|${recentConnections.length} `); - done(); - }, err => { - // Must call done here so test indicates it's finished if errors occur - done(err); - }); + credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.exactly(numCreds)); + let recentConnections = connectionStore.getActiveConnections(); + assert.equal(numCreds, recentConnections.length, `expect number of active connection ${numCreds}|${recentConnections.length} `); + }); + + test('addActiveConnection with addToMru as false should not add any recent connections', async () => { + // setup memento for MRU to return a list we have access to + credentialStore.setup(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns(() => Promise.resolve(true)); + + const numCreds = 3; + let connectionStore = new ConnectionStore(storageServiceMock.object, context.object, undefined, workspaceConfigurationServiceMock.object, + credentialStore.object, capabilitiesService, connectionConfig.object); + connectionStore.clearActiveConnections(); + connectionStore.clearRecentlyUsed(); + + for (let i = 0; i < 3; i++) { + let cred = Object.assign({}, defaultNamedProfile, { serverName: defaultNamedProfile.serverName + i }); + let connectionProfile = new ConnectionProfile(capabilitiesService, cred); + await connectionStore.addActiveConnection(connectionProfile, false); + + let recentConnections = connectionStore.getRecentlyUsedConnections(); + let activeConnections = connectionStore.getActiveConnections(); + assert.equal(recentConnections.length, 0, `expect no entries to be saved to MRU`); + assert.equal(activeConnections.length, i + 1, `expect all credentials to be saved to activeConnections ${activeConnections.length}|${i + 1} `); + } + credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.exactly(numCreds)); }); test('getRecentlyUsedConnections should return connection for given provider', () => { @@ -275,7 +290,7 @@ suite('SQL ConnectionStore tests', () => { assert.equal(connections.every(c => c.providerName === 'Provider2'), true); }); - test('addActiveConnection should add same connection exactly once', (done) => { + test('addActiveConnection should add same connection exactly once', async () => { // setup memento for MRU to return a list we have access to credentialStore.setup(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns(() => Promise.resolve(true)); @@ -286,25 +301,19 @@ suite('SQL ConnectionStore tests', () => { credentialStore.object, capabilitiesService, connectionConfig.object); connectionStore.clearActiveConnections(); connectionStore.clearRecentlyUsed(); - let promise = Promise.resolve(); let cred = Object.assign({}, defaultNamedProfile, { serverName: defaultNamedProfile.serverName + 1 }); let connectionProfile = new ConnectionProfile(capabilitiesService, cred); - promise = promise.then(() => { - return connectionStore.addActiveConnection(defaultNamedConnectionProfile); - }).then(() => { - return connectionStore.addActiveConnection(connectionProfile); - }).then(() => { - return connectionStore.addActiveConnection(connectionProfile); - }).then(() => { - let current = connectionStore.getRecentlyUsedConnections(); - assert.equal(current.length, 2, 'expect 2 unique credentials to have been added'); - assert.equal(current[0].serverName, cred.serverName, 'Expect most recently saved item to be first in list'); - assert.ok(!current[0].password); - }).then(() => done(), err => done(err)); + await connectionStore.addActiveConnection(defaultNamedConnectionProfile, true); + await connectionStore.addActiveConnection(connectionProfile, true); + await connectionStore.addActiveConnection(connectionProfile, true); + + let recentConnections = connectionStore.getRecentlyUsedConnections(); + assert.equal(recentConnections.length, 2, 'expect 2 unique credentials to have been added'); + assert.equal(recentConnections[0].serverName, cred.serverName, 'Expect most recently saved item to be first in list'); + assert.ok(!recentConnections[0].password); }); - test('addActiveConnection should save password to credential store', (done) => { - + test('addActiveConnection should save password to credential store', async () => { // Setup credential store to capture credentials sent to it let capturedCreds: any; credentialStore.setup(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny())) @@ -316,11 +325,11 @@ suite('SQL ConnectionStore tests', () => { }) .returns(() => Promise.resolve(true)); - // Given we save 1 connection with password and multiple other connections without let connectionStore = new ConnectionStore(storageServiceMock.object, context.object, undefined, workspaceConfigurationServiceMock.object, credentialStore.object, capabilitiesService, connectionConfig.object); connectionStore.clearActiveConnections(); connectionStore.clearRecentlyUsed(); + let integratedCred = Object.assign({}, defaultNamedProfile, { serverName: defaultNamedProfile.serverName + 'Integrated', authenticationType: 'Integrated', @@ -334,39 +343,38 @@ suite('SQL ConnectionStore tests', () => { let connectionProfile = new ConnectionProfile(capabilitiesService, defaultNamedProfile); let expectedCredCount = 0; - let promise = Promise.resolve(); - promise = promise.then(() => { - expectedCredCount++; - return connectionStore.addActiveConnection(connectionProfile); - }).then(() => { - let current = connectionStore.getRecentlyUsedConnections(); - // Then verify that since its password based we save the password - credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once()); - assert.strictEqual(capturedCreds.password, defaultNamedProfile.password); - let credId: string = capturedCreds.credentialId; - assert.ok(credId.includes(ConnectionStore.CRED_PROFILE_USER), 'Expect credential to be marked as an Profile cred'); - assert.ok(!current[0].password); - }).then(() => { - // When add integrated auth connection - expectedCredCount++; - let integratedCredConnectionProfile = new ConnectionProfile(capabilitiesService, integratedCred); - return connectionStore.addActiveConnection(integratedCredConnectionProfile); - }).then(() => { - let current = connectionStore.getRecentlyUsedConnections(); - // then expect no to have credential store called, but MRU count upped to 2 - credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once()); - assert.equal(current.length, expectedCredCount, `expect ${expectedCredCount} unique credentials to have been added`); - }).then(() => { - // When add connection without password - expectedCredCount++; - let noPwdCredConnectionProfile = new ConnectionProfile(capabilitiesService, noPwdCred); - return connectionStore.addActiveConnection(noPwdCredConnectionProfile); - }).then(() => { - let current = connectionStore.getRecentlyUsedConnections(); - // then expect no to have credential store called, but MRU count upped to 3 - credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once()); - assert.equal(current.length, expectedCredCount, `expect ${expectedCredCount} unique credentials to have been added`); - }).then(() => done(), err => done(err)); + expectedCredCount++; + // Connection with stored password + await connectionStore.addActiveConnection(connectionProfile, true); + let recentConnections = connectionStore.getActiveConnections(); + + // Then verify that saveCredential was called and correctly stored the password + credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once()); + assert.strictEqual(capturedCreds.password, defaultNamedProfile.password); + let credId: string = capturedCreds.credentialId; + assert.ok(credId.includes(ConnectionStore.CRED_PROFILE_USER), 'Expect credential to be marked as an Profile cred'); + assert.ok(!recentConnections[0].password); + + // Integrated auth + expectedCredCount++; + let integratedCredConnectionProfile = new ConnectionProfile(capabilitiesService, integratedCred); + await connectionStore.addActiveConnection(integratedCredConnectionProfile, true); + + recentConnections = connectionStore.getActiveConnections(); + // We shouldn't see an increase in the calls to saveCredential, but the MRU should be increased + credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once()); + assert.equal(recentConnections.length, expectedCredCount, `expect ${expectedCredCount} unique credentials to have been added`); + + // Connection with blank (no) password + expectedCredCount++; + let noPwdCredConnectionProfile = new ConnectionProfile(capabilitiesService, noPwdCred); + await connectionStore.addActiveConnection(noPwdCredConnectionProfile, true); + + recentConnections = connectionStore.getActiveConnections(); + // We shouldn't see an increase in the calls to saveCredential, but the MRU should be increased + credentialStore.verify(x => x.saveCredential(TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.once()); + assert.equal(recentConnections.length, expectedCredCount, `expect ${expectedCredCount} unique credentials to have been added`); + }); test('can clear connections list', (done) => {