6
\$\begingroup\$

I am currently working on a small website for my scuba diving club, and it needs a "simple" login system to allow members-only features etc.

I've been using the php-login-minimal as the base for the PHP login: https://github.com/panique/php-login-minimal It works by creating a session cookie if a user is logged in. This works just perfectly well on my local XAMPP server, but when uploaded to my external domain (one.com), logging in does not always work. It is as if no session cookie is being set. Eventually, after a bunch of tries, it'll work. These troubles do not occur on localhost, however, so I might just have to contact the domain support about that.

What I would also like to get feedback on, is if everything is done reasonably or if I should work on improving/changing various parts of my website system.

Any feedback in general is greatly appreciated. Have never done much PHP/HTML, so would love to improve.

Here's what a member-only page currently looks like:

members.php (shows the members in the diveclub, fetched from the MySQL db)

<?php
require_once('authorize.php');
?>

<!DOCTYPE html>

<?php
require_once('includes/load.php');
GLOBAL $db;

require_once('includes/functions.php');
?>

<html>
    <head>
        <meta charset="utf-8">
        <link rel="stylesheet" type="text/css" href="css/stylesheet.css">

        <title>Medlemsliste - <?php echo SITE_TITLE ?></title>
    </head>

    <body>
    <div id="container">
        <div id="header">
            <div id="header-menu">
                <img id="logo" src="img/nsdk_logo_header.png">
                <ul id="main-menu">
                    <li><a href="index.php">Forsiden</a></li>
                    <li><a href="omklubben.php">Om klubben</a></li>
                    <li><a href="kontakt.php">Kontakt</a></li>
                </ul>
            </div>
        </div>

        <img id="top-image" src="img/header/foto.jpg">

        <?php include("sidebar.php"); ?>

        <div id="content-container">
            <span class="content-header">Medlemsliste</span>
            <div class="content">
                <h1>Medlemmer af NSDK</h1>
                <p>Du kan sende mail til et medlem ved at sende en mail til <code>fornavn.efternavn@nsdk.dk</code>, f.eks. <a href="mailto:anders.and@nsdk.dk">anders.and@nsdk.dk</a>.


<?php
$is_moderator = (get_access_from_username($_SESSION['username']) > 0);

if ($is_moderator)
    echo '<p><b>NB: Du er logget ind som et bestyrelsesmedlem og bliver derfor også oplyst adresse på medlemmer!</b></p>';
?>

                <!-- LIST BEGIN -->
                <table id="table-members" width="100%">
                    <tr>
                        <th><a href="members.php?order=firstname">Fornavn</a></th>
                        <th><a href="members.php?order=lastname">Efternavn</a></th>
                        <th>Tlf. nr.</th>
                        <th><a href="members.php?order=certificate">Certifikat</a></th>
                        <?php if ($is_moderator) { ?>
                        <th>Adresse</th>
                        <?php } ?>
                    </tr>
<?php
if (!empty($_GET['order']))
{
    if ($_GET['order'] == 'certificate')
        $queryResult = $db->query('SELECT id, firstname, lastname, phone, certificate, address FROM dc_members ORDER BY certificate, lastname, firstname;');
    else if ($_GET['order'] == 'lastname')
        $queryResult = $db->query('SELECT id, firstname, lastname, phone, certificate, address FROM dc_members ORDER BY lastname, firstname;');
    else if ($_GET['order'] == 'firstname')
        $queryResult = $db->query('SELECT id, firstname, lastname, phone, certificate, address FROM dc_members ORDER BY firstname, lastname;');
    else
        $queryResult = $db->query('SELECT id, firstname, lastname, phone, certificate, address FROM dc_members ORDER BY lastname, firstname;');

} else
    $queryResult = $db->query('SELECT id, firstname, lastname, phone, certificate, address FROM dc_members ORDER BY lastname, firstname;');

$members_amount = 0;
while ($row = mysqli_fetch_array($queryResult)) {
    echo '<tr>';
    echo '<td>'.$row['firstname'].'</td>';
    echo '<td>'.$row['lastname'].'</td>';
    echo '<td>'.$row['phone'].'</td>';
    echo '<td>'.$row['certificate'].'</td>';

    if ($is_moderator)
    {
        echo '<td>'.$row['address'].'</td>';
    }

    echo '</tr>';

    $members_amount++;
}

