Passky Password Manager router with authentication and validation guards

Question

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)

KIKO Software · Accepted Answer · 2021-12-01 16:37:55Z

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!

Improved the code slightly. It now also detects correctly when no action is supplied and I corrected the argument of userSentToManyRequests(). — KIKO Software, Dec 1, 2021 at 16:37

Sᴀᴍ Onᴇᴌᴀ · Accepted Answer · 2021-12-01 18:01:39Z

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.

Žiga Zajc · Accepted Answer · 2021-12-01 18:44:35Z

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);
}

Welcome to Code Review! While OPs are encouraged to answer their own questions bear in mind that "Every answer must make at least one insightful observation about the code in the question. Answers that merely provide an alternate solution with no explanation or justification do not constitute valid Code Review answers and may be deleted."... Perhaps asking a follow-up question would be more appropriate. When doing that it is common to include a link to this question. — Sᴀᴍ Onᴇᴌᴀ, Dec 1, 2021 at 19:31
Thanks. So I should delete the answer and mark KOKO Software's answer as accepted? — Žiga Zajc, Dec 2, 2021 at 6:51
You could delete the answer or if you changed anything other than what we suggested then you could add a description of those changes, including what improvement that has over the original. "Accepting an answer is not mandatory; do not feel compelled to accept the first answer you receive. Wait until you receive an answer that answers your question well." — Sᴀᴍ Onᴇᴌᴀ, Dec 3, 2021 at 0:56

Stack Exchange Network

Passky Password Manager router with authentication and validation guards

3 Answers 3

Your Answer

Not the answer you're looking for? Browse other questions tagged
php
or ask your own question.

Hot Network Questions

Passky Password Manager router with authentication and validation guards

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged php or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
php
or ask your own question.