Gå til innhold

Klasse for admin-login

ZoRaC

Anbefalte innlegg

Videoannonse

Videoannonse
Annonse
detonator

detonator

Skrevet
Skrevet

Synes ikke det er helt god objektorientering at du bruker en konstant til å definere passordet. Hadde kanskje vært bedre å sende inn passordet i en konstruktør som setter en objektvariabel. Da tvinger du den/de som bruker klassen til å faktisk sende med passordet, og du kan ha flere objekter samtidig uten at de skal jobbe opp mot en felles konstant.

Ellers så det greit ut, som jeg kunne se sånn ved første øyekast. :)

Lenke til kommentar
detonator

detonator

Skrevet
Skrevet
Jeg tror den konstanten var bare for å teste at den funket ;)

6249240[/snapback]

Tror du misforsto hele poenget mitt. Hvis du tenker på konstant-verdien så skjønner jeg selvfølgelig det, men uansett hvor han henter passordet fra og hiver det inn i samme konstanten så er det like dårlig objektorientering.

Lenke til kommentar
CruellaDeVille

CruellaDeVille

Skrevet
Skrevet
Har aldri skrevet klasser før, annet enn å brukt/modifisert ferdige klasser fra nett.

Har når prøvd meg på en enkel klasse, kan noen kommentere måten jeg har gjort det på og klassens funksjon? :)

 

EDIT:

Kan sees her istedet: http://pastebin.com/760592

 

EDIT 2:

Klassen skal brukes til å ta seg av admin-pålogging på sidene mine.

6248792[/snapback]

 

Jeg har lest de andre trådene og er forsåvidt enig i at et konstant passord ikke er av det smarteste, men i testøyemed.... så okay.

 

Likevel har jeg noen små kommentarer. En klasse skal omhandle "en ting", i dette tilfelle et loginforsøk. Da syns jeg det er litt merkelig å bruke loginforsøket til å håndtere utskrift av et skjema. I tillegg syns jeg det var litt merkelig å bruke loginforsøk-klassen til å logge ut igjen inni klassen. Dette gjør jeg nå på utsiden.

 

Noen av metodene dine kan gjøres adskillig kortere, og heller flytte noe av funksjonaliteten ut av klassen. (som å sette sessionvariabler)

 

Feks i funksjonen handleLogin - har jeg flyttet ut setting av sessionvariabler og bare returnert en strcmp mellom input og konstant passord.

 

I funksjonen realsafehtml skjønner jeg ikke helt hvorfor du har $oldstring. Den såkalte løkken din vil bare gå en gang uansett, siden $oldstring blir satt til $string med en gang. Dermed kan en bare fjerne hele løkken.

 

I funksjonen logout har du glemt å ødelegge selve sessionen, med session_destroy()

Hvorfor du har med returtype der forstår jeg ikke, siden det bare var med true, dvs den feiler aldri. Dermed er det meningsløst å sjekke om metoden returnerer true, siden den alltid gjør det.

 

Min kode er ikke særlig mye kortere enn ditt, men selve klassen er mindre.

<?php
session_start();
$page = $_SERVER['PHP_SELF'];
$login = new login();

if (isset($_GET['logout'])) {
$login->logout();	
   print ("Du er logget ut!");
}
if(isset($_POST['adm_pass'])) {
$pass = trim($_POST['adm_pass']);
if (!$login->handleLogin($pass)) {
 $login->logout();	
 header( "refresh:4; url=$page");
   	print ("Feil passord!");
}else{  
 $_SESSION['md5_string'] = md5($_SERVER['HTTP_USER_AGENT'].$_SERVER['SERVER_SOFTWARE']);
 $_SESSION['md5_pass'] = md5($login->input_pass);
 if ($login->verifySession()) 
   echo "Du er logget inn";
 else{
 	header( "refresh:4; url=$page");
     print ("Det gikk ikke så bra!");
 }
}
}else {
print <<<HTML
<TABLE border=0>
 <FORM method="post" action="">
 	<TR>
   <TD><B>Administrator passord:</B></TD>
   <TD><INPUT MAXLENGTH="20" TYPE="password" NAME="adm_pass" SIZE="20" value=""></TD>
 	</TR>
 
 	<TR>
   <TD COLSPAN=2 ALIGN="CENTER"><INPUT TYPE="submit" VALUE="OK"></TD>
 	</TR>
 </FORM>
</TABLE>
HTML;
}
class login {
var $input_pass;
 	static $ADM_PASS;

function Login(){
 self::$ADM_PASS =  md5("test");
}
/* Check the password */
function handleLogin($input_pass) {
 $this->input_pass = $this->realsafehtml($input_pass);
 return strcmp(md5($this->input_pass), self::$ADM_PASS)== 0;
}

/* Check the session-vars */
function verifySession() {
 return $_SESSION['md5_pass'] == self::$ADM_PASS && $_SESSION['md5_string'] === md5($_SERVER['HTTP_USER_AGENT'].$_SERVER['SERVER_SOFTWARE']);
}

/* Logout */
function logout() {
 unset($_SESSION['md5_pass']);
 unset($_SESSION['md5_string']);
 session_destroy();
}

/* Secure $str against injections */
function realsafehtml($str) {
        //nuke script and header tags and anything inbetween
        $str = preg_replace("'<script[^>]*>.*</script>'siU", "", $str);
        $str = preg_replace("'<head[^>]*>.*</head>'siU", "", $str);

        //listed of tags that will not be striped but whose attributes will be (br|b|i, etc)
        $allowed = "";
        //start nuking those suckers. don you just love MS Word's HTML?
        $str = preg_replace("/<((?!\/?($allowed)\b)[^>]*>)/xis", "", $str);
        $str = preg_replace("/<($allowed).*?>/i", "<\\1>", $str);
   	return mysql_escape_string($str);
}
}
?>

 

