Forum d'entraide PHPFrance

hello,

je voulais savoir si mon code etait bon au niveau de la securisation lors du login
par ailleurs: je dois faire un mysql_close a la fin de chaque "case" ?

desole si le code est un peu long...

merci

<?php

// we must never forget to start the session
session_start();

$errorMessage = '';
if (isset($_POST['email']) && isset($_POST['password'])) 
   {
   include '../includes/config.php';

   $email_temp = $_POST['email'];
   $password_temp = $_POST['password'];
   
   $email = mysql_real_escape_string(htmlentities($email_temp, ENT_QUOTES));
   $password = mysql_real_escape_string(htmlentities($password_temp, ENT_QUOTES)); 

// check if the user id and password combination exist in database
   $query = "SELECT * FROM clients WHERE email = '$email' AND password = md5('$password')";
   $result = mysql_query($query) or die('Query failed. ' . mysql_error());

// update date of last visit
	$time = time();
	$query2 = (" UPDATE clients SET last_visit = $time WHERE email = '$email' AND password = md5('$password') ");
	mysql_query($query2) or die('Query failed. ' . mysql_error());

//store INSERT query info into an array, that will be useful to initiate the LEVEL difference between users and admin
   $data = mysql_fetch_assoc($result); 
   
// the user id and password match,  
   if (mysql_num_rows($result) == 1) 
   {
      
      // set the session
      $_SESSION['in'] = true;
	  $_SESSION['id_clients'] = $data["id_clients"];
	  $_SESSION['level'] = $data["level"];
	  $_SESSION['fname'] = $data["fname"];
	  $_SESSION['points'] = $data["points"];
	  
	  //check if recommended user
	  $referral = $data["referral"];
	  $confirmation = $data["confirmation"];
	  
	  //$referral = implode("",$referral_temp);
	  //$confirmation = implode("",$confirmation_temp);
	  
	  //check that client has been recommended + that points have not been attributed to the referring client
	  if($referral !== "0" && $confirmation == "0")
	  {
	  $query = "SELECT id_clients FROM grids_clients WHERE id_clients = '$_SESSION[id_clients]'";
	  $result = mysql_query($query) or die('Query failed. ' . mysql_error());
	  
		  if (mysql_num_rows($result) > 1)
		  {
		  $query = "UPDATE clients SET confirmation = 1 WHERE id_clients = '$_SESSION[id_clients]'";
		  mysql_query($query) or die ('Invalid query: ' . mysql_error());
		  
		  //give away 1000 points per registered user
		  $query = "UPDATE clients SET points = points + 1000 WHERE id_clients = '$referral'"; 
		  mysql_query($query) or die ('Invalid query: ' . mysql_error());
		  }	  
	  }	  	  
      switch($data["level"])
	  {
	  case "0":
	  //user mode
	  
	  //update visit counter
	  $query = "UPDATE clients SET counter = counter + 1 WHERE email = '$email' AND password = md5('$password')"; 
	  mysql_query($query) or die ('Invalid query: ' . mysql_error());
	  
	  
      header('Location: ../index4.php');
      break;
	  
	  //VIP mode
	  case "1":
	  
	  //update visit counter
	  $query = "UPDATE clients SET counter = counter + 1 WHERE email = '$email' AND password = md5('$password')"; 
	  mysql_query($query) or die ('Invalid query: ' . mysql_error());
	  
	  header('Location: ../index4.php');
      break;
	  
	  //GOD mode
	  case "2":
	  
	  //update visit counter
	  $query = "UPDATE clients SET counter = counter + 1 WHERE email = '$email' AND password = md5('$password')"; 
	  mysql_query($query) or die ('Invalid query: ' . mysql_error());	  
	  
	  header('Location: ../index4.php');
	  }
	  
   } 
   else 
   {
      session_start();
	  $_SESSION['error'] = "2";
	  header("Location: ../errors/error.php");
   }
   mysql_close();
}
?>

Bah à priori ca m'a l'air bon... je pourrais chipoter un peu pour dire qu'il vaut mieux sortir les variables des chaines (cf. requêtes), que tu n'a pas besoin de mettre de guillemets autour d'un nombre (sauf si tu veux absolument le transformer en chaine) et que dès le moment où tu fais un session_start() dès le début du code, il est inutile de le remettre dans le else

Pour ton switch, je n'en vois pas l'intérêt si tes 3 cases sont scrupuleusement identiques. Autant extraire les portions communes ou virer le switch s'il ne te sert pas plus que ça

Une habitude à prendre par ailleurs, c'est de toujours coller un exit() après un header(). En effet, le header envoi l'information au navigateur d'une redirection mais n'interrompt pas l'exécution du code. Du coup, malgré une redirection, il peut continuer un traitement et modifier des valeurs en session sans que tu comprennes pourquoi

Quant au mysql_close(), il sera fait automatiquement à la fin de l'exécution (donc si tu colles un exit() entre autre). A toi de voir du coup s'il est utile de le faire sachant que le serveur s'en chargera à la fin du script. Personnellement je ne l'utilise que pour libérer des ressources quand je sais que le script ne se terminera pas tout de suite

merci pour ces conseils!
je mets en applcaition de suite

pour les "cases" dans le switch ils sont cependant justifies car j'ai d'autres requetes a faire (qui ne sont pas encore dans ce script)

merci!!