Page 1 of 2
Check my Code
Posted: Fri Dec 09, 2011 7:25 am
by ibnclaudius
I am new to PHP.
I developed this class, I wonder if there's anything "wrong" or that I can improve.
Thanks in advance.
Code: Select all
<?php
class network {
public $userID;
public $schoolID;
public $userEnrollment;
public $userName;
public $userPass;
public $dbHost;
public $dbUser;
public $dbName;
public $dbPass;
public $dbUserTable;
public $dbSchoolTable;
function dbInfo() {
$this->dbHost = 'localhost';
$this->dbUser = '';
$this->dbPass = '';
$this->dbName = '';
$this->dbUserTable = '';
$this->dbSchoolTable = '';
}
function registerUser($userEnrollment, $userName, $userPass) {
$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);
if(!$dbLink) die("Could not connect to database: " . mysql_error());
mysql_select_db($this->dbName);
$query = "INSERT INTO $this->dbUserTable VALUES (NULL, \"$userEnrollment\", \"$userName\", \"$userPass\")";
$result = mysql_query($query);
if(!$result) {
echo "Fail.";
} else {
$this->userID = mysql_insert_id();
}
mysql_close($dbLink);
$this->userName = $userName;
$this->userPass = $userPass;
}
function registerSchool($schoolName) {
$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);
if(!$dbLink) die("Could not connect to database: " . mysql_error());
mysql_select_db($this->dbName);
$query = "INSERT INTO $this->dbSchoolTable VALUES (NULL, \"$schoolName\")";
$result = mysql_query($query);
if(!$result) {
echo "Fail.";
} else {
$this->schoolID = mysql_insert_id();
}
mysql_close($dbLink);
$this->schoolName = $schoolName;
}
function userLogin() {
$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);
if(!$dbLink) die("Could not connect to database: " . mysql_error());
mysql_select_db($this->dbName);
$query = "SELECT * FROM $this->dbUserTable WHERE userEnrollment = \"$this->userEnrollment\" AND userPass = \"$this->userPass\" LIMIT 1";
$result = mysql_query($query);
if(!$result) {
echo "Fail.";
} else {
$row = mysql_fetch_array($result);
session_regenerate_id();
$_SESSION['userEnrollment'] = $this->userEnrollment;
session_write_close();
}
mysql_close($dbLink);
}
function changePass($newPass) {
$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);
if(!$dbLink) die("Could not connect to database: " . mysql_error());
mysql_select_db($this->dbName);
$query = "SELECT * FROM $this->dbUserTable WHERE userName = \"$this->userName\" LIMIT 1";
$result = mysql_query($query);
if(!$result) {
echo "Fail.";
} else {
$query = "UPDATE $this->dbUserTable SET userPass = \"$newPass\" WHERE userName = \"$this->userName\"";
$result = mysql_query($query);
if(!$result) {
echo "Fail";
} else {
$this->userPass = $newPass;
}
}
mysql_close($dbLink);
}
}
?>
Re: Check my Code
Posted: Fri Dec 09, 2011 7:45 am
by phphelpme
Just having a quick browse over it seems ok to me. Have you tested it yet and any error messages appear.

Re: Check my Code
Posted: Fri Dec 09, 2011 8:14 am
by social_experiment
Database related variables can be declared
protected because you don't want them to be changed from outside the class.
Code: Select all
<?php
function dbInfo() {
$this->dbHost = 'localhost';
$this->dbUser = '';
$this->dbPass = '';
$this->dbName = '';
$this->dbUserTable = '';
$this->dbSchoolTable = '';
}
// try this
public function __construct($host, $user, $pass, $db)
{
$this->dbHost = $host;
$this->dbUser = $user;
$this->dbPass = $pass;
$this->dbName = $db;
}
?>
Do this part inside a constructor and when you instantiate the class you give the values like this
Code: Select all
<?php $obj = new network($host, $user, $pass, $db); ?>
There aren't any escaping for data passed to the database (that i can see); never trust user input; even if you know the source.
Code: Select all
<?php
$query = "SELECT * FROM $this->dbUserTable WHERE userEnrollment = \"$this->userEnrollment\" AND userPass = \"$this->userPass\" LIMIT 1";
?>
If there is proper checking for duplicate values you can remove the LIMIT 1 from this query. Duplicate values shouldn't be allowed in such instances.
Re: Check my Code
Posted: Fri Dec 09, 2011 8:32 am
by Celauran
I see SELECT *, I see mysql_ functions. These are both better avoided. Select the columns you need. Use mysqli or PDO. Passwords aren't being hashed, that's also a problem.
Re: Check my Code
Posted: Fri Dec 09, 2011 9:48 am
by phphelpme
Typical I was just looking for errors a but actually what you both have said could constitute as minor errors on the security side of things.

