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.
Page 1 of 2 12 LastLast
Results 1 to 15 of 19
  1. #1
    Regular Coder
    Join Date
    Jan 2006
    Posts
    377
    Thanks
    8
    Thanked 1 Time in 1 Post

    Write the perfect activation script

    Hello,

    I wrote this code below as an activation script. Sometimes it causes problems with users, giving a "You have already activated your account!" message at first click on the activation link in the email.

    Then I went through the code and did not like my logic. I think it could have been written better and simpler. Can you please look at it and make suggestions? Thanks.

    PHP Code:
    <?php

    $uniqueid 
    $_GET['uniqueid'];
    $usrid $_GET['usrid'];

    include (
    "db.php");

    // Check if already activated
    $check1 mysql_query("SELECT * FROM members WHERE ID='$usrid' AND uniqid='$uniqueid' AND activated='1'");
    if(
    mysql_num_rows($check1) == 1) {
        include (
    "header.php");
        echo 
    '<p class="error">You have already activated your account!</p>';
        include (
    "footer.php");
        exit();
    }

    // Check if not activated yet
    $check2 mysql_query("SELECT * FROM members WHERE ID='$usrid' AND uniqid='$uniqueid' AND activated='0'");
    if(
    mysql_num_rows($check2) == 0) {
        include (
    "header.php");
        echo 
    '<p class="error">Activation failed!</p>';
        include (
    "footer.php");
        exit();
    }

    if(
    mysql_num_rows($check2) == 1) {
        
    $sql mysql_query("UPDATE members SET activated='1' WHERE ID='$usrid' AND uniqid='$uniqueid'");
        
    $doublecheck mysql_query("SELECT * FROM members WHERE ID='$usrid' AND uniqid='$uniqueid' AND activated='1'");
        if (
    mysql_num_rows($doublecheck) == 0) {
            include (
    "header.php");
            echo 
    '<p class="error">Activation failed!</p>';
            include (
    "footer.php");
            exit();
        }
        if (
    mysql_num_rows($doublecheck) == 1) {
            include (
    "header.php");
            echo 
    '<p class="success">You have successfully activated your account! Click <a href="login.php">here</a> to login.</p>';
            include (
    "footer.php");
        }
    }
        
    ?>

  • #2
    Senior Coder
    Join Date
    Aug 2003
    Location
    One step ahead of you.
    Posts
    2,815
    Thanks
    0
    Thanked 3 Times in 3 Posts
    The doublecheck is not needed. You only need to see if the update query has affected any rows.
    I'm not sure if this was any help, but I hope it didn't make you stupider.

    Experience is something you get just after you really need it.
    PHP Installation Guide Feedback welcome.

  • #3
    Regular Coder
    Join Date
    Jan 2006
    Posts
    377
    Thanks
    8
    Thanked 1 Time in 1 Post
    Hi Marek, updated the code:
    Apart from mysql_affected_rows, does the logic look OK?


    PHP Code:
    <?php

    $uniqueid 
    $_GET['uniqueid'];
    $usrid $_GET['usrid'];

    include (
    "db.php");

    // Check if already activated
    $check1 mysql_query("SELECT * FROM members WHERE ID='$usrid' AND uniqid='$uniqueid' AND activated='1'");
    if(
    mysql_num_rows($check1) == 1) {
        include (
    "header.php");
        echo 
    '<p class="error">You have already activated your account!</p>';
        include (
    "footer.php");
        exit();
    }

    // Check if not activated yet
    $check2 mysql_query("SELECT * FROM members WHERE ID='$usrid' AND uniqid='$uniqueid' AND activated='0'");
    if(
    mysql_num_rows($check2) == 0) {
        include (
    "header.php");
        echo 
    '<p class="error">Activation failed!</p>';
        include (
    "footer.php");
        exit();
    }

    if(
    mysql_num_rows($check2) == 1) {
        
    $sql mysql_query("UPDATE members SET activated='1' WHERE ID='$usrid' AND uniqid='$uniqueid'");
        if(
    mysql_affected_rows() == 1) {
            include (
    "header.php");
            echo 
    '<p class="success">You have successfully activated your account! Click <a href="login.php">here</a> to login.</p>';
            include (
    "footer.php");
            exit();
        } else {
            include (
    "header.php");
            echo 
    '<p class="error">Activation failed!</p>';
            include (
    "footer.php");
        }
    }
        
    ?>

  • #4
    Regular Coder meth's Avatar
    Join Date
    Jan 2003
    Posts
    262
    Thanks
    0
    Thanked 9 Times in 9 Posts
    There's a gaping hole in your security.

    What happens when a user loads this URL (change the domain/page to yours):

    Code:
    http://www.myDomain.com/activationPage.php?uniqueid=1;SHOW%20TABLES;#&usrid=1
    Never trust external data, whether it comes from a form POST or a URL GET.
    I do Web Design, Brisbane based.
    More time spent in PHP/MySQL Web Development.
    And Search Engine Optimisation takes up the rest of it.

  • #5
    Regular Coder
    Join Date
    Jan 2006
    Posts
    377
    Thanks
    8
    Thanked 1 Time in 1 Post
    Hi,

    It happens nothing. Gives me a "Activation Failed!" message.

    uniqueid is a 32 char randomly generated string based on timestamp.

    7ebbefdf38b0fe16547ea4cb725f632b is a uniqueid.

    This is an email activation. The unique goes to the inbox of the user. Another uniqueid is a bit difficult to guess. What other option do I have other than trusting this external data?

  • #6
    Senior Coder koyama's Avatar
    Join Date
    Dec 2006
    Location
    Copenhagen, Denmark
    Posts
    1,246
    Thanks
    1
    Thanked 5 Times in 5 Posts
    Do you need to send the $usrid ?

  • #7
    Regular Coder
    Join Date
    Jan 2006
    Posts
    377
    Thanks
    8
    Thanked 1 Time in 1 Post
    No, I guess not. But don't want to rely on uniqueid alone. I send the usrid under a different variable (e.g. "confirm") and expect the careful user not to understand this.

  • #8
    Regular Coder meth's Avatar
    Join Date
    Jan 2003
    Posts
    262
    Thanks
    0
    Thanked 9 Times in 9 Posts
    Just ensure the token string is unique before sending the url. Query your db for the same string; if true, send the email; if false, generate another and check again. Perhaps concatenate the userid to the timestamp before md5. This way you can eliminate userid from the url and ensure the token is unique the first time it's generated.

    You also may want to sanitize $_GET['uniqueid'] before using it in a query.
    I do Web Design, Brisbane based.
    More time spent in PHP/MySQL Web Development.
    And Search Engine Optimisation takes up the rest of it.

  • #9
    Senior Coder
    Join Date
    Jan 2007
    Posts
    1,648
    Thanks
    1
    Thanked 58 Times in 54 Posts
    Base your uniqid on the user ID, that way you know it is a unique number (assuming that your user's ID is generated through an auto_increment column).

    You shouldn't need 4 queries to do activation.

    You just select where activated equals 0 and it has the right uniqid. If it has a row (only 1), then update the table to set activated.

    If it doesn't have any rows, print something like "Activation failed or you have already activated your account."

    There is no need to do queries like that. If you really needed to know whether they are activated already (versus it failing), just select without the where activated. And do the logic in PHP (if row['activated'] == 0, etc.).

  • #10
    Regular Coder
    Join Date
    Jan 2006
    Posts
    377
    Thanks
    8
    Thanked 1 Time in 1 Post
    I wrote the following to generate the uniqid, could be done with a single function?


    PHP Code:

    function CheckToken($token) {
        include(
    "db.php");
        
    $result mysql_query("SELECT uniqid FROM members WHERE uniqid='$token'") OR die(mysql_error());
        if (
    mysql_num_rows ($result) == 0) { // does not exist
            
    return FALSE;
        } else { 
    // exists
            
    return TRUE;
        }
    }


    function 
    GenerateToken() {
        
    $token md5(uniqid(rand(), TRUE));
        while(
    CheckToken($token)) {
            
    GenerateToken();
        }
        return 
    $token;
    }


    // generate uniqid
    $uniqid GenerateToken(); 

  • #11
    Senior Coder
    Join Date
    Jan 2007
    Posts
    1,648
    Thanks
    1
    Thanked 58 Times in 54 Posts
    That's a pretty bad setup.

    Although uniqid is generally pretty unique and similar numbers don't occur often, you are setting yourself up for a really bad situation where a simple script will drag down the database and server.

    Like I said, your best bet is to generate it from the User ID.

    PHP Code:
    query("INSERT INTO users(id, name) VALUES(NULL, 'John Doe')");

    $id mysql_insert_id();

    $uniqid uniqid($id);

    query("UPDATE users SET uniqid = '{$uniqid}' WHERE id = {$id}"); 

  • #12
    Regular Coder
    Join Date
    Jan 2006
    Posts
    377
    Thanks
    8
    Thanked 1 Time in 1 Post
    So you say I should add the User ID into the uniqid as a prefix? Hmmm. Can't say I like that that much

  • #13
    Super Moderator Inigoesdr's Avatar
    Join Date
    Mar 2007
    Location
    Florida, USA
    Posts
    3,647
    Thanks
    2
    Thanked 406 Times in 398 Posts
    Quote Originally Posted by guvenck View Post
    So you say I should add the User ID into the uniqid as a prefix? Hmmm. Can't say I like that that much
    You don't need the user ID to make it random, there are literally hundreds of reasonable possibilities. This should work fine:
    PHP Code:
    $uniqid sha1(uniqid(time(),1)); 
    md5() will work fine instead of sha1() too. You could substr() it if you wanted to a smaller string, or even generate a random number from the strlen() to substr it to.

  • #14
    Regular Coder
    Join Date
    Jan 2006
    Posts
    377
    Thanks
    8
    Thanked 1 Time in 1 Post
    Hi, Inigoesdr, I am trying to make it unique rather than random.

    I decided to use this:

    Code:
    $uniqid = uniqid(md5($userid));
    That generates 45 characters in total. I've also added a check in case the number exists, it exits and emails me

    Couldn't write something to regenerate it if it exists

  • #15
    Super Moderator Inigoesdr's Avatar
    Join Date
    Mar 2007
    Location
    Florida, USA
    Posts
    3,647
    Thanks
    2
    Thanked 406 Times in 398 Posts
    Using that code will always be unique. You don't need to check.


  •  
    Page 1 of 2 12 LastLast

    Posting Permissions

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