4
\$\begingroup\$

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
\$\endgroup\$
1
\$\begingroup\$

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)
\$\endgroup\$
  • \$\begingroup\$ I get what you are saying, this a much more elegant way to organize the code! \$\endgroup\$ – recursion.ninja Mar 21 '14 at 17:53
1
\$\begingroup\$

(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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your input, it has given me a lot to think about! \$\endgroup\$ – recursion.ninja Mar 21 '14 at 17:41

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.