Re: Check my Code
Posted: Fri Dec 09, 2011 10:41 am
by ibnclaudius
Here is my code...
login.php
Code: Select all
<form method="post" action="ajax_login.php">
Matrícula: <input type="text" name="userEnrollment" maxlength="32"><br>
Senha: <input type="password" name="userPass" maxlength="32"><br>
<input type="submit">
</form>
ajax_login.php
Code: Select all
<?php
session_start();
include 'class/network.php';
$D = new network($dbHost, $dbUser, $dbPass, $dbName);
$D->userEnrollment = mysql_real_escape_string($_POST['userEnrollment']);
$D->userPass = hash('sha512', $_POST['userPass']);
$D->userLogin();
echo $_SESSION['userEnrollment'];
?>
class/network.php
Code: Select all
<?php
class network {
public $userID;
public $schoolID;
public $userEnrollment;
public $userName;
public $userPass;
public $dbHost;
public $dbUser;
public $dbName;
public $dbPass;
public $dbUserTable;
public $dbSchoolTable;
/*
function dbInfo() {
$this->dbHost = 'localhost';
$this->dbUser = '';
$this->dbPass = '';
$this->dbName = '';
$this->dbUserTable = '';
$this->dbSchoolTable = '';
}
*/
public function __construct($dbHost, $dbUser, $dbPass, $dbName) {
$this->dbHost = $dbHhost;
$this->dbUser = $dbUser;
$this->dbPass = $dbPass;
$this->dbName = $dbName;
$this->dbUserTable = $dbUserTable;
$this->dbSchoolTable = $dbSchoolTable;
}
function registerUser($userEnrollment, $userName, $userPass) {
$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);
if(!$dbLink) die("Could not connect to database: " . mysql_error());
mysql_select_db($this->dbName);
$query = "INSERT INTO $this->dbUserTable VALUES (NULL, \"$userEnrollment\", \"$userName\", \"$userPass\")";
$result = mysql_query($query);
if(!$result) {
echo "Fail.";
} else {
$this->userID = mysql_insert_id();
}
mysql_close($dbLink);
$this->userName = $userName;
$this->userPass = $userPass;
}
function registerSchool($schoolName) {
$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);
if(!$dbLink) die("Could not connect to database: " . mysql_error());
mysql_select_db($this->dbName);
$query = "INSERT INTO $this->dbSchoolTable VALUES (NULL, \"$schoolName\")";
$result = mysql_query($query);
if(!$result) {
echo "Fail.";
} else {
$this->schoolID = mysql_insert_id();
}
mysql_close($dbLink);
$this->schoolName = $schoolName;
}
function userLogin() {
$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);
if(!$dbLink) die("Could not connect to database: " . mysql_error());
mysql_select_db($this->dbName);
$query = "SELECT * FROM $this->dbUserTable WHERE userEnrollment = \"$this->userEnrollment\" AND userPass = \"$this->userPass\" LIMIT 1";
$result = mysql_query($query);
if(!$result) {
echo "Fail.";
} else {
$row = mysql_fetch_array($result);
session_regenerate_id();
$_SESSION['userEnrollment'] = $this->userEnrollment;
session_write_close();
}
mysql_close($dbLink);
}
function changePass($newPass) {
$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);
if(!$dbLink) die("Could not connect to database: " . mysql_error());
mysql_select_db($this->dbName);
$query = "SELECT * FROM $this->dbUserTable WHERE userName = \"$this->userName\" LIMIT 1";
$result = mysql_query($query);
if(!$result) {
echo "Fail.";
} else {
$query = "UPDATE $this->dbUserTable SET userPass = \"$newPass\" WHERE userName = \"$this->userName\"";
$result = mysql_query($query);
if(!$result) {
echo "Fail";
} else {
$this->userPass = $newPass;
}
}
mysql_close($dbLink);
}
}
?>
What should I change? What is "wrong"? What should I improve? I'm newbie.

