Problema con funzione cambia password

[furion89]

Salve a tutti, io ho dei problemi con una funzione che uso per cambiare le password, mi spiego meglio: la funzione in questione effettua una serie di controlli:

-Se i campi sono tutti settati,
-Se le password corrispondono tra loro,
-Se la nuova password è compresa tra 8 e 15 caratteri,
-Se la password è alfanumerica,
-Se la vecchia password è corretta.

il problema è che la funzione salta alcuni controlli, ad esempio: se i campi sono settati, le password corrispondono, la lunghezza è esatta e la password vecchia è corretta viene saltato in toto il controllo se la nuova password è alfanumerica e lo script aggiorna la password.

Per controllare se la password è alfanumerica ho provato sia con ctype_alnum e adesso con preg_match('/^[a-zA-Z0-9]+$/', $password).

Per comodità ho tolto la funzione md5 dallo script.

qui di seguito i file:

changepassword.class.php

<?php
error_reporting(E_ALL);

require('db_config.php');
 
class ModifyPassword
{

        public function ChangePassword()
        {
            $this->ErrorReport();
        }

        protected function IsEmptyField()
        {
            if(empty($_POST['oldpassword']) OR empty($_POST['password']) OR empty($_POST['password2']))
            {
                return TRUE;
            }
            else
            {
                return FALSE;
            }
        }

        protected function VerifyPassword()
        {
            if($_POST['password'] == $_POST['password2'])
            {
                return TRUE;
            }
            else
            {
                return FALSE;
            }
        }

        protected function VerifyLengthPassword()
        {
            if(strlen(trim($_POST['password'])) >= 8 && strlen(trim($_POST['password'])) <= 15)
            {
                return TRUE;
            }
            else
            {
                return FALSE;
            }
        }

        protected function VerifyAlphanumericPassword()
        {

            $password = $_POST['password'];
            if(preg_match('/^[a-zA-Z0-9]+$/', $password))
            {
                return TRUE;
            }
            else
            {
                return FALSE;
            }
        }

        protected function PasswordExists()
        {
                        $oldpassword = mysql_real_escape_string($_POST['oldpassword']);
            $sql = "SELECT password FROM users WHERE password='$oldpassword'";
            $res = mysql_query($sql);
            if($row = mysql_fetch_array($res))
            {  
                return TRUE;
            }
            else
            {   
                return FALSE;
            }
        }


        public function ErrorResult($num)
        {
            header("Location: change_password.php?alert=" . $num);
            die;
        }
 
        protected function ErrorReport()
        {
            if($this->IsEmptyField())
            {
                $this->ErrorResult(1);
            }
 
            if(!$this->VerifyPassword())
            {
                $this->ErrorResult(2);
            }

            if(!$this->VerifyLengthPassword())
            {
                $this->ErrorResult(3);
            }

            if(!$this->VerifyAlphanumericPassword())
            {
                $this->ErrorResult(4);
            }
 
            if(!$this->PasswordExists())
            {
                $this->ErrorResult(5);
            }


            $this->ChangePasswordUser();
 
        }

        protected function ChangePasswordUser()
        {
            $oldpassword = mysql_real_escape_string($_POST['oldpassword']);
            $newpassword = mysql_real_escape_string($_POST['password']);
 
            $sql = "UPDATE users SET password = '$newpassword' WHERE password = '$oldpassword'";

            $res = mysql_query($sql);
            mysql_close();

            if($res === TRUE)
            {
            die ('Password aggiornata con succeso!');
            }
            else
            {
            die ("Errore con l'aggiornamento: " . mysql_error());
            }

        }
}        
?>

change_password.php

<?php
include 'error_definition.php';
?>
<form action="modify_password.php" method="POST">
Inserisci la tua vecchia password:

<input type="text" name="oldpassword" />



Inserisci la nuova password:

<input type="text" name="password" />



Ripeti la nuova password:

<input type="text" name="password2" />




<input type="submit" value="invia" />
</form>

modify_password.php

<?php
 
include 'changepassword.class.php';
 
$obj = new ModifyPassword();
$obj->ChangePassword();

?>

error_definition.php

<?php
 
if(isset($_GET['alert']))
{
 
    switch($_GET['alert'])
    {
        case 1:
        die ('Tutti i campi devono essere compilati.');
        break;
 
        case 2:
        die ('Le due password inserite non corrispondono.');
        break;
 
        case 3:
        die ('La password deve essere tra 8 e 15 caratteri.');
        break;

        case 4:
        die ('La password deve essere alfanumerica.');
        break;
 
        case 5:
        die ('La password fornita è errata.');
        break;
 
    }

}

?>

Qualcuno sa dirmi perchè?

Un altro probblema che affligge questo script avviene se setto mysql_close($db); invece di mysql_close(); ricevendo l'errore:

Notice: Undefined variable: db in /membri2/furion/changepassword.class.php on line 126

Warning: mysql_close() expects parameter 1 to be resource, null given in /membri2/furion/changepassword.class.php on line 126

anche questo perchè avviene?

