Check my code

Haris · 10-03-2007, 11:20 PM

PHP Code:


			
<?php

    /* 
    - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
    Title : Schools Management
    Author : Muhammad Haris
    URL : http://www.mharis.net
    CONTACT: isharis@gmail.com
    
    Description : School list with show, add, edit or delete
    functionality.
    
    Created : 29th September 2007
    Modified : 3th October 2007
    
    - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
    */

    include_once('../includes/includes.php');
    
    //
    // Initiates new objects
    //
    
    $tpl =& new Savant2();
    $validate = new FormValidator;
    $database = new Database;
    $auth = new Auth;
    $users = new Users;
    
    
    //
    // Check if user is logged in
    //
    
    if($auth->check() == false || $auth->admin_auth() == false){
        echo 'Please login as administrator';
    }
    
    // If logged in ->
    else {
        include_once('navigation.php'); // Includes Navigation
        
        //
        // Assigning post values to variables
        //
        
        $szName = $_POST['name'];
        $szURL = $_POST['url'];
        $szLocationField1 = $_POST['locationField1'];
        $szLocationField2 = $_POST['locationField2'];
        $szCity = $_POST['city'];
        $szState = $_POST['state'];
        $iZipCode = $_POST['zipCode'];
        $iAreaCode = $_POST['areaCode'];
        $szTelephoneNumber = $_POST['telephoneNumber'];
        $szUser = $_POST['user'];
        
        //
        // Validation rules
        //
        
        if(isset($_POST['add_school'])){
            $validate->isEmpty('szName', 'Please enter an username');
            $validate->isEmpty('szURL', 'Please enter an URL');
            $validate->isURL('szURL', 'Please enter a valid URL (www.domain.ext)');
            $validate->isEmpty('szLocationField1', 'Please fill in a location field 1');
            $validate->isEmpty('szCity', 'Please enter a city');
            $validate->isEmpty('szState', 'Please enter a state');
            $validate->isUSZipCode('iZipCode', 'Please enter correct US zip code format (00000)');
            $validate->isUSAreaCode('iAreaCode', 'Please enter correct US area code format (000)');
            $validate->isUSTelephone('szTelephoneNumber', 'Please enter correct US telephone number format (000-111-0000)');
        }
        
        //
        // Fetches usernames from users table
        //
        
        $szSQL = "SELECT user FROM users WHERE rank = 2 AND assigned = 0";
        $aResult = $database->execute($szSQL);
        while($szRow = mysql_fetch_array($aResult, MYSQL_ASSOC)){
            $aUser[] = $szRow['user'];
        }
        $tpl->assign('aUser', $aUser);
        
        
        //
        // Assigns errors to add school form
        //
        
        if($validate->isError() && isset($_POST['add_school'])){
            $tpl->assign('aError', $validate->getErrorList());
        }
        
        //
        // Add a new school
        //
        
        if(isset($_POST['add_school']) && $validate->isError() == false){
            
            // Fetches ID of the selected user
            $szSQL = "SELECT id FROM users WHERE user = '$szUser' LIMIT 0,1";
            $iResult = $database->fetch($szSQL);
            foreach($iResult as $iUserID){
                $iUser = $iUserID;
            }
            
            $aColumnNames = array(    
                'name',
                'url',
                'locationField1',
                'locationField2',
                'city',
                'state',
                'zipCode',
                'areaCode',
                'telephoneNumber',
                'user_id'
            );
            
            $aValue = array(
                "'$szName'",
                "'$szURL'",
                "'$szLocationField1'",
                "'$szLocationField2'",
                "'$szCity'",
                "'$szState'",
                "'$iZipCode'",
                "'$iAreaCode'",
                "'$szTelephoneNumber'",
                "'$iUser'",
            );
            
            $szColumnNames = '('.implode(", ", $aColumnNames).')';
            $szValues = '('.implode(", ", $aValue).')';

            $szSQL = "UPDATE users SET assigned = '1' WHERE id = $iUser";
            $database->execute($szSQL);
            $szSQL = "INSERT INTO schools $szColumnNames VALUES $szValues";
            $bResult = $database->execute($szSQL);
            
            if($bResult == True){
                $tpl->assign('szSuccess', 'Successfully added new school');
            }
            
        }
        
        //
        // Schools table
        //
        
        
        
        //
        // School Details
        //
        
        if($_GET['details']){
            
            $iID = $_GET['details'];
            $szSQL = "SELECT * FROM schools WHERE id= $iID";
            $aResult = $database->execute($szSQL);
            while($szRow = mysql_fetch_array($aResult, MYSQL_ASSOC)){
                $aData = array($szRow);
            }
            
            $tpl->assign('aSchoolDetails', $aData);
            
        }
        
        //
        // Delete School
        //
        
        elseif($_GET['delete']){
            
            $iID = $_GET['delete'];
            
            $szSQL = "SELECT user_id FROM schools WHERE id = $iID";
            $iResult = $database->fetch($szSQL);
            foreach($iResult as $iUserID){
                $iUserID = $iUserID;
            }
            
            $szSQL = "UPDATE users SET assigned = '0' WHERE id = $iUserID";
            $database->execute($szSQL);
            
            $szSQL = sprintf("DELETE FROM schools WHERE id= %d", $iID);
            $bResult = $database->execute($szSQL);
            if($bResult == True){
                $tpl->assign('szSuccess', 'Successfully delete the school with ID '.$iID);
            }
            
        }
        
        //
        // Edit School
        //
        
        elseif($_GET['edit']){
            
            $iID = $_GET['edit'];
            
            $szSQL = "SELECT user FROM users WHERE rank = 2";
            $aResult = $database->execute($szSQL);
            while($szRow = mysql_fetch_array($aResult, MYSQL_ASSOC)){
                $editAUser[] = $szRow['user'];
            }
            
            $tpl->assign('editAUser', $editAUser);
            
            //
            // Assigning post values to variables
            //
        
            $szName = $_POST['edit_name'];
            $szURL = $_POST['edit_url'];
            $szLocationField1 = $_POST['edit_locationField1'];
            $szLocationField2 = $_POST['edit_locationField2'];
            $szCity = $_POST['edit_city'];
            $szState = $_POST['edit_state'];
            $iZipCode = $_POST['edit_zipCode'];
            $iAreaCode = $_POST['edit_areaCode'];
            $szTelephoneNumber = $_POST['edit_telephoneNumber'];
            $szUser = $_POST['edit_user'];
            
            $szSQL = "SELECT id FROM users WHERE user = '$szUser' LIMIT 0,1";
            $iResult = $database->fetch($szSQL);
            foreach($iResult as $iUserID){
                $iUser = $iUserID;
            }
            
            
            $aFields = array(
                            array('field' => 'name', 'value' => $szName),
                            array('field' => 'url', 'value' => $szURL),
                            array('field' => 'locationField1', 'value' => $szLocationField1),
                            array('field' => 'locationField2', 'value' => $szLocationField2),
                            array('field' => 'city', 'value' => $szCity),
                            array('field' => 'state', 'value' => $szState),
                            array('field' => 'zipCode', 'value' => $iZipCode),
                            array('field' => 'areaCode', 'value' => $iAreaCode),
                            array('field' => 'telephoneNumber', 'value' => $szTelephoneNumber),
                            array('field' => 'user_id', 'value' => $iUser)
                            );
                            
            foreach($aFields as $iKey => $szValue){
                if(trim($szValue['value']) !=  ''){
                    $aDataToUpdate[$szValue['field']] = $szValue['value'];
                }
            }
            
            if(isset($_POST['edit_school'])){
                if(array_key_exists('url', $aDataToUpdate)){
                    $validate->isURL('szURL', 'Please enter a valid URL (www.domain.ext)');
                }
                if(array_key_exists('zipCode', $aDataToUpdate)){
                    $validate->isUSZipCode('iZipCode', 'Please enter correct US zip code format (00000)');
                }
                if(array_key_exists('areaCode', $aDataToUpdate)){
                    $validate->isUSAreaCode('iAreaCode', 'Please enter correct US area code format (000)');
                }
                if(array_key_exists('telephoneNumber', $aDataToUpdate)){
                    $validate->isUSTelephone('szTelephoneNumber', 'Please enter correct US telephone number format (000-111-0000)');
                }
                
                if($validate->isError()){
                    $tpl->assign('aError', $validate->getErrorList());
                }
                else{
                    
                    foreach($aDataToUpdate as $szColumnName => $szValue){
                        $szSQL = "UPDATE schools SET $szColumnName='$szValue' WHERE id=$iID";
                        $bResult = $database->execute($szSQL);
                    }
                    if($bResult == True){
                        $tpl->assign('szSuccess', 'Successfully edited the school with ID '.$iID);
                    }
                }
            }
            
        }
        
        // Table
        
        if(isset($_POST['find_school'])){
            $szSQL = sprintf("SELECT * FROM schools WHERE name = '%s' ORDER BY id", $_POST['name']);
        }
        else{
            $szSQL = "SELECT * FROM schools ORDER BY id";
        }
        
        $szResult = $database->execute($szSQL);
        while($szRow = mysql_fetch_array($szResult, MYSQL_ASSOC)){
            $aSchoolID[] = $szRow['id'];
            $aSchoolName[] = $szRow['name'];
        }
        
        $tpl->assign('aSchoolID', $aSchoolID);
        $tpl->assign('aSchoolName', $aSchoolName);
        
        $tpl->display(ADMINTEMPLATE_PATH.'schools.tpl.php');
        
    }
    
