Jump to content
Larry Ullman's Book Forums

Modularizing A Website With Access Restriction


Recommended Posts

Hi Larry,

 

Sorry I've been away from from the forum for a while. Was taking a break from php and busy at work.

 

What I am trying to do is merge the example on Website Modularization in Chapter 2 with your code from Site 1 from the E-commerce book. The modularization code is straight forward, however, what I want to do is limit the pages which can be accessed depending on whether a user is logged in or not.

 

I'm still in the brainstorming phase, but I imagine that since the login form is in the footer, then that can be left as is and won't affect the code which handles the "modularization". However, how would I best handle restricting the access of a "content module" only to a user who has successfully logged in? I'm just afraid because the page that is trying to be accessed is passed via the url. I can control the links displayed in the footer based on whether the user is logged in or not, but what if they try to hard code a page they are not supposed to access into the url? Am I correct in thinking that I should just use a conditional to check whether a session variable is present or not before loading the content module for a restricted page?

 

For example:

case 'somePage':
    if (isset($_SESSION['user_id'])) {
       $page = 'somepage.inc.php';
       $pageTitle = 'Some Page';
       break;
   }
   else {
       $page = 'main.inc.php';
       $pageTitle = 'Site Home Page';
       break;
   }

Or would there be a better way of doing this?

I only need to restrict 2 pages, so this shouldn't be too complicated!

 

Thanks in advance for any help you can give!

 

Matt

Link to comment
Share on other sites

I love how you always direct your questions directly at Larry. I'm still gonna take a stab at the answer.

 

I think your logic is sound, and you can have a basic script that would work for all pages like the following:

 

<?php

 if (isset($_SESSION['user_id'])) {

   $page = $_GET['page'];

 } else {

   $page = 'login_error.html';

 }

 // Redirect here.

?>

 

By doing that, this simple script would work for all pages since $page is always equal to the value retrieved from the GET superglobal, assuming the user is logged in. And if the user isn't logged in, regardless of what restricted page they're trying to access, you can very easily direct them to a "Sorry, but you must be logged in to view this page." page and then provide a form for logging in.

 

Peace out, boi!

Link to comment
Share on other sites

At the moment to prevent people trying to include files that do not exist you're using the switch statement and then inside that you'd have to check if they are logged in or not to decide whether they have access to that particular page.

 

The way I'd do it is like so:

 

$anyone = array('page_a', 'page_b', 'page_c', 'page_e');
$logged_in = array('page_d'); 

$pages = isset($_SESSION['user_id']) ? array_merge($anyone, $logged_in) : $anyone;
$include = in_array($_GET['page'], $pages) ? $_GET['page'] . '.php' : 'index.php';

include($include);

 

The code basically establishes 2 arrays (one all the content that anyone can see and one for content that only logged in users can view). Then it creates a master lookup array containing the content that should be available to that user. Next we check if the page submitted exists in the $pages array and if so we save the value ready for inclusion - if not we include the default index include.

 

Just incase you're not familiar with the ternary operator:

 

$pages = isset($_SESSION['user_id']) ? array_merge($anyone, $logged_in) : $anyone;

 

Is the same as:

 

if (isset($_SESSION['user_id'])){

$pages = array_merge($anyone, $logged_in);

} else {

$pages = $anyone;	

}

Link to comment
Share on other sites