P.S: infine ho notato una cosa strana, premetto che questi script sono hostati su altervista, detto questo nel file db_config.php che contiene le info per collegarsi al db, la password era errata ma lo script riusciva lo stesso a collegarsi al db e modificare i dati...come è possibile una cosa del genere?

Grazie anticipatamente!

[Alhazred]

Visto che hai molte richieste, facciamo una cosa alla volta, intanto leggi questo.

Prima di tutto rivedrei un po' di cose nel codice per mantenere un approccio object oriented:

1) assegnerei alla classe 4 attributi, uno per password e l'id di chi chiede il cambio (leggendo vedrai a cosa serve) da inizializzare con il costruttore

class ModifyPassword
{
    private $old_pass = '';
    private $new_pass1 = '';
    private $new_pass2 = '';
    private $userid = '';

    public function __contruct($p1,$p2,$p3,$id)
    {
        $this->old_pass = $p1;
        $this->new_pass1 = $p2;
        $this->new_pass2 = $p3;
        $this->userid = $id
    }
    //...
}

//prepara i dati con cui chiamare la classe, anche con altri controlli se necessario
$pass1 = trim(mysql_real_escape_string($_POST['oldpassword']));
$pass2 = trim(mysql_real_escape_string($_POST['password']));
$pass3 = trim(mysql_real_escape_string($_POST['password2']));
$userid = //boh, non lo so come è fatta la tua applicazione, ma deve essere l'id di chi chiede il cambio della password

//per chiamare la classe
$modifica_password = new ModifyPassword($pass1,$pass2,$pass3,$userid); 

2) ora in tutte le funzioni devi usare $this->... invece di $_POST['...']
per esempio

protected function VerifyPassword()
{
    if($this->new_pass1 == $this->new_pass2)
    {
        return TRUE;
    }
    else
    {
        return FALSE;
    }
} 

3) la funzione isEmptyField() diventa

protected function IsEmptyField()
{
    if($this->old_pass == '' OR $this->new_pass1 == '' OR $new_pass2 == '')
    {
        return TRUE;
    }
    else
    {
        return FALSE;
    }
} 

4) La funzione passwordExists() come l'hai fatta tu penso che non funzioni correttamente.
Supponi che io voglia cambiare la mia vecchia password che è pippo1, che digitandola mi sbagli e scriva poppo1. Invio la richiesta e la cosa giusta sarebbe che la tua funzione mi restituisca FALSE, giusto?
Che succede invece se un altro utente come password usa poppo1???
La tua funzione mi fa cambiare la password pensando che la vecchia sia corretta, perché poppo1 da qualche parte la trova.
Quindi nella clausola where dovresti anche passarci l'id o l'username (se univoco) dell'utente che richiede il cambio della password, così hai la certezza che se viene trovata, appartiene a chi sta chiedendo il cambio e non ad un altro utente.

protected function PasswordExists()
{
    $sql = "SELECT password FROM users WHERE password='$this->old_pass' AND id=$this->userid";
    $res = mysql_query($sql);

    if(mysql_num_rows($res) == 1)
    {  
        return TRUE;
    }
    else
    {   
        return FALSE;
    }
}

//anche nella funzione che cambia la password sul db hai lo stesso
//problema, devi aggiungere l'id o l'username dell'utente, altrimenti
//se 2 o più utenti usano una password uguale, la cambi a tutti 

Apportando le modifiche del caso alla tua classe secondo le indicazioni che ti ho dato, vedi com'è la situazione e proponi i problemi che sono rimasti.

[furion89]

Prima di tutto ti ringrazio per la risposta Poi passando al dunque:

- Si uso l'id dell'user nelle query ma per prova ho rimosso quella parte e ho provato lo script solo su una riga del db.

- Ho cercato di seguire quello che mi hai detto ma non mi sono ben chiare alcune cose quindi sono andato ad intuito:

il file changepassword.class.php l'ho cambiato così (per comodità ho assegnato io manualmente l'id):

changepassword.class.php

<?php
error_reporting(E_ALL);

require('administrator/db_config.php');
 
class ModifyPassword
{

           private $old_pass = ''; 
            private $new_pass1 = ''; 
            private $new_pass2 = ''; 
            private $userid = ''; 

            public function __construct($p1,$p2,$p3,$id) 
            { 
                $this->old_pass = $p1; 
                $this->new_pass1 = $p2; 
                $this->new_pass2 = $p3; 
                $this->userid = $id;
            }

        public function ChangePassword()
        {
            $this->ErrorReport();
        }

        protected function IsEmptyField()
        {
            if($this->old_pass == '' OR $this->new_pass1 == '' OR $new_pass2 == '')
            {
                return TRUE;
            }
            else
            {
                return FALSE;
            }
        }

        protected function VerifyPassword()
        {
            if($this->new_pass1 == $this->new_pass2)
            {
                return TRUE;
            }
            else
            {
                return FALSE;
            }
        }

        protected function VerifyLengthPassword()
        {
            if(strlen($this->new_pass2) >= 8 && strlen($this->new_pass2) <= 15)
            {
                return TRUE;
            }
            else
            {
                return FALSE;
            }
        }

