Basic login script

ICTscripters maakt gebruik van cookies. Door het gebruiken en browsen naar onze site gaat je automatisch akkoord met het gebruik van cookies. Klik hier voor meer informatie

  • Hallo,

    Dit is een simpel login script. Het is geprogrammeerd in PHP PDO (prepared statments).
    Het is makkelijk uit te breiden en je kan er nog alle kanten mee op gaan!

    Graag hoor ik op bouwende kritiek en wens andere er veel plezier mee!
    Bestanden

    3,183 keer bekeken

Reacties 8

  • FangorN -

    Okay, waar te beginnen.

    Als je dit soort functionaliteit gaat schrijven, komen er een heleboel dingen bij elkaar. Niet alleen op security gebied, maar ook de manier waarop de verschillende "webpagina's" van je website/applicatie worden opgebouwd. Dit laatste heeft weer directe invloed op hoe je je code (handig zou) moet(en) opzetten. En dit alles haakt (nogal ingrijpend) op elkaar in. Daarom is het des te belangrijker dat je komt tot een goede opzet en integratie van dit alles. In de aangeleverde code kan in dit opzicht nog veel veranderd (en mogelijk verbeterd) worden.

    Op een mogelijk aangepaste aanpak kom ik misschien later nog terug.

    Wat misschien ook handig is is wat uitleg over installatie en (initieel) gebruik, eventueel aangevuld met notities over minimale versies voor PHP/MySQL en/of specifieke configuratie. Hier is niets over gedocumenteerd :/.

    Maar laten we eerst eens kijken wat de code *doet*.

    Allereerst, het gebruik van enkel cookies op deze manier is niet veilig. Als je het zelf voor elkaar krijgt om een cookie te setten met iemand zijn gebruikersnaam -wat niet echt moeilijk is- is deze daarmee effectief ingelogd als deze gebruiker. Zoals @r8934 aangeeft is het beter om gebruik te maken van sessies. Vreemdgenoeg geeft hij daarbij aan dat cookies bij een hoop mensen uitgeschakeld zouden zijn. Los van deze stelling (bron? percentages?) maken sessies standaard ook gebruik van cookies. Middels een cookie wordt het sessie-id onthouden. Daarbij is het handig als je de gebruiker bij het inloggen de mogelijkheid geeft of de login onthouden mag worden ingeval je achter je vaste PC zit. In dat geval wordt een succesvolle login gekoppeld aan een vast IP-adres.

    Dan, zoals @r8934 terecht opmerkt, produceren de pagina's een fout op het moment dat je je database-gegevens nog niet hebt ingesteld. Naast het herstellen van de typefout van PDOExCeption in het catch-blok is het ook nodig om ook $error te hernoemen naar $errorReport, of nog beter, alles te hernoemen naar simpelweg $e of $exception, zodat de naam van deze variabele beter de lading dekt. En wellicht beter is dus ook om een of meer exception classes te schrijven.

    Vervolgens de volgende observatie: de username is bij inloggen niet case-sensitive. Op zich is dit niet zo'n probleem, ware het niet dat in het cookie letterlijk deze naam gehanteerd wordt (deze naam zou eigenlijk uit je profiel moeten komen en in een user object opgeslagen moeten worden). Registreer ik mij dus als FangorN kan ik inloggen als fangorn, FANGORN, FaNgOrN etc. en vervolgens wordt mijn naam ook zo weergegeven.

    Dan de validatie bij registratie. Hier wordt je input niet echt fantastisch gefilterd. Ik kan bijvoorbeeld de volgende namen gebruiken:
    * " " (twee spaties :))
    * </div>
    * <script>alert('hoi')</script>
    Het eerste geval is nogal onpraktisch. Het tweede voorbeeld kan je layout breken als deze naam niet goed ge-escaped wordt bij uitvoer (zie index.php) en met het derde voorbeeld is je site mogelijk vatbaar voor XSS (Cross Site Scripting)
    Ook hier geldt nog steeds: filter input, escape output

    Als je inlogt/uitlogt zie je heel kort nog oude informatie voor de (meta) refresh. Ik zou je aandringen deze meta-refreshes eruit te gooien en de verschillende acties beter te compartimenteren in aparte onderdelen.

    Tot slot zou ik de restrictie op uniek IP per gebruiker bij registratie laten varen, want het kan best zo zijn dat meerdere mensen via dezelfde verbinding aan het internet hangen (denk bijvoorbeeld aan een studentenhuis of bedrijf).

    Okay dat waren wat pointers voor het gebruik van de functionaliteit, nu de code *cracks knuckles*.

    Een vrij belangrijk aspect van een goede/veilige werking van webpagina's is dat overal een (en bij voorkeur dezelfde) character encoding wordt gehanteerd:
    - bij de opslag van je code-bestanden zelf (als is dat niet zo vaak een issue)
    - in de character encoding van je HTML-document (middels meta tag of PHP header)
    - bij de connectie met je database
    - in de tabel- en kolomdefinities van je tabellen
    - en tot slot, de daadwerkelijke OPSLAG van je data in die character encoding

    En daar ga je eigenlijk al de mist in:
    Geen van de (voorbeeld)pagina's zijn volledige noch kloppende HTML-pagina's.
    Bij de connectie met je database specificeer je ook niet expliciet een character encoding.

    Als ik het mysql-bestand bekijk vallen mij de volgende dingen op:
    - waarom niet gewoon 1 creatie statement in plaats van 3 queries? dit kan eenvoudiger.
    - uiteindelijk maak je van je van je id kolom een auto_increment veld; maar deze is niet gedefineerd als UNSIGNED (dat wil zeggen, mag geen negatieve waarden bevatten) - gooi je daarmee niet effectief de helft van het aantal te benutten id's weg? en BIGINT(255) is wellicht een beetje overkill :)
    - wat ik niet snap is dat je hier kiest voor de latin1 character encoding, als je dan toch een standaard character encoding kiest, maak dan gebruikt van (op zijn minst) UTF-8 (utf8 in MySQL) of equivalent. Het is dan (en eigenlijk altijd al) wel HEEL belangrijk dat je vervolgens overal je character encoderingen correct definieert
    - waarom begint jouw auto_increment bij 2? :/
    - en ja, je username kolom is niet case-sensitive, dus alle vergelijkingen in SQL-statements hiermee ook niet; dat is niet per se een probleem, maar wel iets om rekening mee te houden zoals eerder aangegeven

    Als we het over de afzonderlijke pagina's hebben (index.php, module/login.php, module/register.php) moeten we het eigenlijk ook over de opbouw van pagina's in het algemeen hebben. Je hebt nu met deze drie pagina's al drie "ingangen" (voordeuren) in je applicatie. Het is om meerdere redenen waarschijnlijk handiger/verstandiger om één voordeur (one point of entry) te hebben. Hiervoor zou je index.php kunnen gebruiken. Vervolgens moet je besluiten hoe je andere pagina's via index.php inlaadt. Dit kan op legio manieren. In zijn eenvoudigste vorm zou je zoiets kunnen doen als index.php?page=X&action=Y, waarbij X bijvoorbeeld "login" is en Y "process". Op deze manier kun je ook acties in behapbare stukken op te delen zonder te verzanden in een if-elseif-elseif-elseif-else hel.

    Je gebruik overal ob_start(). Dat is niet direct nodig als je je code wat meer organiseert, en niet te vroeg of onnodig begint met het afdrukken van output.

    Dan over require_once: hier include je op voorhand code. Code die je wellicht helemaal niet gaat gebruiken. Het is beter om classes "op afroep" te laden. Dit kun je doen met een autoloader. Op het moment dat je een object van een klasse aanmaakt zal via de autoloader getracht worden het bijbehorende klasse-bestand te laden. Op die manier wordt precies die code gebruikt die nodig is, niet meer, niet minder.

    Omdat dit verhaal al redelijk lang is zal ik voor nu afsluiten met nog wat korte pointers:
    - normaal is het niet de bedoeling dat bestanden waarin classes gedefinieerd zijn iets "doen" - jij maakt in de classes direct een object aan van die class, dat is nogal ongebruikelijk en verwarrend - het bestand staat op deze manier "op scherp", ik zou de objecten creeren waar en wanneer je ze gebruikt (met gebruikmaking van een autoloader)
    - de defines in je database class horen daar echt niet thuis, maar in een apart configuratie-bestand wat je bijvoorbeeld include (dat kan dan mogelijk niet met een autoloader) in je index.php
    - als je "global" gebruikt in je classes, is er iets (grondig) mis - als je een object nodig hebt in een ander object, geef deze dan mee als parameter bij creatie van het object, of trek deze uit een registry als je deze nodig hebt ofzo, maar niet op deze manier, dat doet het hele werken via OO een beetje teniet
    - je $database class variable in je database class zou niet public moeten zijn, op deze manier zou je rechtstreeks van je PDO-connectie gebruik kunnen maken via $db->database... maar daarvoor had je toch die database class?
    - alle methoden binnen de user class hebben "user" in de naam, dat is niet nodig, dit is namelijk al evident
    user_check --> check
    new_user --> create
    user_login --> login
    - de HashSalt class zou een helper class moeten zijn met static methoden, de checkUserPassWebsite methode hoort hier niet in thuis, deze hoort eerder thuis in de user class (in een methode "authenticate" ofzo)
    - klassenamen zouden met een Hoofdletter moeten beginnen

    Hier laat ik het even bij, dit zijn al een hele hoop pointers, maar lang niet alle.

    Dit loginscript bespreken in afzondering is net zoiets als enkel de akkoorden van één instrument in een muzieknummer bespreken: het dekt de lading van het nummer niet. En net zoals in de muziek zou het geluid wat deze instrumenten tezamen produceren enigszins in harmonie moeten zijn, anders is het nummer waarschijnlijk niet om aan te horen :).

    Mocht je vragen hebben en/of geinteresseerd zijn in een aangepaste variant waarin de bovenstaande punten zitten verwerkt dan kun je mij een PM sturen. Mogelijk kunnen we dan samen een versie 2 presenteren (of 0.2 :)).

  • Jesse -

    ik heb hem uitgepakt en naar mijn usbwebserver gezet nu heb ik alles in mij root filter gezet maar nu krijg ik fouten

  • Luc -

    Yes inmiddels werkt het weer om hem te openen;)

    @jesse66, er zit een registratie en loginscript bij inderdaad:)

  • Jesse -

    Wat zijn de functie zit er registratie bij ??

  • r8934 -

    @luc
    moet je 7zip downloaden, dan werkt hij wel.

    @Frenzo.Brouwer:
    Ipv cookies, zou ik sessies gebruiken. Cookies zijn bij een hoop mensen uitgeschakeld. En sessies zijn net iets veiliger.

    Heb je al eens gekeken naar php.net/manual/en/function.password-hash.php en php.net/manual/en/function.password-verify.php?
    Dat scheelt je de class HashSalt.

    De check op ip adres zou ik weglaten. Mensen met school/bedrijf of gedeeld netwerk worden nu benadeeld.

    De styling bij de exceptions werkt niet.
    kleine typo, je bent de c vergeten.
    catch(PDOExeption $error){

    Als je een zelf een exception handler schrijft, dan hoef je de styling niet steeds opnieuw eromheen te zetten.

  • Luc -

    ZIp lijkt kapot te zijn..

    • Frenzo.Brouwer -

      Hoe kan ik dat zien, of wat kan ik daar aan doen?

    • Luc -

      Als je hem vanaf deze site download en probeert te openen krijg je de melding dat de ZIP is beschadigd. Je zou kunnen proberen om een nieuwe zip te maken van de bestanden en deze hier toe te voegen (wel van tevoren even testen of de ZIP werkt).