4
\$\begingroup\$

I wrote a code which is creating a Session and write in the Session Arrays diffrent things like: Username and Rank.

I wrote then a function which is checking the Visited Profile Name and Rank and the Own Name and Rank to be sure that you have enough power to Promote/Demote.

I want to know if there is a smarter version or if this is the best version I can wrote.

I want to say that I am a newbie in PHP.

The code workes fine btw. I just want ideas and suggestions for improve my code.

Demote/Promote Button Script:

<?php
        if (isset($_SESSION['besucht']) && $_SESSION['besucht'] == 1) {
            if (!empty($own_username) && $_SESSION['user-permissions']-1 == 6 || $_SESSION['user-permissions']-1 == 7) {
                if ($_SESSION['visit_user-permissions']-1 < 6 && $_SESSION['visit_user-permissions']-1 !=  $_SESSION['user-permissions']-1) {
                    echo('
                    <form action="./tools/php/demoter_promoter/index.php" method="POST">
                        <table style="width: 100%;">
                            <tr style="width: 100%;">
                                <td style="width: 50%;">
                                ');
                                if($visit_permissions-1 > 0) {
                                    echo('
                                    <button style="float: left;" title="Den Rank um 1 Degradieren" id="demoter" name="demoter" value="'.$visit_username.'">
                                        Demote
                                    </button>
                                    ');
                                }
                                echo('
                                </td>
                                <td style="width: 50%;">
                                ');
                                if($visit_permissions-1 < 5) {
                                    echo('
                                    <button style="float: right;" title="Den Rank um 1 Befördern" id="promoter" name="promoter" value="'.$visit_username.'">
                                        Promote
                                    </button>
                                    ');
                                }
                                echo('
                                </td>
                            </tr>
                        </table>
                    </form>
                    ');
                }
            }
        }
    ?>

Promote Function:

<?php
    FUNCTION promote($user) {
        require('../connector/connector.php');
        $query = "SELECT * FROM `members` WHERE `username` = ?";
        $stmt = $db->prepare($query);
        $stmt->bind_param('s', $user);
        $stmt->execute();
        $result = $stmt->get_result();
        $stmt->close();
        $obj = $result->fetch_array();
        $permissions = $obj['permissions'];
        if ($permissions < 5) {
            $query = "UPDATE `members` SET `permissions` = ? WHERE `username` = '".$user."';";
            $stmt = $db->prepare($query);
            $permissions = $permissions + 1;
            $stmt->bind_param('s', $permissions);
            $stmt->execute();
            $stmt->close();
            back_to_home($db);
        }
        else {
            back_to_home($db);
        }
    }
?>

The $user variable is seted by the $_SESSION['visit_user-username']

Demote Function:

<?php
    FUNCTION demote($user) {
        require('../connector/connector.php');
        $query = "SELECT * FROM `members` WHERE `username` = ?";
        $stmt = $db->prepare($query);
        $stmt->bind_param('s', $user);
        $stmt->execute();
        $result = $stmt->get_result();
        $stmt->close();
        $obj = $result->fetch_array();
        $permissions = $obj['permissions'];
        if ($permissions != 0 && $permissions < 6) {
            $query = "UPDATE `members` SET `permissions` = ? WHERE `username` = '".$user."';";
            $stmt = $db->prepare($query);
            $permissions = $permissions - 1;
            $stmt->bind_param('s', $permissions);
            $stmt->execute();
            $stmt->close();
            back_to_home($db);
        }
        else {
            back_to_home($db);
        }
    }
?>

The back_to_homefunction is a simple function which is unset all $_SESSION['visit_user-*'] and send back to the stard page.

This is the function caller I wrote:

<?php
    session_start();

    if(isset($_POST['promoter'])) {
        $user = "";
        $user = $_POST['promoter'];
        promote($user);
    }
    else {
        if(isset($_POST['demoter'])) {
            $user = "";
            $user = $_POST['demoter'];
            demote($user);
        }
        else {
            header('Location: ../../../');
            exit;
        }
    }
?>

This is the User Page Loader Function I wrote:

<?php
    $value = "";
    $value = basename($_SERVER['REQUEST_URI']);
    if(isset($_SESSION['user-username']) && $value == "?logout") {
        require('./tools/php/pageloader/pages/logout.php');
    }
    else {
        $search  = '?';
        $replace = '';
        $subject = basename($_SERVER['REQUEST_URI']);
        $new_value = str_replace($search, $replace, $subject);
        $query = "SELECT * FROM `members` WHERE `username` = ?";
        $stmt = $db->prepare($query);
        $stmt->bind_param('s', $new_value);
        $stmt->execute();
        $result = $stmt->get_result();
        $stmt->close();
        $obj = $result->fetch_array();
        if ($obj['username'] != null && $obj['username'] == $new_value) {
            $_SESSION['visit_user-username'] = $obj['username'];
            $_SESSION['visit_user-e_mail'] = $obj['e_mail'];
            $_SESSION['visit_user-register_date'] = $obj['register_date'];
            $_SESSION['visit_user-last_login_date'] = $obj['last_login_date'];
            $_SESSION['visit_user-steam_id'] = $obj['steam_id'];
            $_SESSION['visit_user-permissions'] = $obj['permissions'] + 1;
            $_SESSION['visit_user-ban_status'] = $obj['banned'] + 1;
            require('./tools/php/pageloader/pages/usercheck.php');
        }
        else {
            //echo("<meta http-equiv='refresh' content='0; url=./'>");
        }
    }
?>

My permissions stats with 0 so I needed to write them with + 1 in to the Array and check them with - 1 other wise the MySQL code wouldn't give me the result xD

\$\endgroup\$
  • \$\begingroup\$ I would use a framework, even if just a micro one like Slim or Lumen. \$\endgroup\$ – Alex Aug 2 at 10:27
2
\$\begingroup\$

I don't completely understand your code, which can be a problem when reviewing it, but I think I can get quite far.

Let me start with the two functions, promote() and demote(), that are at the center of your code. There are several things I notice straight away:

  • Both functions are, apart from an + or - sign, identical. There's a well known principle in programming that says: Don't repeat yourself or DRY. Your code is not dry, it is wet. You would only need one function where you use two.

  • There is a require() inside these functions. That is highly unusual in PHP. It means you drag everything that is in that script into the local scope of the function. That is a bad idea because when you're working on the script you might not be aware of the variables in the function, and visa versa.

  • You are preparing the queries, which is good for security, but then you insert the $user variable directly into the query string of the second query, and this variable is user input. That's a big security hole. You did it right in the first query. See: SQL_injection

  • You don't check the result of database operations. It could be that prepare() or execute() return false, in that case you cannot proceed.

  • The change of permission can be done with one simple query, see code below.

From what I can understand of your code we would need two functions: One to read the value of permissions from the database, and the other to update the value permissions. I'm not even looking at the back_to_home() function now, because it should not be part of what we're doing here. So, let's code the two functions we need.

<?php

function getUserPermissions($link, $username)
{
    $query = "SELECT `permissions` FROM `members` WHERE `username` = ?";
    if ($stmt = $link->prepare($query)) {
        $stmt->execute();
        $stmt->bind_result($permissions);
        $stmt->fetch();
        $stmt->close();
        return $permissions;
    }   
    return false;
}

function setUserPermissions($link, $username, $permissions)
{
    $query = "UPDATE `members` SET `permissions` = ? WHERE `username` = ?";
    if ($stmt = $link->prepare($query)) {
        $stmt->bind_param('si', $username, $permissions);
        $success = $stmt->execute();
        $stmt->close();
        return $success;
    }   
    return false;
}

Now let's go one step up in the chain and write the script that accepts the form submission. You located this at "./tools/php/demoter_promoter/index.php".

<?php

session_start();

$input_change = filter_input(INPUT_POST, 'change', FILTER_SANITIZE_NUMBER_INT);

if (isset($_SESSION['user-username'])) {

    require('../connector/connector.php');

    $username    = $_SESSION['user-username'];
    $permissions = getUserPermissions($db, $username);

    switch($input_change) { 
        case +1 : if ($permissions <= 6) {
                      setUserPermissions($db, $username, $permissions + 1);
                  }
                  break;
        case -1 : if ($permissions >= 1) {
                      setUserPermissions($db, $username, $permissions - 1);
                  }
                  break;
    }
}

First of all, for obvious reasons of security I do not post the user name in the form. This would allow any hacker to try other user names. I retrieve the user name from the session, as it was set when the user logged in. This has the added advantage that it checks whether the user is actually logged in.

Instead of using the posted field name, as a way to distinguish between promotion and demotion of the permission, I chose one integer field with the name change. Simply because this is easier. The script only reacts to the values +1 and -1.

I always use the filter functions to do the first filtering of user input at the top of the script. This gives me clear control over the input into the script. These functions do not completely clean user input, but it is a first step. To make clear to myself that something is user input I prefix it with input_, that way I know I have to treat these variables with care.

As you can see I put the require() here, in the global scope.

Now finally the form, where it all starts, the demote and promote button script:

<?php

session_start();

if (isset($_SESSION['user-username'])) {

    require('../connector/connector.php');

    $permissions = getUserPermissions($db, $_SESSION['user-username']);
}

?>
<form action="./tools/php/demoter_promoter/index.php" method="POST">
  <table class="permissionsTable">
    <tr>
      <td><?php 

if (isset($permissions) && ($permissions < 6)) {
    echo '<button id="promoteButton" title="Den Rank um 1 Befördern" ' .  
         'value="+1" name="change">Promote</button>';
} 

     ?></td><td><?php

if (isset($permissions) && ($permissions > 1)) {
    echo '<button id="demoteButton" title="Den Rank um 1 Degradieren" ' .
         ' value="-1" name="change">Demote</button>';
} 

    ?></td>
    </tr>
  </table>
</form>      

I've tried to keep this script simple. Also, I don't understand all the if () conditions your script starts with. You should not use local styles in your HTML, instead use a stylesheet. I'm sure you're familiar with these. A comparable sheet for your local styles would be:

<styles> 

.permissionsTable {
   width: 100%;
}

.permissionsTable td {
   width: 50%;
}

#promoteButton {
   float: right;
}

#demoteButton {
   float: left;
}

</styles>

I hope this answer contains some useful hints for you. Finally a warning: All the code was written without thorough testing. There might be bugs.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.