Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Results 1 to 6 of 6
  1. #1
    Regular Coder
    Join Date
    Jul 2003
    Location
    New Zealand
    Posts
    435
    Thanks
    1
    Thanked 0 Times in 0 Posts

    question with my login system

    alright, ive written a login system where it checks ur input (duh) and then gets a number, 0 or 1 from an Administrator column in the db, and if its 1 it takes you to admin.php and if its 0 it takes you to user.php

    now the admin one if working fine, but when i login to the user.php one, i can see my content. i think its something to do with my if statements, ill post the code for both login.php and user.php

    ( index.php?id=7 is a login form )

    Code:
    include 'config.php';    
    $username = $_POST['username'];  
    $password = md5($_POST['password']);   
    $query = mysql_query("SELECT * FROM users") or die(mysql_error());  
    while($row=mysql_fetch_array($query)){  	
    if(($username==$row['Username'])&&($password==$row['Password'])){  		  		
    if($row['Administrator'] == 1){  			
    $_SESSION['isrobbieloggedin'] = 1;  			
    $_SESSION['adminname'] = $row['Name'];  			
    header("Location: admin.php");  		} else {  			
    $_SESSION['username'] = $row['Name'];  			
    $_SESSION['isuserloggedin'] = 1;  			
    $_SESSION['userid'] = $row['id'];  			
    header("Location: user.php");  		
    }  	  	
    }	  
    }  
    if(!$username == NULL){ echo $username.', y'; } else { echo 'Y'; }  
    echo 'our username or password was incorrect. Please hit the back button and try again.';
    ( login.php )

    Code:
    	$id = $_SESSION['userid'];
    	$query = mysql_query("SELECT * FROM users WHERE id = '$id'") or die(mysql_error());
    	while($row = mysql_fetch_array($query)){
    		
    	if(isset($_POST['Submit'])){
    		$id = $_SESSION['userid'];
    		$name = $_SESSION['username'];
    		$website = $_POST['website'];
    		$testemonial = $_POST['testemonial'];
    		mysql_query("INSERT INTO testemonials(Testemonial, Author, Website, UserID) VALUES('$testemonial','$name','$website','$id')") or die(mysql_error());
    		mysql_query("UPDATE users SET HasPosted = '1' WHERE id = '$id'") or die(mysql_error());
    		echo 'Your testimonial has been added. You <b>cannot</b> edit or post another or delete your testimonial.';
    	} else {
    		if($row['HasPosted']==0){ ?>
    			<div align="center">
    		<form action="user.php" method="post" name="post" id="post">
    		<p>&nbsp;</p>
    		  <table width="500" border="0">
                <tr> 
                  <td width="95" align="center" valign="middle"><div align="center">Your 
                      testemonial:</div></td>
                  <td width="395"> <div align="center"> 
                      <textarea name="testemonial" cols="60" rows="10" id="textarea"></textarea>
                    </div></td>
                </tr>
                <tr> 
                  <td align="center" valign="middle"><div align="center">Posted by:</div></td>
                  <td><div align="center"><? echo $_SESSION['username']; ?> </div></td>
                </tr>
                <tr> 
                  <td align="center" valign="middle"><div align="center">Your URL:</div></td>
                  <td> <div align="center"> 
                      <input type="text" name="website">
                    </div></td>
                </tr>
                <tr> 
                  <td colspan="2" align="center" valign="middle"> <div align="center"> 
                      <input type="submit" name="Submit" value="Submit">
                    </div></td>
                </tr>
              </table>
    			<p>&nbsp;</p>
    			</form>
    			</div>
    		
        </td>
    	  <? 	
    		
    		} else {
    			echo 'You have already posted your testimonial.';
    		}
    		}
    		
    		
    		
    	}
    ( user.php )

    i think it's where my if statement says if($condition== ... im not sure why but i get different results when i change the two == to one =.. which should i use and how to get this script working?

    btw, instead of seeing what i should on user.php i just see blank.

  • #2
    Regular Coder
    Join Date
    May 2002
    Location
    Virginia, USA
    Posts
    621
    Thanks
    0
    Thanked 6 Times in 6 Posts
    I'll have to check the code @ work (where i have sessions enabled). In the meantime, I wonder if you should change this line:

    PHP Code:
    if(!$username == NULL){ echo $username.', y'; } else { echo 'Y'; } 
    to this:
    PHP Code:
    if($username != NULL){ echo $username.', y'; } else { echo 'Y'; } 

  • #3
    Regular Coder
    Join Date
    May 2002
    Location
    Virginia, USA
    Posts
    621
    Thanks
    0
    Thanked 6 Times in 6 Posts
    I'd also put exit() statements after your header() statements.


    Here's the other problem....

    Your while() statement continues on and on and on...
    even after it's done, it will continue until user...


    Change your sql statement.


    Code:
    "SELECT * FROM users WHERE Username = '$username')"
    That way, it only pulls from the database what's needed. If no rows are returned, then the username doesn't exist.





    An Example:
    PHP Code:
    <?
    include 'config.php';    
    $username $_POST['username'];  
    $password md5($_POST['password']);   
    $sql "SELECT * FROM users WHERE Username = '$username'";
    $result mysql_query($sql) or die(print mysql_error());

    if (
    $result) {
        while (
    $row mysql_fetch_assoc($result)) {
             
              
    /* catch bad password */
              
    if ($password != $row["Password"]) {
                    print 
    "Bad Password";
                    break 
    2;
              }


              
    /* check administrator password */
              
    if ($row["Administrator"] == "1") {
                    print 
    "Houston, we have a winner.";
                    break 
    2;
              } else {
                    print 
    "Houston, we have a loser, er...I mean....USER...";
                    break 
    2;
              }
             

        } 
    // while
    // if


    ?>

    I typed that here in this post, so if it doesn't work, uhm...i'll revise it.


    Hope That helps, John
    Last edited by Celtboy; 12-19-2003 at 10:28 PM.

  • #4
    raf
    raf is offline
    Master Coder
    Join Date
    Jul 2002
    Posts
    6,589
    Thanks
    0
    Thanked 0 Times in 0 Posts
    I don't understand that code. Why isn't the password included in the where clause?
    And why break 2 ? There is only one structure you can break out of in that code.
    I also don't understand why you don't check if mysql_num_rows == 1 You should, on succesful login, only get 1 row and there is no need for a while loop (even in your present code)
    die(print mysql_error()); --> why the print? Die will automatically print what's between the brackets (unledd it's an integer)

    You'd better also check your inputted values agains SQL-injections and cerainly use addslashes() on the string-formvalues you use inside the sql-statement.


    I would also recommend using sha1() or sha2() instead of md5(). (md5() seams to be compromised and is apparently explicitely refused by VISA).

    Also, I personaly would store the pageadress to redirect to after a succesfull login, inside the db, and use that inside your code --> more flexability at no cost. Users could then pick their own startpage and your code gets quit a bit shorter.
    Because if you incule both username and pwd inside the where clause, you only need
    PHP Code:
    if (mysql_num_row($result)== 1) {
       
    $rowmysql_fetch_assoc($result);
       
    header("Location: " .  $row['url]' ".php");
    } else {
      echo 
    'Invalid login. Try again';


  • #5
    Regular Coder
    Join Date
    May 2002
    Location
    Virginia, USA
    Posts
    621
    Thanks
    0
    Thanked 6 Times in 6 Posts
    Hmmmm. raf, thou hast made excellent points. The break didn't need 2, you're correct. as well on the print mysql_error(). I realized both upon posting, but didn't have time to edit (I had a party to go to!!!)

    I like the idea of just checking the num_rows returned. Hadn't thought of that. THAT is what happens when you code php for years in a little hole and never actually DISCUSS it with anyone.... lol... thanks for the pointer, I think I'll switch my login functions

    Although, I must add, if I can find a reason NOT to use the num_rows_returned, I'll tell ya hee hee.

    AH-HA! I just thought of a reason....

    What if you store the password somewhere else...like in an LDAP server! HA-HA! Of course, you'd prolly store the username there too....but that's beside the point!! MUA HAHHAA.. ok... so you're right. In this particular case, there is ABSOLUTELY no point in making things more complicated than they need to be.

    Good advice!

    And I'll totally agree with the encryption (hashing) of the password. I didn't even look at that part of the code, just accepted it.....lol

    excellent suggestions....thanks for pointing them out!

  • #6
    raf
    raf is offline
    Master Coder
    Join Date
    Jul 2002
    Posts
    6,589
    Thanks
    0
    Thanked 0 Times in 0 Posts
    i'm glad you agree.

    by the way, inside the original code, there was the "select * "you copied ---> NEVER use "select * " Always specify the variables you actually need (even if you do actually need all variables --> if you later on add a column to your db, it will also be returned while you don't us it).

    the sessionvariable of the orgiginal code are also a bit to complicated.
    Instead of having seperate sessionvariables for the admin and the regular user, i woul use one sessionvariable, and store a security-profile value inthere. Admin = 10, regular user = 4 or so., not logged in = 0 or Empty. Then , for each page, you can compaire the securityprofile with the minimum required securityprofile for that page (or, for that part of the pages content or whatever.) For bigger apps, i always keep a pages-table to dynamically build the html-header, and where i store the minimum security info and navigationoption is.

    So for all page-profile compinations, you can do with one check
    if ($_SESSION['secprof'] >= $pagesecprof) {
    --> less code; less sessionvariables to manage; the code actually expresses what you logically want to do; completey dynamic etc.


  •  

    Posting Permissions

    • You may not post new threads
    • You may not post replies
    • You may not post attachments
    • You may not edit your posts
    •