4
\$\begingroup\$

I have a long switch statement and it would be great if someone know how to make it cleaner.

Code is available on Github: https://github.com/Rabbit-Company/Passky-Server/blob/main/php/index.php

And for those who don't want to visit Github here is the code:

    switch($_GET['action']){
    case "getInfo":
        if(Database::userSentToManyRequests('getInfo')){
            echo Display::json(429);
            return;
        }

        echo Database::getInfo();
    break;
    case "getToken":
        if(Database::userSentToManyRequests('getToken')){
            echo Display::json(429);
            return;
        }

        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW']) || !isset($_POST['otp'])){
            echo Display::json(403);
            return;
        }

        echo Database::getToken($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW'], $_POST['otp']);
    break;
    case "createAccount":
        if(Database::userSentToManyRequests('createAccount')){
            echo Display::json(429);
            return;
        }
    
        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW']) || !isset($_POST['email'])){
            echo Display::json(403);
            return;
        }

        echo Database::createAccount($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW'], $_POST['email']);
    break;
    case "getPasswords":
        if(Database::userSentToManyRequests('getPasswords')){
            echo Display::json(429);
            return;
        }

        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW'])){
            echo Display::json(403);
            return;
        }

        echo Database::getPasswords($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW']);
    break;
    case "savePassword":
        if(Database::userSentToManyRequests('savePassword')){
            echo Display::json(429);
            return;
        }

        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW']) || !isset($_POST['website']) || !isset($_POST['username']) || !isset($_POST['password']) || !isset($_POST['message'])){
            echo Display::json(403);
            return;
        }

        echo Database::savePassword($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW'], $_POST['website'], $_POST['username'], $_POST['password'], $_POST['message']);
    break;
    case "importPasswords":
        if(Database::userSentToManyRequests('importPasswords')){
            echo Display::json(429);
            return;
        }

        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW']) || file_get_contents('php://input') == null){
            echo Display::json(403);
            return;
        }

        echo Database::importPasswords($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW'], file_get_contents('php://input'));
    break;
    case "editPassword":
        if(Database::userSentToManyRequests('editPassword')){
            echo Display::json(429);
            return;
        }

        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW']) || !isset($_POST['password_id']) || !isset($_POST['website']) || !isset($_POST['username']) || !isset($_POST['password']) || !isset($_POST['message'])){
            echo Display::json(403);
            return;
        }

        echo Database::editPassword($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW'], $_POST['password_id'], $_POST['website'], $_POST['username'], $_POST['password'], $_POST['message']);
    break;
    case "deletePassword":
        if(Database::userSentToManyRequests('deletePassword')){
            echo Display::json(429);
            return;
        }

        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW']) || !isset($_POST['password_id'])){
            echo Display::json(403);
            return;
        }

        echo Database::deletePassword($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW'], $_POST['password_id']);
    break;
    case "deleteAccount":
        if(Database::userSentToManyRequests('deleteAccount')){
            echo Display::json(429);
            return;
        }

        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW'])){
            echo Display::json(403);
            return;
        }

        echo Database::deleteAccount($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW']);
    break;
    case "forgotUsername":
        if(Database::userSentToManyRequests('forgotUsername')){
            echo Display::json(429);
            return;
        }
    
        if(!isset($_POST['email'])){
            echo Display::json(403);
            return;
        }

        echo Database::forgotUsername($_POST['email']);
    break;
    case "enable2fa":
        if(Database::userSentToManyRequests('enable2fa')){
            echo Display::json(429);
            return;
        }
    
        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW'])){
            echo Display::json(403);
            return;
        }

        echo Database::enable2Fa($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW']);
    break;
    case "disable2fa":
        if(Database::userSentToManyRequests('disable2fa')){
            echo Display::json(429);
            return;
        }
    
        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW'])){
            echo Display::json(403);
            return;
        }

        echo Database::disable2Fa($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW']);
    break;
    case "addYubiKey":
        if(Database::userSentToManyRequests('addYubiKey')){
            echo Display::json(429);
            return;
        }
    
        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW']) || !isset($_POST['id'])){
            echo Display::json(403);
            return;
        }

        echo Database::addYubiKey($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW'], $_POST['id']);
    break;
    case "removeYubiKey":
        if(Database::userSentToManyRequests('removeYubiKey')){
            echo Display::json(429);
            return;
        }
    
        if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW']) || !isset($_POST['id'])){
            echo Display::json(403);
            return;
        }

        echo Database::removeYubiKey($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW'], $_POST['id']);
    break;
    default:
        echo Display::json(401);
    break;
}

As you can see in every switch statement function userSentToManyRequests appear in every case in a switch with the same error code, but different parameter. This is easy fixable with another function. But how would I shorten the rest (Check if all parameters in POST was supplied for each action and to provide them in the right function)

\$\endgroup\$
0
3
\$\begingroup\$

I'm quite surprised that you placed the userSentToManyRequests() function in every case. If you do that then you can just start with it before the switch ().

To get rid of the switch you can use an array containing the names of the arguments for each action function. I've done it for the first four cases, but it is easy to do them all:

$argumentNames = ['getInfo'       => [],
                  'getToken'      => ['PHP_AUTH_USER', 'PHP_AUTH_PW', 'otp'],
                  'createAccount' => ['PHP_AUTH_USER', 'PHP_AUTH_PW', 'email'],
                  'getPasswords'  => ['PHP_AUTH_USER', 'PHP_AUTH_PW']];

