Klasa logowania usera Opcje

[michalboss]

Witam

Napisałem taką klasę logowania użytkownika w systemie.
Nie jest jeszcze skończona, ale chciałem prosić o ewentualne wskazanie błędów w w/w kodzie, ponieważ dopiero zaczynam OOP w php.

[PHP] pobierz, plaintext 
<?php
 
class Login
{
 
    private $_dbh;
    private $_config;
    private $_lang;
    private $_isLoggin = FALSE;
    private $_idSession;
    private $_email;
    private $_password;
 
    public function __construct($dbh, $config, $lang)
    {
        $this -> _dbh = $dbh;
        $this -> _config = $config;
        $this -> _lang = $lang;
    }
 
    /**
     * Metoda sprawdza czy user jest zalogowany w systemie
     * 
     * @access public
     * @return bool
     */
    public function checkLoggin()
    {
        return $this -> _isLoggin;
    }
 
    /**
     * Metoda sprawdza czy istnieją zmienne sesyjne - jeśli tak, to loguje usera w klasie
     * 
     * @access public
     * @return bool
     */
    protected function isLoggin()
    {
        if($_SESSION['isLoggin'] == TRUE)
        {
            $this -> _isLoggin = $_SESSION['isLoggin'];
            $this -> _idSession = session_id();
            $this -> _email = $_SESSION['email'];
            return TRUE;
        }
        else
        {
            return FALSE;
        }
    }
 
    /**
     * Metoda logowania - sprawdzania czy użytkownik jest zalogowany i czy istnieją zmienne formularza logowania 
     * Jeśli tak, to sprawdza w bazie poprawność e-mail oraz hasła i dokonuje logowania w systemie 
     * 
     * @access public
     * @return void
     */
    public function userLogin()
    {
        $errorMsg = '';
        if(!($this -> _isLoggin) && isset($_POST['logEmail']) && isset($_POST['logPassword']))
        {
            $sql = $this -> _dbh -> prepare('SELECT A.IdUser, A.Email, A.Password, A.Salt, A.FirstName, A.LastName, A.ErrorLogin, A.ErrorLoginTime, A.Current  '
                    . 'FROM `users` A '
                    . 'WHERE A.Email=:email LIMIT 1');
            $sql -> bindValue(':email', $_POST['logEmail'], PDO::PARAM_STR);
            $sql -> execute();
            if($sql -> rowCount() > 0)
            {
                if($row = $sql -> fetch())
                {
                    if(self::encode($_POST['logPassword'], $row['Salt']) == $row['Password'])
                    {
                        $_SESSION['isLoggin'] = TRUE;
                        $_SESSION['email'] = $row['Email'];
                        $_SESSION['IdUser'] = $row['IdUser'];
                        $this -> isLoggin();
                    }
                    else
                    {
                        $errorMsg = $this -> _lang['errorLogin'];
                    }
                }
            }
            else
            {
                $errorMsg = $this -> _lang['errorLogin'];
            }
 
            if(!empty($errorMsg))
            {
                throw new Exception($errorMsg);
            }
        }
        else if(!($this -> _isLoggin) && isset($_SESSION['isLoggin']) && $_SESSION['isLoggin'] == TRUE)
        {
            $this -> isLoggin();
        }
    }
 
    /**
     * Metoda wylogowuje uzytkownika i usuwa sesję
     * 
     * @access public
     * @return void
     */
    public function userLogout()
    {
        unset($_SESSION);
        unset($this -> _isLoggin);
        unset($this -> _idSession);
        unset($this -> _email);
        session_destroy();
        header("Location: index.php?msg=logout");
    }
 
    /**
     * Metoda hashuje hasło używając podanej soli z kartoteki usera
     * 
     * @access public
     * @param string $Password
     * @param string $Salt
     * @return string
     */
    public static function encode($Password, $Salt)
    {
        return hash('sha256', $Salt . hash('sha256', $Password . $Salt));
    }
 
    /**
     * Metoda tworzy sól podanego hasła
     * 
     * @access public
     * @param string $Password
     * @return string
     */
    public static function salt($Password)
    {
        return substr(hash('sha256', microtime() . substr($Password, 0, 3)), 0, 5);
    }
 
