2
\$\begingroup\$

I am new to the concept of prepared statement and i am trying to adjust my "user login" function to be a prepared statement. The part i am really struggling with is the fact i have multiple SQL queries in the one function and the biggest hurdle right now is fetching from a prepared statement.

Here is the code i have written for the function so far, i do apologise its a mess and i understand there is parts that shouldn't be in there, i just left everything in.

function login_member() {

    $connection = mysqli_connect("localhost", "root", "", "ecom_db");
    if(isset($_POST['login_button'])) {

        $email = filter_var($_POST['log_email'], FILTER_SANITIZE_EMAIL); 

        $_SESSION['log_email'] = $email; 
        $password = $_POST['log_password']; 

        $stmt = $connection->prepare("SELECT email AND password FROM users WHERE email=? AND password=? AND com_code IS NULL");
        $stmt->bind_param('ss', $email, $password);
        $stmt->execute();
        $stmt->bind_result($email, $password);
        $stmt->store_result();
        if($stmt->execute()) {

        $result = $stmt->get_result();

        while ($row = $result->fetch_assoc()) {

            //result is in row

            }

        }
        if($stmt->num_rows == 1) {
            if($stmt->fetch()) {
                while (($row = $res->fetch_assoc()))
                   $username = $row['username'];


            $closed = $connection->prepare("SELECT email AND password FROM users WHERE email=? AND user_closed=?");
            $closed->bind_param('ss', $email, "yes");
            $closed->execute();
            $closed->bind_result($email, "yes");
            $closed->store_result();
            $closed_res = $closed->get_result();

            if($closed->num_rows == 1) {
                $open = $connection->prepare("UPDATE users SET user_closed=? WHERE email=?");
                $open->bind_param('ss', "no", $email);
                $open->execute();
                $open->bind_result("no", $email);
                $open->store_result();
            }

            $_SESSION['username'] = $username;
            header("Location: ../public/member/index.php");
            exit();
        }
        else {
            set_message("Email or password was incorrect");
        }
    }
}

I understand that it looks like a complete mess, i have left all my working outs in there. My main issue here is after my initial statement, i want to be able to fetch the results from the returned row and set the "username" a column in that row to the variable $username.

My original, working code (without prepared statements) is as follows.

My original - without prepared satatements is as follows.

function login_new_member() {

    if(isset($_POST['login_button'])) {

        $email = filter_var($_POST['log_email'], FILTER_SANITIZE_EMAIL); //sanitize email

        $_SESSION['log_email'] = $email; //Store email into session variable 
        $password = md5($_POST['log_password']); //Get password md5

        $check_database_query = query("SELECT * FROM users WHERE email='$email' AND password='$password' AND com_code IS NULL");
        confirm($check_database_query);
        $check_login_query = mysqli_num_rows($check_database_query);

        if($check_login_query == 1) {
            $row = mysqli_fetch_array($check_database_query);
            $username = $row['username'];

            $user_closed_query = query("SELECT * FROM users WHERE email='$email' AND user_closed='yes'");
            confirm($user_closed_query);
            if(mysqli_num_rows($user_closed_query) == 1) {
                $reopen_account = query("UPDATE users SET user_closed='no' WHERE email='$email'");
                confirm($reopen_account);
            }

            $_SESSION['username'] = $username;
            header("Location: ../public/member/indexhold.php?register_new");
            exit();
        }
        else {
            echo "Email or password was incorrect";
        }


    }


} 
\$\endgroup\$

closed as off-topic by Mike Brant, Graipher, alecxe, Vogel612, tim Mar 17 '17 at 21:49

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Mike Brant, Graipher, alecxe, Vogel612, tim
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ I'ts hard to tell what is your actual problem. you says your problem is with fetch, but posts actually a lot of code that is irrelevant to fetch and is a complete mess even without prepared statements. you better post your original function that works \$\endgroup\$ – Your Common Sense Mar 16 '17 at 9:55
  • \$\begingroup\$ I have edited my post to include my working function. \$\endgroup\$ – Colin Barstow Mar 16 '17 at 10:02
  • \$\begingroup\$ what's the purpose of changing user_closed from yes to no? \$\endgroup\$ – Your Common Sense Mar 16 '17 at 11:27
  • \$\begingroup\$ If a users account has been closed, throughout the site that users profile and posts will be hidden. If the customer would like to reopen their account they can simply log in and as you can see if the user_closed is set to yes it will change it to no and effectively "reactivate" their account.. \$\endgroup\$ – Colin Barstow Mar 16 '17 at 11:30
3
\$\begingroup\$

There are several problems with your code.

  1. An extra query to get exactly the same row you just requested
  2. Unreliable password hashing
  3. Inconsistent query handling with strange query()/confirm() pair.
  4. A lot of things intermixed here - POST, database internals, everything.

To make it better you have to store passwords after hashing them with password_hash() function and also utilize an improved version of query() function. So the final code would be like this (it is using my simple DB wrapper which is based on PDO, but could be implemented using mysqli as well). So the idea is that you do all the prepared statements' related stuff inside the database class, and never touch it inside a user function.

function login_new_member($email, $password)
{
    $user = DB::run("SELECT * FROM users WHERE email=? AND com_code IS NULL", [$email]);

    // you really should switch to password_hash/password_verify    
    // and make the following line as 
    // if ($user && password_verify($password, $user['pass']))
    // but for now
    if ($user && md5($password) === $user['password']))
    {
        if ($user['user_closed'] == 'yes')
        {
            DB::run("UPDATE users SET user_closed='no' WHERE email=?", [$email]);
        }
        $_SESSION['log_email'] = $user['email'];
        $_SESSION['username'] = $user['username'];
        header("Location: ../public/member/indexhold.php?register_new");
        exit();
    } else {
        echo "Email or password was incorrect";
    }
}

so this function should be called like this

if(isset($_POST['login_button'])) {
    login_new_member($_POST['log_email'], $_POST['log_password'])
}
\$\endgroup\$
  • \$\begingroup\$ i appreciate your reply, I am sorry for the messiness of my code, The query and confirm functions are simply little helper functions i create at the start of my functions file to save typing out queries every time. (should of explained that) What you have scripted out there is very tidy and appreciate you taking the time to help me. \$\endgroup\$ – Colin Barstow Mar 16 '17 at 12:11
  • \$\begingroup\$ can i just add that i have not used PDO before. in the part of this code where you have "$user = DB::run" for me it returns an error message of "Fatal error: Class 'DB' not found" I take it DB is the database, would i need to declare this? \$\endgroup\$ – Colin Barstow Mar 16 '17 at 12:56
  • \$\begingroup\$ yes. this code serves for the demonstration purpose mostly, to give you an idea \$\endgroup\$ – Your Common Sense Mar 16 '17 at 13:02
  • \$\begingroup\$ However you can take that DB class from the link in my answer. \$\endgroup\$ – Your Common Sense Mar 16 '17 at 13:26
  • \$\begingroup\$ Yes i am just reading through all of that, it obviously seems like a great tool. It's definitely something i'd like to take full advantage of. The only issue I'm going to have is I will need to rewrite all my code now to accommodate this new method. Like i say i've never used PDO now i get the following error "Warning: call_user_func_array() expects parameter 1 to be a valid callback, class 'PDO' does not have a method 'connection' in C:\xampp\htdocs\OakleyFinal\resources\config.php on line 49" sorry to keep bothering you. \$\endgroup\$ – Colin Barstow Mar 16 '17 at 13:30

Not the answer you're looking for? Browse other questions tagged or ask your own question.