Sorry for mye kode.

Lenke til kommentar
robgar

robgar

Skrevet
Skrevet
Jeg tror den konstanten var bare for å teste at den funket ;)

6249240[/snapback]

Tror du misforsto hele poenget mitt. Hvis du tenker på konstant-verdien så skjønner jeg selvfølgelig det, men uansett hvor han henter passordet fra og hiver det inn i samme konstanten så er det like dårlig objektorientering.

6249372[/snapback]

 

 

:blush: Så hva du mente nå... Istedenfor å bruke konstanten, så bør han bruke f.eks ved constructor når han kaller opp classen: 

$obj = new login($pass);

og sette et object i constructoren, eg. 

 $this->admin_pass = $pass;

Lenke til kommentar
CruellaDeVille

CruellaDeVille

Skrevet
Skrevet
Jeg tror den konstanten var bare for å teste at den funket ;)

6249240[/snapback]

Tror du misforsto hele poenget mitt. Hvis du tenker på konstant-verdien så skjønner jeg selvfølgelig det, men uansett hvor han henter passordet fra og hiver det inn i samme konstanten så er det like dårlig objektorientering.

6249372[/snapback]

 

 

:blush: Så hva du mente nå... Istedenfor å bruke konstanten, så bør han bruke f.eks ved constructor når han kaller opp classen: 

$obj = new login($pass);

og sette et object i constructoren, eg. 

 $this->admin_pass = $pass;

6249806[/snapback]

 

 

Nah... Så direkte bør det vel neppe gjøres. I såfall må konstruktøren kalle på en metode som sjekker om $pass (param) er rikig passord, og ikke tilordne admin_pass direkte.

 

Et annet problem er at loginobjektet ikke kan brukes til å håndtere utlogging.

Løsningen på det kan være å fremdeles ha en tom konstruktør, som da gjør at en får tilgang til metoder i klassen, og ha en egen setter for å legge til input for testing.

Lenke til kommentar
Peter

Peter

Skrevet
Skrevet
(*snip*)

Håper du ikke har noe imot at jeg la koden din i en pastebin. Forumet er dårlig på fargekoding.

 

http://pastebin.com/760876

 

Så, litt kommentarer:

Først og fremst reagerte jeg veldig på at du kaller session_destroy i en logout funksjon. OOP handler som nevnt om å atomisere koden, og etter det jeg vet, så vet Login svært lite om hva som er lagret i sessions, bortsett fra de to variablene klassen selv (etter min mening) burde ha lagt til, istedenfor en annen funksjon.

Hvordan vet klassen din _egentlig_ hvilke to variabler den skal fjerne, når den ikke har satt dem selv?

 

Personlig så følger det nesten alltid én klasse, i form av en modul, med en annen klasse. F.eks. ville Login i dette tilfellet også hatt en Module_loginmanager som sørget for å fore Login med riktige variabler, uten å måtte ofre modulariteten.

Nå synes jeg også at det faktisk er enklere å skrive fullstendig OOP, enn å mikse sekvensiell og OO. Sålangt har det funket, men jeg er ikke ferdig ennå, blir spennende å se om jeg klarer å løpe linen ut :)

Lenke til kommentar
CruellaDeVille

CruellaDeVille

