Prosty system logowania / rejestracji - prośba o ocenę

pytanie zadane 25 listopada 2018 w PHP przez niezalogowany
edycja 25 listopada 2018

Dzień dobry,

Stworzyłem w ramach nauki php / sql prosty system logowania oraz rejestracji użytkowników.

Prosiłbym o wstępną ocenę tego tworu. O to link gitlab: https://gitlab.com/raptoor2/login-system

Wiem, że kodu nie ma zbyt dużo ale może ktoś zwróci uwagę na jakieś podstawowe błędy w samych założeniach.
Za wszystkie wskazówki, porady będę wdzięczny!

Pozdrawiam

2 odpowiedzi

odpowiedź 26 listopada 2018 przez Arkadiusz Waluk Ekspert (287,950 p.)

Już @HaKIM główny problem stwierdził, ale jeśli chcesz mogę dorzucić kilka słów od siebie:

https://gitlab.com/raptoor2/login-system/blob/master/change-email.php#L16 - możesz zrobić filter_input() zamiast filter_var, nie trzeba wtedy robić tego sprawdzenia istnienia i odwoływać się do $_POST
https://gitlab.com/raptoor2/login-system/blob/master/change-email.php#L31 - masz połączenie z bazą w wielu plikach. Gdy będzie trzeba ustawić hasło myślisz, że wygodne będzie szukanie tego wszędzie i ręczne zmienianie? Teraz masz 4, wyobraź sobie że masz 40. Tak, możesz zrobić "znajdź i zamień" w edytorze/IDE, tyle że prościej wyprowadzić config w jedno miejsce i zmieniać raz.
https://gitlab.com/raptoor2/login-system/blob/master/change-email.php#L40 - brak zabezpieczenia przed sql injection. Akurat tutaj nic nie powinno się stać bo wcześniej robisz filtr na email więc dziwne znaki tam nie powinny przejść. Co nie zmienia faktu, że wszystkie wartości podstawiane do bazy bym bindował.
https://gitlab.com/raptoor2/login-system/blob/master/change-email.php#L42 - łączysz się z bazą przez funkcję (mysqli_connect), następnie wykonujesz zapytanie już obiektowo, warto byłoby to ujednolicić i korzystać tylko z obiektowego sposobu. No i możesz też pomyśleć o PDO, w razie np. chęci zmiany silnika bazy będzie prościej.
https://gitlab.com/raptoor2/login-system/blob/master/change-email.php#L49 - a to to co? Może dla Ciebie jest przydatne, ale jak ktoś przegląda kod to kompletnie nie wiem co to ma wnieść :)
https://gitlab.com/raptoor2/login-system/blob/master/change-email.php#L56 - łączenie HTML z PHP (widoku z logiką) to słaby pomysł, co już wspomniał HaKIM. Co więcej nie widzę tu nawet sensownego HTMLa, gdzie jest DOCTYPE, head, body - cała struktura?
https://gitlab.com/raptoor2/login-system/blob/master/change-email.php#L60 - zamiast robić <?php echo $email ... można zrobić <?= $email ?>
https://gitlab.com/raptoor2/login-system/blob/master/change-email.php#L35 - jesteś pewien, że wyświetlenie wszystkim treści komunikatu błędu wprost z bazy danych to dobry pomysł? Wydaje mi się że niezbyt.
https://gitlab.com/raptoor2/login-system/blob/master/login.php#L29 - hasło powinno być hashowane przez BCRYPT (password_hash/password_verify).
https://gitlab.com/raptoor2/login-system/blob/master/login.php#L52 - placheloder nie zastępuje labela
wszędzie bardzo dużo warunków, błędy przypisywane do jednej zmiennej - gdyby kod był lepiej podzielony i napisany trzeba pomyśleć o wyjątkach

Jeśli to Twój całkowity początek to tragedii nie ma - też tak niestety zaczynałem. Jednak tak się teraz nie pisze w PHP, jak najszybciej powinieneś zmienić sposób pisania. Aplikacja powinna mieć jedno wejście (jeden plik na który kierowane są wszystkie requesty), następnie tam powinno nastąpić wywołanie odpowiednich rzeczy, wykonanie logiki i zwrócenie odpowiedzi. Całość pisze się obiektowo, najczęściej stosuje MVC lub rzeczy do niego podobne (rozdzielenie na kontrolery, modele/encje, widoki). Parę haseł, z którymi warto (a nawet bym powiedział że trzeba) się na początek zapoznać: Composer, MVC, Twig, PSR-4, PSR-2.

komentarz 26 listopada 2018 przez HaKIM Szeryf (87,590 p.)

https://gitlab.com/raptoor2/login-system/blob/master/login.php#L29 - hasło powinno być hashowane przez BCRYPT (password_hash/password_verify).

Niekoniecznie. Do wyboru mamy jeszcze Argon2i (PHP 7.2^), które swoją drogą jest bardziej bezpieczne.

https://gitlab.com/raptoor2/login-system/blob/master/change-email.php#L49 - a to to co? Może dla Ciebie jest przydatne, ale jak ktoś przegląda kod to kompletnie nie wiem co to ma wnieść :)

Ja tylko dodam, że tego typu komentarze to spore zło, tymbardziej gdy pracujemy z teamem. Team się boi usunąć komentarz, bo nie wie czy ktoś tego potrzebuje a autor takowego natomiast może zapomnieć o co z tym chodziło i też nie jest pewien czy można go usunąć - i tak oto powstaje odpad w kodzie.

1	komentarz 26 listopada 2018 przez Arkadiusz Waluk Ekspert (287,950 p.) Do wyboru mamy jeszcze Argon2i (PHP 7.2^), które swoją drogą jest bardziej bezpieczne. Racja, kompletnie o nim zapomniałem. Na pewno nie mogą to być jakieś sha512 czy md5.

odpowiedź 26 listopada 2018 przez HaKIM Szeryf (87,590 p.)

Największą bolączką tego projektu jest brak podziału na warstwy.

Chciałbym Ci bardziej pomóc, ale ten kod jest zbyt czasożerny. Gdybyś skorzystał z funkcji, ułatwiłoby to o niebo sprawę. Przepisz projekt na paradygmat obiektowy/funkcyjny, wtedy dostaniesz o wiele większy feedback na forum. Odniesienie się do funkcji - wykonującej konkretne zadanie - i jej zrozumienie jest o wiele prostsze niżeli przytaczanie pojedynczych linijek kodu.

Poradniki odnośnie obiektówki:

https://symfonycasts.com/search?q=oop&sort=most%20relevant&page=1&types=course

https://webmastah.pl/jak-programowac-obiektowo-cz-1-wstep/

Ja jedynie dodam od siebie, żebyś się nie bał się rozdzielać kodu na małe kawałki.