@GhostDog:
Wypada jednak zauważyć, że autor przynajmniej trochę się postarał. Zobacz sobie np. to, co wymodził Perykles. W kodzie Peryklesa hasła są zapisywane w bazie jawnie -- tutaj są hashe. W kodzie Peryklesa zmienne wstawione są na pałę do zapytania SQL bez żadnego unieszkodliwiania, co jest niesamowitą wręcz, ziejącą japą w kodzie -- tu jest użyta funkcja, w dodatku ta właściwa (mysql_real_escape_string). Więc corey przynajmniej przyłożył się do tych podstaw, czego nie da się powiedzieć o wielu skryptach wklejanych tu na forum. Dodatkowo sam napisał swój kod i może dopiero się uczy, to jednak jest chyba kimś, komu warto pomagać. Choć przyznam, że sam w takich wypadkach nieraz piszę trochę bardziej "oschle", bo jak gościu potrafi samodzielnie myśleć i uwzględnia takie rzeczy jak bezpieczeństwo, to można go skierować do książek i da radę je zrozumieć i wykorzystać.
@corey:
GhostDogowi zapewne chodziło, że Twoja implementacja jest stosunkowo prosta i nie posiada żadnych dodatkowych, zaawansowanych zabezpieczeń. Przykładowo, nie masz tam żadnych zabezpieczeń przed atakiem słownikowym na hasło. Łącza są dziś coraz szybsze, maszyny też. Ale ludziom wciąż nie chce się pamiętać pokręconych haseł. Dzięki postępowi technologicznemu serwer można zasypać dużo większą liczbą żądań, które sprawdzałyby po kolei różne hasła ze słownika (z pewnymi wariacjami typu dodanie na koniec jednej lub dwóch liczb). Co jednak zrobi serwer z Twoim skryptem, jeśli wyśle mu ktoś 1000 żądań? Ano bardzo chętnie je obsłuży i powie, że nie, te 1000 haseł nie było tym właściwym. I po którymś tysiącu nastąpi zalogowanie. Całość ataku może zająć np. godzinkę, czy nawet dużo krócej.
Zabezpieczeniem byłoby tu wprowadzenie blokady, że po podaniu np. 3 nieprawidłowych haseł trzeba poczekać 10 minut. I dzięki temu małemu ruchowi w ciągu jednej godziny da się sprawdzić zaledwie około 20 haseł, a nie iluś tam tysięcy.
Podobnie, nie ma tu dodatkowych zabezpieczeń np. przed przechwyceniem sesji (co jednak jest już ciut wyższą szkołą jazdy i tak czy siak nie zapewnia 100% skuteczności).
Ogólnie polecam zapoznanie się z tą książką, którą wymienił @GhostDog. O ile pamiętam, tam był wręcz gotowy skrypt do logowania. W dodatku był w miarę szczegółowo omówiony. W ogóle ta kniga to bestseller, być może najlepsza dostępna książka o PHP. Na pewno Ci się przyda. Ja mam ciut starsze wydanie i poważnie się zastanawiałem, czy nie kupić najnowszego (znacznie ładniejszego, swoją drogą).
Zapewnienie takiego prawdziwego, porządnego bezpieczeństwa przy logowaniu jest naprawdę troszkę skomplikowaną sprawą i po prostu warto o tym poczytać, bo ta wiedza zawsze się przyda.
Przy okazji mogę Ci powiedzieć jeszcze parę rzeczy, których nie znajdziesz w książce "PHP. Zaawansowane programowanie" (bo niestety już chyba postanowiliśmy za Ciebie, że ją kupujesz ;-) ). Rzeczy te są ważne i dotyczą jakości i czytelności kodu. Nie będę tego analizował bardzo głęboko, ale zwrócę Twoją uwagę na kilka prostych, ale ważnych rzeczy.
Niejednolite/niepoprawne nazwy funkcji. Porównaj kilka funkcji, które tam masz:
function re_start() { ... }
function log_in() { ... }
function logout() { ... }
re_start pisze się razem: restart. Tak samo masz log_in, ale masz logout. Podejrzewam, że nie chciałeś mieszać nazwy akcji (czasownik log in -- zaloguj) i nazwy użytkownika (login jako nazwa użytkownika). W takim wypadku jednak powinieneś nazwać tę drugą funkcję log_out lub log_off. W razie wątpliwości odpal dictionary.com i sprawdź.
Zduplikowany kod w gałęziach wyrażenia warunkowego. To popularny błąd, choć jak na dłoni widać, że przeczy zasadzie DRY. Masz tak:
public function re_start(){
if (!session_id()){
session_start();
$_SESSION['user_id'] = 0;
}
else {
session_destroy();
session_start();
$_SESSION['user_id'] = 0;
}
}
To samo możesz zapisać tak, bez zbędnych powtórzeń (przy okazji poprawiłem nazwę):
public function restart(){
if (session_id()) {
session_destroy();
}
session_start();
$_SESSION['user_id'] = 0;
}
Podobną zmianę wypadałoby też wprowadzić do funkcji log_in.
Niezbyt trafne nazwy funkcji. Zobacz na funkcję user_correct($login, $password). Po samej nazwie można by sądzić, że funkcja tak naprawdę powinna się nazywać is_user_correct i zwracałaby TRUE, gdyby login i hasło się zgadzały lub FALSE w przeciwnym wypadku. Tymczasem prawdziwe działanie funkcji musiałeś opisać w komentarzu: funkcja zwraca normalnie id użytkownika na podstawie loginu i hasła, a gdy te do siebie nie pasują, zwraca -1. Trzeba ja więc nazwać od jej głównej funkcji, czyli po prostu get_user_id($login, $password). Tu po samej sygnaturze można się domyślić, że zwraca ona ID, a nie TRUE czy FALSE.
Tak samo nazwanie funkcji "action" może być odrobinę kontrowersyjne. Mogłoby być całkiem OK gdyby klasa była komendą, ale wtedy nie powinna zawierać funkcji log_out. Dostępna publicznie funkcja wykonująca zalogowanie mogłaby się nazywać log_in, a ta od wylogowania log_out. Funkcja prywatna, która loguje użytkownika o danym id, mogłaby się nazywać login_user_with_id($id). Jeśli w tym momencie wkurzałoby Cię, że klasa nazywa się Login i ma metodę log_in, to klasę możesz nazwać np. LoginManager lub po prostu Authentication / Auth.