[RESOLVED] Login system most likely not worth keeping

[cluelessPHP]

Well I vaguely suspect this is a case of delete almost everything and start again, but...

<?php 

require "classes/Login.class.php";
require "loadclasses.php";

if ($_SERVER['REQUEST_METHOD'] == 'POST')
{
	$login = new Login($email, $password);
	$email = $pass = "";
	$post = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING);		

	$email = $post['emaillogin'];
	$pass =  $post['passwordlogin'];		
	$errors = array();

	$fields = array(
  'emaillogin' => array(        

    'validate' => 'validEmail',
    'message' => 'Enter a valid email address',
    'value' => $email,
  ),
  'passwordlogin' => array(         

    'validate' => 'emptyPassword',
    'message' => 'Password required',
    'value' => $pass,
	),
	'passwordlogin' => array(         

    'validate' => 'confirmPassword',
    'message' => 'Incorrect username/password',
    'value' => $pass,
	),
	'emaillogin' => array(         

    'validate' => 'liveAccount',
    'message' => 'Please ensure your account is active',
    'value' => $pass,
  )
);


	foreach($fields as $key => $value) 
	{
		$validation_result = $login->$value['validate']($value['value']);

    	if(!$validation_result) 
		{
    		$errors[] = ['name' => $key, 'error' => $value['message']];
		}
	}

 if(empty($errors))  
    {  
  ///do something clever here
    }  

}

header('Content-Type: application/json');
if (empty($errors))
{
	echo json_encode($success);
}
else
{
	echo json_encode(["errors" => $errors]);
}			

<?php

require ("Database.class.php");

class Login
{	
	private 
	$email,
	$password,
	$database,
	$db = null;

public function __construct()
{
	$this->db = new Database;
}

public function validEmail($email)
{	
	return (filter_var($email, FILTER_VALIDATE_EMAIL) !== FALSE);  
}	

public function emptyPassword($password)
{
	return(empty($password) !== TRUE);
}

public function liveAccount($email)
{
	$db = new Database;
	$db->query('SELECT eMail, password FROM username WHERE eMail = :emaillogin AND activecode = "Active"');
	$db->bind(':emaillogin', $email);  
	$db->execute();
	if(!$results = $db->resultset())  
	{  
		return false;
	}  
}

public function confirmPassword($email, $password)
{
	$db = new Database;
	$db->query('SELECT eMail, password FROM username WHERE eMail = :emaillogin');
	$db->bind(':emaillogin', $email);  
	$db->execute();
	if(!$results = $db->resultset())  
	{  
		return false;
	}  
	else
	{
		if(!password_verify($password, $post['passwordlogin']))  
		{  
			return false;   
		}
	}
}

}

Feel free to say delete and start again if I'm way off track

[Weedpacket]

One thing that jumps out is that you create a new Database object when you want to run a query, instead of using the one you created and stored during construction.

[cluelessPHP]

<?php

require ("Database.class.php");

class Login
{	
	private 
	$email,
	$password,
	$database,
	$db = null;

public function __construct()
{
	$this->db = new Database;
}

public function validEmail($email)
{	
	return (filter_var($email, FILTER_VALIDATE_EMAIL) !== FALSE);  
}	

public function emptyPassword($password)
{
	return(empty($password) !== TRUE);
}

public function liveAccount($email)
{
	$stmt = $this->db->query('SELECT eMail, password FROM username WHERE eMail = :emaillogin AND activecode = "Active"');
	$stmt->bind(':emaillogin', $email);  
	$stmt->execute();
	if(!$results = $stmt->resultset())  
	{  
		return false;
	}  
}

public function confirmPassword($email, $password)
{

	$stmt = $this->db->query('SELECT eMail, password FROM username WHERE eMail = :emaillogin');
	$stmt->bind(':emaillogin', $email);  
	$stmt->execute();
	if(!$results = $stmt->resultset())  
	{  
		return false;
	}  
	else
	{
		if(!password_verify($password, $post['passwordlogin']))  
		{  
			return false;   
		}
	}
}

}

Still way off track I think, need to read some tutorial on user input er..functions in oop?

[Weedpacket]

You'll get an argument that the Database object should be constructed elsewhere and passed into Login's constructor, rather than have the latter create the former itself (fancy Googleable term: dependency injection).

function __constructor(Database $db)
{
    $this->db = $db;
}

First reason is that now the same Database object can be used in more than one Login object, instead of each creating their own. Second reason is that the Login is no longer tied down to a single implementation of Database, but instead need only require that whatever it gets implements the appropriate interface.
But whether either main argument is relevant for this application (will you have multiple Login objects that could share a common Database? How many different implementations of Database are there going to be?) is a matter for discussion.

[pbismad]

Are you developing and testing your code with php's error_reporting set to E_ALL and either display_errors set to ON or log_errors set to ON? There are variables and call-time parameters scatted throughout the code that don't exist or don't match up and would be producing errors.