Re: Check my Code
Posted: Fri Dec 09, 2011 11:35 am
by Celauran
I've made a few changes and added some comments. This is still far from being perfect, but it should point you in the right direction.
Code: Select all
<?php
class network
{
public $userID;
public $schoolID;
public $userEnrollment;
public $userName;
public $dbUserTable;
public $dbSchoolTable;
protected $sql;
/**
* This doesn't really fit into this class. The constructor should create
* a new user, not a new DB connection IMO.
*/
public function __construct($dbHost, $dbUser, $dbPass, $dbName)
{
$dsn = "mysql:host={$dbHost};dbname={$dbName}";
try
{
$this->sql = new PDO($dsn, $dbUser, $dbPass);
}
catch (PDOException $e)
{
throw new Exceptopn($e->getMessage());
}
/**
* Where are these coming from? Since they're not passed into the
* constructor, they're currently NULL
* You'll run into the same problem with $userName and $userPass.
*/
$this->dbUserTable = $dbUserTable;
$this->dbSchoolTable = $dbSchoolTable;
}
public function registerUser($userEnrollment, $userName, $userPass)
{
$this->userName = $userName;
$hashedPass = $this->hashPassword($userPass);
$query = "INSERT INTO {$this->dbUserTable}
VALUES (NULL, :enrollment, :username, :password)";
$stmt = $this->sql->prepare($query);
$result = $stmt->execute(array(':enrollment' => $userEnrollment,
':username' => $userName,
':password' => $hashedPass));
return ($result === TRUE)
? $this->sql->lastInsertId()
: FALSE;
}
public function registerSchool($schoolName)
{
$this->schoolName = $schoolName;
$query = "INSERT INTO {$this->dbSchoolTable}
VALUES (NULL, :schoolName)";
$stmt = $this->sql->prepare($query);
$result = $stmt->execute(array(':schoolName' => $schoolName));
return ($result === TRUE)
? $this->sql->lastInsertId()
: FALSE;
}
/**
* I have no idea what's going on here. Login without username?
* This function takes no arguments, but there's nothing in the
* constructor about $userEnrollment, so it may well be NULL when this
* function is called. Ditto $userPass (though I don't think that should
* even exist). Finally, you're not doing anything with the query results,
* so what's the point of the query?
*/
public function userLogin()
{
$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);
if (!$dbLink)
die("Could not connect to database: " . mysql_error());
mysql_select_db($this->dbName);
$query = "SELECT * FROM $this->dbUserTable WHERE userEnrollment = \"$this->userEnrollment\" AND userPass = \"$this->userPass\" LIMIT 1";
$result = mysql_query($query);
if (!$result)
{
echo "Fail.";
}
else
{
$row = mysql_fetch_array($result);
session_regenerate_id();
$_SESSION['userEnrollment'] = $this->userEnrollment;
session_write_close();
}
mysql_close($dbLink);
}
public function changePass($newPass)
{
// I'm assuming this is just to ensure the user exists in the database
$query = "SELECT COUNT(*)
FROM {$this->dbUserTable}
WHERE userName = :username
LIMIT 1";
$stmt = $this->sql->prepare($query);
$result = $stmt->execute(array(':username' => $this->userName));
if (!$result)
{
throw new Exception('User does not exist.');
}
else
{
$hashedPass = $this->hashPassword($newPass);
$query = "UPDATE {$this->dbUserTable}
SET userPass = :password
WHERE userName = :username";
$stmt = $this->sql->prepare($query);
$result = $stmt->execute(array(':password' => $hashedPass,
':username' => $this->userName));
return ($result === TRUE)
? TRUE
: FALSE;
}
}
private function hashPassword($password)
{
$salt = "This shouldn't really be hard-coded into the function";
$hashed = crypt($password, '$2a$12$' . substr(md5($salt), 0, 22));
return $hashed;
}
}
?>
Re: Check my Code
Posted: Fri Dec 09, 2011 5:01 pm
by ibnclaudius
You really confused me.

There are a a lot of new lines and functions that I didn't understand or know. Could you please make more specific comments? Check the lines I got in trouble:
Code: Select all
$dsn = "mysql:host={$dbHost};dbname={$dbName}";
try
{
$this->sql = new PDO($dsn, $dbUser, $dbPass);
}
catch (PDOException $e)
{
throw new Exceptopn($e->getMessage());
}
Code: Select all
$query = "INSERT INTO {$this->dbUserTable}
VALUES (NULL, :enrollment, :username, :password)";
$stmt = $this->sql->prepare($query);
$result = $stmt->execute(array(':enrollment' => $userEnrollment,
':username' => $userName,
':password' => $hashedPass));
return ($result === TRUE) ? $this->sql->lastInsertId() : FALSE;
Code: Select all
$query = "SELECT COUNT(*)
FROM {$this->dbUserTable}
WHERE userName = :username
LIMIT 1";
Is it wrong to do this:
Code: Select all
if (!$result)
{
return FALSE;
}
else
{
$row = mysql_fetch_array($result);
session_regenerate_id();
$_SESSION['userEnrollment'] = $this->userEnrollment;
session_write_close();
}
When should I use variable inside ( ) of a function and when don't?
From where I call dbinfo? I'll have to make a config.php file?
Look at these files, am I doing it right?
login.php
Code: Select all
<form method="post" action="ajax_login.php">
Matrícula: <input type="text" name="userEnrollment" maxlength="32"><br>
Senha: <input type="password" name="userPass" maxlength="32"><br>
<input type="submit">
</form>
ajax_login.php
Code: Select all
<?php
session_start();
include 'class/network.php';
$D = new network();
$D->userEnrollment = mysql_real_escape_string($_POST['userEnrollment']);
$D->userPass = mysql_real_escape_string($_POST['userPass']);
$D->userLogin();
echo $_SESSION['userEnrollment'];
?>
I know it's "incorrect" to ask others to do the code, bud could you make the "correct" login system? Would be very helpful to understand how everything works, so I could do the other systems from your.
Thanks to all of you, you're helping me a lot.
Re: Check my Code
Posted: Fri Dec 09, 2011 7:14 pm
by Celauran
ibnclaudius wrote:You really confused me.