    public static function timemicro()
    {
        list($z1, $se) = explode(" ", microtime());
        return ((float) $z1 + (float) $se);
    }
 
}
?>
[PHP] pobierz, plaintext 

[Damonsson]

Żeby było szybciej napiszę co jest dobrze: nic. Nie jestem złośliwy w tym wypadku, uwierz. Może komuś będzie się chciało tłumaczyć, więc nie kasuj wątku.

Wygoogluj sobie: php oop example, php oop login example itp.

[CuteOne]

Szybkie code review:
1.

[PHP] pobierz, plaintext 
$this -> _dbh
[PHP] pobierz, plaintext 

znacznie czytelniejsza wersja

[PHP] pobierz, plaintext 
$this->dbh
[PHP] pobierz, plaintext 

2. Nazwy metod z 4 liter. np "checkLoggin()"
3. $_SESSION['isLoggin'] rzuci notice jeżeli klucz nie istnieje
4. Odpowiedz mi na pytanie, po cholerę w metodzie, która ma przypisać coś do czegoś robisz return bool? Zamiast zwyczajnie przypisać i opuścić metodę

[PHP] pobierz, plaintext 
/**
     * Metoda sprawdza czy istnieją zmienne sesyjne - jeśli tak, to loguje usera w klasie
     * 
     * @access public
     * @return bool
     */
    protected function isLoggin()
    {
        if($_SESSION['isLoggin'] == TRUE)
        {
            $this -> _isLoggin = $_SESSION['isLoggin'];
            $this -> _idSession = session_id();
            $this -> _email = $_SESSION['email'];
            return TRUE;
        }
        else
        {
            return FALSE;
        }
    }
[PHP] pobierz, plaintext 

5. Zbędny else

[PHP] pobierz, plaintext 
if (1 == 1) {
   return true;
}
return false;
[PHP] pobierz, plaintext 

6. Przeczytaj co to SÓL, bo ta metoda nie ma z nią nic wspólnego
7. Po co ci statyczne metody?

Reszty już nie chce mi się sprawdzać - wszystko jest nie tak jak być powinno

@edit a o hashowaniu hashó masz pogadankę w podczepionych

Ten post edytował CuteOne 31.07.2015, 21:53:09

[michalboss]

Dzięki za odp - poprawię ten kod i dam jeszcze raz.
Nie rozumiem tylko punktu 2 oraz 6 - możesz rozwinąć?

[Skie]

2. Tutaj koledze chodziło o to, że nazwy metod jakie dobierasz odbiegają daleko od tego co dana metoda robi - np. isLoggin (lepsza nazwa to byłoby bodajże isLogged) sugeruje, że akcja sprawdza czy użytkownik jest zalogowany - tylko to. Twoja wersja z kolei jeszcze pod spodem robi własne logowanie, o czym nazwa metody nie mówi nic a nic.

6. Poczytaj o soli tutaj Temat: podwojne hashowanie hasel . Generalnie chodzi o to, że sól to pewny tajny string, którego używasz do konkatenacji z wpisanym przez użytkownika hasłem, by bronić się przed tęczowymi tablicami. Ta sól powinna być zahardkodowana gdzieś w kodzie Twojej aplikacji lub pobierana z jakichś plików źródłowych - nie powinna być trzymana w bazie danych koło hasła, bo wtedy skuteczność używania tej metody spada praktycznei do zera.

[Damonsson]

Ta sól powinna być zahardkodowana gdzieś w kodzie Twojej aplikacji lub pobierana z jakichś plików źródłowych - nie powinna być trzymana w bazie danych koło hasła, bo wtedy skuteczność używania tej metody spada praktycznei do zera.

Jeżeli będzie zahardkodowana w kodzie, to będzie taka sama dla każdego usera i jej użyteczność bardzo mocno spadnie, w przypadku jej poznania. Znowu jeżeli będzie w bazie danych to będzie widoczna dla każdego hasła. Dlatego jestem zwolennikiem trzymania soli zarówno zahardkodowanej w kodzie jak i w bazie danych dla każdego hasła innej. Gdybym musiał wybierać nad tylko jednym sposobem trzymania soli, wybieram zdecydowanie opcję w bazie danych.