?>
                </table>
                <!-- LIST END -->

                <br>
                <p>Antal medlemmer: <?php echo '<b>'.$members_amount.'</b>' ?> </p>
            </div>
        </div>

    </div>
    </body>
</html>

authorize.php

<?php
require_once('includes/load.php');

require_once('includes/login.php');
$login = new Login();

if (!$login->isUserLoggedIn()) {
    // not logged in, redirect to login
    header('Location: http://www.nsdk.dk/ny/login.php');
    echo "FEJL: Denne side kræver login!";
    die();
}
?>

includes/load.php

<?php
if (!file_exists('configuration.php')) {
    require_once('configuration_default.php');
} else {
    require_once('configuration.php');
}

// initialize mysql db
$db = new database(DB_USER, DB_PASSWORD, DB_NAME, DB_HOST);

// https://github.com/panique/php-login-minimal
// check for minimum php version
if (version_compare(PHP_VERSION, '5.3.7', '<')) {
        exit("Sorry, Simple PHP Login does not run on a PHP version smaller than 5.3.7 !");
} else if (version_compare(PHP_VERSION, '5.5.0', '<')) {
    // if you are using PHP 5.3 or PHP 5.4 you have to include the password_api_compatibility_library.php
    // (this library adds the PHP 5.5 password hashing functions to older versions of PHP)
    require_once("libraries/password_compatibility_library.php");
}
?>

includes/login.php (only differs slightly from the original php-login-minimal)

https://github.com/panique/php-login-minimal/blob/master/classes/Login.php

includes/database.php (loaded from configuration.php)

<?php

class database {
    protected $dblink;

    protected $dbuser;
    protected $dbpassword;
    protected $dbname;
    protected $dbhost;

    // query result
    protected $result;

    function __construct($dbuser, $dbpassword, $dbname, $dbhost){
        $this->dbuser = $dbuser;
        $this->dbpassword = $dbpassword;
        $this->dbname = $dbname;
        $this->dbhost = $dbhost;

        $this->connect();
    }

    function connect(){
        $this->dblink = mysqli_connect($this->dbhost, $this->dbuser, $this->dbpassword, $this->dbname)
        or die('Could not connect: ' . mysqli_error($link ));
    }

    function query($query){
        $this->result = mysqli_query($this->dblink, $query) or die('Query failed: ' . mysqli_error($this->dblink));
        return $this->result;
    }

    function get_row($query = null) {
        if ($query) {
            $this->query($query);
        } else {
            return null;
        }
    }
}

?>
\$\endgroup\$
2
\$\begingroup\$

Overall your code looks fine

If you are having trouble with sessions, check the live server configuration, especially the cookie domain and path.

I have put some inline comments in the code, the changes I have done will make it a bit easier to maintain (in my opinion) but are not essential.

<?php
require_once('authorize.php');
?>

<!DOCTYPE html>

<?php
require_once('includes/load.php');
GLOBAL $db;

require_once('includes/functions.php');


$is_moderator = (get_access_from_username($_SESSION['username']) > 0);

// move database access to top of code
// if an error occurs you can show an error message before you render the page, rather then have a mess in the middle of the page
if (!empty($_GET['order']))
{
    if ($_GET['order'] == 'certificate')
        $queryResult = $db->query('SELECT id, firstname, lastname, phone, certificate, address FROM dc_members ORDER BY certificate, lastname, firstname;');
    else if ($_GET['order'] == 'lastname')
        $queryResult = $db->query('SELECT id, firstname, lastname, phone, certificate, address FROM dc_members ORDER BY lastname, firstname;');
    else if ($_GET['order'] == 'firstname')
        $queryResult = $db->query('SELECT id, firstname, lastname, phone, certificate, address FROM dc_members ORDER BY firstname, lastname;');
    else
        $queryResult = $db->query('SELECT id, firstname, lastname, phone, certificate, address FROM dc_members ORDER BY lastname, firstname;');

} else {
    $queryResult = $db->query('SELECT id, firstname, lastname, phone, certificate, address FROM dc_members ORDER BY lastname, firstname;');
}


