From a30ff33187805642e95199315f50c7092ab53c9e Mon Sep 17 00:00:00 2001 From: Mitchell Sternke Date: Wed, 31 Aug 2016 16:04:04 -0700 Subject: [PATCH] Addressing code review feedback --- .../Connection/ConnectionService.cs | 38 +++--- .../Contracts/ConnectParamsExtensions.cs | 43 ++++-- .../Connection/ConnectionServiceTests.cs | 126 ++++++++++++------ 3 files changed, 142 insertions(+), 65 deletions(-) diff --git a/src/Microsoft.SqlTools.ServiceLayer/Connection/ConnectionService.cs b/src/Microsoft.SqlTools.ServiceLayer/Connection/ConnectionService.cs index 198813e8..2824ee38 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/Connection/ConnectionService.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/Connection/ConnectionService.cs @@ -111,11 +111,19 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection public ConnectResponse Connect(ConnectParams connectionParams) { // Validate parameters - if(connectionParams == null || !connectionParams.IsValid()) + string paramValidationErrorMessage; + if (connectionParams == null) { return new ConnectResponse() { - Messages = "Error: Invalid connection parameters provided." + Messages = "Error: Connection parameters cannot be null." + }; + } + else if (!connectionParams.IsValid(out paramValidationErrorMessage)) + { + return new ConnectResponse() + { + Messages = paramValidationErrorMessage }; } @@ -168,7 +176,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection public bool Disconnect(DisconnectParams disconnectParams) { // Validate parameters - if (disconnectParams == null || String.IsNullOrEmpty(disconnectParams.OwnerUri)) + if (disconnectParams == null || string.IsNullOrEmpty(disconnectParams.OwnerUri)) { return false; } @@ -203,7 +211,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection { // Verify parameters var owner = listDatabasesParams.OwnerUri; - if (String.IsNullOrEmpty(owner)) + if (string.IsNullOrEmpty(owner)) { throw new ArgumentException("OwnerUri cannot be null or empty"); } @@ -355,11 +363,11 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection connectionBuilder["Password"] = connectionDetails.Password; // Check for any optional parameters - if (!String.IsNullOrEmpty(connectionDetails.DatabaseName)) + if (!string.IsNullOrEmpty(connectionDetails.DatabaseName)) { connectionBuilder["Initial Catalog"] = connectionDetails.DatabaseName; } - if (!String.IsNullOrEmpty(connectionDetails.AuthenticationType)) + if (!string.IsNullOrEmpty(connectionDetails.AuthenticationType)) { switch(connectionDetails.AuthenticationType) { @@ -370,7 +378,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection connectionBuilder.IntegratedSecurity = false; break; default: - throw new ArgumentException("Invalid value \"" + connectionDetails.AuthenticationType + "\" for AuthenticationType. Valid values are \"Integrated\" and \"SqlLogin\"."); + throw new ArgumentException(string.Format("Invalid value \"{0}\" for AuthenticationType. Valid values are \"Integrated\" and \"SqlLogin\".", connectionDetails.AuthenticationType)); } } if (connectionDetails.Encrypt.HasValue) @@ -397,15 +405,15 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection { connectionBuilder.ConnectRetryInterval = connectionDetails.ConnectRetryInterval.Value; } - if (!String.IsNullOrEmpty(connectionDetails.ApplicationName)) + if (!string.IsNullOrEmpty(connectionDetails.ApplicationName)) { connectionBuilder.ApplicationName = connectionDetails.ApplicationName; } - if (!String.IsNullOrEmpty(connectionDetails.WorkstationId)) + if (!string.IsNullOrEmpty(connectionDetails.WorkstationId)) { connectionBuilder.WorkstationID = connectionDetails.WorkstationId; } - if (!String.IsNullOrEmpty(connectionDetails.ApplicationIntent)) + if (!string.IsNullOrEmpty(connectionDetails.ApplicationIntent)) { ApplicationIntent intent; switch (connectionDetails.ApplicationIntent) @@ -417,11 +425,11 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection intent = ApplicationIntent.ReadWrite; break; default: - throw new ArgumentException("Invalid value \"" + connectionDetails.ApplicationIntent + "\" for ApplicationIntent. Valid values are \"ReadWrite\" and \"ReadOnly\"."); + throw new ArgumentException(string.Format("Invalid value \"{0}\" for ApplicationIntent. Valid values are \"ReadWrite\" and \"ReadOnly\".", connectionDetails.ApplicationIntent)); } connectionBuilder.ApplicationIntent = intent; } - if (!String.IsNullOrEmpty(connectionDetails.CurrentLanguage)) + if (!string.IsNullOrEmpty(connectionDetails.CurrentLanguage)) { connectionBuilder.CurrentLanguage = connectionDetails.CurrentLanguage; } @@ -445,11 +453,11 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection { connectionBuilder.Replication = connectionDetails.Replication.Value; } - if (!String.IsNullOrEmpty(connectionDetails.AttachDbFilename)) + if (!string.IsNullOrEmpty(connectionDetails.AttachDbFilename)) { connectionBuilder.AttachDBFilename = connectionDetails.AttachDbFilename; } - if (!String.IsNullOrEmpty(connectionDetails.FailoverPartner)) + if (!string.IsNullOrEmpty(connectionDetails.FailoverPartner)) { connectionBuilder.FailoverPartner = connectionDetails.FailoverPartner; } @@ -465,7 +473,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection { connectionBuilder.PacketSize = connectionDetails.PacketSize.Value; } - if (!String.IsNullOrEmpty(connectionDetails.TypeSystemVersion)) + if (!string.IsNullOrEmpty(connectionDetails.TypeSystemVersion)) { connectionBuilder.TypeSystemVersion = connectionDetails.TypeSystemVersion; } diff --git a/src/Microsoft.SqlTools.ServiceLayer/Connection/Contracts/ConnectParamsExtensions.cs b/src/Microsoft.SqlTools.ServiceLayer/Connection/Contracts/ConnectParamsExtensions.cs index d8596447..9f2c7356 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/Connection/Contracts/ConnectParamsExtensions.cs +++ b/src/Microsoft.SqlTools.ServiceLayer/Connection/Contracts/ConnectParamsExtensions.cs @@ -15,15 +15,42 @@ namespace Microsoft.SqlTools.ServiceLayer.Connection.Contracts /// /// Check that the fields in ConnectParams are all valid /// - public static bool IsValid(this ConnectParams parameters) + public static bool IsValid(this ConnectParams parameters, out string errorMessage) { - return !( - String.IsNullOrEmpty(parameters.OwnerUri) || - parameters.Connection == null || - String.IsNullOrEmpty(parameters.Connection.Password) || - String.IsNullOrEmpty(parameters.Connection.ServerName) || - String.IsNullOrEmpty(parameters.Connection.UserName) - ); + errorMessage = string.Empty; + if (string.IsNullOrEmpty(parameters.OwnerUri)) + { + errorMessage = "Error: OwnerUri cannot be null or empty."; + } + else if (parameters.Connection == null) + { + errorMessage = "Error: Connection details object cannot be null."; + } + else if (string.IsNullOrEmpty(parameters.Connection.ServerName)) + { + errorMessage = "Error: ServerName cannot be null or empty."; + } + else if (string.IsNullOrEmpty(parameters.Connection.AuthenticationType) || parameters.Connection.AuthenticationType == "SqlLogin") + { + // For SqlLogin, username/password cannot be empty + if (string.IsNullOrEmpty(parameters.Connection.UserName)) + { + errorMessage = "Error: UserName cannot be null or empty when using SqlLogin authentication."; + } + else if( string.IsNullOrEmpty(parameters.Connection.Password)) + { + errorMessage = "Error: Password cannot be null or empty when using SqlLogin authentication."; + } + } + + if (string.IsNullOrEmpty(errorMessage)) + { + return true; + } + else + { + return false; + } } } } diff --git a/test/Microsoft.SqlTools.ServiceLayer.Test/Connection/ConnectionServiceTests.cs b/test/Microsoft.SqlTools.ServiceLayer.Test/Connection/ConnectionServiceTests.cs index 1c3cc8d9..29367d4c 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.Test/Connection/ConnectionServiceTests.cs +++ b/test/Microsoft.SqlTools.ServiceLayer.Test/Connection/ConnectionServiceTests.cs @@ -152,15 +152,19 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.Connection /// Verify that when connecting with invalid parameters, an error is thrown. /// [Theory] - [InlineDataAttribute(null, "my-server", "test", "sa", "123456")] - [InlineDataAttribute("file://my/sample/file.sql", null, "test", "sa", "123456")] - [InlineDataAttribute("file://my/sample/file.sql", "my-server", "test", null, "123456")] - [InlineDataAttribute("file://my/sample/file.sql", "my-server", "test", "sa", null)] - [InlineDataAttribute("", "my-server", "test", "sa", "123456")] - [InlineDataAttribute("file://my/sample/file.sql", "", "test", "sa", "123456")] - [InlineDataAttribute("file://my/sample/file.sql", "my-server", "test", "", "123456")] - [InlineDataAttribute("file://my/sample/file.sql", "my-server", "test", "sa", "")] - public void ConnectingWithInvalidParametersYieldsErrorMessage(string ownerUri, string server, string database, string userName, string password) + [InlineData("SqlLogin", null, "my-server", "test", "sa", "123456")] + [InlineData("SqlLogin", "file://my/sample/file.sql", null, "test", "sa", "123456")] + [InlineData("SqlLogin", "file://my/sample/file.sql", "my-server", "test", null, "123456")] + [InlineData("SqlLogin", "file://my/sample/file.sql", "my-server", "test", "sa", null)] + [InlineData("SqlLogin", "", "my-server", "test", "sa", "123456")] + [InlineData("SqlLogin", "file://my/sample/file.sql", "", "test", "sa", "123456")] + [InlineData("SqlLogin", "file://my/sample/file.sql", "my-server", "test", "", "123456")] + [InlineData("SqlLogin", "file://my/sample/file.sql", "my-server", "test", "sa", "")] + [InlineData("Integrated", null, "my-server", "test", "sa", "123456")] + [InlineData("Integrated", "file://my/sample/file.sql", null, "test", "sa", "123456")] + [InlineData("Integrated", "", "my-server", "test", "sa", "123456")] + [InlineData("Integrated", "file://my/sample/file.sql", "", "test", "sa", "123456")] + public void ConnectingWithInvalidParametersYieldsErrorMessage(string authType, string ownerUri, string server, string database, string userName, string password) { // Connect with invalid parameters var connectionResult = @@ -172,7 +176,8 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.Connection ServerName = server, DatabaseName = database, UserName = userName, - Password = password + Password = password, + AuthenticationType = authType } }); @@ -181,6 +186,40 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.Connection Assert.NotEqual(String.Empty, connectionResult.Messages); } + /// + /// Verify that when using integrated authentication, the username and/or password can be empty. + /// + [Theory] + [InlineData(null, null)] + [InlineData(null, "")] + [InlineData("", null)] + [InlineData("", "")] + [InlineData("sa", null)] + [InlineData("sa", "")] + [InlineData(null, "12345678")] + [InlineData("", "12345678")] + public void ConnectingWithNoUsernameOrPasswordWorksForIntegratedAuth(string userName, string password) + { + // Connect + var connectionResult = + TestObjects.GetTestConnectionService() + .Connect(new ConnectParams() + { + OwnerUri = "file:///my/test/file.sql", + Connection = new ConnectionDetails() { + ServerName = "my-server", + DatabaseName = "test", + UserName = userName, + Password = password, + AuthenticationType = "Integrated" + } + }); + + // check that the connection was successful + Assert.NotEmpty(connectionResult.ConnectionId); + Assert.Null(connectionResult.Messages); + } + /// /// Verify that when connecting with a null parameters object, an error is thrown. /// @@ -201,38 +240,38 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.Connection /// Verify that optional parameters can be built into a connection string for connecting. /// [Theory] - [InlineDataAttribute("AuthenticationType", "Integrated")] - [InlineDataAttribute("AuthenticationType", "SqlLogin")] - [InlineDataAttribute("Encrypt", true)] - [InlineDataAttribute("Encrypt", false)] - [InlineDataAttribute("TrustServerCertificate", true)] - [InlineDataAttribute("TrustServerCertificate", false)] - [InlineDataAttribute("PersistSecurityInfo", true)] - [InlineDataAttribute("PersistSecurityInfo", false)] - [InlineDataAttribute("ConnectTimeout", 15)] - [InlineDataAttribute("ConnectRetryCount", 1)] - [InlineDataAttribute("ConnectRetryInterval", 10)] - [InlineDataAttribute("ApplicationName", "vscode-mssql")] - [InlineDataAttribute("WorkstationId", "mycomputer")] - [InlineDataAttribute("ApplicationIntent", "ReadWrite")] - [InlineDataAttribute("ApplicationIntent", "ReadOnly")] - [InlineDataAttribute("CurrentLanguage", "test")] - [InlineDataAttribute("Pooling", false)] - [InlineDataAttribute("Pooling", true)] - [InlineDataAttribute("MaxPoolSize", 100)] - [InlineDataAttribute("MinPoolSize", 0)] - [InlineDataAttribute("LoadBalanceTimeout", 0)] - [InlineDataAttribute("Replication", true)] - [InlineDataAttribute("Replication", false)] - [InlineDataAttribute("AttachDbFilename", "myfile")] - [InlineDataAttribute("FailoverPartner", "partner")] - [InlineDataAttribute("MultiSubnetFailover", true)] - [InlineDataAttribute("MultiSubnetFailover", false)] - [InlineDataAttribute("MultipleActiveResultSets", false)] - [InlineDataAttribute("MultipleActiveResultSets", true)] - [InlineDataAttribute("PacketSize", 8192)] - [InlineDataAttribute("TypeSystemVersion", "Latest")] - public void ConnectingWithOptionalParametersBuildsConnectionString(string propertyName, object propertyValue) + [InlineData("AuthenticationType", "Integrated", "Integrated Security")] + [InlineData("AuthenticationType", "SqlLogin", "Integrated Security")] + [InlineData("Encrypt", true, "Encrypt")] + [InlineData("Encrypt", false, "Encrypt")] + [InlineData("TrustServerCertificate", true, "TrustServerCertificate")] + [InlineData("TrustServerCertificate", false, "TrustServerCertificate")] + [InlineData("PersistSecurityInfo", true, "Persist Security Info")] + [InlineData("PersistSecurityInfo", false, "Persist Security Info")] + [InlineData("ConnectTimeout", 15, "Connect Timeout")] + [InlineData("ConnectRetryCount", 1, "ConnectRetryCount")] + [InlineData("ConnectRetryInterval", 10, "ConnectRetryInterval")] + [InlineData("ApplicationName", "vscode-mssql", "Application Name")] + [InlineData("WorkstationId", "mycomputer", "Workstation ID")] + [InlineData("ApplicationIntent", "ReadWrite", "ApplicationIntent")] + [InlineData("ApplicationIntent", "ReadOnly", "ApplicationIntent")] + [InlineData("CurrentLanguage", "test", "Current Language")] + [InlineData("Pooling", false, "Pooling")] + [InlineData("Pooling", true, "Pooling")] + [InlineData("MaxPoolSize", 100, "Max Pool Size")] + [InlineData("MinPoolSize", 0, "Min Pool Size")] + [InlineData("LoadBalanceTimeout", 0, "Load Balance Timeout")] + [InlineData("Replication", true, "Replication")] + [InlineData("Replication", false, "Replication")] + [InlineData("AttachDbFilename", "myfile", "AttachDbFilename")] + [InlineData("FailoverPartner", "partner", "Failover Partner")] + [InlineData("MultiSubnetFailover", true, "MultiSubnetFailover")] + [InlineData("MultiSubnetFailover", false, "MultiSubnetFailover")] + [InlineData("MultipleActiveResultSets", false, "MultipleActiveResultSets")] + [InlineData("MultipleActiveResultSets", true, "MultipleActiveResultSets")] + [InlineData("PacketSize", 8192, "Packet Size")] + [InlineData("TypeSystemVersion", "Latest", "Type System Version")] + public void ConnectingWithOptionalParametersBuildsConnectionString(string propertyName, object propertyValue, string connectionStringMarker) { // Create a test connection details object and set the property to a specific value ConnectionDetails details = TestObjects.GetTestConnectionDetails(); @@ -243,6 +282,9 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.Connection string connectionString = ConnectionService.BuildConnectionString(details); Assert.NotNull(connectionString); Assert.NotEmpty(connectionString); + + // Verify that the parameter is in the connection string + Assert.True(connectionString.Contains(connectionStringMarker)); } ///