Что сделано не так?

Question

[Андрей Фомин]

Здравствуйте!
Есть вот такой код на языке C#, который выполняется при нажатии кнопки на форме:

private void Vhod_Click(object sender, EventArgs e)
        {
            try
            {
                SHA1 hash = new SHA1();
                LoginTextbox = LoginField.Text;
                HashTextbox = hash.Hash(PasswordField.Text);
                RoleTextbox = RoleField.Text;
                SQLiteConnection conn = new SQLiteConnection("Data source = accounts.db");
                conn.Open();
                SQLiteCommand cmd = new SQLiteCommand($"SELECT login,password,role FROM users WHERE login = '{LoginTextbox}' AND password = '{HashTextbox}' AND role = '{RoleTextbox}'", conn);
                SQLiteDataReader reader = cmd.ExecuteReader();
                while (reader.Read())
                {
                    if (LoginTextbox.Trim() == reader["login"].ToString() && HashTextbox.Trim() == reader["password"].ToString() && RoleTextbox.Trim() == reader["role"].ToString())
                    {
                        if (RoleTextbox == "Администратор")
                        {
                            this.Hide();
                            var MainForm = new MainForm();
                            MainForm.Closed += (s, args) => this.Close();
                            MainForm.Show();
                        }
                        else if (RoleTextbox == "Менеджер")
                        {
                            this.Hide();
                            var MainForm = new MainForm1();
                            MainForm.Closed += (s, args) => this.Close();
                            MainForm.Show();
                        }
                        else if (RoleTextbox == "Бухгалтер")
                        {
                            this.Hide();
                            var MainForm = new MainForm2();
                            MainForm.Closed += (s, args) => this.Close();
                            MainForm.Show();
                        }
                    }
                    else
                    {
                        MessageBox.Show("Неверный логин или пароль! Пожалуйста,повторите попытку. Также возможно, что вы забыли выбрать роль пользователя.", "Ошибка!");
                        LoginField.Text = "";
                        LoginField.Clear();
                        PasswordField.Text = "";
                        PasswordField.Clear();
                        RoleField.Text = "";
                        RoleField.Clear();
                    }
                }
                reader.Close();
                conn.Close();
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message);
            }
        }

С помощью данного кода осуществляестся авторизация
И вот в чем проблема: для случаев, когда пользователь ввел неправильный логин, пароль или роль был написан вот этот кусок:

else
                    {
                        MessageBox.Show("Неверный логин или пароль! Пожалуйста,повторите попытку. Также возможно, что вы забыли выбрать роль пользователя.", "Ошибка!");
                        LoginField.Text = "";
                        LoginField.Clear();
                        PasswordField.Text = "";
                        PasswordField.Clear();
                        RoleField.Text = "";
                        RoleField.Clear();
                    }

Данный кусок выводит сообщение и очищает поля для ввода данных, и если ввести правильные данные, то авторизация пройдет успешно и откроется другая форма, а если данные введены неправильно, то сообщение о неправильно введенных данных не появляется, и я не могу понять в чем причина этого.
Заранее спасибо

Comments

[Foggy Finder]

написал комментарий чтобы подтолкнуть в правильном направлении, дайте знать если ответ не до конца понятен, будет разбираться дальше

[Андрей Фомин]

Спасибо еще раз за помощь
Хотел только вот уточнить по поводу DataBase и GetUserOrNull:
DataBase мы делаем класс private или internal, и может быть я неправильно задал метод, но правильно ли делать так?

while (reader.Read())
                {
                    var user = DataBase.GetUserOrNull(LoginTextbox, HashTextbox, RoleTextbox);
                    if (user == null)
                {
                    MessageBox.Show("Неверный логин или пароль! Пожалуйста,повторите попытку. Также возможно, что вы забыли выбрать роль пользователя.", "Ошибка!");
                    LoginField.Text = "";
                    LoginField.Clear();
                    PasswordField.Text = "";
                    PasswordField.Clear();
                    RoleField.Text = "";
                    RoleField.Clear();
                }
                else
                {
                    if (RoleTextbox == "Администратор")
                    {
                        this.Hide();
                        var MainForm = new MainForm();
                        MainForm.Closed += (s, args) => this.Close();
                        MainForm.Show();
                    }
                    else if (RoleTextbox == "Менеджер")
                    {
                        this.Hide();
                        var MainForm = new MainForm1();
                        MainForm.Closed += (s, args) => this.Close();
                        MainForm.Show();
                    }
                    else if (RoleTextbox == "Бухгалтер")
                    {
                        this.Hide();
                        var MainForm = new MainForm2();
                        MainForm.Closed += (s, args) => this.Close();
                        MainForm.Show();
                    }
                }
                }
                reader.Close();

