0

I’m using Slim PHP to try and generate and display multiple validation messages at the same time when a user inputs incorrect or no data into a sign up form on my Angular front-end application.

For example if a user leaves the username and password field blank it should have two error messages for both the username and password field being empty, but only the username error if the username error is blank and the password is filled.

The way I’ve implemented it so far is by throwing two exceptions and catching them in my authentication controller. Below is all the logic to make this happen when a user clicks submit on the form.

The two exceptions:

InvalidUsername.php

class InvalidUsername extends \LogicException {
    public function __construct(string $message, $code = 0, Throwable $previous = null) {
        parent::__construct($message, $code, $previous);
    }

    public function getUsernameError() {
        return $this->message;
    }
}

InvalidPassword.php

class InvalidPassword extends \LogicException {
    public function __construct(string $message, $code = 0, Throwable $previous = null) {
        parent::__construct($message, $code, $previous);
    }

    public function getPasswordError(){
        return $this->message;
    }
}

Where the two exceptions are called:

Username.php

class Username {
    private $username, $error_message;

    public function __construct(string $tainted_username) {
        $cleaned_username = filter_var($tainted_username, FILTER_SANITIZE_STRING);

        if($this->isUsernameValid($cleaned_username))
            throw new InvalidUsername($this->error_message);

        $this->username = $cleaned_username;
    }

    private function isUsernameValid($cleaned_username) {
        return $this->isEmpty($cleaned_username) || $this->isTooShort($cleaned_username) ||
        $this->isTooLong($cleaned_username) ? true : false;
    }

    private function isEmpty(string $username) {
 return empty($username) ?
            true && $this->error_message = 'A username is required' : false;
    }

    public function __toString() {
        return $this->username;
    }
}

Password.php

class Password {
    private $password, $error_message;

    public function __construct(string $tainted_password) {
        $cleaned_password = filter_var($tainted_password, FILTER_SANITIZE_STRING);

        if ($this->isPasswordValid($cleaned_password))
        throw new InvalidPassword($this->error_message);

        $this->password = $cleaned_password;
    }

    private function isPasswordValid($cleaned_password) {
        return $this->isEmpty($cleaned_password) || $this->isTooShort($cleaned_password) ||
        $this->isTooLong($cleaned_password) ? true : false;
    }

    private function isEmpty(string $password) {
        return empty($password) ?
            true && $this->error_message = 'A password is required' : false;
    }

    public function __toString() {
        return $this->password;
    }
}

The service and controller classes:

AuthenticateService.php

class AuthenticationService {
    protected $userMapper;

    public function __construct(UserMapper $userMapper) {
        $this->userMapper = $userMapper;
    }

    public function registerUser (string $tainted_username, string $tainted_password) {
        $username = new Username($tainted_username);
        $password = new Password($tainted_password);

        $user = new User($email, $username, $password);

        return $this->userMapper->saveUser($user);
    }
}

AuthenticateController.php

class AuthenticationController {
    private $authenticationService;

    public function __construct(AuthenticationService $authenticationService) {
        $this->authenticationService = $authenticationService;
    }

    public function registerUser($request, $response) {
        $tainted_username = $request->getParsedBody()['username'];
        $tainted_password = $request->getParsedBody()['password'];
        $validationResponse = null;

        try {
            $this->authenticationService->registerUser($tainted_username, $tainted_password);
        }

        catch(InvalidUsername | InvalidPassword $e) {
                $validationResponse = ['usernameError' => true, 'usernameMessage' => $e->getUsernameError(),
                    'passwordError' => true, 'passwordMessage' => $e->getPasswordError()];
        }
        return $jsonResponse = $response->withJson($validationResponse);
    }
}

However it’s not really working as intended. I think the problem lies in the catch statement of the registerUser() method in the AuthenticationController class.

If I leave both the username and password blank and submit I get this response: message: Call to undefined method App\Models\Exceptions\InvalidUsername::getPasswordError()

However if I fill in the username field and leave the password blank I get this response: message: Call to undefined method App\Models\Exceptions\InvalidPassword::getUsernameError()

And if I leave the username field blank and fill in the password field I get this response: message: Call to undefined method App\Models\Exceptions\InvalidUsername::getPasswordError()

It seems to be that I can’t catch both the InvalidUsername and InvalidPassword exceptions at the same time. It seems to catch one exception then tries to call methods that are intended for the other exception.

Is there anyway I can get it so the InvalidUsername and the InvalidPassword are both caught at the same time, so both the getUsernameError() and getPasswordError() can be called and used to set the outgoing json response?

