There is still work to be done, but keep at it. I suggest you download my PDO Bumpstart Database from my signature and study the code until you understand it. Also, take note of my code formatting.
I am not going to evaluate your latest code except to tell you to NEVER EVER put variables in your queries, Don’t SELECT *. Select the columns by name that you need and don’t create variables for nothing.
hi, thanks for that… i downloaded the PDO Bumpstart and made notes of bits… i noticed it was from 2014 and some of it is outdated? ie useing globals and SELECT * i seen ;D
i have amended your suggestions… here is my latest FINALY with login attempts !
could you just tell me if what im doing here is valid… and anymore suggestions?
[php]<?php
//CLEAN USERNAME AND PASSWORD
if (isset($_POST[‘jamin_username’]) && isset($_POST[‘jamin_password’])) {
//user
$user = $_POST[‘jamin_username’];
$user = $conn->real_escape_string($user);
$user = trim($user);
$user = strip_tags($user);
$user = htmlspecialchars($user);
$user = $purifier->purify($user);
//pass
$pass = $_POST[‘jamin_password’];
$pass = trim($pass);
$pass = strip_tags($pass);
$pass = htmlspecialchars($pass);
} else {
$user = null;
$pass = null;
}
//CHECK IF THE USER IS LOCKED OUT OR NOT
if ($login_attempts = $conn->prepare(“SELECT address FROM login_attempts WHERE address = ?”)) {
$login_attempts->bind_param(“s”, $remote_ip);
$login_attempts->execute();
$result = $login_attempts->get_result();
$row_count = $result->num_rows;
if ($row_count >= 3) {
$error = (“You have tried to log in too many times
You have been locked out until the administrator investigates this log!”);
$locked_out = true;
if ($page === Admin) {
header(‘location: ./index.php’, true);
exit();
}
}
if ($page === ‘Admin’) {
if ($row_count === 2) {
$warning = (“WARNING: Your about to be locked out!
You have 1 more attempt!
”);
}
if ($row_count === 1) {
$warning = (“WARNING: You have 2 more attempts!
”);
}
}
$login_attempts->close();
}
//ADMIN LOGIN SCRIPT
if ($locked_out === false) {
if ($action === ‘JaminLogin’) {
if (isset($_POST[‘jamin_login’])) {
if (empty($user) || empty($pass)) {
$error = (“You must fill all fields!”);
} else {
if ($jamin_login = $conn->prepare(“SELECT username, password FROM jamin_users WHERE username = ?”)) {
$jamin_login->bind_param(“s”, $user);
$jamin_login->execute();
$result = $jamin_login->get_result();
$row = $result->fetch_assoc();
if ($row) {
if (password_verify($pass, $row[‘password’])) {
$_SESSION[‘jamin_user’] = $row[‘username’];
$jamin_login->close();
header(‘location: ./?page=Admin’, true);
exit();
} else {
if ($login_attempts = $conn->prepare(“INSERT INTO login_attempts (address) VALUES (?)”)) {
$login_attempts->bind_param(“s”, $remote_ip);
$login_attempts->execute();
$login_attempts->close();
}
$error = (“Your password dose not match!”);
header(‘Refresh: 3;url=./?page=Admin’, true);
}
} else {
$error = (“Your username or password do not match!”);
}
}
}
}
}
}
//SET SESSION USERNAME VAR
if (isset($_SESSION[‘jamin_user’])) {
$jamin_user = $_SESSION[‘jamin_user’];
} else {
$jamin_user = null;//if its not set then set it to null.
}
//ADMIN LOG OUT
if ($action === ‘Logout’) {
if (isset($_SESSION[‘jamin_user’]))
{
unset($_SESSION[‘jamin_user’]);
}
header(‘location: ./?page=Admin’, true);
exit();
}
//scripts that can only be run if there is a jamin session and user is not locked out
if ($locked_out === false && $jamin_user) {
###############//start secure scripts//###############
//REGISTER JAMIN SCRIPT
if ($action === ‘JaminRegister’) {
if (isset($_POST[‘jamin_register’])) {
if (empty($user) || empty($pass)) {
$error = (“You must fill both fields!”);
} else {
if ($error === null) {
if ($user_check = $conn->prepare(“SELECT username FROM jamin_users WHERE username = ?”)) {
$user_check->bind_param(“s”, $user);
$user_check->execute();
$result = $user_check->get_result();
$row = $result->fetch_assoc();
}
if (!$row) {
if ($jamin_register = $conn->prepare(“INSERT INTO jamin_users (username, password) VALUES (?, ?)”)) {
$pass = password_hash($pass, PASSWORD_BCRYPT);
$jamin_register->bind_param(“ss”, $user, $pass);
$jamin_register->execute();
if ($jamin_register) {
$success = (“Jamin Registerd!”);
} else {
$error = (“There was a problem!”);
}
$jamin_register->close();
}
} else {
$error = (“This username already exists!”);
}
$user_check->close();
}
}
}
}
//SELECT AND OUTPUT JAMIN USERS TABLE
if ($action === ‘JaminUsers’ || $action === ‘JaminRegister’) {
if ($jamin_users = $conn->prepare(“SELECT id, username FROM jamin_users”)) {
$jamin_users->execute();
$result = $jamin_users->get_result();
$row_count = $result->num_rows;
if ($row_count >= 1) {
while ($row = $result->fetch_assoc()) {
$user_table .= (“
thankyou again
Apparently you did not see the update stamp in the post
Last Edit: October 29, 2016, 02:03:06 pm
I see there is an old function in there. I will handle on the next update… The whole point is to get people off Mysql and Mysqli and use PDO without out having all the excuses not to. Good for you about knowing about not using *.
hi,
so is what im doing so far at a standard? if you will… valid?..
thanks?
Your code needs some help with the logic and needs to have unneeded bits (computer pun not intended) removed. It is running portions of the code and queries using inputs that may not exist. The logic should only run dependent code/queries when there is valid input data.
Your post method form processing code needs to be grouped together and first test if a post method form was submitted, before referencing any of the form data. Any get method code, that is responsible for producing content for the page, should be after the end of all the post method form processing code. If there are multiple sections of form processing code, you should use a switch/case statement or individual if(){} statements to determine which form processing code to run. All the form processing code for any one form should be inside the correct case…: break; or if(){…} block.
If any particular form processing code needs other qualifying conditions to run, such as if the current visitor is logged in/out or is an admin…, add these tests as needed to each section of form processing logic. For the login form processing code, the current visitor should not be logged in.
Once you have determined that any particular form processing code should run, it needs to validate the inputs, then only use the input data if it is valid. If you use an array to hold error messages, you can validate all the inputs at once, then test if the array is empty to determine if you should use the input data.
The validation you do on each input is context dependent. For example, for the login form processing, all you care is that there are non-empty values to use. Your validation logic, other than trimming data, should not alter what the data is, only validate that is it usable or not in the current context and set up error message(s) to tell the visitor what is wrong with the submitted data. Your current use of the real escape string (with prepared queries, this is not needed) , strip tags (this alters the data and in fact removes things that should be valid for usernames and passwords), htmlspecialchars (this is an output function, and is only used when you output data to the browser), and what ever the $purifier->purify() does, are either misapplied/not needed or are altering the data values, rather than validating them. In fact, you aren’t testing the result of the validation step before using the data.
I believe, in previous posted information, it was stated that the composite value of the username and ip address is what you need to use to keep track of login attempts. The current code is only using the ip and it is using a variable $remote_id that’s not defined in the posted code, and is using both a a defined constant Admin and the string ‘Admin’. Concerning the variable and defined constant/string, do you have php’s error_reporting set to E_ALL and display_errors set to ON, so that php would report and display all the errors it detects? You will save a ton of time and be able to find and fix simple errors yourself.
You are putting b[/b] around strings in assignment statements. The () are unnecessary and are just cluttering up your code. You are also testing the current $page is ‘Admin’/Admin (see the defined constant/string problem mentioned above) to perform redirects, but you are not doing anything if that’s not what the current page is. This makes no sense. Any error during the login process should cause the error(s) to be displayed on this same page.
You should retrieve any data from a select query immediately after running the query. This will mean that any close() methods you call to deal with the out of sync errors when running other queries will be near the associated query code and it will separate the database specific code from the code using the data. Also, within the code dealing with any query, there’s no need to come up with different variable names. Only the fetched data needs to be in a uniquely named variable, that indicates the meaning of the data. This will keep the majority of the query code the same each time you use it. If you form the sql query statement in a php variable, it will also make your code more general purpose and reusable. This will also make it easier to convert to use the php PDO extension, since the sql query statement won’t be a part of the php code dealing with the query.
“Your password dose (does) not match!” You should NOT output a message identifying if it is the username or the password that caused the login to fail. Output the same - “Your username or password do not match!” in both cases.
You are missing some exit; statements after header() redirects.
You are executing queries inside of loops (in both sets of delete related code). One of the points of using prepared queries is you only prepare them once, then can execute them multiple times. You should prepare and bind the inputs, before the start of any loop, then only populate the bound input variable and execute the query inside of the loop. Also, since your code allows multiple deletions, you would not want to do any sort of header() redirect until you have finished.
There’s more but that’s all I’m willing to type. If you better organize your code and eliminate things it doesn’t need, you can eliminate a lot of the variables and logic you have now and switching to the php PDO extension will simplify the code you are using around each sql query.
hi,
thanks for all your help… i am going to work on all you have said here… it looks like i will have to start it again with what i have learnd so far
because i have dyslexia i take longer to learn when its reading involved to learn…
thanks again ill post back with a update as soon as i update it.
hi, im slowly getting through all my errors etc… i have made the following changes but dont want to carry on changing everthing unless its ok to do so…
here is what ive got
[php]###############################################################################################
/*
/ADMIN FUNCTIONS - START
/
//CHECK IF THE USER IS LOCKED OUT OR NOT
function admin_login_check($remote_ip, $conn) {
if ($stmt = $conn->prepare(“SELECT address FROM login_attempts WHERE address = ?”)) {
$stmt->bind_param(“s”, $remote_ip);
$stmt->execute();
$result = $stmt->get_result();
$row_count = $result->num_rows;
if ($row_count >= 3) {
//You have tried to log in too many times
return true;
} else {
return false;
}
$stmt->close();
}
}
//ADMIN LOGIN FUNCTION
function admin_login($user, $pass, $remote_ip, $conn) {
if (admin_login_check($remote_ip, $conn) == true) {
//user is locked out
return false;
} else {
if ($stmt = $conn->prepare(“SELECT username, password FROM jamin_users WHERE username = ?”)) {
$stmt->bind_param(“s”, $user);
$stmt->execute();
$result = $stmt->get_result();
$row = $result->fetch_assoc();
if ($row) {
if (password_verify($pass, $row[‘password’])) {
$_SESSION[‘jamin_user’] = $row[‘username’];
return true;
} else {
if ($login_attempts = $conn->prepare(“INSERT INTO login_attempts (address) VALUES (?)”)) {
$login_attempts->bind_param(“s”, $remote_ip);
$login_attempts->execute();
$login_attempts->close();
}
//password does not match!
return false;
}
} else {
//username or password do not match!
return false;
}
$stmt->close();
}
}
}
/
/ADMIN FUNCTIONS - END
*/
###############################################################################################
//CLEAN FORM USERNAME AND PASSWORD
if (!empty($_POST)) {
if (isset($_POST[‘jamin_username’]) && isset($_POST[‘jamin_password’])) {
//user
$user = filter_input(INPUT_POST, ‘jamin_username’, FILTER_SANITIZE_STRING);
//pass
$pass = filter_input(INPUT_POST, ‘jamin_password’, FILTER_SANITIZE_STRING);
} else {
$user = null;
$pass = null;
}
}
//LOGIN SCRIPT
if ($page === ‘Admin’ && $action === ‘JaminLogin’) {
if (!empty($_POST)) {
if (isset($_POST[‘jamin_login’])) {
if (empty($user) || empty($pass)) {
$error = “You must fill in all fields!”;
} else {
if (admin_login($user, $pass, $remote_ip, $conn) == true) {
// Login success
header(‘location: ./?page=Admin’, true);
exit();
} else {
// Login failed
$error = “Login Failed!”;
}
}
}
}
}
//SET SESSION USERNAME VAR IF IS SET
if (isset($_SESSION[‘jamin_user’])) {
$jamin_user = $_SESSION[‘jamin_user’];
} else {
$jamin_user = null;//if its not set then set it to null.
}
//ADMIN LOG OUT
if ($page === ‘Admin’ && $action === ‘Logout’) {
if (isset($_SESSION[‘jamin_user’]))
{
unset($_SESSION[‘jamin_user’]);
}
session_destroy();
header(‘location: ./?page=Admin’, true);
exit();
}[/php]
as you can see i have started to use functions and changed a few other bits in there
also i sorted my variables out best i can so far
i have already sorted out my sql where i had prepared statements inside loops but i have not updated the structure of all that yet
thanks
hi,
im still getting through it… im just wondering why when i add BREAK; to my foreach loops it stops the loops after 1? for me i would rather not have it and just allow it to loop as many rows in the database… i know i can add break 5; to allow 5 loops but i want to allow as many as there are…
here is my latest (delete script etc still not updated so not displaying)
[php]#############################################
/*
/ADMIN FUNCTIONS - START
*/
//CHECK IF THE USER IS LOCKED OUT OR NOT
function admin_login_check($conn) {
if ($stmt = $conn->prepare(“SELECT address FROM login_attempts WHERE address = ?”)) {
$stmt->bind_param(“s”, $_SERVER[‘REMOTE_ADDR’]);
$stmt->execute();
$result = $stmt->get_result();
$row_count = $result->num_rows;
if ($row_count >= 3) {
//You have tried to log in too many times
return true;
} else {
return false;
}
$stmt->close();
}
}
//ADMIN LOGIN FUNCTION
function admin_login($user, $pass, $conn) {
if (admin_login_check($conn) == true) {
//user is locked out
return false;
} else {
if ($stmt = $conn->prepare(“SELECT username, password FROM jamin_users WHERE username = ?”)) {
$stmt->bind_param(“s”, $user);
$stmt->execute();
$result = $stmt->get_result();
$row = $result->fetch_assoc();
if ($row) {
if (password_verify($pass, $row[‘password’])) {
$_SESSION[‘jamin_user’] = $row[‘username’];
return true;
} else {
if ($login_attempts = $conn->prepare(“INSERT INTO login_attempts (address) VALUES (?)”)) {
$login_attempts->bind_param(“s”, $_SERVER[‘REMOTE_ADDR’]);
$login_attempts->execute();
$login_attempts->close();
}
//password does not match!
return false;
}
} else {
//username or password do not match!
return false;
}
$stmt->close();
}
}
}
//CHECK IF A ADMIN USER ALREADY EXISTS
function check_admin_exists($user, $conn) {
if ($stmt = $conn->prepare(“SELECT username FROM jamin_users WHERE username = ?”)) {
$stmt->bind_param(“s”, $user);
$stmt->execute();
$result = $stmt->get_result();
$row = $result->fetch_assoc();
if ($row) {
return true;
}
$stmt->close();
}
}
//REGISTER A ADMIN
function admin_register($user, $pass, $conn) {
if ($stmt = $conn->prepare(“INSERT INTO jamin_users (username, password) VALUES (?, ?)”)) {
$stmt->bind_param("ss", $user, password_hash($pass, PASSWORD_BCRYPT));
$stmt->execute();
if ($stmt) {
//Jamin registered successfully!
return true;
}
$stmt->close();
}
}
//ADMIN CPANEL - COLLECTS DYNAMIC HTML PAGES
function get_admin_cp($conn, $page, $action, $jamin_user, $user_table, $louser_table) {
//IF USER IS VALID
if ($jamin_user) {
##############
//SECURE PAGES
##############
require_once INCLUDES_BASEDIR.ADMIN_BASEDIR.‘panel.html’;//admin panel
/////////////////////
//ADMIN DYNAMIC PAGES
////////START////////
##
//ADMIN USER LIST
if ($page === ‘Admin’ && $action === ‘JaminUsers’) {
require_once INCLUDES_BASEDIR.ADMIN_BASEDIR.‘users.html’;//jamin users
}
//JAMING LOCKED OUT USERS
if ($page === ‘Admin’ && $action === ‘LOUsers’) {
require_once INCLUDES_BASEDIR.ADMIN_BASEDIR.‘lousers.html’;//locked out users
}
//ADMIN REGISTER
if ($page === ‘Admin’ && $action === ‘JaminRegister’) {
require_once INCLUDES_BASEDIR.ADMIN_BASEDIR.‘register.html’;//register jamin
require_once INCLUDES_BASEDIR.ADMIN_BASEDIR.‘users.html’;//jamin user list
}
##
///////////
//END PAGES
///////////
} else {
require_once INCLUDES_BASEDIR.ADMIN_BASEDIR.‘login.html’;//display login if there is no session set.
}
}
/*
/ADMIN FUNCTIONS - END
*/
#############################################
//CLEAN FORM USERNAME AND PASSWORD
if (!empty($_POST)) {
if (isset($_POST[‘jamin_username’]) && isset($_POST[‘jamin_password’])) {
//user
$user = filter_input(INPUT_POST, ‘jamin_username’, FILTER_SANITIZE_STRING);
//pass
$pass = filter_input(INPUT_POST, ‘jamin_password’, FILTER_SANITIZE_STRING);
} else {
$user = null;
$pass = null;
}
}
//LOGIN SCRIPT
if ($page === ‘Admin’ && $action === ‘JaminLogin’) {
if (admin_login_check($conn) == true) {
$warning = “You are locked out!”;
} else {
if (!empty($_POST)) {
if (isset($_POST[‘jamin_login’])) {
if (empty($user) || empty($pass)) {
$error = “You must fill in all fields!”;
} else {
if (admin_login($user, $pass, $conn) == true) {
// Login success
header(‘location: ./?page=Admin’, true);
exit();
} else {
// Login failed
$error = “Login Failed!”;
}
}
}
}
}
}
//SET SESSION USERNAME VAR IF IS SET
if (isset($_SESSION[‘jamin_user’])) {
$jamin_user = $_SESSION[‘jamin_user’];
} else {
$jamin_user = null;//if its not set then set it to null.
}
//ADMIN LOG OUT
if ($page === ‘Admin’ && $action === ‘Logout’) {
if (isset($_SESSION[‘jamin_user’]))
{
unset($_SESSION[‘jamin_user’]);
}
session_destroy();
header(‘location: ./?page=Admin’, true);
exit();
}
//scripts that can only be run if there is a jamin session and user is not locked out
if ($jamin_user) {
###############//start secure scripts//###############
//REGISTER JAMIN SCRIPT
//JAMIN REGISTER SCRIPT
if ($page === 'Admin' && $action === 'JaminRegister') {
if (!empty($_POST)) {
if (isset($_POST['jamin_register'])) {
if (empty($user) || empty($pass)) {
$error = "You must fill in all fields!";
} else {
if (check_admin_exists($user, $conn) == true) {
$warning = "This username already exists!";
} else {
if (admin_register($user, $pass, $conn) == true) {
// Login success
$success = "Jamin registered successfully!";
} else {
// Login failed
$error = "Registration Failed!";
}
}
}
}
}
}
}[/php]
sorry for so many posts
thanks
Why are you adding BREAK in the first place?
I think I mite have miss understood an earlyer post… I thaught I had to end a forech with break
So I dont have to use break?
Thanks
No.