There are a a lot of new lines and functions that I didn't understand or know. Could you please make more specific comments? Check the lines I got in trouble:
Visibility
ibnclaudius wrote:Code: Select all
$dsn = "mysql:host={$dbHost};dbname={$dbName}";
try
{
$this->sql = new PDO($dsn, $dbUser, $dbPass);
}
catch (PDOException $e)
{
throw new Exceptopn($e->getMessage());
}
PDO
Exceptions
ibnclaudius wrote:Is it wrong to do this:
Code: Select all
if (!$result)
{
return FALSE;
}
else
{
$row = mysql_fetch_array($result);
session_regenerate_id();
$_SESSION['userEnrollment'] = $this->userEnrollment;
session_write_close();
}
I don't see what purpose it serves. You're not doing anything with the contents of $row, so why bother with the query?
ibnclaudius wrote:When should I use variable inside ( ) of a function and when don't?
Variable scope
Function arguments
ibnclaudius wrote:From where I call dbinfo? I'll have to make a config.php file?
Or another class. I'd use define() inside a config file, though.
ibnclaudius wrote:Look at these files, am I doing it right?
login.php
Code: Select all
<form method="post" action="ajax_login.php">
Matrícula: <input type="text" name="userEnrollment" maxlength="32"><br>
Senha: <input type="password" name="userPass" maxlength="32"><br>
<input type="submit">
</form>
This looks alright, though I'm unclear on what constitutes userEnrollment v. userName. Logins are generally done with username and password.
ibnclaudius wrote:ajax_login.php
Code: Select all
<?php
session_start();
include 'class/network.php';
$D = new network();
$D->userEnrollment = mysql_real_escape_string($_POST['userEnrollment']);
$D->userPass = mysql_real_escape_string($_POST['userPass']);
$D->userLogin();
echo $_SESSION['userEnrollment'];
?>
Your constructor requires four arguments but you're passing none. mysql_ functions should be avoided. There's no need for escaping when using prepared statements, either. I don't understand your userLogin() function at all.
Re: Check my Code
Posted: Fri Dec 09, 2011 7:29 pm
by ibnclaudius
Surely I will read all the articles, thank you.
In my case, userEnrollment is working like userName.
mysql_ functions should be avoided, so what should I use instead?
I just want to make a "nice" login form, but I'm confused about how to do this. Could you please show me a working example? It would help a lot!
Thanks, I'm learning a lot.
Re: Check my Code
Posted: Fri Dec 09, 2011 8:09 pm
by Celauran
ibnclaudius wrote:mysql_ functions should be avoided, so what should I use instead?
Either mysqli or PDO.
ibnclaudius wrote:I just want to make a "nice" login form, but I'm confused about how to do this. Could you please show me a working example? It would help a lot!
It's incomplete, but the basic functionality is there:
Code: Select all
<?php
class User
{
protected $id;
protected $username;
protected $email;
private $exists = FALSE;
protected $sql;
public function __construct($username)
{
if (empty($username))
{
throw new Exception('Username cannot be blank.');
}
$this->username = $username;
$this->sql = new PDO(DSN, DBUSER, DBPASS);
$this->exists = $this->validate();
}
private function createLoginToken($id)
{
$token = $id . md5(microtime());
$expires = new DateTime();
$expires->add(new DateInterval('P30D'));
$query = "INSERT INTO sessions (userID, token, expires)
VALUES (:id, :token, :expires)";
$stmt = $this->sql->prepare($query);
$stmt->execute(array(':id' => $id,
':token' => $token,
':expires' => $expires->format('Y-m-d H:i:s')));
setcookie('token', $token, $expires->getTimestamp(), '/');
}
private function hashPassword($password, $salt)
{
$string = PASSWORD_SALT . $password . md5($salt);
$hashed = crypt($string, '$2a$12$' . substr(md5($salt), 0, 22));
return $hashed;
}
private function validate()
{
$query = "SELECT COUNT(id)
FROM users
WHERE username = :username";
$stmt = $this->sql->prepare($query);
$stmt->execute(array(':username' => $this->username));
$count = $stmt->fetchColumn();
return ($count > 0) ? TRUE : FALSE;
}
public function exists()
{
return $this->exists;
}
public function login($password, $remember = FALSE)
{
$query = "SELECT id, password, UNIX_TIMESTAMP(created) AS salt
FROM users
WHERE username = :username";
$stmt = $this->sql->prepare($query);
$stmt->execute(array(':username' => $this->username));
$row = $stmt->fetch(PDO::FETCH_OBJ);
$hashed = $this->hashPassword($password, $row->salt);
if ($row->password == $hashed)
{
if ($remember)
{
$this->createLoginToken($row->id);
}
return $row->id;
}
return FALSE;
}
public function register($password, $email)
{
if (!filter_var($email, FILTER_VALIDATE_EMAIL))
{
throw new Exception('Email does not appear to be valid.');
}
$this->email = $email;
$date = new DateTime();
$hashed = $this->hashPassword($password, $date->getTimestamp());
$query = "INSERT INTO users (username, password, email, created)
VALUES (:username, :password, :email, :created)";
$stmt = $this->sql->prepare($query);
$success = $stmt->execute(array(':username' => $this->username,
':password' => $hashed,
':email' => $email,
':created' => $date->format('Y-m-d H:i:s')));
return ($success === TRUE)
? $this->sql->lastInsertId()
: FALSE;
}
public function verifyCookie($token)
{
$query = "SELECT userID
FROM sessions
WHERE token = :token
AND expires > NOW()";
$stmt = $this->sql->prepare($query);
$stmt->execute(array(':token' => $token));
return $stmt->fetchColumn();
}
}
?>
Here is a very crude login form:
Code: Select all
<?php
session_start();
if (isset($_POST['username']) && isset($_POST['password']))
{
$user = new User($_POST['username']);
if ($user->exists())
{
$login = $user->login($_POST['password']);
if ($login)
{
$_SESSION['user_id'] = $login;
}
else
{
echo "Login failed.";
}
}
else
{
header("Location: register.php");
}
}
?>
<!DOCTYPE html>
<html>
<head>
<title>Login Form</title>
</head>
<body>
<form action="" method="post">
<label for="username">Username: </label>
<input type="text" name="username" /><br />
<label for="password">Password: </label>
<input type="password" name="password" /><br />
<input type="submit" value="Submit" />
</form>
</body>
</html>
Re: Check my Code
Posted: Fri Dec 09, 2011 8:32 pm
by ibnclaudius
I already have some doubts, but I'll take a look at everything and then search, so I will pack all my questions and post here soon.
Shouldn't include the class in login form?
About the database, could you show me the "skeleton"?
Re: Check my Code
Posted: Fri Dec 09, 2011 8:52 pm
by Celauran
ibnclaudius wrote:About the database, could you show me the "skeleton"?
Code: Select all
mysql> DESCRIBE users;
+----------+--------------+------+-----+---------+----------------+
| Field | Type | Null | Key | Default | Extra |
+----------+--------------+------+-----+---------+----------------+
| id | int(11) | NO | PRI | NULL | auto_increment |
| username | varchar(30) | NO | UNI | NULL | |
| password | varchar(60) | NO | | NULL | |
| email | varchar(100) | NO | | NULL | |
| created | datetime | NO | | NULL | |
+----------+--------------+------+-----+---------+----------------+
mysql> DESCRIBE sessions;
+---------+-------------+------+-----+---------+----------------+
| Field | Type | Null | Key | Default | Extra |
+---------+-------------+------+-----+---------+----------------+
| id | int(11) | NO | PRI | NULL | auto_increment |
| userID | int(10) | NO | MUL | NULL | |
| token | varchar(50) | NO | MUL | NULL | |
| expires | datetime | NO | | NULL | |
+---------+-------------+------+-----+---------+----------------+
Re: Check my Code
Posted: Sat Dec 10, 2011 3:37 pm
by ibnclaudius
I do not quite understand the use of cookies, and you have functions to verify it, but never calls.
Re: Check my Code
Posted: Sat Dec 10, 2011 3:48 pm
by Celauran
Checking cookies for a valid "remember me" entry would be done before the user arrives at the login page.