Obiektowa rejestracja - Code Review

Question

Witam.

Dopiero zaczynam z oop dlatego prosiłbym o ocenę kodu. Czy zawarłem tutaj podstawowe funkcje umożliwiające bezpieczną i szybką rejestrację? Oraz może jakieś wskazówki jak napisać to krócej?

Dodam, że kod działa, hasła są hashowane, a do łączenia z bazą wykorzystana jest metoda PDO wraz z użyciem prepared statements.

Niektóre funkcje użyte w kodzie pochodzą z mojej innej klasy, o ich działanie się nie martwię i sam jeszcze nad nimi troszkę posiedzę.

Kod:

<?php
    class Register {
        private $login;
        private $password;
        private $password2;
        private $email;
        private $reCaptcha_response;
        private $reCaptcha_secret_key = 'sercret_key;

        public $error;

        public function register_process($post_login, $post_password, $post_password2, $post_email, $post_reCaptcha_response) {
            $this->login = $post_login;
            $this->password = $post_password;
            $this->password2 = $post_password2;
            $this->email = filter_var($post_email, FILTER_VALIDATE_EMAIL);
            $this->reCaptcha_response = $post_reCaptcha_response;

            // Check login length
            if (strlen($this->login) < 5 || strlen($this->login) > 16) {
                $this->error = 'Login musi zawierać od 5 do 16 znaków';
                return $this->error;
            }

            // Check login's chars
            if (!ctype_alnum($this->login)) {
                $this->error = 'Login może zawierać tylko litery i cyfry (bez polskich znaków)';
                return $this->error;
            }

            // Check password length
            if ((strlen($this->password) < 6) || (strlen($this->password) > 20)) {
                $this->error = 'Hasło musi zawierać od 6 do 20 znaków';
                return $this->error;
            }

            // Check passwords disparateness 
            if ($this->password !== $this->password2) {
                $this->error = 'Podane hasła nie są identyczne';
                return $this->error;
            }

            // Check reCaptcha
            $check_reCaptcha = file_get_contents('https://www.google.com/recaptcha/api/siteverify?secret='. $this->reCaptcha_secret_key .'&response='. $this->reCaptcha_response);
            $reCaptcha_answer = json_decode($check_reCaptcha);

            if (!$reCaptcha_answer->success) {
                $this->error = 'Potwierdź, że nie jesteś botem, a może jednak nim jesteś? :o';
                return $this->error;
            }

            // Check login in database
            $base_obj = new Base_queries;

            if ($base_obj->user_check_login($this->login) > 0) {
                $this->error = 'Ten login jest już w użyciu';
                return $this->error;
            }

            // Check email in database
            if ($base_obj->user_check_email($this->email) > 0) {
                $this->error = 'Ten email jest już w użyciu';
                return $this->error;
            }

            // Register new user
            $base_obj->user_register($this->login, $this->password, $this->email);
            $this->error = 'Konto zostało zarejestrowane!';
            return $this->error;
        }
    }

 

Czy po każdym return powinienem używać exit(), aby przeglądarka nie musiała wczytywać reszty kodu?

Czy proces rejestracji powinien być rozbity na więcej metod?

 

 

Pozdrawiam!

Comments

Czemu ustawiasz jakiekolwiek limity na hasło?

---

@Bakkit, Ten login jest już w użyciu';

Czegoś takiego w ogóle nie powinno się wyświetlać dla użytkownika. Luka przed atakami słownikowymi.

---

W jaki sposób można takie coś wykorzystać do włamania się na konto?

Answer 1

Mało obietkówki w ten obietkówce :P
Już mówie co jest źle:

1. Liczba parametrów we funkcji `register_process`. Rozwiązanie to dodać jakiś obiekt Dto zamiast tylu parametrów.

2. Walidacja danych powinna odbywać się w zewnętrzym serwisie. Tam powinno być wszystko walidowane a później zwracany wynik czy przeszła walidacja czy nie i ew. zrówcić błęde wartości.

3. Base_queries -> za bardzo generyczna nazwa wg. powinno być klasa np.: UserRepository i ona powinna wyciągać dane i zapisywać już gotowe wyniki.

4. `reCaptcha_secret_key` nie powinno być zahardcodowane, powinna być ta zmienna wczytywana z configa, nawet jeżeli chodzi o bezpieczeństwo twójego klucza prywatnego. Najlepiej dodać go do jakiegoś .env to możesz wtedy gitem ignorować ten plik i nie wystawiać go na świat.

5. Brak typów zmiennych oraz zdefiniowanej wartości która jest zwracana przez funkcje `register_process`.
Pomijam fakt że nazwy zmiennych są bez jakichkolwiek standardów i tak samo dotyczy się nazwy klasy.

Klasa powinna być rzeczownikiem a tutaj jest nazwa klasy jest czasownikiem. Nazwy metod powinny być czasownikami.

Comments

1. Liczba parametrów we funkcji `register_process`. Rozwiązanie to dodać jakiś obiekt Dto zamiast tylu parametrów.