private class DataBase
        {
            internal static object GetUserOrNull(string LoginTextbox, string HashTextbox, string RoleTextbox)
            {
                 throw new NotImplementedException();
            }
        }

[Андрей Фомин]

Foggy Finder, ну раз вы готовы объяснить то, что мне непонятно тогда помогите правильно составить метод, как я понял, цикл while лучше всего поместить в метод, но тогда что метод должен возвращать?

[Foggy Finder]

Андрей Фомин, да, готов. Не только сам цикл while но и весь запрос для извлечения пользователя по введенным параметрам - смотрите сигнатуру метода в ответе. Сейчас его расширю

[Foggy Finder]

Андрей Фомин, не заметил ваш первый комментарий - пожалуйста, нажимайте на ник к кому обращаетесь чтобы пришло уведомление. Сейчас уже я подписался на вопрос и буду отслеживать любую активность, но не всегда это бывает удобно.

Answer 1

[Foggy Finder]

Дело в том что вы поместили проверку на то, что пользователя с такими параметрами не существует, внутрь цикла while. Сам запрос в случае ошибок ввода просто не вернет никаких данных и выполнение до этой проверки даже не дойдет.

Чтобы избежать подобных проблем советую вынести считывание из БД в отдельный метод.

Что этот метод может возвращать? Как раз то, что мы хотим проверить - а существует ли такой пользователь у нас в системе и какие у него есть права доступа.
Для удобства добавим отдельный класс для хранения и использования в дальнейшем этой информации. Роль лучше хранить не в виде строк, а в виде перечислений - компактней и работать проще.

public enum Role 
    { 
        Admin, 
        Manager,
        Accountant
    }

    public class User
    {
        public User(string login, Role role)
        {
            Login = login;
            Role = role;
        }

        public string Login { get; }

        public Role Role { get; }
    }

Итак, новый метод будет возвращать экземпляр класса User в случае если параметры введены верно и null если такого пользователя нет.
В вашем коде роль пользователь выбирает сам, что кажется немного странным, на мой взгляд, лучше эту проверку памяти опустить.

public static User GetUserOrNull(string login, string password)
{
    using (var connection = new SqliteConnection(connectionString))
    {
        connection.Open();
        using (var command = connection.CreateCommand())
        {
            var query =
                @"SELECT role FROM users 
                    WHERE login = @login AND password = @password";

            command.CommandText = query;
            command.Parameters.AddWithValue("login", login);
            command.Parameters.AddWithValue("password", password);

            using (var reader = command.ExecuteReader())
            {
                while (reader.Read())
                {
                    // GetOrdinal на тот случай если будем возвращать другие значения
                    var role = reader.GetInt32(reader.GetOrdinal("role"));

                    return new User(login, (Role)role);
                }
            }

            return null;
        }
    }
}

сознательно опускаю обработку ошибок и исключений, это остается уже вам.
Также обратите внимание при составлении SQL запроса значения не вставляются через интерполяцию строк а используются параметры.

Теперь обработка будет намного проще и чище - ведь сам запрос будет находится в другом месте.

private void Vhod_Click(object sender, EventArgs e)
{
    try
    {
        SHA1 hash = new SHA1();
        var login = LoginField.Text;
        var pHash = hash.Hash(PasswordField.Text);
        var user = DataBase.GetUserOrNull(login, pHash);
        if (user != null)
        {
            this.Hide();
            var MainForm = new MainForm();
            MainForm.Closed += (s, args) => this.Close();
            MainForm.Show();
        }
        else
        {
            MessageBox.Show("Неверный логин или пароль! Пожалуйста,повторите попытку.", "Ошибка!");
            LoginField.Text = "";
            LoginField.Clear();
            PasswordField.Text = "";
            PasswordField.Clear();
        }
    }
    catch (Exception ex)
    {
        MessageBox.Show(ex.Message);
    }
}

