Правильно ли сделана авторизация через сессии?

Question

[4sadly]

if (isset($_COOKIE['sid'])) {
    session_id($_COOKIE['sid']);
    session_start();
    if (isset($_SESSION['ip']) and isset($_SESSION['browser'])) {
        if ($_SESSION['ip'] !== $_SERVER['REMOTE_ADDR'] or $_SESSION['browser'] !== $_SERVER['HTTP_USER_AGENT']) {
            session_abort();
        }
    }
}
if (isset($_SESSION['login'])) {
    echo $_SESSION['login'];
} else {
    if (isset($_POST['submit'])) {
        $errors = array();
        $stmt = $conn->prepare('SELECT id,password FROM users WHERE login = ?');
        $stmt->execute(array($_POST['login']));
        $user = $stmt->fetch();
        if ($user) {
            if ($user['password'] == $_POST['password']) {
                session_id($user['id']);
                session_start();
                $_SESSION['ip'] == $_SERVER['REMOTE_ADDR'];
                $_SESSION['browser'] = $_SERVER['HTTP_USER_AGENT'];
                $_SESSION['id'] = $user['id'];
                $_SESSION['login'] = $_POST['login'];
            }
        } else {
            $errors[] = "Пользователь не найден";
        }
    }
}

Все хорошо или это может сломаться?
Шифрование паролей будет добавлено, про это не говорите, пока тестовый вариант)
Будет ли нормальным вариант проверять по hash, а не по ip+user agent пользователя?

Answer 1

[ThunderCat]

session_start не требует проверки на существование чего-либо. Его просто надо вызывать в начале и все.

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

в сессии лучше всего хранить отдельно флаг логина $_SESSION['is_logged']; и отдельно массив $_SESSION['user']; где часто используемые данные юзера. Или внутри юзера: $_SESSION['user']['is_logged'];

Comments

[4sadly]

А как тогда лучше проверять тот человек или нет? и как я положу в массив $_SESSION['user'] пользователя, если при его получении там есть пароль, или мне лучше создать новый массив без пароля и засунуть туда?
И вообще как я получу ту же самую сессию? мне нужно, чтобы это работало после закрытия и открытия браузера
Может тогда создать таблицу сессий где будет уникальный ключ и id пользователя, чтобы не было того, о чем вы говорите

[ThunderCat]

4sadly,

и как я положу в массив $_SESSION['user'] пользователя, если при его получении там есть пароль, или мне лучше создать новый массив без пароля и засунуть туда?

$stmt = $conn->prepare('SELECT id,password FROM users WHERE login = ?');

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

А как тогда лучше проверять тот человек или нет?

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

[ThunderCat]

4sadly,

if ($user['password'] == $_POST['password']) {
                session_id($user['id']);
                session_start(); // убрать, только в начале нужно

[4sadly]

ThunderCat, для этого это и нужно, чтобы в следующий раз не вводить пароль

[ThunderCat]

для этого это и нужно, чтобы в следующий раз не вводить пароль

что нужно???

[4sadly]

ThunderCat,

if (isset($_COOKIE['sid'])) {
    session_id($_COOKIE['sid']);
    session_start();
    if (isset($_SESSION['ip']) and isset($_SESSION['browser'])) {
        if ($_SESSION['ip'] !== $_SERVER['REMOTE_ADDR'] or $_SESSION['browser'] !== $_SERVER['HTTP_USER_AGENT']) {
            session_abort();
        }
    }
}

если есть куки, он найдет сессию с таким айди и если у сессии совпадают ip и user agent, то он пускает, если нет - то нет
если не пустило, то вводим логин и пароль
или лучше просто хранить hash и через session_name получать эту сессию?

[ThunderCat]

4sadly, как то у вас странно вообще понимание авторизации в голове легло.
1) ВСЕГДА стартуем сессию в начале.
2) Сессию проверяем на наличие флага логина. Все, больше ничего для этого не нужно, никаких хешей, агентов и айпи. if($_SESSION['user']['is_logged'] == true){...}
3) Куки трогать НЕ НАДО, у вас есть нормальный интерфейс сессии.
4) Если в сессии нет флага логина - выводим форму логина, если есть - из сессии же из юзера берем имя и выводим.
5) При логине, при совпадении логина и пароля в сессию пишем флаг авторизации и нужные данные пользователя.
Конец.

[4sadly]

ThunderCat, так сессия же закончится при закрытии браузера

[ThunderCat]

4sadly, это во первых зависит от настроек и вовсе не обязательно. А во вторых к авторизации никак не относится. Тем более к айпи и браузеру. И реализуется по другому.

[4sadly]

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

[ThunderCat]

4sadly, сессионные куки убиваются если вы закрыли браузер. Обычные куки имеют время жизни, которое вы указываете. Для автологина достаточно завести куку у клиента с определенным ключом, сгенерированным рандомно, например соленый хеш от текущего времени. Этот же ключ заносится в бд пользователя, как ключ авторизации по куке. При заходе пользователя на сайт достаточно проверить что он 1) Не залогинен 2) Имеет куку с ключом. Проверяем правильный ли ключ и логиним в случае если верный.

