Jump to content

Archived

This topic is now archived and is closed to further replies.

Chips

Advice on code structure/technique

Recommended Posts

As you may have guessed by the title, I am not profficient in php at all - I am learning as I go along. Indeed, I learnt the login system via a tutorial on this very site (which was excellent) - however, I am expanding parts of the tutorial.

Essentially I have my login pages, verify, validate (for registering) register, etc - but once a user is logged in I am trying to create a profile area where they can access their personal profile.

I have done this, but am worried that the coding and way of implementation leaves a lot to be desired, hence my asking. The code is below, essentially if a user clicks on a link to their profile once logged in, they get taken to the profile.php page (I use sessions during the log in process). Different parts of the script will be loaded dependant upon what the user is wanting to do.

Here is the file overall:

[code]<?
session_start();
include 'db.php';

/*
* This will be the profile page for users to change passwords to more acceptable passwords.
* User not logged in will be sent straight back to the login form.
*/

//check if session exists...
if($_SESSION['access_level']) {     //if not
  //send them to the login page!
  header('location: http://'.$_SERVER['HTTP_HOST'].dirname($_SERVER['PHP_SELF']).'/login.php');
  exit();
}

/*
* If a user wants to change their password, this is the code from here down. First it will provide a
* form for the user to log in via the same page. After this, it will then take the value of the password
* entered in the form and change their password accordingly. Finally user is returned back to their profile area
* as per the code below this section.
*/

//check whether the request of change password was past or not.
  if($_REQUEST['change_password']) {
     echo "<form name=\"change_pass\" action=\"profile.php\" method=\"post\"><span>Enter new password:</span><input type=\"password\" name=\"password\" value=\"\"/><br /><span>Verify new password:</span><input type=\"password\" name=\"password2\" value=\"\"/><br /><input type=\"submit\" value=\"change\" /></form>";
     exit();
  }
  
  //check whether a post value for password has been received.
  if($_POST['password']) {
    $password = md5($_POST['password']);
    $password_check = md5($_POST['password2']);
    if($password != $password_check) {
      echo "passwords did not match, please try again.<br />
      <a href=\"profile.php?change_password=true\">Try again</a>";
      exit();
    }
    $userId = $_SESSION['user_id'];
    $email = $_SESSION['email_address'];
    $sql_update = mysql_query("UPDATE users SET password = '$password' where userId = '$userId' and email = '$email';");
    $sql = mysql_query("SELECT userId FROM users WHERE userId = '$userId' and password = '$password' and email = '$email';");
    $sql_check = mysql_num_rows($sql);
         if($sql_check > 0) {
                 echo "<p>Password update succcessful</p>
                 <a href=\"profile.php\">My Profile</a>";
                 exit();
         } else {
                 echo "Password update has failed";
                 header('location: http://'.$_SERVER['HTTP_HOST'].dirname($_SERVER['PHP_SELF']).'/profile.php');
                 exit();
         }
    }
    
    
    /*
    * Default display for when a user first logs into their area.
    */
    $name = $_SESSION['first_name'];  //get the users name
    
    //provide the html links for the user to choose what to do.
    echo "<p>Welcome $name, this is your personal profile area.</p>
    <a href=\"profile.php?change_password=true\">Change password</a><br />
    Link 2<br />
    Link 3<br />
    Link 4<br />
    <a href=\"logout.php\">Logout</a>";

?>[/code]

Mainly these bits - if a user clicks on this link:[code]
<a href=\"profile.php?change_password=true\">Change password</a><br />[/code]
You can see it links back to this page but with a request of change_password=true.
This in turn will load up this part of the script:
[code]if($_REQUEST['change_password']) {
     echo "<form name=\"change_pass\" action=\"profile.php\" method=\"post\"><span>Enter new password:</span><input type=\"password\" name=\"password\" value=\"\"/><br /><span>Verify new password:</span><input type=\"password\" name=\"password2\" value=\"\"/><br /><input type=\"submit\" value=\"change\" /></form>";
     exit();
  }[/code]

Essentially it loads the page with two password text boxes. The user inserts their password twice and hits submit. When submitted it submits to the same old profile.php once more - where it should be picked up by this part:[code]if($_POST['password']) {
    $password = md5($_POST['password']);
    $password_check = md5($_POST['password2']);
    if($password != $password_check) {
      echo "passwords did not match, please try again.<br />
      <a href=\"profile.php?change_password=true\">Try again</a>";
      exit();
    }
    $userId = $_SESSION['user_id'];
    $email = $_SESSION['email_address'];
    $sql_update = mysql_query("UPDATE users SET password = '$password' where userId = '$userId' and email = '$email';");
    $sql = mysql_query("SELECT userId FROM users WHERE userId = '$userId' and password = '$password' and email = '$email';");
    $sql_check = mysql_num_rows($sql);
         if($sql_check > 0) {
                 echo "<p>Password update succcessful</p>
                 <a href=\"profile.php\">My Profile</a>";
                 exit();
         } else {
                 echo "Password update has failed";
                 header('location: http://'.$_SERVER['HTTP_HOST'].dirname($_SERVER['PHP_SELF']).'/profile.php');
                 exit();
         }
    }[/code]

Obvioulsy passwords are hashed before being entered into database, and checked to ensure they match.
The question is - is this good or bad practice to keep linking back to the same file but just to different portions of it? If it's okay, is there a better way to execute this behaviour than the way I have done (for example by directing them to the profile.php?change_password=true)? Same for the form too - is that okay?
I've done a similiar thing with my login.php script where if a session exists, then you don't get the login form - but instead a greeting and a link to the profile.php and a logout link instead.

Furthermore, the use of echo to print out things in combiation with a header(location: ) doesn't seem to go down well. One or the other works fine, but not both... so is using an include right for the behaviour of (say) displaying the profile page again but with an added message of changing passwords failed or something?

Any hints/tips/comments/critique/suggestions etc are very welcome, and I will post up any of the code requested. I am programming this for a small company at the moment, and I want to not only make the best job I possibly can... but to also learn [i]good[/i] practices too! Although the finished deal is a long way off and this is just the ugly implementation of it, I need all the help I can get to make sure I don't miss/mess up/botch things up.

Also - the login is via email/password. The email is checked to be a valid email address (via a check of the characters making it up), and the password hashed and checked against the stored hashed password. Is this safe against an sql_injection type of attack without needing to do the slashes issue?

And lastly, there is no way for anyone to read your php files are there from a browser? It's a server side executed language, so the script isn't transmitted, only the data generated right?

Here's hoping for some tips/thoughts etc, and thanks for taking the time to read the post.

Share this post


Link to post
Share on other sites

×

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.