// i have re-written this part, there is nothing wrong with your code
// this is personal preference for readability, see what you think
$sql = 'SELECT id, firstname, lastname, phone, certificate, address FROM dc_members ORDER BY ';

$param_order = (!empty($_GET['order'])) ? $_GET['order'] : null;

switch($param_order) {
    case 'certificate':
        $order_by = 'certificate, lastname, firstname';
        break;

    case 'firstname':
        $order_by = 'firstname, lastname';
        break;

    case 'lastname':
    default:
        $order_by = 'lastname, firstname';
        break;
}

// only do query in one place, simpler if you want to handle errors
$queryResult = $db->query($sql.$order_by);


// mysqli_fetch_array returns a number indexed array and a associative array combined, mysqli_fetch_assoc is all you need
// while ($row = mysqli_fetch_array($queryResult)) {

$members = array();

while ($row = mysqli_fetch_assoc($queryResult)) {
    $members[] = $row;
}

// $members_amount = 0; $members_amount++;
$members_amount = count($members);

// only put code for presentation logic below here

?>

<html>
<head>
    <meta charset="utf-8">
    <link rel="stylesheet" type="text/css" href="css/stylesheet.css">

    <title>Medlemsliste - <?php echo SITE_TITLE ?></title>
</head>

<body>
<div id="container">
    <div id="header">
        <div id="header-menu">
            <img id="logo" src="img/nsdk_logo_header.png">
            <ul id="main-menu">
                <li><a href="index.php">Forsiden</a></li>
                <li><a href="omklubben.php">Om klubben</a></li>
                <li><a href="kontakt.php">Kontakt</a></li>
            </ul>
        </div>
    </div>

    <img id="top-image" src="img/header/foto.jpg">

    <?php include("sidebar.php"); ?>

    <div id="content-container">
        <span class="content-header">Medlemsliste</span>
        <div class="content">
            <h1>Medlemmer af NSDK</h1>
            <p>Du kan sende mail til et medlem ved at sende en mail til <code>fornavn.efternavn@nsdk.dk</code>, f.eks. <a href="mailto:anders.and@nsdk.dk">anders.and@nsdk.dk</a>.


                <?php
                // move non-presentation code to top
                // $is_moderator = (get_access_from_username($_SESSION['username']) > 0);

                if ($is_moderator)
                    echo '<p><b>NB: Du er logget ind som et bestyrelsesmedlem og bliver derfor også oplyst adresse på medlemmer!</b></p>';
                ?>

                <!-- LIST BEGIN -->
            <table id="table-members" width="100%">
                <tr>
                    <th><a href="members.php?order=firstname">Fornavn</a></th>
                    <th><a href="members.php?order=lastname">Efternavn</a></th>
                    <th>Tlf. nr.</th>
                    <th><a href="members.php?order=certificate">Certifikat</a></th>
                    <?php if ($is_moderator) { ?>
                        <th>Adresse</th>
                    <?php } ?>
                </tr>

                <?php
                    // member info could contain html characters, make sure we escape them with htmlspecialchars or htmlentities
                    foreach ($members as $member):
                ?>
                    <tr>
                        <td>><?php echo htmlspecialchars($member['firstname']) ?></td>
                        <td><?php echo htmlspecialchars($member['lastname']) ?></td>
                        <td><?php echo htmlspecialchars($member['phone']) ?></td>
                        <td><?php echo htmlspecialchars($member['certificate']) ?></td>

                        <?php if ($is_moderator): ?>
                            <td><?php echo htmlspecialchars($member['address']) ?></td>
                        <?php endif ?>

                    </tr>
                <?php endforeach ?>
            </table>
            <!-- LIST END -->

            <br>
            <?php
                // why do we echo <b> tag?
                // <p>Antal medlemmer i Nakskov Sportsdykkerklub: <?php echo '<b>'.$members_amount.'</b>' ?> </p>
            ?>
            <p>Antal medlemmer i Nakskov Sportsdykkerklub: <b><?php echo $members_amount ?></b> </p>

        </div>
    </div>

</div>
</body>
</html>
\$\endgroup\$
  • \$\begingroup\$ Thanks for your feedback! I will run through your comments and most certainly try to get in contact with the domain support, to see if I can get the session cookies fixed! Cheers. \$\endgroup\$ – Marcus Møller Jan 27 '14 at 7:17

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.