was wondering if there is anything wrong with this code it works fine but i wanna know if there is anything experts or pro would do diff y and what this is just the class that does the processing ect. im gonna write another page to filter input ect before it sends the information here
$conn=new pdo("mysql:host=".host.";dbname=".database_name.";charset=utf8",username,password);
}
catch(pdoexception $e)
{
echo"sorry the connection to database has failed ".$e;
}
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Untitled Document</title>
</head>
<body>
<?php
session_start();
require ("config.php");
class users
{
public $uid;
public $u_name;
protected $u_password;
protected $u_info;
protected $u_salt;
public function login ($u_name,$u_password)
{
$now=time();
global $conn;
$this->getsalt($u_name);
$stmnt=$conn->prepare("select*from users where name=:name and pass=ass");
$stmnt->execute(array(":name"=>$u_name,"ass"=>sha1($u_password.$this->u_salt)));
$result=$stmnt->fetch(pdo::FETCH_OBJ);
if(!$result==null)
{
$this->uid=$result->id;
$this->u_name=$result->name;
$this->u_password=$result->pass;
$this->u_info=$result;
if($this->brute_check($result->id)==false)
{
$_session['u_name']=$this->u_name;//registers a session if all checks out
//deletes the previously failed loggin attempts from table
$delete_prev_failed_attempts=$conn->prepare("delete from login_attempts where u_id=:id");
$delete_prev_failed_attempts->execute(array(":id"=>$this->uid));
echo"<p>login sucessfull</p><br/>ect......whatever page xanda chooses them to see";
}
else
{
echo"user account locked for the next hour";
}
}
else
{
$query=$conn->prepare("select*from users where name=:u_name");
$query->execute(array(":u_name"=>$u_name));
$name=$query->fetch(PDO::FETCH_OBJ);
if($name==null)
{
die("<p>Username or password is incorrect........ill bring you back to the login page or ask alex to register you</p>");
}
$id=$name->id;
$insert=$conn->prepare("insert into login_attempts(u_id,time) values(:id,:time)");
$insert->execute(array(":id"=>$id,":time"=>$now));
if($this->brute_check($id)==true)
{
echo"user account locked for the next hour";
}
else
{
echo"<p>Username or Password is incorrect........ill bring you back to the login page or ask alex to register you</p>";
}
}
}
//function to check amount of login attempts with a hour time period refrence table login attemts
public function brute_check($id)
{
global $conn;
$now=time();
$hour_ago=$now-(1*60*60);
$stmnt=$conn->prepare("select time from login_attempts where u_id=:uid and time>=ast1_hour");
$stmnt->execute(array(":uid"=>$id,"ast1_hour"=>$hour_ago));
$rows=$stmnt->rowcount();
if($rows>5)
return true;
else
return false;
}
/*this is the login function note to try create a new user ect. ect. via certain things being true*/
public function register($req_name,$req_pass)
{
global $conn;
$time=time();
$salt=$time;
$this->u_salt=$salt;
$pass=$this->hash_pass($req_pass,$salt);
$check=$conn->prepare("select*from users where name=:req_name");
$check->execute(array(":req_name"=>$req_name));
$rows_check=$check->rowcount();
if(!$rows_check==null)
{
echo"username already taken";
return false;
}
else
{
try{
$insert=$conn->prepare("insert into users(name,pass,salt) values(:req_name,:req_pass,:salt)");
$insert->execute(array(":req_name"=>$req_name,":req_pass"=>$pass,":salt"=>$salt));
echo"user created you may now login";
return true;
}
catch(pdoexception $e)
{
echo"error ".$e;
}
}
}
//function to hash passwords
public function hash_pass($req_pass,$salt)
{
$hashed_pass=sha1($req_pass.$salt);
return $hashed_pass;
}
/*im so soryy if this code is cumbersome the point of good code is to be easily understood
This function Gets the salt from the username and also varifies the user exists*/
public function getsalt($u_name)
{
global $conn;
$get_salt=$conn->prepare("select name,salt from users where name=:name");
$get_salt->execute(array(":name"=>$u_name));
$result=$get_salt->fetch(PDO::FETCH_OBJ);
if($get_salt->rowcount()>0)
{
$this->u_salt=$result->salt;
return true;
}
}
lol the editer here posted my named place holers as smilies but i think you all should get it as said it just does the processing all filtering ect will be done on a layer of php before.and also am useing phpmyadmin to create tables ect.the tables are connected and i notice if i tried to alter a feild thats connected inwards it dosnt allow makes sense but what do i do if a user says need his name changed i would have to delete all the data under that users name in forighn tables comments and advice please
If you post your code withing this forum's [php]...[/php] bbcode tags, it will be much easier for us to read (including keeping indentation, not displaying smilies, and even including syntax highlighting).
"Please give us a simple answer, so that we don't have to think, because if we think, we might find answers that don't fit the way we want the world to be."
~ Terry Pratchett in Nation
NogDog: Did you mean to include the "not" before disabling smilies? Disabling them seems like a good thing when posting code :P
I believe I typed "not displaying" versus "not disabling", unless your browser is displaying (or disabling?) something else.
However, I did mean to type "within" instead of "withing", but sometimes my fingers have a mind of their own.
"Please give us a simple answer, so that we don't have to think, because if we think, we might find answers that don't fit the way we want the world to be."
~ Terry Pratchett in Nation
$conn=new pdo("mysql:host=".host.";dbname=".database_name.";charset=utf8",username,password);
}
catch(pdoexception $e)
{
echo"sorry the connection to database has failed ".$e;
}
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Untitled Document</title>
</head>
<body>
<?php
session_start();
require ("config.php");
class users
{
public $uid;
public $u_name;
protected $u_password;
protected $u_info;
protected $u_salt;
public function login ($u_name,$u_password)
{
$now=time();
global $conn;
$this->getsalt($u_name);
$stmnt=$conn->prepare("select*from users where name=:name and pass=ass");
$stmnt->execute(array(":name"=>$u_name,"ass"=>sha1($u_password.$this->u_salt)));
$result=$stmnt->fetch(pdo::FETCH_OBJ);
if(!$result==null)
{
$this->uid=$result->id;
$this->u_name=$result->name;
$this->u_password=$result->pass;
$this->u_info=$result;
if($this->brute_check($result->id)==false)
{
$_session['u_name']=$this->u_name;//registers a session if all checks out
//deletes the previously failed loggin attempts from table
$delete_prev_failed_attempts=$conn->prepare("delete from login_attempts where u_id=:id");
$delete_prev_failed_attempts->execute(array(":id"=>$this->uid));
echo"<p>login sucessfull</p><br/>ect......whatever page xanda chooses them to see";
}
else
{
echo"user account locked for the next hour";
}
}
else
{
$query=$conn->prepare("select*from users where name=:u_name");
$query->execute(array(":u_name"=>$u_name));
$name=$query->fetch(PDO::FETCH_OBJ);
if($name==null)
{
die("<p>Username or password is incorrect........ill bring you back to the login page or ask alex to register you</p>");
}
$id=$name->id;
$insert=$conn->prepare("insert into login_attempts(u_id,time) values(:id,:time)");
$insert->execute(array(":id"=>$id,":time"=>$now));
if($this->brute_check($id)==true)
{
echo"user account locked for the next hour";
}
else
{
echo"<p>Username or Password is incorrect........ill bring you back to the login page or ask alex to register you</p>";
}
}
}
//function to check amount of login attempts with a hour time period refrence table login attemts
public function brute_check($id)
{
global $conn;
$now=time();
$hour_ago=$now-(1*60*60);
$stmnt=$conn->prepare("select time from login_attempts where u_id=:uid and time>=ast1_hour");
$stmnt->execute(array(":uid"=>$id,"ast1_hour"=>$hour_ago));
$rows=$stmnt->rowcount();
if($rows>5)
return true;
else
return false;
}
/*this is the login function note to try create a new user ect. ect. via certain things being true*/
public function register($req_name,$req_pass)
{
global $conn;
$time=time();
$salt=$time;
$this->u_salt=$salt;
$pass=$this->hash_pass($req_pass,$salt);
$check=$conn->prepare("select*from users where name=:req_name");
$check->execute(array(":req_name"=>$req_name));
$rows_check=$check->rowcount();
if(!$rows_check==null)
{
echo"username already taken";
return false;
}
else
{
try{
$insert=$conn->prepare("insert into users(name,pass,salt) values(:req_name,:req_pass,:salt)");
$insert->execute(array(":req_name"=>$req_name,":req_pass"=>$pass,":salt"=>$salt));
echo"user created you may now login";
return true;
}
catch(pdoexception $e)
{
echo"error ".$e;
}
}
}
//function to hash passwords
public function hash_pass($req_pass,$salt)
{
$hashed_pass=sha1($req_pass.$salt);
return $hashed_pass;
}
/*im so soryy if this code is cumbersome the point of good code is to be easily understood
This function Gets the salt from the username and also varifies the user exists*/
public function getsalt($u_name)
{
global $conn;
$get_salt=$conn->prepare("select name,salt from users where name=:name");
$get_salt->execute(array(":name"=>$u_name));
$result=$get_salt->fetch(PDO::FETCH_OBJ);
if($get_salt->rowcount()>0)
{
$this->u_salt=$result->salt;
return true;
}
}
Actually, this code is not good for many reasons (some of them are the following):
- No html-php separation
- You rely on globals for db connection
- Echo usage inside the class methods
- You use $_SESSION directly in your class
- You use die inside you class
- You should stop the script's execution, using exit or die, on database connection failure
- You should prefer private scope for variables and methods instead of public or protected. Use protected when your class will be inherited by another class and public when you need to access a method from code outside of the class
Actually, this code is not good for many reasons (some of them are the following):
- No html-php separation
- You rely on globals for db connection
- Echo usage inside the class methods
- You use $_SESSION directly in your class
- You use die inside you class
- You should stop the script's execution, using exit or die, on database connection failure
- You should prefer private scope for variables and methods instead of public or protected. Use protected when your class will be inherited by another class and public when you need to access a method from code outside of the class
firstly thank you for your reply and sorry for assuming you would know i actually sace the db connection and its test in config.php and that is why i refer to the connection as a global and i found this the best method for me.
also the echoes you see are stricly for my purposes they will be replaced with redirections.
also i didnt see the need to filter the session in the class seeing as now input from the user reaches that part of the code it is used for was i wrong?
and thanks for the tips but i have sepeerated my html do you see anything that is to be displad on the web page their?
for the protected stuff im new to this so just trying to play it a little safe will do.
any other things you said their were a few more?
and how do you suggest i optimpize?
i can go through and clean it up
As I can see, your html and php code are in the same file. You should move all classes to their own files (e.g. user.php for class User).
Globals are the easy way, but not the right way. Each class should be independent from globals. A better way to use the db connection inside your class is the use of dependency injection. For example:
PHP Code:
// constructor injection
public function __construct(PDO $db)
{
$this->db = $db;
}
or
PHP Code:
// method injection
public function setDatabase(PDO $db)
{
$this->db = $db;
}
You shouldn't use echo, exit, die or redirects inside your class. It is better to return a value from methods and use it inside your controllers to do redirect or whatever you want.
The same applies to the $_SESSION use. What will happen if oneday you decide not to use sessions?
Generally, every class should be as much independent as it can be.
ps. $_POST['op'] could throw a notice if op does not exist in $_POST array. A better way to get variables from arrays is the use of empty or isset. e.g.
PHP Code:
if (isset($_POST['op']) && $_POST['op'] == 'login')
As I can see, your html and php code are in the same file. You should move all classes to their own files (e.g. user.php for class User).
Globals are the easy way, but not the right way. Each class should be independent from globals. A better way to use the db connection inside your class is the use of dependency injection. For example:
PHP Code:
// constructor injection public function __construct(PDO $db) { $this->db = $db; }
or
PHP Code:
// method injection public function setDatabase(PDO $db) { $this->db = $db; }
You shouldn't use echo, exit, die or redirects inside your class. It is better to return a value from methods and use it inside your controllers to do redirect or whatever you want.
The same applies to the $_SESSION use. What will happen if oneday you decide not to use sessions?
Generally, every class should be as much independent as it can be.
ps. $_POST['op'] could throw a notice if op does not exist in $_POST array. A better way to get variables from arrays is the use of empty or isset. e.g.
PHP Code:
if (isset($_POST['op']) && $_POST['op'] == 'login')
thanks for the info.
i agree with you for all except that for the the dependensy in this case as i relly saw the benifits of using the connection as a global in a seperate config file when i had to move from my test local host to a remote test server.
dreamweaver sets up the page like that when you ask it for a php file so i assumed it was ok to leave those html tags ill remove them.
thanks for all your help could you give an example of how you would use the session as i also dont see the bad of if i had to change from the sessions are you saying where i initialised my session i should have captured it in say a variable?
The first half of the article From Flat PHP to Symfony2 could help you. It takes you step by step from the kind of code you have now to a more structured and organized architecture.
ass");
Reply With Quote
Originally Posted by evenstar7139
Bookmarks