[michalboss]

To chyba najlepiej podwójne hashowanie sola z bazy + solą ogólną dla systemu?

[Pyton_000]

A nie lepiej użyć jednego kodowania ale mocnego? Dajmy na to BlowFish, sha512 lub whirlpool ? Złamanie takiego hasła jest *nie możliwe (* nie możliwe w rozsądnym czasie)

[Skie]

Dlatego jestem zwolennikiem trzymania soli zarówno zahardkodowanej w kodzie jak i w bazie danych dla każdego hasła innej.

Bez sensu trzymać sól w bazie danych, skoro używa się jej właśnie po to, by włamywacz nie był w stanie odszyfrować hasz haseł za pomocą tęczowych tablic po zdobyciu dostępu / zrzutu bazy danych aplikacji. Jest super mało prawdopodobne, że ktoś taki zdobędzie same hasze z bazy , ale soli już nie, dlatego tak jak pisałem - albo hardkodujemy sól w kodzie aplikacji albo pobieramy z zewnętrznego źródła. Polecam jednak hardkodować, bo jak w jakiś sposób stracisz sole wygenerowane dla pojedynczych użytkowników to będzie przeje....

[borabora]

Bez sensu trzymać sól w bazie danych, skoro używa się jej właśnie po to, by włamywacz nie był w stanie odszyfrować hasz haseł za pomocą tęczowych tablic po zdobyciu dostępu / zrzutu bazy danych aplikacji.

nikt nie powiedział, że kod musi wyglądać tak:

[PHP] pobierz, plaintext 
haszowanie($sol.$haslo)
[PHP] pobierz, plaintext 

sól z bazy możesz zmienić w skrypcie w sposób, w jaki Ci się podoba

Ten post edytował borabora 1.08.2015, 20:42:51

[michalboss]

Trochę zmieniłem kod klasy - może to jest trochę bardziej prawidłowe od poprzedniego?
Jeśli chodzi o metody statyczne to zdefiniowałem je aby także użyć w innych klasach jak user -> rejestracja.

[PHP] pobierz, plaintext 
<?php
 
class Login
{
 
    private $dbh;
    private $config;
    private $lang;
    private $isLogged = FALSE;
    private $idSession;
    private $email;
    private $idUser;
 
    public function __construct($dbh, $config, $lang)
    {
        $this->dbh = $dbh;
        $this->config = $config;
        $this->lang = $lang;
    }
 
    /**
     * Metoda sprawdza czy user jest zalogowany w systemie
     * 
     * @access public
     * @return bool
     */
    public function checkLogged()
    {
        return $this->isLogged;
    }
 
    /**
     * Metoda sprawdza czy uzytkownik jest zalogowany (w klasie) - jeśli tak, to zapisuje dane z klasy do sesji
     * 
     * @access public
     * @return bool
     */
    protected function setSessionData()
    {
        if($this->isLogged == TRUE)
        {
            $_SESSION['isLogged'] = $this->isLogged;
            $_SESSION['idUser'] = $this->idUser;
            $_SESSION['email'] = $this->email;
        }
    }
 
    /**
     * Metoda sprawdza czy uzytkownik jest zalogowany (sesję) - jeśli tak, to zapisuje dane z sesji do klasy
     * 
     * @access public
     * @return bool
     */
    protected function getSessionData()
    {
        if(isset($_SESSION['isLogged']) && $_SESSION['isLogged'] == TRUE)
        {
            $this->isLogged = $_SESSION['isLogged'];
            $this->idUser = $_SESSION['idUser'];
            $this->email = $_SESSION['email'];
            $this->idSession = session_id();
        }
    }
 
