Validating user credentials and logging into a Symfony site

Question

I have the following code in a Symfony site that validates a user's credentials and logs them in via the Session class:

/**
 * Validates an Affiliate
 */
public function validateUser($username, $password, $session, $securityContext) 
{
    $repo = $this->em->getRepository('JCMainBundle:Affiliate');
    $user = $repo->findOneBy(["username" => $username]);

    if (!is_object($user)) return false;

    $passwordValidated = password_verify($password, $user->getPassword());

    if (!$passwordValidated) return false;

    // Create token for user
    $token = new UsernamePasswordToken($user, null, "affiliate_dashbaord", ['ROLE_AFFILIATE']);
    $securityContext->setToken($token);
    $session->set('_security_affiliate_dashboard', serialize($token));

    return true;
}

As you can see, the function has very simple return values--just booleans. The function that calls this function looks at the return value of this function and determines whether or not to redirect the user to the affiliate dashboard.

How can I handle the return values better? I have two possible paths than can return false, but the calling function will have no idea what caused "false" to be returned.

In the past I've done something like this:

$response = ["success" => false, "message" => "error message here"];

I return the response with the success and a message. Using this design, I can return false AND return what made it return false. However, this seems a bit unorthodox for use on the backend and seems to fit more in a REST API.

I could use an exception, but the role of the function is to validate, so it wouldn't make sense to throw an exception if the user credentials are incorrect.

Answer 1

I have taken the 'returning an array with contextual information from a function call' route before and I can say that it is probably never the route you want to take. When a function tries to log in a user, then the inability to log in should be considered an exceptional case and is therefore Exception worthy.

Also, your function has the side effect of not only validating a user, which is what its name suggests, but also creating the session. This is not what you want to be doing. You want functions that perform a single task and are good at performing that specific task. Create a function that simply validates the user and throws an exception if the user does not pass. The exception part is key.

Consider the function verifyUserCredentials(). What do you expect from this function? You would probably expect it to take some user credentials, say a username or user object and password, and verify that they are valid. You could have gone the route of creating a boolean function like areUserCredentialsValid(), but exception throwing functions allow the funtion that is calling it to maintain better readability.

Good Example

Let me explain what I mean with the following example:

public function loginUser ($username, $password, $session, $securityContext) {
    try {
        $user = $this->getUserByCredentials($username, $password);

        $this->startLoginSession($user);
    } catch (UserVerificationException $e) {
        // Inform the user of username/password mismatch
    }
}

private function getUserByCredentials ($username, $password) {
    $repo = $this->em->getRepository("JCMainBundle:Affiliate");
    $user = $repo->findOneBy("username" => $username]);

    $this->verifyUserCredentials($user, $password);

    return $user;
}

private function verifyUserCredentials ($user, $password) {
    if (!is_object($user)) {
        throw new UserVerificationException("User does not exist");
    }

    if (!password_verify($password, $user->password) {
        throw new UserVerificationException("Passwords don't match");
    }
}

private function startLoginSession ($user) {
    $token = new UsernamePasswordToken($user, null, "affiliate_dashbaord", ['ROLE_AFFILIATE']);
    $securityContext->setToken($token);
    $session->set('_security_affiliate_dashboard', serialize($token));
}

The single function you had written has been split up into four functions, each performing a specific task. The main entry function loginUser() is now extremely easy to understand. It simply retrieves the user and starts a login session. Oh and apparantly a verification exception can happen. Within a few seconds you have enough information about this function to know what it does, without looking at the guts of it.

If you were to look at the functions that are being called you would see that each performs a single task without any side effects, only exceptions. Each of these individual functions is easy to read, understand and maintain, which is very important.

Bad Example

Now compare the code above to the code below:

public function loginUser ($username, $password, $session, $securityContext) {
    $result = $this->validateUser($username, $password, $session, $securityContext);
    if (!$result["success"]) {
        // Inform user of username/password mismatch
    }
}

public function validateUser ($username, $password, $session, $securityContext) {
    $repo = $this->em->getRepository('JCMainBundle:Affiliate');
    $user = $repo->findOneBy(["username" => $username]);

    if (!is_object($user)) return ["succes" => false, "message" => "User does not exist"];;

    $passwordValidated = password_verify($password, $user->getPassword());

    if (!$passwordValidated) return ["succes" => false, "message" => "Passwords don't match"];

    // Create token for user
    $token = new UsernamePasswordToken($user, null, "affiliate_dashbaord", ['ROLE_AFFILIATE']);
    $securityContext->setToken($token);
    $session->set('_security_affiliate_dashboard', serialize($token));

    return ["succes" => true, "message" => "We did it!"];;
}

What the hell is going on here? The main entry function is small, which is good. However, it has become unclear what exactly it is doing. The function name suggests it is logging in a user, but at closer inspection it seems to only be validating the user input. We should probably check that function out to see what is actually going on.

A bunch of stuff is happening. The user is retrieved from the repository. The result is being checked for validity and in the meantime some weird array construction is being created. Aah and there it is at the bottom, the session for the user is also created!

I hope this (not so) short example has given you some insight into why the flow of your code from one single responsibility function to another is important and why exceptions are the preferred way for dealing with these sorts of situations. The flow of your functions becomes much clearer when you are not constantly checking for things in the meantime, but instead have a catch block at the end of your flow.

Answer 2

Note: the suggestion below is founded on a certain vision about readability, that may not be shared by everybody, as illustrated by the totally opposite approach proposed by @Thijs Riezebeek.

A pretty simple, light, and though efficient solution I frequently use in such cases is to merely:

• Return the appropriate explainatory message when there is an error.
• Do nothing otherwise.

First it makes your function as simple as possible (when successfull you don't even need to return anything).

Then from the app which invokes the function it's easy to use while being readable, assumed you change the function name for something like invalidUser(). So the app can work like this:

if ($errorMessage = invalidUser(...)) {
  // take any appropriate action, using $errorMessage
}
// otherwise merely continue