How to write a stable WCF service

Question

I'm writing a WCF service for validating password and getting company ID for the clients that requests the services. The service runs on a server locally as a windows service, which sends queries to the database and returns the appropriate info back to the client.

I'd first like some comments on how the overall design looks like as I'm going to be adding more operations but wanted to avoid any situations where I have to rewrite them all just in case if I'm headed the wrong direction.

I have a specific question regarding the stability of this service. In case if the operation somehow fails on the server's end, such as an exception being thrown because the server couldn't connect to a database.

• Where should I handle the exception? Should I wrap my methods in try catch statements on the service methods or where the call is being made (client)?
• What can I do to increase the stability of this service? Under any circumstances, I cannot allow the service to shut down aside from the user intentionally doing so.

These are probably trivial questions but I'd like some guidance on this matter. Thank you.

namespace TopwareOutlookSyncService
{
    [ServiceContract]
    public interface ITopwareOutlookSyncService
    {
        [OperationContract]
        bool IsPasswordValid(string password);

        [OperationContract]
        string GetCompanyID(string userID);
    }
    public class TopwareOutlookSyncService : ITopwareOutlookSyncService
    {
        private SqlConnection con;
        public void ConnectDB()
        {
            if (!IsDBConnected())
            {
                con = new SqlConnection("user id=nexol;" +
                                           "password=;server=666.666.666.666;" +
                                           "database=hell; " +
                                           "connection timeout=15");
                con.Open();
            }
        }

        public bool IsDBConnected()
        {
            return (con != null && con.State == ConnectionState.Open);
        }

        public bool IsPasswordValid(string userID, string password)
        {
            if (!IsDBConnected())
                ConnectDB();
            bool isPasswordValid = false;

            string sql = "SELECT USER_PW FROM HELL.GRP_USER WHERE USER_ID = '" + userID + "'";
            string encryptedPassword = PasswordTable.GetEncryptedString(password);
            SqlCommand command = new SqlCommand(sql, con);
            SqlDataReader reader = command.ExecuteReader();

            while (reader.Read())
            {
                if (reader["USER_PW"].ToString() == encryptedPassword)
                {
                    isPasswordValid = true;
                    break;
                }
            }

            reader.Close();
            con.Close();
            return isPasswordValid;
        }

        public string GetCompanyID(string userID)
        {
            if (!IsDBConnected())
                ConnectDB();
            string companyID = String.Empty;
            string sql = "SELECT COM_CD FROM HELL.GRP_USER WHERE USER_ID = '" + userID + "'";
            SqlCommand command = new SqlCommand(sql, con);
            SqlDataReader reader = command.ExecuteReader();

            while (reader.Read())
            {
                companyID = reader["COM_CD"].ToString();
            }

            reader.Close();
            con.Close();
            return companyID;
        }
    }
}

Answer 1

You should never generate dynamic SQL by concatenating incoming values into an SQL string that is executed later. This type of code leads to SQL injection vulnerability, and by using your service as it is now it's very easy to get access to any user account without knowing its password.

If you plan to develop your service further take your time to learn one of the ORM frameworks (Entity Framework, NHibernate, or maybe one of the NoSQLs like MongoDB or RavenDB) and get rid of low-level DB management code from your service. Alternatively (if you prefer inventing a wheel :)) move DB-related code to separated class and name it "DB repository" so that your service doesn't have too many responsibilities (currently it has business logic responsibilities combined with DB access management).

About exception handling - here is a good article that describes what you should and should not do. Basically you should only catch specific exceptions, and only if you know how to recover from that exception in a meaningful way, that doesn't leave your application in an inconsistent state. In case when WCF service has no way to return a meaningful response (e.g. when response is based on DB data and database is not available) you should pass exception to the client.

And finally, store your connection strings in the configuration files rather than hardcoded.