    /**
     * Metoda logowania - sprawdza czy użytkownik nie jest już zalogowany i czy istnieją zmienne formularza logowania 
     * Jeśli tak, to sprawdza w bazie poprawność e-mail oraz hasła i dokonuje logowania w systemie
     * Sprawdzana jest także ilość błędnych logowań - max 5 oraz czas od pierwszego błędnego logowania. 
     * 
     * @access public
     * @return void
     */
    public function userLogin()
    {
        $errorMsg = '';
        if(!($this->isLogged) && isset($_POST['logEmail']) && isset($_POST['logPassword']))
        {
            $sql = $this->dbh->prepare('SELECT A.IdUser, A.Email, A.Password, A.Salt, A.FirstName, A.LastName, A.ErrorLogin, A.ErrorLoginTime, A.Current  '
                    . 'FROM `users` A '
                    . 'WHERE A.Email=:email LIMIT 1');
            $sql->bindValue(':email', $_POST['logEmail'], PDO::PARAM_STR);
            $sql->execute();
            if($sql->rowCount() > 0)
            {
                if($row = $sql->fetch())
                {
                    $timeActual = strtotime(date("Y-m-d H:i:s"));
                    $timeLogin = strtotime($row['ErrorLoginTime']);
                    $timeCheck = $timeActual - $timeLogin;
                    $timeCheckMin = round(60 - ($timeCheck / 60), 0);
 
                    if(self::decode($_POST['logPassword'], $row['Password']) == TRUE)
                    {
                        if($row['Current'] == 0)
                        {
                            $errorMsg = $this->lang['errorLoginCurrentError'];
                        }
                        else if($row['ErrorLogin'] >= 5 && ($timeCheck <= 3600))
                        {
                            $errorMsg = $this->lang['errorLoginTime'] . ' ' . $timeCheckMin . ' min.';
                        }
                        else
                        {
                            $this->isLogged = TRUE;
                            $this->email = $row['Email'];
                            $this->idUser = $row['IdUser'];
                            $this->idSession = session_id();
                            $this->setSessionData();
                            if($row['ErrorLogin'] > 0 || $row['ErrorLoginTime'] != NULL)
                            {
                                $this->resetErrorLogin();
                            }
                        }
                    }
                    else
                    {
                        if($row['ErrorLogin'] >= 5 && ($timeCheck <= 3600))
                        {
                            $errorMsg = $this->lang['errorLoginTime'] . ' ' . $timeCheckMin . ' min.';
                        }
                        else
                        {
                            $this->updateErrorLogin($_POST['logEmail']);
                            $errorMsg = $this->lang['errorLogin'];
                        }
                    }
                }
            }
            else
            {
                $this->updateErrorLogin($_POST['logEmail']);
                $errorMsg = $this->lang['errorLogin'];
            }
 
            if(!empty($errorMsg))
            {
                throw new Exception($errorMsg);
            }
        }
        else if(!($this->isLogged) && isset($_SESSION['isLogged']) && $_SESSION['isLogged'] == TRUE)
        {
            $this->getSessionData();
        }
    }
 
    /**
     * Metoda resetuje ilośc błędów logowania oraz czas, 
     * dla użytkownika, w przypadku powodzenia logowania
     * 
     * @access public
     * @return void
     */
    protected function resetErrorLogin()
    {
        $sql2 = $this->dbh->prepare('UPDATE `users` SET ErrorLogin = 0, ErrorLoginTime = NULL '
                . 'WHERE Email=:email');
        $sql2->bindValue(':email', $this->email, PDO::PARAM_STR);
        $sql2->execute();
    }
 
    /**
     * Metoda zwiększa ilośc błędnych logowań oraz zapisuje czas pierwszego błednego logowania
     * 
     * @access public
     * @return void
     */
    protected function updateErrorLogin($logEmail)
    {
        $sql2 = $this->dbh->prepare('UPDATE `users` SET '
                . 'ErrorLogin = ErrorLogin+1, '
                . 'ErrorLoginTime = if(ErrorLoginTime IS NULL, \'' . date("Y-m-d H:i:s") . '\', ErrorLoginTime) '
                . 'WHERE Email=:email');
        $sql2->bindValue(':email', $logEmail, PDO::PARAM_STR);
        $sql2->execute();
    }
 