$action  = $_GET['action'] ?? 'No action given';
$errorNo = 0;
if (!in_array($action, array_keys($argumentNames))) {
     $errorNo = 401; // action does not exist
} elseif (Database::userSentToManyRequests($action)) {
     $errorNo = 429; // too many requests were sent
} else {
     $arguments = [];
     foreach ($argumentNames[$action] as $argumentName) {
         if (isset($_GET[$argumentName])) {
             $arguments[] = $_GET[$argumentName];
         } else {
             $errorNo = 403; // action argument does not exist
             break;
         }
     }
     if ($errorNo == 0) call_user_func_array($action, $arguments);
}
if ($errorNo > 0) echo Display::json($errorNo);

As you can see there's no more repeated code here. I also only report any errors once.

The trick here is that you need to know that call_user_func_array() exists.

Important note: Code is untested!

\$\endgroup\$
2
  • \$\begingroup\$ Improved the code slightly. It now also detects correctly when no action is supplied and I corrected the argument of userSentToManyRequests(). \$\endgroup\$ Dec 1 '21 at 16:37
  • \$\begingroup\$ Thanks, I will test that today or tomorrow. \$\endgroup\$
    – Žiga Zajc
    Dec 1 '21 at 17:08
3
\$\begingroup\$

As Kiko pointed out, there are many repeated blocks to check if the user has sent too many requests for the given action - e.g.

if(Database::userSentToManyRequests('getToken')){
    echo Display::json(429);
    return;
}

This violates the Don't Repeat Yourself principle . That block can be moved out of the switch statement - e.g.

$action = $_GET['action'];
//optionally ensure action is within whitelist of allowed actions
if($action && Database::userSentToManyRequests($action)){
    echo Display::json(429);
    return;
}

There are many blocks like this

   if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW'])){
       echo Display::json(403);
       return;
   }

And in some cases it also checks if a POST parameter is missing. The check to ensure that the user is authenticated could be abstracted to a separate function - e.g.

function ensureUserAuthed() {
       if(!isset($_SERVER['PHP_AUTH_USER']) || !isset($_SERVER['PHP_AUTH_PW'])){
           echo Display::json(403);
           exit;
       }
}

And if a POST parameter is missing, it would perhaps be appropriate to return a different response code - e.g. 400 or 422.

\$\endgroup\$
0
\$\begingroup\$

Thank you both for helping me out. I have upvoted both of your answers.

This is what I have made and it works great so far, I would still need to test every API call just to be sure.

if(empty($_GET['action'])){
    echo Display::json(400);
    return;
}

$argumentNames = [
    'getInfo'           => [],
    'getToken'          => ['PHP_AUTH_USER', 'PHP_AUTH_PW', 'otp'],
    'createAccount'     => ['PHP_AUTH_USER', 'PHP_AUTH_PW', 'email'],
    'getPasswords'      => ['PHP_AUTH_USER', 'PHP_AUTH_PW'],
    'savePassword'      => ['PHP_AUTH_USER', 'PHP_AUTH_PW', 'website', 'username', 'password', 'message'],
    'importPasswords'   => ['PHP_AUTH_USER', 'PHP_AUTH_PW', 'php://input'],
    'editPassword'      => ['PHP_AUTH_USER', 'PHP_AUTH_PW', 'password_id', 'website', 'username', 'password', 'message'],
    'deletePassword'    => ['PHP_AUTH_USER', 'PHP_AUTH_PW', 'password_id'],
    'deleteAccount'     => ['PHP_AUTH_USER', 'PHP_AUTH_PW'],
    'forgotUsername'    => ['email'],
    'enable2fa'         => ['PHP_AUTH_USER', 'PHP_AUTH_PW'],
    'disable2fa'        => ['PHP_AUTH_USER', 'PHP_AUTH_PW'],
    'addYubiKey'        => ['PHP_AUTH_USER', 'PHP_AUTH_PW', 'id'],
    'removeYubiKey'     => ['PHP_AUTH_USER', 'PHP_AUTH_PW', 'id']
];

$action  = $_GET['action'] ?? 'No action given';

if (!in_array($action, array_keys($argumentNames))){
    echo Display::json(401);
    return;
}

if (Database::userSentToManyRequests($action)) {
    echo Display::json(429);
    return;
}

$arguments = [];
$errorNo = 0;

foreach ($argumentNames[$action] as $argumentName) {
    if($argumentName == 'PHP_AUTH_USER' || $argumentName == 'PHP_AUTH_PW'){
        if(isset($_SERVER[$argumentName])){
            $arguments[] = $_SERVER[$argumentName];
        }else{
            $errorNo = 403;
            break;
        }
    }elseif($argumentName == 'php://input'){
        if(file_get_contents('php://input') != null){
            $arguments[] = file_get_contents('php://input');
        }else{
            $errorNo = 403;
            break;
        }
    }else{
        if(isset($_POST[$argumentName])){
            $arguments[] = $_POST[$argumentName];
        }else{
            $errorNo = 403;
            break;
        }
    }
}

if ($errorNo == 0){
    echo call_user_func_array(__NAMESPACE__ . '\Database::' . $action, $arguments);
}else{
    echo Display::json($errorNo);
}
\$\endgroup\$
3

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.