A couple of issues I found:
GetConnection method:
var connectionString = ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString;
_connection = new MySqlConnection(connectionString);
_connection.Open();
Here you're creating a new connection object and storing it in _connection. This is not defined anywhere in the code, but seems like a private field. This shouldn't be necessary, as you're simply returning it and leaving it to the caller to handle it. Just use a normal variable and get rid of the field:
MySqlConnection connection = new MySqlConnection(connectionString);
connection.Open();
return connection;
catch (MySqlException ex) {
switch (ex.Number)
{
//0: Cannot connect to server.
case 0:
MessageBox.Show("Cannot connect to server. Contact administrator");
break;
//1045: Invalid user name and/or password.
case 1045:
MessageBox.Show("Invalid username/password, please try again");
break;
}
}
This seems like a bad idea to me. For one, you're using MessageBox.Show to display errors, which ties it to the presentation layer. This breaks Separation of concerns, as the class should only do one thing. Leave it to the caller to display errors the way it likes.
Another issue is that any further detail about the error is lost, like the stack trace, message or any inner exceptions. A second, major issue is that any error with a number other than 0 or 1045 is simply shallowed and the caller has no idea it failed, leaving it with a closed, failed connection. My suggestion is to simply remove the try-catch althogether and make the caller handle errors as it finds appropriate.
AuthenticateUser method:
string query = "SELECT * FROM users WHERE user_name = @userName LIMIT 1";
A small issue with the LIMIT 1 is that it implies that there could be more than one user in the DB with the same username. That can be an indication of an issue on the DB design, lacking an UNIQUE key in the user_name column, it shouldn't be needed. Another problem is the use of SELECT * which brings more data than needed. Specify those columns you actually use and leave the rest out.
Console.Write("connection");
This is odd to put here. For one, other pieces of code refer to MessageBox to display errors and messages, while this uses the console. Why this inconsistency? Either use one display technology or the other, but clearly not both. As before, this creates a poor user experience in seeing random debug messages out of nothing. Most likely, this could be replaced by some kind of log to a file or similar, but remember to add more context to it, so you can extract some useful information out of the file.
cmd.Parameters.AddWithValue("@userName", this.UserName);
It's always nice to see properly parametrized queries. But remember that AddWithValue is a bad practice, declare the exact data type for the parameter and make sure it matches the server side declaration exactly.
while (reader.Read())
There is no need to loop here. As before, the query will return at most only 1 row, or nothing is the user name is wrong, so just replace it by a single call. if (!reader.Read()) return false; will do, and will exit early is no user exist.
password = BCrypt.Net.BCrypt.Verify(this._password + reader["salt"].ToString(), reader["password"].ToString());
Great to see a properly stored password and a nice validation! There is a small thing though. Keep the scope of the password variable as small as possible, possibly with a return at this point to avoid having to read until the end of the method. Also, "password" don't seems like a proper name for this one, something like "validPassword" would better describe what's going on.
this._user_id = int.Parse(reader["id"].ToString());
This is possibly the gravest problem. The method uses class fields all other the place (to input the username and password and to return the user id), which seems odd to me, because that ties the result of this method to the state of the class, which can be used elsewhere (although we don't have that much context here). Why not use method parameters and return values instead? So you can get rid of all private class-level variables. I find a method declaration like this to be much better:
public int? AuthenticateUser(string userName, string password)
The username and password are passed as parameters instead of class fields, and the return value is the id of the logged user or null if nothing is found. This clearly expresses what's going on and what the method expects and return. Also, this allows to simplify the body by doing something like this:
//Check if the password matches, and exit if not
if(!BCrypt.Net.BCrypt.Verify(this._password + reader["salt"].ToString(), reader["password"].ToString())) return null;
//We've got a valid user and password, return its id
return int.Parse(reader["id"].ToString());
catch (MySqlException ex)
{
throw new ArgumentException(ex.Message);
}
Once again the same problem. The exception is caught and a new one is thrown. But here the problem is that it's misleading. For one, the type ArgumentException is wrong, you didn't make a mistake in the arguments, something blew up while contacting the DB. Moreover, you keep only the message, but you discard the stacktrace, any inner exception and DB-specific exception information which can help debugging. This is will likely give you problems later on. My suggestion is to simply remove the try-catch and leave it to the caller to handle the exception and log, display something or retry. At most, throw a specific exception adding some context, but keep all of the original exception as an inner exception.
