Нормально ли так проверять авторизацию?

Question

[karasique]

Сейчас я проверяю через функцию, есть ли куки. Сравниваю их с данными в базе и принимаю решение.
Если в __construct прописать условие проверки $_COOKIE['id'] на пустоту, то можно убрать как минимум одно условие которое растянуто на всю страницу if (){тело страницы}else{header location}.

class worker extends user {
    public $workerData;
    function __construct()
    {
        if (is_empty($_COOKIE['id'])){
            header("Location: {x/auth");
            exit;
        }
       return $this->workerData = user::getDataFromDB('id',$_COOKIE['id'],'*','staff');
    }

Comments

[Arman]

а если я вручную себе добавлю в куки id?

[karasique]

Arik, ну, я условие через всю страницу хочу заменить на что-то типа такого: if ($worker->isLogined() == true) {}.
В isLogined я сверяю все данные, что есть в куках, смотрю логи авторизации. Если чего-то не хватает, что должно быть, то отправляю данные в лог ошибок и перебрасываю на страницу авторизации.

[karasique]

а, черт, я понял, в чем проблема

[Андрей Седышев]

А можно увидеть код, окружающий место, где вы делаете $worker = new worker();?
Кстати, почему даёте классам имена с маленьких букв?

$worker = new Worker(); читается гораздо лучше + соответствует PSR-1

[karasique]

Arik, убрал нафиг из конструктора запрос всех данных, спасибо)

[karasique]

Андрей,

<?
require_once '../config.php';
require_once "{$siteUri}kint.phar";
$pageTemplate = new Template();
$worker = new worker();

    if ($worker->isLogined() == true) {}

        $pageuUrl = pathinfo(__FILE__);
        $route = dirname($_SERVER["PHP_SELF"]).'/'.$pageuUrl['basename'];

        if($_COOKIE['rank'] == 835 or $_COOKIE['rank'] == 717 ){

            ?>

[Андрей Седышев]

karasique, ну в текущем виде у вас любой код после этой строчки
$worker = new worker();
при отсутствии куки даже не выполнится, притом это скрыто в конструкторе. Вообще не очевидно. Я бы очень, очень сильно ругался на такой код :)

[karasique]

Андрей, я пока заменил на

function __construct()
    {
        if (utils::is_empty($_COOKIE['id']) or (utils::is_empty($_COOKIE['hash']))){
            header("Location: {$GLOBALS['siteUri']}auth");
   
        }

[Андрей Седышев]

karasique, дело ваше, конечно, но делать header() в конструкторе класса - это ну прям ТАКОЕ.
Почему бы не оставить конструктор пустым, а уже в методе isLogined, как вы и хотели, проверять?

$worker = new worker();
if (!$worker->isLogined()) { header(); exit; }

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

Answer 1

[Кирилл Несмеянов]

Всё печально. Давай по-порядку:

Одной из ключевых задач ООП является абстракция, что требует декомпозиции кода. Следовательно, первым шагом для причёсывания кода будет:

class Worker extends User // <- Не стоит забывать о PSR
{
    public function __construct(array $data)
    {
        if (empty($data)) {
            header(....);
            exit;
        }

        // В конструкторе нет ретурна, так что это ошибка
    }
}

Шаг 1 Результат: Мы отвязались от кукисов и можем эту же логику переиспользовать вообще с чем угодно.

Теперь мы больше не связаны с окружением (ещё header надо убрать) и имеем код, позволяющий переиспользовать себя в различных ситуациях:
$worker = new Worker($_COOKIE);

Ну и так далее.

При этом есть фатальные косяки (отвечая на твой вопрос "Нормально ли так проверять авторизацию"): Что если я возьму, открою консоль в браузере и заменю кукис id=1 на id=42?

Решаем эту проблему:
$worker = new Worker($_SESSION);

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

Помимо этого - сам процесс входа в систему называется "аутентификацией", а не "авторизацией". Авторизация - это процесс проверки прав доступа к какому-то функционалу.

Результат не идеален и содержит ещё кучку проблем, но с фатальными мы вроде разобрались.

Твоя задача далее, разу уж ты поставил тег "ООП" - смотреть на код, который ты написал и попробовать его перенести в другие условия. Если он там уже работать не будет без изменений его внутренностей - значит в коде есть набор косяков. Пример:
1) Можно ли использовать твой код, получая данные не из БД, а из файлов?
2) Можно ли создать нового "рабочего" не передавая туда ID, чтобы он сгенерировался сам на основе, например, auto increment поля в БД?
3) Можно ли добавить условия в запрос к БД (например, что "рабочий" не забанен)?
4) и т.д.

Comments

[karasique]

Спасибо за обстоятельный и развернутый ответ :)
Буду исправлять!

[karasique]

А зачем может вообще пригодится

2) Можно ли создать нового "рабочего" не передавая туда ID, чтобы он сгенерировался сам на основе, например, auto increment поля в БД?

?

[Кирилл Несмеянов]

А зачем может вообще пригодится

karasique, а как ты создашь нового рабочего?

$worker = new Worker();
$worker->save();

// Ну или так:
$database->save($worker);