Comments

[Андрей Фомин]

Foggy Finder,
Вопросы, которые накопились за время анализа кода:

1. здесь нам надо добавить сборку или просто создать класс?

2. DataBase чем у нас будет - классом, параметром при обработке нажатия кнопки или переменной?

[Foggy Finder]

Андрей Фомин,

1. Да, создать класс User как в ответе. Если это уже сделали то посмотрите чтобы был добавлен using на пространство имен

2. статическим классом

[Андрей Фомин]

Foggy Finder,
Вот что получилось в итоге:

static class DataBase
    {
        public static User GetUserOrNull(string LoginTextbox, string HashTextbox, string RoleTextbox)
        {
            using (var connection = new SQLiteConnection("Data source=accounts.db"))
            {
                connection.Open();
                using (var command = connection.CreateCommand())
                {
                    var query =
                        @"SELECT * FROM users 
                    WHERE login = @login AND password = @password AND role = @role";

                    command.CommandText = query;
                    command.Parameters.AddWithValue("login", LoginTextbox);
                    command.Parameters.AddWithValue("password", HashTextbox);
                    command.Parameters.AddWithValue("role", RoleTextbox);

                    using (var reader = command.ExecuteReader())
                    {
                        while (reader.Read())
                        {
                            // GetOrdinal на тот случай если будем возвращать другие значения
                            var role = reader.GetInt32(reader.GetOrdinal("role"));
                        }
                    }
                    return null;
                }
            }
        }
    }

В моем случае роль должен вводить пользователь, поэтому я изменил sql-запрос, но нужен ли в таком случае вот этот кусок:

while (reader.Read())
                        {
                            // GetOrdinal на тот случай если будем возвращать другие значения
                            var role = reader.GetInt32(reader.GetOrdinal("role"));
                        }

Также с классом Users у меня пока путаница с переменными, поэтому я пока оставил его пустым:

public class User
    {
        
    }

[Foggy Finder]

Андрей Фомин, в таком случае вам даже не нужен reader - просто перепишите запрос на проверку существования, например через Count и затем через ExecuteScalar() проверяйте значение - возможно есть и более оптимальные способы то я навскидку их не скажу. Должно быть что-то вроде функции EXISTS

[Foggy Finder]

Также с классом Users у меня пока путаница с переменными, поэтому я пока оставил его пустым:

Андрей Фомин, а в чем именно путаница? если хотите использовать строки вместо перечислений то просто замените тип у свойства Role

[Foggy Finder]

то есть тогда вам даже лучше не пользователя возвращать а bool значение и вызывать IfUserExists(...)

[Андрей Фомин]

Foggy Finder,

public class User
    {
        public User(string login, string password, string role)
        {
            Login = login;
            Role = role;
           Password = password;
        }

        public string Login { get; }

        public string Role { get; }

        public string Password { get; }
    }

Просто ради интереса: а можно ли это было реализовать без изпользования User в методе?

[Андрей Фомин]

Foggy Finder,

как я понял этот кусок просто не нужен, поскольку login, password, role просто нигде не используются

[Foggy Finder]

Андрей Фомин, конечно можно, но так удобнее.

Сейчас - не нужен, но смотрите на следующей странице вам может понадобиться узнать текущего пользователя и дать тот или иной доступ - например, скрыть или заблокировать определенный функционал.

И пароль я специально не стал записывать в класс пользователя - эта информация не должна пробрасывать по всему приложению, ведь нам важно только проверить что параметры введены верно.

[Андрей Фомин]

Foggy Finder, а как бы выглядел более простой вариант без использования User?

[Foggy Finder]

Андрей Фомин, в методе обращения к БД? так я написал выше что метод вернул бы bool в зависимости от того существует ли пользователь