?>

I would like some feedback on my code.

What is my common mistake? How can I cut down on repetitive tasks? What are the best ways to do the things I do.

bluesaga · 10-04-2007, 06:17 PM

meh double post

bluesaga · 10-04-2007, 06:17 PM

Code:

$szSQL = "SELECT id FROM users WHERE user = '$szUser' LIMIT 0,1";

Would be better as:

Code:

$szSQL = sprintf("SELECT id FROM users WHERE user = '%s' LIMIT 0,1", mysql_escape_string($szUser));

Its more secure to use that.

Code:

            if($bResult == True){

There is no need to use the "== True" just use:

Code:

               if($bResult){

Other than that, the code looks good, but i haven't looked THAT hard at it lol

Salathe · 10-04-2007, 06:43 PM

When you're checking that the visitor is authorised to view the page, if the check fails you should display the message and stop the execution of the script immediately. That way you can get rid of the else safely and know that the script will run only if logged in.

Yours

PHP Code:


			
    //

    // Check if user is logged in

    //

    

    if($auth->check() == false || $auth->admin_auth() == false){

        echo 'Please login as administrator';

    }

Suggested alternative

PHP Code:


			
    //

    // Check if user is logged in

    //

    

    if($auth->check() == false || $auth->admin_auth() == false){

        //echo 'Please login as administrator';

        $tpl->assign('szErrorMessage', 'Please log in as administrator.');

        $tpl->display(ADMINTEMPLATE_PATH.'error.tpl.php');

        exit; // I don't know if $tpl->display() stops the script, so exit here.

    }

I haven't looked below the first else yet, so there may be more comments to follow. :)

Haris · 10-04-2007, 07:55 PM

Quote:

Originally Posted by bluesaga

Code:

$szSQL = "SELECT id FROM users WHERE user = '$szUser' LIMIT 0,1";

Would be better as:

Code:

$szSQL = sprintf("SELECT id FROM users WHERE user = '%s' LIMIT 0,1", mysql_escape_string($szUser));

Its more secure to use that.

Code:

            if($bResult == True){

There is no need to use the "== True" just use:

Code:

               if($bResult){

Other than that, the code looks good, but i haven't looked THAT hard at it lol

Thanks and I thought sprintf was only used for user submitted data?

Quote:

Originally Posted by Salathe

When you're checking that the visitor is authorised to view the page, if the check fails you should display the message and stop the execution of the script immediately. That way you can get rid of the else safely and know that the script will run only if logged in.

Yours

PHP Code:


			
    //

    // Check if user is logged in

    //

    

    if($auth->check() == false || $auth->admin_auth() == false){

        echo 'Please login as administrator';

    }

Suggested alternative

PHP Code:


			
    //

    // Check if user is logged in

    //

    

    if($auth->check() == false || $auth->admin_auth() == false){

        //echo 'Please login as administrator';

        $tpl->assign('szErrorMessage', 'Please log in as administrator.');

        $tpl->display(ADMINTEMPLATE_PATH.'error.tpl.php');

        exit; // I don't know if $tpl->display() stops the script, so exit here.

    }

I haven't looked below the first else yet, so there may be more comments to follow. :)

Thanks Salathe. Saves two bunch of lines.

Salathe · 10-04-2007, 08:26 PM

Quote:

Originally Posted by Haris

Thanks and I thought sprintf was only used for user submitted data?

$szUser is submitted data ($szUser = $_POST['user'];). Just because you're using a SELECT here doesn't mean that a nasty user couldn't inject other code to UPDATE/DELETE/whatever.

From looking at your original post, it doesn't look like you're escaping any of the POSTed data. The validation class might help a bit (I don't know, we can't see what it does) but you should always, always use the escaping functions for all input data.

Haris · 10-04-2007, 09:18 PM

Thanks for yet another tip. I thought they couldn't inject SELECT

Haris · 10-04-2007, 09:24 PM

Any advice how could I turn my insert query in sprintf with using the array?

bluesaga · 10-05-2007, 07:54 AM

You don't have to use sprintf for everything, its simply a neater way of doing it, and forcing integers or strings.

You can do:

PHP Code:


			
            $aValue = array(
                "'".mysql_escape_string($szName)."'",
                "'".mysql_escape_string($szURL)."'",
                "'".mysql_escape_string($szLocationField1)."'",
                "'".mysql_escape_string($szLocationField2)."'",
                "'".mysql_escape_string($szCity)."'",
                "'".mysql_escape_string($szState)."'",
                "'".mysql_escape_string($iZipCode)."'",
                "'".mysql_escape_string($iAreaCode)."'",
                "'".mysql_escape_string($szTelephoneNumber)."'",
                "'".mysql_escape_string($iUser)."'",
            );

Has the same effect

Haris · 10-05-2007, 10:56 AM

Oh, thanks.

sketchMedia · 10-21-2007, 04:16 PM

Another little point what version of PHP are you using, as (correct me if im wrong) i think PHP5 returns a reference from the new automatically so using =& in this context is deprecated and produces E_STRICT level message:

PHP Code:


			
$tpl =& new Savant2();

if your using PHP4 then just ignore me.