Skrevet
Skrevet (endret)
Først og fremst reagerte jeg veldig på at du kaller session_destroy i en logout funksjon. OOP handler som nevnt om å atomisere koden, og etter det jeg vet, så vet Login svært lite om hva som er lagret i sessions, bortsett fra de to variablene klassen selv (etter min mening) burde ha lagt til, istedenfor en annen funksjon.

Hvordan vet klassen din _egentlig_ hvilke to variabler den skal fjerne, når den ikke har satt dem selv?

Hmm, kanskje du har rett, men hvis Login klarer å sammenligne med sessionvariabler så vet den vel også hvilken sessionvariabler som skal ødelegges? Jeg klarte aldri å teste dette.

Personlig så følger det nesten alltid én klasse, i form av en modul, med en annen klasse. F.eks. ville Login i dette tilfellet også hatt en Module_loginmanager som sørget for å fore Login med riktige variabler, uten å måtte ofre modulariteten.

Nå synes jeg også at det faktisk er enklere å skrive fullstendig OOP, enn å mikse sekvensiell og OO. Sålangt har det funket, men jeg er ikke ferdig ennå, blir spennende å se om jeg klarer å løpe linen ut :)

6250070[/snapback]

Da tenker du på en klasse som sjekker om riktig passord er satt? Vil ikke da Login miste hele sin funksjon? Forklar gjerne. Jeg skjønte ikke det der.

Endret av CruellaDeVille
Lenke til kommentar
Peter

Peter

Skrevet
Skrevet (endret)

Det var litt merkelig forklart, delvis fordi jeg ikke har laget denne komponenten til siden min ennå.

 

Først og fremst føler jeg bare at jeg må oppklare noe som kan være en uklarhet, nemlig begrepet "Login".

Når jeg snakker om login snakker jeg egentlig mer om autorisasjon, dvs. et komponent som sørger for at ingen uvedkommende har tilgang. Dvs. at "Login" ikke bare har ansvar for det å logge inn, men også for å sjekke tilgang og til slutt logge ut.

 

Min klasse Login, som jeg kanskje burde kalle "Auth" vil derfor (såvidt jeg kommer på nå) ha tre funksjoner:

login($username, $password)

is_logged_in($user_id);

logout($user_id);

 

login vil sørge for å sjekke $username og $password mot en datakilde, dette kan være fil, database e.l. og deretter sette en passende session (I mitt tilfelle passer det veldig bra med en user_id). Dette vil være normal hendelsesfly, variasjonene er jo f.eks. om loginen var gal.

Deretter har du is_logged_in($user_id) som sjekker om brukeren med id $user_id er logget inn (dvs. sjekker sessionen).

Så har vi logout, som fjerner da sessionene satt i login.

 

Noen vil kanskje føle at is_logged_in er smør på flesk, men grunnen til denne er at vi ikke vet noe om _hvordan_ systemet holder styr på om brukeren er logget inn. For alt vi vet (utenfor login-klassen) kan innloggede brukere holdes styr på via filer e.l.

 

 

 

P.S

Dette er bare slik jeg tenker nu, og jeg har som nevnt ikke fått laget dette ennå, derfor kan det hende at jeg må gjør endringer senere som jeg ikke har sett nå.

Endret av Nazgul
Lenke til kommentar
ZoRaC

ZoRaC

Skrevet
  • Forfatter
Skrevet (endret)

Først må jeg bare si at jeg er imponert over aktiviteten/responsen på dette forumet! :w00t:

 

I funksjonen realsafehtml skjønner jeg ikke helt hvorfor du har $oldstring. Den såkalte løkken din vil bare gå en gang uansett, siden $oldstring blir satt til $string med en gang. Dermed kan en bare fjerne hele løkken.

 

Den funksjonen har jeg funnet blant kommentarene på PHP.net...

Hva skjer med input som: <scr<script>ipt>alert('Hello'!);</scrip</script>t> etter en runde i "din" realsafehtml?

Derfor må man loope, slik at den fjerner alt sånt.. ;) (Tror i hvertfall at det er det som er tanken bak, hehe...) ;)

 

I funksjonen logout har du glemt å ødelegge selve sessionen, med session_destroy()

Hvorfor du har med returtype der forstår jeg ikke, siden det bare var med true, dvs den feiler aldri. Dermed er det meningsløst å sjekke om metoden returnerer true, siden den alltid gjør det.

 

Kan ikke kjøre session_destroy(); siden jeg potensielt har annen data lagret i session'en også, som det også blir poengtert. ;)

 

Ellers ser jeg at jeg kan ALT for lite om klasser, osv, mye "fremmedord" her jeg egentlig burde skjønt, for her skjønte jeg ikke stort: :p