Before I was using the Exception getMessage() method, which did work, but it only made it possible for one error message to be set and displayed at a time. For example it started with username errors, then if the username field was correctly filled out, it then transitioned to the password errors, almost like the validation messages were being set and displayed in a sequence. I soon realised this was probably because the getMessage was final, which is why I have the getUsernameError() and getPasswordError() methods in each of the exceptions.

Any help would be appreciated.

SneakyShrike
  • 723
  • 1
  • 10
  • 31
  • 1
    Try not to use exceptions to branch your code in the first place. – Filip Halaxa Dec 02 '19 at 17:59
  • @FilipHalaxa Not sure what you mean by branching my code. But what would you recommend instead? – SneakyShrike Dec 03 '19 at 13:44
  • 1
    In general, exceptions should be used in exceptional situations. Invalid username or invalid password are pretty common situations, expectable ones. The less exceptions the less worries about catching them. Ad branching - you use exceptions "branching" instead of if/else branching. – Filip Halaxa Dec 04 '19 at 15:16
  • That makes sense for the use of exceptions, this is the first time i've came across them so i wasn't sure what i was doing, so thank you for the insight into that. And on branching did you mean i was using try and catch instead of if and else statements? Also is there a paticular approach to doing back end form validation? It seems that exceptions should't be the approach to take for this. – SneakyShrike Dec 04 '19 at 17:35
  • Yes, I meant usage of exceptions instead of if/else in expectable or not panic situations. Some frameworks (code usually built by professionals) use this approach, some don't. So it is not a strict rule but rather philosophy or approach if you like. I myself someimes rather use some kind of result object with properties like `isValid`, `result`, `message` and so on and keep exceptions for really harsh runtime situations or library code misuse by a programmer (logic exceptions). Exceptions are necessary but in necessary situations. – Filip Halaxa Dec 05 '19 at 17:07

1 Answers1

0

Try this for registerUser method:

public function registerUser($request, $response)
{
    $tainted_username   = $request->getParsedBody()['username'];
    $tainted_password   = $request->getParsedBody()['password'];
    $validationResponse = [];

    try {
        $this->authenticationService->registerUser($tainted_username, $tainted_password);
    } catch (InvalidUsername $e) {

        $validationResponse['usernameError'] = true;
        $validationResponse['usernameMessage'] = $e->getUsernameError();
    }
    catch (InvalidPassword $e) {
        $validationResponse['passwordError'] = true;
        $validationResponse['passwordMessage'] = $e->getPasswordError();
    }

    return $jsonResponse = $response->withJson($validationResponse);
}

I'd also recommend to use generic message getters names, instead of getPasswordError() and getUsernameError() why not use generic getMessage() from Exception class? Class name InvalidUsername already tells you that the object is about username, you don't need to duplicate this information in method names.

S3Mi
  • 491
  • 4
  • 10
  • I tried what you did, but i'm having the same issue as i had with using the getMessage method. Only the username message is being set and displayed. The other catch for the password is not even being caught. I don't think seperating the catches will work, because that means it only does one at a time. – SneakyShrike Dec 03 '19 at 14:23
  • But why do you need these events at the same time? If any of them fails to validate, the login process is invalid, job done. You should not display messages like "bad password" or "bad username". You should use generic "login invalid". You should not let potential attacker know which part of login is invalid. You allow them to crawl your usernames this way. Ps. you can't have two exceptions thrown at the same time. – S3Mi Dec 04 '19 at 15:21
  • That's a good point actually in regard to giving away information. I'm quite new to programming so i wasn't aware of things like that. I read elsewhere that you should do backend validation and I thought that meant giving the user feedback on the form with what they did wrong. Should I do feedback on the frontend? And If so, does this mean I even need to display a back-end validation message? I am a little confused, but I understand backend validation is very important. – SneakyShrike Dec 04 '19 at 17:16
  • It doesn't really matter who produces the text message as long as it's the back-end that has final say regarding the "boolean" state of the validation and you don't disclose too much sensitive information in the feedback. I second to what @FilipHalaxa said about not using exceptions for this scenario. Do some regular if() conditions and treat both failed scenarios as one generic failure to login. Exceptions should be saved for situations like problems with DB connection etc. – S3Mi Dec 04 '19 at 18:39
  • That makes sense for logging in and not disclosing too much information but what about for signing up? When you sign up you're entering new data not checking against old data. Would the same still apply for signing up? – SneakyShrike Dec 08 '19 at 00:51