        protected function VerifyAlphanumericPassword()
        {

                        if(preg_match('/^[a-zA-Z0-9]+$/', $this->new_pass2))
            {
                return TRUE;
            }
            else
            {
                return FALSE;
            }
        }

        protected function PasswordExists()
        {
            $sql = "SELECT password FROM users WHERE password='$this->old_pass' AND id='$this->userid'";
            $res = mysql_query($sql);
            if($row = mysql_fetch_array($res))
            {  
                return TRUE;
            }
            else
            {   
                return FALSE;
            }
        }


        public function ErrorResult($num)
        {
            header("Location: change_password.php?alert=" . $num);
            die;
        }
 
        protected function ErrorReport()
        {
            if($this->IsEmptyField())
            {
                $this->ErrorResult(1);
            }
 
            if(!$this->VerifyPassword())
            {
                $this->ErrorResult(2);
            }

            if(!$this->VerifyLengthPassword())
            {
                $this->ErrorResult(3);
            }

            if(!$this->VerifyAlphanumericPassword())
            {
                $this->ErrorResult(4);
            }
 
            if(!$this->PasswordExists())
            {
                $this->ErrorResult(5);
            }


            $this->ChangePasswordUser();
 
        }

        protected function ChangePasswordUser()
        {
            $sql = "UPDATE users SET password = '$this->new_pass1' WHERE password='$this->old_pass' AND id='$this->userid'";

            $res = mysql_query($sql);
                        mysql_close();

                        if($res === TRUE)
                {
                        die ('Password aggiornata con successo!');
                }
                else
                {
                        die ("Errore con l'\aggiornamento: " . mysql_error());
                }

        }
}        

?>

e il file modify_password.php così (presumo che i controlli sui campi non vadano qui )

modify_password.php

<?php
 
include 'changepassword.class.php';
 
$obj = new ModifyPassword($pass1,$pass2,$pass3,$userid);
$obj->ChangePassword();

            $pass1 = trim(mysql_real_escape_string($_POST['oldpassword'])); 
            $pass2 = trim(mysql_real_escape_string($_POST['password'])); 
            $pass3 = trim(mysql_real_escape_string($_POST['password2'])); 
            $userid = 6;

?>

Inoltre non sono sicuro ma questo:

            public function __construct($p1,$p2,$p3,$id) 
            { 
                $this->old_pass = $p1; 
                $this->new_pass1 = $p2; 
                $this->new_pass2 = $p3; 
                $this->userid = $id;
            } 

non dovrebbe essere così:

            public function __construct($pass1,$pass2,$pass3,$userid) 
            { 
                $this->old_pass = $pass1; 
                $this->new_pass1 = $pass2; 
                $this->new_pass2 = $pass3; 
                $this->userid = $userid;
            } 

può essere anche che mi sbagli io vista l'ora

[Alhazred]

La classe così ad occhio sembrerebbe a posto.

Originariamente inviato da furion89
il file modify_password.php così (presumo che i controlli sui campi non vadano qui )
modify_password.php

<?php
 
include 'changepassword.class.php';
 
$obj = new ModifyPassword($pass1,$pass2,$pass3,$userid);
$obj->ChangePassword();

$pass1 = trim(mysql_real_escape_string($_POST['oldpassword'])); 
$pass2 = trim(mysql_real_escape_string($_POST['password'])); 
$pass3 = trim(mysql_real_escape_string($_POST['password2'])); 
$userid = 6;

?>

I controlli li devi fare prima di chiamare la classe, farlo dopo non serve a niente perché ormai i dati alla classe li hai passati.

Inoltre non sono sicuro ma questo:

            public function __construct($p1,$p2,$p3,$id) 
            { 
                $this->old_pass = $p1; 
                $this->new_pass1 = $p2; 
                $this->new_pass2 = $p3; 
                $this->userid = $id;
            } 

non dovrebbe essere così:

            public function __construct($pass1,$pass2,$pass3,$userid) 
            { 
                $this->old_pass = $pass1; 
                $this->new_pass1 = $pass2; 
                $this->new_pass2 = $pass3; 
                $this->userid = $userid;
            } 

No, va bene la prima versione.
In realtà vanno bene entrambe, ma la seconda a chi legge il codice potrebbe generare dubbi perché i nomi delle variabili sono gli stessi di quelle definite dentro la classe (che sono vuote), ma il loro contenuto è invece quello che arriva dalla chiamata.
In pratica $pass1,$pass2,$pass3,$userid dentro a __construct() non sono le stesse variabili definite come private dentro la classe, sono un'altra cosa, quindi meglio chiamarle in un altro modo.

[furion89]

E allora deve esserci un problema perché se setto così come mi hai detto tu lo script si ferma al primo errore "Tutti i campo devono essere compilati" pur essendo tutti settati
Se escludo quel controllo lo script si blocca a "La password deve essere tra 8 e 15 caratteri."...a questo punto presumo che le variabili rimangano vuote