Better PHP Ajax Request Processing Structure

Question

I have a server side PHP file which processes Ajax requests from the client browser. Some requests are only valid if they originate from an Administrator in the application, others are only valid if they originate from a Manager in the application, and the rest are valid if any system user makes the request.

I covered all the user authentication code above this snippet and and am quite happy with the way it turned out. When I got to the request processing part I came up with the following:

Ajax Request Processing in PHP:

 $user = User::load($_COOKIE['login']); $requestData = json_decode($_POST['json']); switch($requestData->requestType) { case 'CreateUser': if(!$user->isAdmin()) die('Unauthorized Request'); else createUser($requestData); break; case 'DeleteUser': if(!$user->isAdmin()) die('Unauthorized Request'); else deleteUser($requestData); break; case 'PasswordReset': if(!$user->isAdmin()) die('Unauthorized Request'); else passwordReset($requestData); break; case 'ChangeTitle': if(!$user->isAdmin()) die('Unauthorized Request'); else changeTitle($requestData); break; case 'ChangeWorkStatus': if(!$user->isAdmin()) die('Unauthorized Request'); else changeWorkStatus($requestData); break; case 'ChangeVacationDays': if(!$user->isAdmin()) die('Unauthorized Request'); else changeVacationDays($requestData); break; case 'AddToSchedule': if(!$user->isManager()) die('Unauthorized Request'); else addToSchedule($requestData); break; case 'RemoveFromSchedule': if(!$user->isManager()) die('Unauthorized Request'); else removefromSchedule($requestData); break; case 'UserAvailability': if(!$user->isManager()) die('Unauthorized Request'); else getUserAvailability($requestData); break; case 'UserInfo': getUserInfo($requestData); break; case 'UserPhone': getPhoneNumbers($requestData); break; case 'AddPhone': addPhoneNumber($requestData); break; case 'PhonePriority': phonePriority($requestData); break; case 'RemovePhone': removePhoneNumber($requestData); break; case 'UserEmail': getEmails($requestData); break; case 'AddEmail': addEmail($requestData); break; case 'EmailPriority': emailPriority($requestData); break; case 'RemoveEmail': removeEmail($requestData); break; case 'UserList': userList($requestData); break; default: die('Invalid Request Specification'); } 

I don't want to re-instantiate $user inside each method of the switch statement to check if $user is of the correct type because the instantiation is expensive (database access) and aside from checking the $user type, the $user would be unnecessary to "do the thing".

I also don't want to pass the $user into the function because again, the $user would be unused except for type checking. Hence having $user in the method signature would be "incorrect".

Lastly, to keep things at a single level of abstraction, I think the validation of the user should be done outside of the method calls, because the methods "do the thing", not "validate if the user can do the thing"and"do the thing".

I'm pretty convinced that the $user type validation and the request processing should be done on this level of abstraction, yet I don't really like the way the code is structured. The switch statement with the internal if statements looks awful.

Is there a better way to handle this kind of logic? Perhaps a design pattern I am overlooking?

---

Edit:

Inspired from aileronajay's input, I think the following very nicely separates the abstractions.

$user = User::load($_COOKIE['login']); $requestData = json_decode($_POST['json']); $adminOnlyRequests = array('CreateUser','DeleteUser','PasswordReset' ...); $managerOnlyRequests = array('AddToSchedule','RemoveFromSchedule', ...); if( (!$user->isAdmin() && in_array($requestData->requestType,$adminOnlyRequests)) || (!$user->isManager() && in_array($requestData->requestType,$managerOnlyRequests))) die('Unauthorized Request'); // Now the user MUST be authorized to make the request (or the request is unspecified) // No longer need $user instantiated processRequest($requestData); // Bury switch statement in here 

Answer 1

you could find out initially whether the user is admin or manager and then maintain negative lists for the two.

boolean isManager = isManager(); boolean isAdmin = isAdmin(); managerNegativeList = {addToSchedule,...}; adminNegativeList = { changeTitle,...} if(isManager){ if(inMangerNegativeList){ die(); } } if(isAdmin){ if(inAdminNegativeList){ die(); } } processRequest(requestType,$requestData) 

Answer 2

(I don't work with PHP nowadays, so code is pseudocode.)

if(!$user->isAdmin()) die('Unauthorized Request'); else createUser($requestData); 

You could reformat the line above to

 if(!$user->isAdmin()) { die('Unauthorized Request'); } else { createUser($requestData); } 

From this it's clear that the else block is unnecessary, since die stops the processing entirely, so the following is the same:

 if(!$user->isAdmin()) { die('Unauthorized Request'); } createUser($requestData); 

Here you can extract out a checkAdmin($user) function:

function checkAdmin($user) { if(!$user->isAdmin()) { die('Unauthorized Request'); } } 

Usage:

 case 'CreateUser': checkAdmin(); createUser($requestData); break; case 'DeleteUser': checkAdmin(); deleteUser($requestData); break; ... 

Another, more object-oriented approach is creating separate classes for the commands which implement a common interface:

public interface Command() { String getCommandName(); void run($requestData); } public class CreateUserCommand implements Command { public String getCommandName() { return "CreateUser"; } public void run($requestData) { ... } } 

You can also create an interface for the access checker:

public interface AccessChecker { void checkAccess($user); } public class AdminAccessChecker { public void checkAccess($user) { if(!$user->isAdmin()) { die('Unauthorized Request'); } } } 

Then combine them in a composite class:

public class AccessCheckedCommand { private Command command; private AccessChecker accessChecker; public AccessCheckedCommand(Command command, AccessChecker accessChecker) { // assign fields } public String getCommandName() { return command.getName(); } public void run($user, $requestData) { accessChecker.checkAccess($user); command.run($requestData); } } 

Usage:

AdminAccessChecker adminAccessChecker = new AdminAccessChecker(); List<AccessCheckedCommand> commands = ...; commands.put(new AccessCheckedCommand(new CreateUserCommand(), adminAccessChecker)); ... for (AccessCheckedCommand accessCheckedCommand: commands) { if (command.getCommandName() == $requestData->requestType) { command.run($user, $requestData); return; } } die('Invalid Request Specification'); 

I guess it shows the idea and you could modify that in a lot of ways according to your needs.