Clever design, Stuart, but it seems overly complicated for no reason (unless I'm missing something).

 

Certainly, it doesn't hurt to have multiple ways of accomplishing the same thing, but why not just use the one if statement like I suggested above? Heck, use a ternary operation, if you really want!

 

Anyway, I'm not criticizing. More than anything, I just wanted to debate the issue and learn why you think your method is better.

Link to comment
Share on other sites

Hi Hartley no worries - happy to explain my reasoning. So this is the code from Chapter 2:

 

// Determine what page to display:
switch ($p) {

case 'about':
	$page = 'about.inc.php';
	$page_title = 'About This Site';
	break;

case 'this':
	$page = 'this.inc.php';
	$page_title = 'This is Another Page.';
	break;

case 'that':
	$page = 'that.inc.php';
	$page_title = 'That is Also a Page.';
	break;

case 'contact':
	$page = 'contact.inc.php';
	$page_title = 'Contact Us';
	break;

case 'search':
	$page = 'search.inc.php';
	$page_title = 'Search Results';
	break;

// Default is to include the main page.
default:
	$page = 'main.inc.php';
	$page_title = 'Site Home Page';
	break;

} // End of main switch.

 

Now if you want to modularise the admin modules too you'll need to add in a series of other options (e.g. change password, manage users, manage news) if you added those in you'd have to conditionally check for the user_id variable in the session on each case. This creates code repetition (think DRY) and makes for (at least in my opinion) messy code. Doing that would look like this:

 

// Determine what page to display:
switch ($p) {

case 'about':
	$page = 'about.inc.php';
	$page_title = 'About This Site';
	break;

case 'this':
	$page = 'this.inc.php';
	$page_title = 'This is Another Page.';
	break;

case 'that':
	$page = 'that.inc.php';
	$page_title = 'That is Also a Page.';
	break;

case 'contact':
	$page = 'contact.inc.php';
	$page_title = 'Contact Us';
	break;

case 'search':
	$page = 'search.inc.php';
	$page_title = 'Search Results';
	break;

case 'change':
	if (isset($_SESSION['user_id'])){
		$page = 'change.inc.php';
		$page_title = 'Change Password';
		break;
	}

case 'manage':
	if (isset($_SESSION['user_id'])){
		$page = 'manage.inc.php';
		$page_title = 'Manage Users';
		break;
	}

case 'user':
	if (isset($_SESSION['user_id'])){
		$page = 'user.inc.php';
		$page_title = 'New User';
		break;
	}

// Default is to include the main page.
default:
	$page = 'main.inc.php';
	$page_title = 'Site Home Page';
	break;

} // End of main switch.

 

Now you could do the check first and present two different switch statements but that again causes code repetition. To achieve the exact same as the above code block using my technique requires:

 

$include = 'main.inc.php';
$page_title = 'Site Homepage';
$anyone = $logged_in = array();

$anyone['about'] = 'About this site';
$anyone['this'] = 'This is another page';
$anyone['that'] = 'That is Also a Page.';
$anyone['contact'] = 'Contact Us';
$anyone['search'] = 'Search Results';

$logged_in['change'] = 'Change Password';
$logged_in['manage'] = 'Manage Users';
$logged_in['user'] = 'Add User'; 

$pages = isset($_SESSION['user_id']) ? array_merge($anyone, $logged_in) : $anyone;

if (array_key_exists($_GET['page'], $pages)){
$include = $_GET['page'] . '.inc.php';
$page_title = $pages[$_GET['page']];
}

 

To me the second option is easy to read/maintain, conforms to the DRY principal, is more portable and shorter (which in general I prefer as long as it doesn't sacrifice readability/flexibility). I appreciate it doesn't make a big difference and going with the switch statement is perfectly fine but I hope that at least illustrates why I presented that as an idea/technique - what are your thoughts?

Link to comment
Share on other sites

Ah, good point. I was under the assumption that Matt was sending the name of the page (file) to be called, not a string that then needs to be matched up to a page. Actually, I don't know which he's doing, but if it's the latter, as is the case in Larry's book, then yeah, your method makes sense. Thanks, and sorry if you get carpal tunnel at a young age due to my silly questions.

Link to comment
Share on other sites

HartleySan and Stuart,

 

Thank you both for your input on this!

 

Stuart, your solution is brilliant! HartleySan, I know you don't have this particular book, but by matching the page the user is trying to access with a value stored in an array (or in Larry's case, by using a switch statement), there is absolutely no way that anyone can access content they're not supposed to! Stuart and Larry are pretty much doing the same thing, except that Stuart further restricts users to certain pages based on whether they are logged in or not, which is essentially what I was trying to accomplish.

 

To me the second option is easy to read/maintain, conforms to the DRY principal, is more portable and shorter (which in general I prefer as long as it doesn't sacrifice readability/flexibility).

I agree!

 

Thanks a lot!

 

Matt

Link to comment
Share on other sites

Stuart,

 

Thank you very much for your help! The code works perfectly!

 

However, I have just discovered a strange problem with using the $_GET method.

 

I am using the following code at the top of the script to check which type of request method was used (i.e. GET, POST, or none) and it works fine:

if ($_SERVER['REQUEST_METHOD'] == 'POST')
{
include ('../../includes/gallery_login.inc.php');
$include = 'gallery_content.inc.php';
}
elseif (($_SERVER['REQUEST_METHOD'] == 'GET') && isset($_GET['p']))
{
if (array_key_exists($_GET['p'], $pages))
{
	$include = $_GET['p'] . '.inc.php';
	$page_title = $pages[$_GET['p']];
}
}
else
{
$include = 'gallery_content.inc.php';
}

However, if I take out the "&& isset($_GET['p'])", then the page throws the following error:

An error occurred in script '/var/www/html/includes/show_gallery.inc.php' on line 43:
Undefined index: p

It's almost as though the page defaults to using the GET request method on first load (when 'p' has not been defined yet) and throws an error. By including the "isset($_GET['p'])" in the conditional it knows that the $_GET array is empty and then moves on to the "else" conditional.

 

This is very strange behavior! The first time the page loads, it should not be via the GET method, and "$_SERVER['REQUEST_METHOD'] == 'GET'" should be false. Does anyone know what is going on here?

 

Thanks,

 

Matt

Link to comment
Share on other sites

Hi Matt,

 

That's the correct behaviour the request being made will always be set e.g. one of: GET, HEAD, POST or PUT.

 

When you access a page normally the request type is GET irrespective of whether any parameters are actually passed in the URL. So it shouldn't be FALSE it will be TRUE unless you're making a POST, PUT or HEAD request.

 

Rather than removing

 

&& isset($_GET['p'])

 

instead remove:

 

($_SERVER['REQUEST_METHOD'] == 'GET') &&

 

And then just leave the rest of the else statement alone and it should work fine.

Link to comment
Share on other sites

When you access a page normally the request type is GET irrespective of whether any parameters are actually passed in the URL.

I had no idea this was the case, but it all makes perfect sense now that I think about it!

 

You definitely know your stuff Stuart! :)

 

Thank you!

 

Matt

Link to comment
Share on other sites

 Share

×
×
  • Create New...