1

Только что глянул видос про PDO и сразу переписал старый код.Насколько он безопасный и грамотный?Что добавить, а что убрать?

<?php
    // Auth
    session_start();

    $host = 'localhost';
    $db_user = 'root';
    $db_password = '';
    $db_name = 'daladno_db';

    $dsn = "mysql:host=$host;dbname=$db_name";

    $connection = new PDO($dsn, $db_user, $db_password);

    if(isset($_POST['do_auth'])) {
        $username = $_POST['username'];
        $password = $_POST['password'];

        if ($username == '') {
            $error = 'Invalid username or password';
        }

        else if($password == '') {
            $error = 'Invalid username or password';
        } 
        // No error
        else {
            $sql = 'SELECT * FROM admins WHERE username = ?';
            $stmt = $connection->prepare($sql);
            $stmt->execute([$username]);

            if($stmt->rowCount() == 0) {
                $error = 'Invalid username or password';

            } else {
                $user = $stmt->fetch(PDO::FETCH_ASSOC);

                if( !password_verify($password, $user['password']) ){
                    $error = 'Invalid username or password';
                } else {
                    // Access!
                    $_SESSION['userLogin'] = $user['username'];
                }
            }
        }
    }
?>
2
  • 2
    В описании RowCount php.net/manual/ru/pdostatement.rowcount.php сказано, что эта функция для запроса select может не вернуть количество строк. После execute делайте сразу fetch и проверяйте что он вернул
    – Mike
    16 фев 2020 в 8:20
  • @Mike эта функция скорее просто бессмысленная - эта цифра попросту никогда не нужна. Поскольку, как вы правильно заметили, если нам надо узнать, вернул ли запрос какие-то данные, мы можем проверить эти самые данные и не делать двойную работу. 25 фев 2020 в 7:58

2 ответа 2

1

Хороший вопрос.

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

Повторений в этом коде слишком много, причем на всех уровнях. представим, что у тебя на сайте не 1 РНР скрипт, а хотя бы 10. И в каждом ты будешь писать код подключения к PDO, серьёзно? А если потом надо будет поменять хост подключения к базе данных? Править во всех файлах?

Нет, конечно. Подключение необходимо вынести в отдельный файл и подключать через require.

Само подключение - это не одна строчка, а много других настроек. Правильный код смотреть здесь

Ещё одна повторяемость на глобальном уровне - это постоянный вызов одних и тех же функций prepare/execute на каждый запрос. Удобнее засунуть их в функцию типа такой

function pdo($pdo, $sql, $args = NULL)
{
    if (!$args)
    {
         return $pdo->query($sql);
    }
    $stmt = $pdo->prepare($sql);
    $stmt->execute($args);
    return $stmt;
}

с её помощью выполнение запросов можно будет писать в 1 строчку:

$user = pdo($pdo, 'SELECT * FROM admins WHERE username = ?', [$username])->fetch();

Ну и наконец, как правильно написал в своём ответе Евгений, нужно убрать многочисленные повторяющиесмя проверки:

<?php
session_start ();
require 'pdo.php';

if (!empty($_POST['username']) && !empty($_POST['password'])) {
    $username = $_POST['username'];
    $password = $_POST['password'];

    $sql = 'SELECT * FROM admins WHERE username = ?';
    $user = pdo($pdo, $sql, [$username])->fetch();
    if($user && password_verify($password, $user['password']) ){
         $_SESSION['userLogin'] = $user['username'];
         header("Location: /admin.php");
         exit;
    }
}
$error = 'Invalid username or password';

Обрати внимание, что после успешного POST запроса всегда должен быть редирект методом GET.

4
  • Для функции pdo следует учесть, что аргументы execute будут забинжены как строки. Это может привести к некоторой путанице (select * from admins limit ?). 25 фев 2020 в 9:04
  • В приведённом запросе (c limit ? ) не приведёт, если правильно подключаться. Вообще, количество случаев, когда нам нужна привязка с указанием типа, стремится к нулю. 25 фев 2020 в 9:08
  • Возможно следует привести пример "правильного" подключения и объяснить, почему именно оно правильное. А то влепит человек функцию, споткнётся на лимите, и вонзит его конкатенацией в запрос, думая что всё безопасно и правильно. 25 фев 2020 в 9:17
  • Так я ж привёл, в ответе :) А пояснение уже слишком далеко будет в сторону от вопроса. Тем более что автор и так биндит строки. 25 фев 2020 в 9:18
1

Ну если Вас устраивает спагетти-код, то можно было бы использовать специальную функцию для проверки !empty ($username). Еще один момент, что переменной $error, Вы во всех случаях присваиваете одну и ту же строку, дублирование кода, плохой тон.

$user = $stmt->fetch (PDO::FETCH_ASSOC) - вернет либо массив, либо булевое значение false, нет смысла вызывать еще один метод чтобы проверить наличие результата.

Хотя бы что-то вроде того.


<?php /* Authorization */
session_start ();

/**
 * Db connection vars
 */
$host        = 'localhost';
$db_user     = 'root';
$db_password = '';
$db_name     = 'daladno_db';
$dsn         = "mysql:host=$host;dbname=$db_name";

/**
 * Default value of error handler
 */
$error = 'Invalid username or password';

/**
 * Create connection
 */
$connection = new PDO ($dsn, $db_user, $db_password);

/**
 * Check data
 */
if (isset ($_POST['do_auth'])) {
    $username = $_POST['username'];
    $password = $_POST['password'];

    if (!empty ($username) and !empty ($password)) {
        $sql = 'SELECT * FROM admins WHERE username = ?';

        $stmt = $connection->prepare ($sql);
        $stmt->execute ([$username]);

        $user = $stmt->fetch (PDO::FETCH_ASSOC);

        if (!is_bool ($user)) {
            if(password_verify ($password, $user['password']) ){
                $_SESSION['userLogin'] = $user['username'];
                $error = 'Access granted.';
            }
        }
    }
}
4
  • 1
    В целом неплохо, только is_bool и empty тут лишние, а в проверке переменных должно быть конечно and а не or. В остальном см. мой ответ. 25 фев 2020 в 8:03
  • @Ипатьев Ну да, с or согласен, чет быстро на скору-руку накидал, а !is_bool, чем не угодил? Вернется либо массив, либо false, каково ваше мнение на данный счет, какую проверку в этом случае писать?
    – Borislav
    25 фев 2020 в 9:51
  • @Ипатьев перечитал ваш ответ, да, согласен, можно две проверки на одну через and переписать, аля $user and password_verify (...), но не прям уж сильно акцентированный момент, никакой фатальности.
    – Borislav
    25 фев 2020 в 9:56
  • ну так false - это уже булево значение, его можно сразу в if() подставлять. А так масло масляное получается. 25 фев 2020 в 9:57

Ваш ответ

Нажимая на кнопку «Отправить ответ», вы соглашаетесь с нашими условиями использования и подтверждаете, что прочитали и поняли наши политику конфиденциальности и нормы поведения.

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.