Some specific problems (or doing thing that don't result in anything useful happening) -

1) The code creating the instance of the login class has two call-time parameters. Neither of these variables exist at that point and would be throwing php errors. That class constructor isn't defined to accept or use any call-time parameters, so, nothing is being done with these two values anyway.

2) Validating data and using it if it is valid are two separate concerns. I would put the email/password/live-account tests inside the "///do something clever here" logic, not as part of the validation steps.

3) Dynamically validating the input data is a good idea, but your logic is trying to use the values when they have validation errors from earlier tests. For your current code (or other code that will have multiple validation tests for any form field), if the email isn't valid, you are still using it in later steps, so, those steps will fail and you will get multiple error messages, but only the first one is the real problem. The others are follow-on errors. Your code needs to stop validating a particulate field upon the first error for it. The easiest way to do this is to use the field name as the $errors array key. You can then test if there is already an error for a field by testing if there's an element in $errors for that field using isset().

4) The 'values' part of your dynamic validation only accepts a single parameter. You are however using it (which may change) to call a method that requires two parameters. You would need to supply an array consisting of the key/value for each of the parameters, then use the elements of that array in the methods that get called.

5) Your methods need to return a TRUE value when they succeed. They are currently returning a NULL value upon success, which your current code will treat the same as a FALSE value.

6) You have apparently written a wrapper class for the php PDO class and have hard-coded database connection credentials into it. The php PDO class is perfectly fine to use. If you need to add functionality to it, then "extend" it. By extending it, you can directly use all of the base class methods and properties, without having to write a bunch of methods that consist of just a call to the corresponding base method (Keep It Simple.) By hard-coding the connection credentials into the class, you are tied to using a single connection at a time, using those credentials. Your code is not general purpose. As soon as you have a need for connections to multiple databases at the same time, all your code that's tied to this database class will 'break' requiring a re-write to get it to work. It's your main application code that knows where/what the database connection credentials are and what data will be stored in which database. If you keep your code general purpose and use dependency injection to get the instance of the database connection into any instance of a class that needs a connection, your main application code, that you write for each different application, can deal with the things that are application specific without you needing to touch any of the 'support' code in functions and classes.

7) If an account isn't active, there's no point in trying to actually log the user in. You should test for the account being active before or at the same time you try to log the user in. If the account isn't active, stop at that point, setting up the appropriate error message, and don't bother to execute the password_verify() code.

8) There are two other problems with the confirmPassword() method. One has already been mentioned (the second call-time parameter, $password, isn't being supplied when this method is being called) and the code is using the $post variable, which doesn't exist in its scope. Both of these things would be throwing php errors and the password_verify() function itself would either be throwing errors about its parameters or it will always return a false value.

[cluelessPHP]

I am looking at this, it's a fair bit to go over at once which is why I haven't said much

[cluelessPHP]

So starting with number 6

class Database 
{
	private $host = "localhost";
	private $user = "root";
	private $pass = "";
	private $dbname = "got";

private $dbh;
private $error;
private $stmt;

public function __construct()
{
	//SET DSN
	$dsn = "mysql:host=". $this->host . ";dbname=" . $this->dbname;

	$options = array
	(
		PDO::ATTR_PERSISTENT 	=> true,
		PDO::ATTR_ERRMODE 		=>PDO::ERRMODE_EXCEPTION,
		PDO::MYSQL_ATTR_FOUND_ROWS => true
	);
//create PDO
try
{
	$this->dbh = new PDO($dsn, $this->user, $this->pass, $options);
}
catch(PDOEception $e)
{
	$this->error = $e->getMessage();
}
}

public function query($query)
{
	$this->stmt = $this->dbh->prepare($query);
}

public function bind($param, $value, $type = null)
{
	if(is_null($type))
	{
		switch(true)
		{
			case is_int($value):
			$type = PDO::PARAM_INT;
			break;
			case is_bool($value):
			$type = PDO::PARAM_BOOL;
			break;
			case is_null($value):
			$type = PDO::PARAM_NULL;
			break;
			default:
			$type = PDO::PARAM_STR;
		}
	}

	$this->stmt->bindValue($param, $value, $type);
}

public function execute()
{
	return $this->stmt->execute();
}


public function rowCount()
{
	return $this->stmt->rowCount();
}

public function connect()
{
	$this->dbh->connect();
}

public function lastInsertId()
{
    $this->dbh->lastInsertId();
}

public function resultset()
{
	$this->execute();
	return $this->stmt->fetchAll(PDO::FETCH_ASSOC);
}
}

This is nice enough but basically useless long term? I'm better off with?

<?php
class Database extends PDO

{
	private $host = 'localhost';
	private $user = 'root';
	private $pass = '';
	private $dbname = 'got';

public function __construct($dsn = null, $user = null, $pass = null, $opts = null)
{
	parent::__construct("mysql:host={$this->host};dbname={$this->dbname}", $this->user, $this->pass, $opts);
	$this->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}
}

[cluelessPHP]

Decided it wasn't work keeping in the end