Wrote A Function, It's Working But Don't Know How To Use It?

[scm22ri]

Hi Everyone,

 

I wrote a function for a login script. I tested the function and it's working but when I include it on my website it's not working. I'm not sure what I'm doing wrong. What do you guys think?

 

Thanks Everyone.

 

syntax for function

 

 

function loggeduser($name, $password){

$name = mysql_real_escape_string($name);
$password = mysql_real_escape_string($password);

$password = sha1($password);

// getting the ID here
$sql_id = mysql_query("SELECT * FROM practice WHERE name='$name' AND password='$password'");
while($row = mysql_fetch_array($sql_id)){
$id = $row["id"];
}

$sql = ("SELECT id FROM practice WHERE name='$name' AND password='$password'");
$result = mysql_query($sql) or die(mysql_error());

$count= mysql_num_rows($result);

if ($count == 1){
$_SESSION['authorized'] = true;
$_SESSION['id']; // <-- not sure if I need this?
$_SESSION['password'] = $password;
$_SESSION['name'] = $name;

header("location:userspage.php");


} else {
echo "Wrong username and password <br>";
}
} 

 

login script

 

<?php
session_start();
include"connect_to_mysql3.php";
$loggedinuser = $_SESSION["id"];
$name = $_SESSION["name"];

// include 'edit-car-function-for-login.php';

if (isset( $_POST['name'], $_POST['password'] )){

$name = $_POST['name'];
$password = $_POST['password'];

$password = sha1($password);

$name = mysql_real_escape_string($name);
$password = mysql_real_escape_string($password);

$sql_id = mysql_query("SELECT * FROM practice WHERE name='$name' AND password='$password'");
while($row = mysql_fetch_array($sql_id)){
$id = $row["id"];
}

$sql = ("SELECT id FROM practice WHERE name='$name' AND password='$password'");

$result = mysql_query($sql);

$count=mysql_num_rows($result);

if($count==1){


$_SESSION["name"] = $name;
$_SESSION["id"] = $id;

header("location:userspage.php");


} else {
echo "Wrong Username and Password.";
}
}

[Pikachu2000]

What about it is "not working"?

[gizmola]

Here's the thing about functions... they don't really help if you never call them.

 

There's also the question of DRY. You have a function and a script that seem to be 90% repeated code. Hard to tell what you're after here.

 

In terms of the function itself:

1. Functions should return something, yours does not. That makes it more of a procedure, but you should rewrite it to return a value that can then be acted upon.
    
   
2. There's no need to escape the password since you're sha1 hashing it.
    
   
3. Don't store the user's password in the session. It's bad practice at best, security hole at worst.
    
   
4. Why are you querying the user data 2x? You don't need to do that just to call mysql_num_rows(). You can call mysql_num_rows and still fetch the result set (or not) afterwards. You want to check for the match 1st, then only if its mysql_num_rows > 0 will you fetch the row. Probably the function should return false otherwise, and true when the user is found by name/password match.
    
   
5. You really want your database to insure that 'name' is unique. Usually you do this by adding a unique constraint(or unique index) on the name column. That way, there is no possibility for there to ever be more than 1 row in the database for a given user. The possibility of your inserting 2 rows with the same username is a major bug that your system should never allow.
   

[thara]

It's good practice use SHA1() when you inserting your password to the database...

 

Eg:

