1

I want to create a simple login system in my website I am developing for academic purpose.

This is what I did so far:

  1. A user will have to fill out a form to input their username and password, and then submit the form (POST method). Both vars will be sanitized.

  2. Query from database 

    SELECT id FROM user WHERE username = x AND
password = y

    Where x and y are username and password acquired using

    $_POST['variable_name']

  3. Then I used PHP function rowCount(). If the result = 1 (and only one), then that user will be notified that the login process is successful and a user id session var will be set.

So, is this kind of login system simple and efficient enough? I don't need any additional security measure (like password hashing) right now.

mnille
  • 1,328
  • 4
  • 16
  • 20
GPraz
  • 298
  • 4
  • 13
  • Sanitizing vars instead of using parameterized queries went out of fashion over 10 years ago (ie not best practise). I see no reason to not add hashing, it's not hard at all using the password api built into php (password_hash and password_verify). Its most common to actually fetch the data of the user, you probably want to add the user id to ession so you know which user is logged in when handling later requests – JimL Jul 13 '16 at 04:36
  • @JimL do you mean parameterized queries using `prepare()` and `execute()` ? If so, yes I can use that. Ditto on `password_hash` and `password_verify`. – GPraz Jul 13 '16 at 04:44
  • Yes, in effect replacing vars in the query string with ? or :parameterName – JimL Jul 13 '16 at 04:46
  • @JimL Okay. And of course user id will be added to session var. I'll update my question shortly. What about using both sanitized vars and parameterized queries? – GPraz Jul 13 '16 at 04:51
  • counting the number of rows is useless, it is faster to just check that the fetched result is !== false. If the username is unique in the db, there can't be more than one. – the_nuts Jul 13 '16 at 05:01
  • @the_nuts yes the id and username is unique in db, that's why I chose to use `rowCount() == 1`, can you explain why checking the fetched result is way faster? – GPraz Jul 13 '16 at 05:15
  • No reason to sanitze as well. It adds nothing for security and makes your code messier – JimL Jul 13 '16 at 05:29
  • In your case the performance difference would be very small and hard to notice (unless you have thousands of logins per seconds), but rowCount() shouldn't be used for that, see also http://stackoverflow.com/a/19110637/3393663 – the_nuts Jul 13 '16 at 06:36

2 Answers2

1

This is how it should be done with more modern standards:

$db = new PDO('mysql:host='.$database_host.';dbname='.$database_name.';charset=utf8', $database_user, $database_password);

$stmt = $db->prepare("SELECT id FROM users WHERE username=? AND password=?");
$stmt->execute([$_POST['username'], $_POST['password']]);

$user = $stmt->fetch(); // Or fetchColumn() to get just the id

if($user) { 
    // Login
} else {
    // The user doesn't exist or the password is wrong
}
the_nuts
  • 5,634
  • 1
  • 36
  • 68
  • Should I create the PDO instance, `prepare()`, and `execute()` inside `try catch` just in case an error happened during connecting to database? – GPraz Jul 13 '16 at 05:38
  • Yes, by adding the fourth parameter `[PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]` to the PDO constructor. You may also want to check if `$_POST['username']` and password are set – the_nuts Jul 13 '16 at 06:31
0

Use PDO or mysqli for prevention of sql injection and fast data retrieval

here is the code

 <?php
    $avail=0;
    require 'connect.php';
    if(isset($_POST['Submit']))
    {        $Username=$_POST['Username'];
             $Password=$_POST['Password'];
             $query=$handler->query('Select * FROM users');     
             while($r=$query->fetch(PDO::FETCH_ASSOC)){
                $U=$r['Username'];
                $P=$r['Password'];
                if($U==$Username&&$P==$Password){
                    $avail=1;
                }
                }
                if($avail==1){
                     echo("<script>window.location = 'welcome.html';</script>");
                }
                if($avail==0){
                     echo("<script>alert('Wrong Username or Password')</script>");


                }

    }

?>
art
  • 226
  • 3
  • 11
  • 30
  • What about using `if($query->rowCount() == 1)` instead of `if($U==$Username && $P==$Password)`? Is it necessary to select all rows from user table and to use while loop? – GPraz Jul 13 '16 at 05:04
  • There is no injection prevention in this "code"... it's bad written and the logic is totally wrong... (cycling all the users?!?)... and it also depends on custom function in `connect.php`, which we can't see... – the_nuts Jul 13 '16 at 05:06
  • i guess connect.php will be specific to every user what my main focus was to use pdo – art Jul 13 '16 at 05:15
  • ah ok, so $handler is a PDO instance... Anyway, if you have 10 users for example, this scripts will print 9 alerts and one redirect if the user found is the last one. It doesn't seem a very good solution :) – the_nuts Jul 13 '16 at 05:20
  • i guess you missed avail variable which i am using here – art Jul 13 '16 at 05:22
  • But you have only one user? $avail is 0 until it finds a valid user – the_nuts Jul 13 '16 at 05:24