3
\$\begingroup\$

I am working on a log in form. Basically, my code searches for a user name, then retrieves the hashed password that is related to that account. It then converts that string hashed password then converts it to its real value. But my issue here is that it is messy.

 private void userVerification() {

            //Instantiate Db
            DataClasses1DataContext myDbContext = new DataClasses1DataContext(dbPath);

            //Search for uservar search

            var hasher = new Hasher();
            //by default salt size is 4
            hasher.SaltSize = 16;

            var encryptedPassword = hasher.Encrypt(txtPassword.Text);


            var searchUser = from user in myDbContext.Accounts
                             where user.accnt_User == txtUser.Text 
                             select user.accnt_Pass;

            //Retrieves the first result.
            var password = searchUser.FirstOrDefault();

            //Check if there's a result or match
            if (searchUser.Count() != 0)
            {
                bool areEqual = hasher.CompareStringToHash(txtPassword.Text, password);

                if (areEqual = true)
                {
                    MessageBox.Show("CORRECT!");
                }
                else {
                    MessageBox.Show("Wrong password!");
                }
            }
            else {
                MessageBox.Show("No Such User in the database");
            }
        }
\$\endgroup\$
  • \$\begingroup\$ A small note is I would always use .Any() when doing Count() != 0 > 0 etc comparisons. I would also probably look at putting the var password inside the if statement closer to it's usage and then do a First or probably better a Single() call on searchUser \$\endgroup\$ – dreza Feb 25 '12 at 4:20
  • \$\begingroup\$ what do you mean .Any() can you provide an example? what is Single() ? \$\endgroup\$ – user962206 Feb 25 '12 at 4:45
  • \$\begingroup\$ .Any() method returns TRUE if the set contains at least one item. So instead of (searchUsers.Count() != 0) you can write (searchUsers.Any()). The result will be same but code is more clear. Also, if you are using ADO.NET EF, Any() method produces more efficient SQL query then Count() \$\endgroup\$ – Artyom Krivokrisenko Nov 15 '12 at 1:41
4
\$\begingroup\$

What I would correct in the code:

  • if anything goes wrong it's best to just display "Login incorrect", instead of specialized messages ('user not found' / 'incorrect passsword'). Giving too many details may lead to information leakage

  • searchUser is not a very good variable name. It in fact represents the result of a search, so I'd name it matchedUser. searchUser would be a good name for a query.

  • the encryptedPassword variable is assigned but never used. You could use it to compare against the password hash in the database

In the end, my code would look like this:

private void userVerification()
{
    var myDbContext = new DataClasses1DataContext(dbPath);

    var hasher = new Hasher { SaltSize = 16 };
    var encryptedPassword = hasher.Encrypt(txtPassword.Text);

    var matchedUser = myDbContext.Accounts
        .Where( user =>
                user.accnt_User == txtUser.Text &&
                user.accnt_Pass == encryptedPassword)
        .FirstOrDefault();

    if(matchedUser != null)
        MessageBox.Show("CORRECT!");
    else
        MessageBox.Show("Invalid user or password.");
}
\$\endgroup\$
  • \$\begingroup\$ why did you use "myDbContext.Accounts .Where( user.accnt_User == txtUser.Text && user.accnt_Pass == encryptedPassword) .FirstOrDefault();" rather than this query? from user in myDbContext.Accounts where user.accnt_User == txtUser.Text select user.accnt_Pass; are there any significant difference? \$\endgroup\$ – user962206 Feb 26 '12 at 2:36
  • \$\begingroup\$ there's also an error on your query, specifically the user.accnt_User \$\endgroup\$ – user962206 Feb 26 '12 at 2:44
  • \$\begingroup\$ @user962206 the significant difference is that in my version of the query the filtering is done in the db for both User Name and Password Hash. If this query yields a result, then the login succeeded. I think it's cleaner to have these checks in one place, if possible. \$\endgroup\$ – Cristian Lupascu Feb 26 '12 at 8:07
  • \$\begingroup\$ @user962206 I don't really see the query error, can you give more details? \$\endgroup\$ – Cristian Lupascu Feb 26 '12 at 8:08
  • \$\begingroup\$ There's a syntax error. 'the name user doesn't exist in the current context' \$\endgroup\$ – user962206 Feb 26 '12 at 9:02
0
\$\begingroup\$

Perhaps something like this. I put in some pre data entry validation but you might not need this dependant on where it is done in the application.

The main thing I would do is remove the MessageBox from this method and do that elsewhere. That way you could at least do some unit testing of it. Perhaps even passing in a database context so that could be mocked as well:

Didn't have access to Visual studio so code may have compile errors sorry.

 private string _errorMessage = string.Empty;

 public string GetErrorMessage() { return _errorMessage; }

 public bool HasError() { return !string.isNullOrEmpty(_errorMessage); }

 private void ClearErrorMessage() { _errormessage = string.Empty; }

 private void CheckValidUserName(string userName) {
    // or any other conditions around a username such as length etch
    if(string.isNullOrWhiteSpace(userName))
       errorMessage  = "User name cannot be empty";
 }

 private void CheckValidPassword(string password) {
    // or any other conditions around a password such as length etc
    if(string.isNullOrWhiteSpace(password))
       errorMessage = "Password cannot be empty"; 
 }

 private string GetUser(string userName) {

        //Instantiate Db
        DataClasses1DataContext myDbContext = new DataClasses1DataContext(dbPath);

        return (from user in myDbContext.Accounts
                where user.accnt_User.Equals(userName)
                select user).SingleOrDefault();
 }

 private bool IsUserValid(string userName, string rawPassword) {

        // can we assume there is only ever one user in the database per accnt_User?
        var user = GetUser(userName);

        if (user == null)
        {
            _errorMessage = "User does not exist";
        }
        else
        {

           var hasher = new Hasher();

           //by default salt size is 4
           hasher.SaltSize = 16;

           string existingHashedPassword = user.accnt_Pass;
           string enteredHashedPassword = hasher.Encrypt(rawPassword);

           if(!hasher.CompareStringToHash(enteredHashedPassword, existingHashedPassword)) {
               _errorMessage = "Passwords do not match";
            }

        }

        return HasError();
    }
\$\endgroup\$
  • \$\begingroup\$ No, continue can't be used that way. You shouldn't want to do it like that anyway even if it was possible, that's just a really bad way to express that. \$\endgroup\$ – Jeff Mercado Feb 25 '12 at 6:01
  • \$\begingroup\$ @JeffMercado Yeah ran out of an idea how how to implement a pre-condition check for those. Thought I was scratching a bit on that \$\endgroup\$ – dreza Feb 25 '12 at 6:41
  • \$\begingroup\$ Apparently I can't there' vast amounts of error. though I did have an idea. question, what type of data types are query results? I wish to store them in a variable \$\endgroup\$ – user962206 Feb 25 '12 at 7:29
  • \$\begingroup\$ Removed the pre-checks as not really part of it I guess. I believe you should be able to find that out by hovering over your variable. But IQueryable or IEnumerable I believe are typically what's returned. \$\endgroup\$ – dreza Feb 25 '12 at 7:33
  • \$\begingroup\$ but why is it that the return type is a string? \$\endgroup\$ – user962206 Feb 25 '12 at 8:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.