    /**
     * Metoda wylogowuje uzytkownika, niszczy sesję i usuwa dane
     * 
     * @access public
     * @return void
     */
    public function userLogout()
    {
        unset($_SESSION);
        unset($this->isLogged);
        unset($this->idSession);
        unset($this->email);
        unset($this->idUser);
        session_destroy();
        header("Location: index.php?msg=logout");
    }
 
    /**
     * Metoda hashuje hasło używając podanej soli z kartoteki usera
     * 
     * @access public
     * @param string $Password
     * @param string $Salt
     * @return string
     */
    public static function encode($password, $salt, $rounds = 10000)
    {
        // pass in the password, the number of rounds, and the salt
        // $5$ specifies SHA256-CRYPT, use $6$ if you really want SHA512
        return crypt($password, sprintf('$6$rounds=%d$%s$', $rounds, $salt));
    }
 
    /**
     * Metoda rozszyfrowuje podane hasło
     * 
     * @access public
     * @param string $Password
     * @param string $Salt
     * @return string
     */
    public static function decode($password, $dbPassword)
    {
        // extract the hashing method, number of rounds, and salt from the stored hash
        // and hash the password string accordingly
        $parts = explode('$', $dbPassword);
        $testPassword = crypt($password, sprintf('$%s$%s$%s$', $parts[1], $parts[2], $parts[3]));
        return ($dbPassword === $testPassword) ? TURE : FALSE;
    }
 
    /**
     * Metoda tworzenia 16-znakowej soli
     * 
     * @access public
     * @return string
     */
    public static function salt()
    {
        //return substr(hash('sha256', microtime() . substr($Password, 0, 3)), 0, 5);
        return substr(str_replace('+','.',base64_encode(md5(mt_rand(), true))),0,16);
    }
}
?>
[PHP] pobierz, plaintext 

[CuteOne]

Już trochę lepiej to wygląda ale gdybyś pracował jako programista taki kod nigdy nie wszedłby na produkcję Dobra rada - spróbuj swoich sił z jakimś frameworkiem np. Zend 1/2, Symfony 2, CakePHP. Zobaczysz jak powinno się pisać aplikacje, czym tak naprawdę jest OPP (to nie zbiór metod w klasie X) i jak z niego korzystać

Szybkie CR
1. Nie rób jednej klasy, która robi masę różnych rzeczy - widziałeś kiedyś bolida F1, który kosi trawnik, ora pole i rozwozi mleko? Podziel tą klasę na mniejsze odpowiadające różnym zadaniom
- coś odpowiedzialnego za obsługę sesji
- coś co filtruje/waliduje dane
- klasa autoryzacji (nie myl z "logowaniem" - klasa logowania jedynie korzysta z klas autoryzacji)
- klasa obsługująca akcje autoryzacyjne - logowanie, wylogowanie czy rejestracja
- klasa trzymająca dane użytkownika - login, id itp.
- klasa obsługująca hashowanie
itp.
2. Za dużo if else. Poważnie... przemyśl czy nie da się tego uprościć/rozbić na kilka metod
3. " header("Location: index.php?msg=logout");" to powinno wylecieć, metoda logout ma TYLKO jedno zadanie - usuń sesję użytkownika - tyle, żadnych redirectów czy echów. To samo tyczy się innych metod
4. Do konstruktora, możesz przekazać tablicę jako handler bazy danych (brak "wymuszenia" typu danych)
5. Pomijam ocenę tych czasochłonnych operacji przy generowaniu soli (kwestia gustu) jednak istnieją prostsze metody jej generowania
6. Cóż to za potworek.. ?

[PHP] pobierz, plaintext 
else if(!($this->isLogged) && isset($_SESSION['isLogged']) && $_SESSION['isLogged'] == TRUE)
[PHP] pobierz, plaintext 

7. I kolejny

[PHP] pobierz, plaintext 
(...)$errorMsg = $this->lang['errorLoginTime'] . ' ' . $timeCheckMin . ' min.';(...)
(...)$errorMsg = $this->lang['errorLogin'];(...)
//itp.
// a później..
if(!empty($errorMsg))
            {
                throw new Exception($errorMsg);
            }