Я писал в основном не о том "пригодится или нет вот прямо сейчас", а о том как можно улучшить всё это дело. Если "рабочий" умеет считываться откуда-то, то наверное он должен уметь и сохраняться туда. Логично?

P.S. Можно, сделать, например вот так:

spoiler

class Worker extends User
{
    // ...
    public function __construct(array $data)
    {
        $this->workerData = $data;
    }

    public function rename(string $name)
    {
        $this->workerData['name'] = $name;
    }
}

class WorkersDatabaseRepository
{
    public function findById(int $id)
    {
        // $data = select * from ... where id = $id
        return new Worker($data);
    }

    public function save(Worker $worker) { /** код обновления */ }
}


// Получаем рабочего по ID из БД
$worker = $db->findById(42);
// Переименовываем 
$worker->rename('Vasya');
// и сохраняем туда новые данные
$db->save($worker);

Что получаем:

spoiler

1) У "рабочего" функционал только его, мы можем один раз его написать и забыть
2) У места, откуда читаются данные и куда сохраняются только одна зона ответственности - получить их и сохранить.
3) Мы можем запросто взять и сохранить того же рабочего в совершенно иное место.
4) Или, например, получить из одной базы и сохранить в другую "$a->save($b->findById(42))"
5) Мы не зависим от кукисов и от заголовков, они в другом месте, которое работает с ними. А значит при ошибках или проблемах с ними - не надо весь код перелопачивать.

А вот пример того, на какие куски можно поделить это всё:

spoiler

class SessionAuthentication
{
    public function __construct(DatabaseRepository $db) { ...}
    public function login(Worker $worker) { ... }
    public function current() { ... }
}

$auth = new SessionAuthentication($db);
$auth->current(); // или Worker или null
$uath->login($worker); // Прописываем в сессию ID переданного рабочего

И опять же получаем что:

spoiler

1) Аутентификация у нас живёт совершенно отдельно
2) Если понадобится - мы можем заменить сессии на что угодно другое, да хоть на БД
3) Очень простой интерфейс логина при передаче воркера) и получения текущего пользователя.

Минусы:
1) Завязано на сессии, т.е. не можем иметь более одного залогиненного пользователя (ну, например, админ хочет походить из под другого пользователя и посмотреть как оно всё выглядит)
2) Завязано на Worker, т.е. мы не можем подменить реализацию рабочего на что-то ещё
3) Возможно внутри это вся завязано на id рабочего, а значит каждый объект обязан будет иметь его.

Продолжать, короче, можно много, но в результате имеем:
1) Рабочий - это некое существо, задача которого просто менять данные в массивчике.
2) Логика получения и сохранения данных отдельно, задача этой шняги - получить этот массивчик и сохранить этот массивчик.
3) Аутентификация - тоже отдельный кусок, задача которого управлять состоянием - получать и сохранять куда-то информацию о статусе текущего юзверя.

[karasique]

Кирилл Несмеянов, звучит логично, за пример спасибо)
Я еще не дошел до переделки работы с данными сотрудников

[karasique]

А как лучше сделать проверку прав? Что может делать пользователь, а что нет. Сейчас у меня кривой костыль с функцией, где я проверяю rank, записаный в куки.

[Андрей Седышев]

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

[Кирилл Несмеянов]

karasique,
1) Андрей всё правильно написал. Проблема работоспособности кода именно в кукисах. Это данные на клиенте, которые можно отредактировать. А значит потенциальная ДЫРИИЩЩЩЩА. Я попытался одновременно и об этом написать, и решение предложить, и упростить код, так, чтобы потом было проще всё это допиливать.
2) А про допил костылей: Следуя логике, что объект - это самодостаточная шляпа, которая отвечает за представление какого-то одного куска функционала - не думаю, что доставит проблем реализовать примитивную проверку прав самому ;)

Подумаем... Создаём класс "проверяльщик прав" (Authorization) с методом "хватает возможностей" (can) а дальше можем завязать его на что угодно: "$auth->can($worker, 'READ_MESSAGES');" Ну или даже так: "$auth->can($worker, $message, 'read')"

Со временем потихоньку становится понятно зачем всякие разработчики используют 100500 разных объектов, интерфейсы и прочую шляпу. Сейчас же можно не заострять на этом внимание: Либо делать топорно, костыльно и в лоб, либо посмотреть как реализовано у других (в Symfony, Zend или Laravel), но думаю сейчас это будет избыточным, т.к. опыта просто не хватит понять "зачем оно всё так".

spoiler

Если знаний языка достаточно, чтобы понять что такое объекты, интерфейсы, трейты, циклы, функции и прочее - можно начинать смотреть на всякие фреймворки. Symfony в этом плане будет довольно тяжеловат для старта, а по одной только конфигурацией его можно просто покончить жизнь самоунынием. Yii - отличная рабочая лошадка, но учиться на нём - печально, т.к. пропагандирует огромное количество сомнительных и крайне паршивых решений ради скорости разработки однотипных проектов. Laravel - довольно сбалансированный вариант между низким уровнем старта и возможностями для развития, а костыли там довольно профессионально скрыты от лишних глаз =)))