Nah... Så direkte bør det vel neppe gjøres. I såfall må konstruktøren kalle på en metode som sjekker om $pass (param) er rikig passord, og ikke tilordne admin_pass direkte.

 

Et annet problem er at loginobjektet ikke kan brukes til å håndtere utlogging.

Løsningen på det kan være å fremdeles ha en tom konstruktør, som da gjør at en får tilgang til metoder i klassen, og ha en egen setter for å legge til input for testing.

 

Ellers ser da din "reviderte" kode ganske "OK" ut, men hva er vitsen med funksjonen "Login()"? ;)

 

Poenget mitt er jo at jeg nå skal kjøre require_once('login.class.php'); i alle adm-sidene mine, men om jeg vil bruke samme klasse til en annen login på samme site, er det ikke da dumt at passordet er definert i klassen?

Hvordan er det da "best" å definere passordet?

 

Edit:

Ser du har fjernet min sleep(3), vil ikke det gjøre scriptet mer sårbart for bruteforce?

For med din header-refresh så vil jo resultatet av påloggingen være synelig for bruteforce-programmet med engang og den kan da sende ny pålogging, uten å vente de 4 sekundene..?

 

Edit2:

Parse error: parse error, unexpected T_STATIC, expecting T_OLD_FUNCTION or T_FUNCTION or T_VAR or '}' in C:\x\login.class.php on line 47

Hva er feil? Aldri brukt/hørt om static jeg :p

Endret av ZoRaC
Lenke til kommentar
Peter

Peter

Skrevet
Skrevet

Login() er kontruktøren i klassen Login (ser den er skrevet med liten bokstav, men garantert det hun mente) Konstruktøren er funksjonen som blir kalt når du oppretter et objekt.

$login = new Login() <- her blir konstruktøren kalt, parametere du gir her er parameterene du gir konstruktøren. I ditt tilfelle kan dette være passordet du vil sammenligne med (delvis slik CD har vist)

Jeg er lite fan av konstruktører i PHP, dessverre, ettersom du ikke har mulighet til å overloade den. (digresjon, ikke spør :D)

 

Det spørs litt hvordan passordet ditt skal brukes, og om det er flere osv.

Ofte er det da greit å bruke databaser eller beskyttede filer.

 

Brute force på et md5-passord eller sha1 er veldig tidkrevende alene, å legge til 3 sekunder gjør egentlig lite annet enn å forsinke deg og dine brukere.

Dersom du er redd for bruteforce, er det mye bedre om du lager en logg av noe slag, som ikke lar en bruker prøve mer enn f.eks. 3 ganger før han/hun blir låst ute i en time e.l.

Lenke til kommentar
mikaelandre

mikaelandre

Skrevet
Skrevet

jeg pleier å lage en user-classe. da kan jeg på toppen av alle sidene skrive $user = new $user();

 

den klassen kan så ha funksjonene login(String username, String pass), isLoggedin() og logout(). andre ting kan være getName, getId, update() osv.

 

ved login kan jeg da kalle $user->login($username, $pass);

 

På denne måten blir det, ihvertfall for meg, mer logisk hvis du tenker oop. en bruker er et objekt, login er en handling.

Lenke til kommentar
Nervetattoo

Nervetattoo

Skrevet
Skrevet

Hva med å gjøre ting noe ala dette:

$pass = clean($_POST['password']);
$userId = UserFinder::findByName($username); // User-objekt eventuelt:
//$user = new User($userId);
$auth = new Md5Authenticator;
if ($auth->auth($user, $pass)) {
$user->storeStatus($sessionRegistry);
}

 

Grunnen til å dele det såpass opp er at man får ganske klare ansvarsområder. Sånn røffly dette designet har jeg brukt før for autentisering. Jeg er i tvil hvorvidt User-objektet bør lagre til session (som er abstrahert bak en registry over session her) eller om den oppgavenbør løses andre steder.

Dette løser selvsagt ingen problemstillinger som logging/beskyttelse mot bruteforcing og nivåkontroll, men det ville vært enkelt å implementert.

 

Jeg serverte bare pseudokode/klientkode siden jeg ikke har implementert dette noe sted hvor lisensiering tilsier at jeg kan paste det her. Men det er jo tross alt design som blir diskutert her og ikke implementasjon.

Lenke til kommentar

Opprett en konto eller logg inn for å kommentere

Du må være et medlem for å kunne skrive en kommentar

Opprett konto

Det er enkelt å melde seg inn for å starte en ny konto!

Start en konto

Logg inn

Har du allerede en konto? Logg inn her.

Logg inn nå
Gå til emneliste
×
×
  • Opprett ny...