[PHP] pobierz, plaintext 

@edit w metodzie 'userLogin()' robisz

[PHP] pobierz, plaintext 
$this->isLogged = TRUE;
$this->email = $row['Email'];
$this->idUser = $row['IdUser'];
$this->idSession = session_id();
$this->setSessionData();
[PHP] pobierz, plaintext 

po czym w setSessionData() robisz ponownie to samo...

Ten post edytował CuteOne 1.08.2015, 22:58:24

[michalboss]

Z tego co piszesz to rozumiem że lepiej zrobić zamiast:

[PHP] pobierz, plaintext 
if($row['Current'] == 0)
                        {
                            $errorMsg = $this->lang['errorLoginCurrentError'];
                        }
                        else if($row['ErrorLogin'] >= 5 && ($timeCheck <= 3600))
                        {
                            $errorMsg = $this->lang['errorLoginTime'] . ' ' . $timeCheckMin . ' min.';
                        }
[PHP] pobierz, plaintext 

to:

[PHP] pobierz, plaintext 
if (!$this->checkCurrent())
                        {
                            throw new Exception($this->lang['errorLoginCurrentError']);
                        }
 
                        if (!$this->checkErrorLoggin())
                        {
                            throw new Exception($this->lang['errorLoginTime'] . ' ' . $timeCheckMin . ' min');
                        }
[PHP] pobierz, plaintext 

Czy taki zapis będzie lepszy od else if ?
Wtedy też likwiduje to ten dziwny zapis:

[PHP] pobierz, plaintext 
if(!empty($errorMsg))
            {
                throw new Exception($errorMsg);
            }
[PHP] pobierz, plaintext 

Ten pierwszy potworek:

[PHP] pobierz, plaintext 
else if(!($this->isLogged) && isset($_SESSION['isLogged']) && $_SESSION['isLogged'] == TRUE)
[PHP] pobierz, plaintext 

sprawdza czy user jest zalogowany (w klasie) i jeśli nie jest, a istnieją dane sesyjne to ma przypisać ich wartości do pól klasy.
Czy po prostu to powinien zrobić konstruktor - np sprawdzić przy tworzeniu obiektu czy są zmienne sesyjne i je przypisać?

Ten post edytował michalboss 3.08.2015, 11:22:19

[marcio]

Jesli chcesz spojrz na moja klase do logowania nie jest to na pewno jakis super kod ale moze ci pomoc, tak jak widzisz klasa uzywa wiele innych bibliotek i jest podzielona na wiecej "czesci"
https://github.com/marcio199226/Volta-frame...ollers/auth.php

[Pyton_000]

I jest równie fatalnie napisana co autora wątku.

[marcio]

I jest równie fatalnie napisana co autora wątku.

No skoro tak twierdzisz to pokaz twoja

[Pyton_000]

A kto powiedział że takową mam. Ja nie wymyślam koła na nowo

[audiodesign10]

Żeby było szybciej napiszę co jest dobrze: nic. Nie jestem złośliwy w tym wypadku, uwierz. Może komuś będzie się chciało tłumaczyć, więc nie kasuj wątku.

Wygoogluj sobie: php oop example, php oop login example itp.

Witam,
więc prosiłbym abyś pokazał dobry przykład jak to się dokładnie robi, aby początkujący programiści nie musieli na przyszłość błądzić.

Pozdrawiam

[marcio]

A kto powiedział że takową mam. Ja nie wymyślam koła na nowo

Ja tez nie, uzywam rozne fw ale to mi nie przeszkadza w napisaniu czegos mojego w wolnym czasie skoro jestes taki dobry tylko w uzywaniu np symfony2 to po co sie odzywasz widze ze ostatnio na tym forum cos sie ludzia w glowkach popitolilo zejdzie na ziemie ludzie.

[!*!]

... widze ze ostatnio na tym forum cos sie ludzia w glowkach popitolilo zejdzie na ziemie ludzie ...

Niektórzy tak lubią, z lenistwem i stagnacją nie wygrasz. A co do dobrej klasy logowania, to praktycznie każdy FW ma takową w dokumentacji.