Jesteś pewien, że DTO, zamiast na przykład VO? ;) 

Klasa powinna być rzeczownikiem a tutaj jest nazwa klasy jest czasownikiem. Nazwy metod powinny być czasownikami.

Rozumiem, że kolega reprezentuje standardy wiedzy z lat '90?

---

Moze być nawet VO/Model/DTO cokolwiek w tym przypadku będzie lepsze niż 6 parametrów które trzeba wprowadzić do metody i jeszcze które nie są otypowane.

Jesteś pewien, że DTO, zamiast na przykład VO? ;)

co do definicji DTO: "Data transfer object (DTO), formerly known as value objects or VO, is a design pattern used to transfer data between software application subsystems."

Rozumiem, że kolega reprezentuje standardy wiedzy z lat '90?

Pewne standardy się nie starzeją w tym przypadku, nawet wujek Bob o tym pisze.
A wg. Ciebie @event15 jak wyglądają dzisiejsze standardy nazywania klas i metod?

 

 

---

DTO jest czymś innym niż Value object.

Dziś częstym podejściem jest opracowanie sensownego modelu zagadnienia a szukanie rzeczowników do opisu klas nie zawsze ma sens. Częściej można spotkać się - o ile nie pomylę pojęcia z gramatyki - z równoważnikami zdań. 

Bo rzeczownikiem na przykład w Event Sourcingu nie są:

• UserPasswordChanged
• TenantProvisioned
• ContactInformation

---

I tutaj brakuje słowa Event po kazdym z ww. przykładów i wszystko się zgadza.
Wtedy jasno nazwa opisuje, że to jest jakiś event do obsłużenia, za to nazwa UserPasswordChanged nic za bardzo nie mówi, tylko że nazwa użytkownika lub hasło zostało zmienione lecz nie mówi o wydarzeniu do obsłużenia

---

Nie brakuje - to przykłady z książki Vaughna Vernona. Sam namespace może Ci sugerować, że jest to event. Poza tym w taki sposób zwykle tworzy się nazwy klas w przypadku event sourcingu - nie trzeba pisać UserPasswordChangedEvent, TenantProvisionedEvent. Tak samo nie robi się w CQRS wszędzie CreateUserCommand ChangePasswordCommand czy ShowInformationQuery. No, ale to zależy już częściej - wydaje mi się - od standardów firmy. Myślę, że nazwa klasy bez Event jasno mówi jakie ma przeznaczenie i działanie.

UserPasswordChanged mówi bardzo wiele i może być obsłużone w Audit Logu do wygenerowania projekcji statystyk zmian haseł.

---

Mógłbym jakiś link do informacji o tym dto? O co to jest, z czym to się je i jak tego używać.

---

Dto czy jak kolega @event mnie poprawił Value Object, to po prostu obiekt który ma na celu przenieść dane z jednej warstwy aplikacji do drugiej. Już Ci daje jakiś przykład, przerobiłem kawałek Twojego kodu na korzystającego VO (Value Object). Poza tym dam jeszcze mała wskazówkę, jeżeli masz właściwość w klasie i ona nie wychodzi poza scope metody to uzywaj zmiennych lokalnych.
 

<?php

class Register
{
    private $login;
    private $password;
    private $password2;
    private $email;
    private $reCaptcha_response;
    private $reCaptcha_secret_key = 'sercret_key';

    public $error;

    public function register_process(RegisterProcessValueObject $registerProcessValueObject)
    {
        $this->login = $registerProcessValueObject->getLogin();
        $this->password = $registerProcessValueObject->getPassword();
        $this->password2 = $registerProcessValueObject->getConfirmPassword();
        $this->email = filter_var($registerProcessValueObject->getEmail(), FILTER_VALIDATE_EMAIL);
        $this->reCaptcha_response = $registerProcessValueObject->getRecaptchaResponse();

        // Check login length
        if (strlen($this->login) < 5 || strlen($this->login) > 16) {
            $this->error = 'Login musi zawierać od 5 do 16 znaków';
            return $this->error;
        }

        // Check login's chars
        if (!ctype_alnum($this->login)) {
            $this->error = 'Login może zawierać tylko litery i cyfry (bez polskich znaków)';
            return $this->error;
        }

// Check password length
        if ((strlen($this->password) < 6) || (strlen($this->password) > 20)) {
            $this->error = 'Hasło musi zawierać od 6 do 20 znaków';
            return $this->error;
        }

// Check passwords disparateness
        if ($this->password !== $this->password2) {
            $this->error = 'Podane hasła nie są identyczne';
            return $this->error;
        }

// Check reCaptcha
        $check_reCaptcha = file_get_contents('https://www.google.com/recaptcha/api/siteverify?secret=' . $this->reCaptcha_secret_key . '&response=' . $this->reCaptcha_response);
        $reCaptcha_answer = json_decode($check_reCaptcha);

        if (!$reCaptcha_answer->success) {
            $this->error = 'Potwierdź, że nie jesteś botem, a może jednak nim jesteś? :o';
            return $this->error;
        }

// Check login in database
        $base_obj = new Base_queries;

        if ($base_obj->user_check_login($this->login) > 0) {
            $this->error = 'Ten login jest już w użyciu';
            return $this->error;
        }

// Check email in database
        if ($base_obj->user_check_email($this->email) > 0) {
            $this->error = 'Ten email jest już w użyciu';
            return $this->error;
        }

// Register new user
        $base_obj->user_register($this->login, $this->password, $this->email);
        $this->error = 'Konto zostało zarejestrowane!';
        return $this->error;
    }
}