INSERT INTO practice ( name,  password) 
VALUES ( '$name,  SHA1('$password'))

 

 

why you use 2 queries in your function? and you don't need use a function as well. You can do the things that you want in the same script in your case it is login.php

 

it is something like this...

 

// database connection
// include file if needed 
if ( isset( $_POST['name']) && isset( $_POST['password'])) {
   $name = $_POST['name'];
   $password = $_POST['password'];
  // check the valid user or not 
 $q = "SELECT * FROM practice WHERE name = \'$name\' AND password = \"$password\" ";'
 $r = mysqli_query( $connection_name, $q);

 $count = mysqli_num_rows($r); 
 if ( $count == 1 ) {
    //valid user 
 } else {
    //not valid 
 }
}

[gizmola]

It's good practice use SHA1() when you inserting your password to the database...

 

Eg:

INSERT INTO practice ( name,  password) 
VALUES ( '$name,  SHA1('$password'))

 

I disagree.. that is far from a good practice. No Salt being used, and a single encryption is far from good practice.

 

However, probably the worst thing about your comment, is that he was already doing a SHA1 hash on the password.

 

Anyone can have a screed of procedural code in a script. Just because you can, doesn't mean that you should. Functions are a good idea, even if the OP hasn't entirely figured out how and where it's best to use them. For something that is done repeatedly, and typically in a number of different places, having a function that can be called is a good idea.

[thara]

@gizmola, I agree with you. Functions are a good way that some code repeatedly and typically use in a number of different places. But just I proposed a solution for OP to solve.

[scm22ri]

Hi Everyone,

 

Thanks for your responses.

 

The reason why I have to query the database twice is because I get this error if I don't

Warning: mysql_fetch_array(): supplied argument is not a valid MySQL result resource in .....

 

I'm going to take this from the top.

 

This url link is the function I'm using. (syntax for function)

http://whatsmyowncarworth.com/class-work/sign3/edit-car-function-for-login2.php

It's echoing out information so it must be working.

 

Below is my entire code on my login page. I'm including the function but it's not working.

 

Gizmola, you said the function had to return something. In this case, would it return a $username and $password? in order for it to work?

 

<?php
session_start();
include"connect_to_mysql3.php";
$loggedinuser = $_SESSION["id"];
$name = $_SESSION["name"];

include 'edit-car-function-for-login2.php';

/* if (isset( $_POST['name'], $_POST['password'] )){

   $name = $_POST['name'];
$password = $_POST['password'];

   $password = sha1($password);

$name = mysql_real_escape_string($name);
   $password = mysql_real_escape_string($password);

$sql_id = mysql_query("SELECT * FROM practice WHERE name='$name' AND password='$password'");  
   while($row = mysql_fetch_array($sql_id)){
   $id = $row["id"];
   } 

$sql = ("SELECT id FROM practice WHERE name='$name' AND password='$password'");

$result = mysql_query($sql);

$count=mysql_num_rows($result);

if($count==1){


   $_SESSION["name"] = $name;
   $_SESSION["id"] = $id;

  header("location:heythere.php");


  } else {
  echo "Wrong Username and Password."; 
 } 
} */


?>

<?php
include 'header.php';
?>

<?php

 if ($loggedinuser == true){

 echo "Hi $name, your logged in! <br>";

 echo "<p><a href=\"http://whatsmyowncarworth.com/class-work/sign3/logout.php\">Log Out</a></p>";


} else {


 echo '<table width="300" border="0" align="center" cellpadding="0" cellspacing="1" bgcolor="#CCCCCC">
<tr>
<form name="form1" method="post" action="valchecklogin.php">
<td>
<table width="100%" border="0" cellpadding="3" cellspacing="1" bgcolor="#FFFFFF">
<tr>
<td colspan="3"><strong>Member Login </strong></td>
</tr>
<tr>
<td width="78">Username</td>
<td width="6">:</td>
<td width="294"><input name="name" type="text" id="name"></td>
</tr>
<tr>
<td>Password</td>
<td>:</td>
<td><input name="password" type="text" id="password"></td>
</tr>
<tr>


<td> </td>
<td> </td>
<td><input type="submit" name="Submit" value="Login"></td>
</tr>
</table>
</td>

</form>
</tr>
</table>';



}
?>

<?php
include 'footer.php';
?>

[White_Lily]

I see the problem, your doing it correct in a way that you are including the file, NOT the function.

 

to use functions:

 

include "connect.php";
connect();

 

connect.php:

 

function connect(){
//connection code
}

[gizmola]

@scm22ri,

Looking at your function, it seems that what you were trying to do is combine a few different things. It's best to try and break things down into individual functions that do 1 thing well. In the case where you have one set of data (login credentials) and you want to do a few different things like:

 

-Check if a user is already logged in

-check credentials against database and determine if username/password match

-Store most of the user information in the session along with a "loggedIn" variable.

 

These are all things you want to do at some point, but the flow and combination of when you need to do these things is varied.

 

Let's say you tried this:

 

function isLoggedIn() {
 if (/*something */) {
    return true;
 } else {
    return false;
 }
}

 

Then the only thing that function should do is return true/false. Then at the top of your scripts you could have this, perhaps in a header() include.

 

if (!isLoggedIn()) {
   header('Location: login.php');
   exit();
}

 

If you try and break your code into smaller pieces, it will be more robust, easier to debug and easier to reuse.