0

So I have the below code that I'm trying to run partly as a login script, and partly to gather some information for another program. I don't get any errors with the script, I just don't get any information from the second and third mysql_fetch_array which I read is a common problem but that should only apply to the same table. Even so, I followed the recommended advice and used mysql_data_seek to reset the $result. I've also tried using different $result and $row variables but I still don't get any data back from those queries. Any thoughts on how I can do this?

    $result = mysql_query("SELECT * FROM user WHERE username = '$username'");
    $row = mysql_fetch_array($result);
    $salt = $row['salt'];
    $id = $row['id'];
    $usergroup = $row['usergroupid'];
    $mysql_pass = $row['password'];
    $md5_pass = md5($password.$salt);
    mysql_data_seek ($result, 0);
        if($mysql_pass == $md5_pass)
        {
            $result = mysql_query("SELECT teamid FROM tmnt_members WHERE teamid = '$id'");
            $row = mysql_fetch_array($result);
            $team = $row['teamid'];
            $captain = $row['leader'];
            $cocaptain = $row['coleader'];
            mysql_data_seek ($result, 0);
            $result = mysql_query("SELECT * FROM tmnt_teams WHERE teamid = '$team'");
            $row = mysql_fetch_array($result);
            $teamname = $row['teamname'];
        }
Uwop
  • 89
  • 1
  • 2
  • 7
  • 1
    general rule of thumb: if you're running nested queries and the inner queries use data from the outer queries, they should be re-written as a single `JOIN`ed query. Beyond that, you're simply asssuming your queries are succeeding and/or matching something. Never assume success. Always assume failure and treat success as a pleasant surprise. – Marc B May 15 '14 at 00:09
  • The only way the inner queries will get run is if the outer query is a success since I am pulling the hashed password and salt from the database. The tables are large so I didn't know if it would be more or less efficient to JOIN the tables, much less try to JOIN 3. – Uwop May 15 '14 at 00:11
  • yes, but the whole point of a join is to return only the records you want anyways. You'd **STILL** be doing index/table scans for your three individual queries. But with the joined verison, you'd at least be saving 2 rountrips to the db and through the parser/execution engine. – Marc B May 15 '14 at 00:14

2 Answers2

2

You're missing your coleader and leader fields in your second query. Your three queries can be written as one quite simply.

SELECT *
FROM user u, tmnt_members m, tmnt_teams t
WHERE u.username = '$username'
AND m.teamid = u.id
AND t.teamid = m.teamid

Or if you would like to keep the authentication a separate query, you could do SELECT * FROM user WHERE username = '$username' followed by:

SELECT *
FROM tmnt_members m, tmnt_teams t
WHERE m.teamid = '$id'
AND t.teamid = m.teamid

While writing this, I noticed there probably is an error in your second query. The condition teamid = '$id' seems pretty strange. Currently, you're fetching the team which has an ID that is the one of the user. That can't be correct, or if it is, your database structure is very, very strange. I guess it should be something like memberid = '$id'.

Also notice that without the corrections suggested in my first paragraph, this query is asking the database to fetch the ID of a team which has the ID $id. In other words, you could've just used $id directly if that query was correct.

Moreover, doing a SELECT * isn't the best practice; it's better to enumerate all the fields you want explicitly. If you change your column names or do some other modifications to your database, your query may still work, but may not do what you expect it to do.

PLPeeters
  • 1,009
  • 12
  • 26
  • I clean the input using function mres($string) { if(get_magic_quotes_gpc()) { $string = stripslashes($string); } return mysql_real_escape_string($string); } – Uwop May 15 '14 at 00:25
  • In this particular case, this should provide enough protection. However, please note that `mysql_*` functions have been [deprecated](http://stackoverflow.com/questions/12859942/why-shouldnt-i-use-mysql-functions-in-php) for a long time. Consider using an up-to-date database API such as [MySQLi](http://www.php.net/manual/en/book.mysqli.php) or [PDO](http://www.php.net/manual/en/book.pdo.php) and [prepared statements](http://www.php.net/pdo.prepared-statements). Anyway, what about the potential error in your second query? – PLPeeters May 15 '14 at 00:36
  • I'm sorry to insist, but I'm quite sure you hadn't. I've updated my answer. – PLPeeters May 15 '14 at 10:57
0

I've converted your code to MySQLi at least. I have quoted my comments inside. And did try to clean your code. Try this:

<?php

$con=mysqli_connect("Host","Username","Password","Database"); /* REPLACE NECESSARY DATA INSIDE */

if(mysqli_connect_errno()){

echo "Error".mysqli_connect_error();
}

$username=mysqli_real_escape_string($con,$_POST['username']); /* ASSUMING $username COMES FROM A POST DATA. JUST REPLACE IF NECESSARY */

$result = mysqli_query($con,"SELECT * FROM user WHERE username = '$username'");
while($row = mysqli_fetch_array($result)){

$salt = mysqli_real_escape_string($con,$row['salt']);
$id = mysqli_real_escape_string($con,$row['id']);
$usergroup = mysqli_real_escape_string($con,$row['usergroupid']);
$mysql_pass=mysqli_real_escape_string($con,$row['password']);
$md5_pass = md5($password.$salt); /* MAKE SURE YOU HAVE $password AND $salt VARIABLES DECLARED ABOVE */

        if($mysql_pass == $md5_pass)
        {
            $result2 = mysqli_query($con,"SELECT teamid, leader, coleader FROM tmnt_members WHERE teamid = '$id'"); /* ADDED leader AND coleader */
            while($row2 = mysqli_fetch_array($result2)){
            $team = mysqli_real_escape_string($con,$row2['teamid']);
            $captain = mysqli_real_escape_string($con,$row2['leader']);
            $cocaptain = mysqli_real_escape_string($con,$row2['coleader']);

                 $result3 = mysqli_query($con,"SELECT * FROM tmnt_teams WHERE teamid = '$team'");
                 while($row3 = mysqli_fetch_array($result3)){
                 $teamname = mysqli_real_escape_string($con,$row3['teamname']);
                 } /* END OF THIRD LOOP */

            } /* END OF SECOND WHILE LOOP */

        } /* END OF IF MYSQL_PASS IS EQUALS TO MD5_PASS */

/* IS THIS WHERE YOU WANT TO PRINT YOUR RESULTS? */
echo $id." ".$usergroup." ".$team." ".$captain." ".$cocaptain. " ".$teamname;

} /* END OF WHILE LOOP */

?>
Logan Wayne
  • 6,001
  • 16
  • 31
  • 49
  • I put in my database information but it gives an access denied error. It works using mysql_connect. I tried using the while loops with mysql rather than mysqli, is that not supported on mysql? – Uwop May 15 '14 at 02:11
  • You just have to thoroughly check your connection when using MySQLi. Replace the necessary host name, username, password and database name. – Logan Wayne May 15 '14 at 02:32
  • I've updated my answer. If you have noticed, I added leader and coleader in $result2. Was leader and coleader row comes from tmnt_members table or from user table? – Logan Wayne May 15 '14 at 06:59