class RegisterProcessValueObject
{
    /**
     * @var string
     */
    private $login;
    /**
     * @var string
     */
    private $email;
    /**
     * @var string
     */
    private $password;
    /**
     * @var string
     */
    private $confirmPassword;
    /**
     * @var string
     */
    private $recaptchaResponse;

    public function __construct(
        string $login,
        string $email,
        string $password,
        string $confirmPassword,
        string $recaptchaResponse
    )
    {
        $this->login = $login;
        $this->email = $email;
        $this->password = $password;
        $this->confirmPassword = $confirmPassword;
        $this->recaptchaResponse = $recaptchaResponse;
    }

    /**
     * @return string
     */
    public function getLogin(): string
    {
        return $this->login;
    }

    /**
     * @return string
     */
    public function getEmail(): string
    {
        return $this->email;
    }

    /**
     * @return string
     */
    public function getPassword(): string
    {
        return $this->password;
    }

    /**
     * @return string
     */
    public function getConfirmPassword(): string
    {
        return $this->confirmPassword;
    }

    /**
     * @return string
     */
    public function getRecaptchaResponse(): string
    {
        return $this->recaptchaResponse;
    }
}

 

Answer 2

Jeśli kod ma być bezpieczny, to powinno się unikać informowania użytkowników na temat tego, czy email lub login już istnieją. 

Poza tym, nie powinno się ograniczać liczby znaków w haśle. Aktualnie korzystam z LastPass i zwykle mam domyślnie ustawiony na 100 znaków ze wszelkimi alfanumerycznymi. Bezpieczniej się czuję, jeśli tu nie mam ograniczeń:

Poza tym.

1. Do zliczania znaków lepiej użyć tego: https://www.php.net/manual/en/function.grapheme-strlen.php (https://stackoverflow.com/questions/40362669/count-character-length-of-emoji)
2. Jeśli to jest kontroler, to proces rejestracji można wydzielić do osobnej klasy, zwykle nazywanej serwisem. Kontrolery powinny mieć możliwie mało wierszy.
3. Dlaczego nie używasz guzzle do obsługi żądania weryfikacji reCaptcha?
4. Czemu recaptcha jest od razu dostępna? Powinno się raczej stosować podejście w którym po, np., 3 próbie włącza się recaptcha

Nie pokazałeś kodu metody user_register więc trudno cokolwiek więcej powiedzieć o bezpieczeństwie. 

Używasz formatowania kodu niezgodnego z PSR:

• https://www.php-fig.org/psr/psr-1/
• https://www.php-fig.org/psr/psr-12/

 

Comments

@event15, ostro mieszasz. Książka Sekuraka, na którą się powołujesz, owszem, mówi o zdawkowych komunikatach, ale w kontekście UWIERZYTELNIANIA, nie tworzenia konta użytkownika (s. 527). To samo zresztą mówi Twój artykuł o enumeracji użytkowników, a w komentarzach jest wyraźnie zaznaczone, że rejestracja to inny case i tam sensownym rozwiązaniem jest po prostu multifactor → https://blog.rapid7.com/2017/06/15/about-user-enumeration/#comment-4506624238

Brak szczegółowej informacji o błędzie w procesie tworzenia konta jest tak bardzo nieprzyjazny dla użytkownika, ze tego typu rozwiązanie nie ma prawa przejść przez sito review UX-owego.

---

@Comandeer, tak - parafrazowałem z pamięci i rzeczywiście w książce jest o uwierzytelnianiu. Mój błąd. 

Jeśli chodzi o formularz rejestracji, https://security.stackexchange.com/questions/234740/on-my-websites-account-creation-form-how-to-avoid-leaking-the-information-that?noredirect=1&lq=1

Myślę, że to jest częste podejście, szczególnie w firmach zajmujących się ecommerce. Czyli wyświetlenie za każdym razem komunikatu, że na adres e-mail został wysłany link aktywacyjny. 

@EDIT: co ciekawe, musiałem wyłączyć uBlocka, żeby zobaczyć te komentarze. 

---

Mógłbym jakiś link do informacji o tym guzzle? O co to jest, z czym to się je i jak tego używać.

---

@event15,

Uncaught Error: Call to undefined function grapheme_strlen()

Przy użyciu tej funkcji.

---

Nie pokazałeś kodu metody user_register więc trudno cokolwiek więcej powiedzieć o bezpieczeństwie.

Funkcja zapisuje dane do bazy.