[4sadly]

ThunderCat, а почему нельзя задать сессии name с этим ключом, а потом просто получить сессию по ключу?

session_name($_COOKIE['sid'];
session_start();

[ThunderCat]

4sadly,

а почему нельзя задать сессии name с этим ключом, а потом просто получить сессию по ключу?

во первых сессионные файлы имеют короткий ливтайм, и это правильно. Во вторых в них могут храниться не валидные данные, например устаревшие из-за изменения с другой машины, в третьих вы через неделю их не найдете, по причине "во первых". Если вы выходили из сервиса, правильнее снова авторизироваться (автоматически или руками - нет разницы). И при закрытии браузера у вас не будет сессионной куки и ваш код выдаст варнинг, то есть вы чего то там придумываете странное и совершенно с кривой логикой, хотя все укладывается в 5 строк кода.

[4sadly]

ThunderCat, об этом решении мне рассказал @DevMan
Как я понял лучшим решением является доп. таблица, в которой будут id пользователя, hash, и дата создания этого ключа доступа, чтобы их потом удалять

[ThunderCat]

4sadly,

об этом решении мне рассказал @DevMan

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

[4sadly]

ThunderCat, а по поводу того, что я сказал выше, как думаешь?

[ThunderCat]

4sadly,

Как я понял лучшим решением является доп. таблица, в которой будут id пользователя, hash, и дата создания этого ключа доступа, чтобы их потом удалять

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

[ThunderCat]

4sadly,

а по поводу того, что я сказал выше, как думаешь?

там много чего, что конкретно?

[4sadly]

ThunderCat, именно того, на что ты ответил, а если куку украсть? может юзер агент хранить?

[ThunderCat]

4sadly, вау, если угонят куку то что мешает юзер агент подделать? На самом деле угон куки - редкое явление, в крайнем случае, если прям такая паранойя - токен еще можно прикрутить... ну и озаботиться больше защитой от xss и прочих скрипт инжекшнов. И то пользы больше будет.

[4sadly]

ThunderCat, просто если даже один человек в компании отдаст куку, это уже будет очень плохо, а куку украсть не так уж и сложно

[ThunderCat]

4sadly, хм, прикольно, прям не так уж сложно? Куда только разработчики браузеров смотрят!..
На самом деле такие уязвимости легко тестируются, есть даже онлайн сервисы, ну и соответственно легко закрываются в случае наличия. Не так все просто. Хотя вообще где вы нашли это, что куку легко угнать?

[ThunderCat]

session_start(); вынесите наверх, за все проверки, его по хорошему вообще в начало индексного файла выносят и больше не вспоминают.
в вашем случае isset() лучше заменить на !empty()

$stmt = $conn->prepare('SELECT user_id FROM access WHERE hash = ?');
    $stmt->execute(array($_COOKIE['sid']));
    $user_id = $stmt->fetchColumn();
    $stmt = $conn->prepare('SELECT * FROM users WHERE id = ?');

здесь зачем 2 запроса? У вас в 1 можно все выбрать, не делайте лишних запросов.

if (isset($_POST['submit'])) {тоже не совсем верная проверка, для этого есть $_SERVER['REQUEST_METHOD'] == 'POST', и дальше у вас без проверок используются всякие post переменные... Их вообще то надо как-то проверить...

if ($user) {старайтесь так не делать, проверяйте хотя бы !empty().

[4sadly]

ThunderCat, пасиб, а как там в 1 запрос все поместить?
сделал класс, в котором есть функции для облегчения работы с запросами
Пока получилось так:

<?php
require 'config/config.php';
$errors = array();
if (!empty($_COOKIE['sid']) and empty($_SESSION['logged_user'])) {
    $user_id = $conn->findColumn('SELECT user_id FROM access WHERE hash = ?', array($_COOKIE['sid']));
    if (!empty($user_id)) {
        $user = $conn->findOne('users', 'id = ?', array($user_id));
        if (!empty($user)) {
            setcookie('sid', $_COOKIE['sid'], time() + 60 * 60 * 24 * 365);
            $_SESSION['logged_user'] = array(
                'id' => $user->id,
                'login' => $user->login,
                'type' => $user->type,
                'hash' => $_COOKIE['sid']
            );
        } else {
            setcookie('sid', null, -1);
        }
    }
}
if (empty($_SESSION['logged_user'])) {
    if ($_SERVER['REQUEST_METHOD'] == 'POST') {
        $user = $conn->findOne('users', 'login = ?', array($_POST['login']));
        if (!empty($user)) {
            if ($user->password == $_POST['password']) {
                $hash = bin2hex(random_bytes(20));
                $conn->cudOne('INSERT INTO access (user_id,hash) VALUES (?, ?)', array($user->id, $hash));
                setcookie('sid', $hash, time() + 60 * 60 * 24 * 365);
                $_SESSION['logged_user'] = array(
                    'id' => $user->id,
                    'login' => $user->login,
                    'type' => $user->type,
                    'hash' => $hash
                );
            }
        } else {
            $errors[] = "Пользователь не найден";
        }
    }
}