2
\$\begingroup\$

So, I have a function which checks if a user is logged in, but it's a long chain of if/else statements.

public static function isLoggedIn(){
    Session::openSession();

    if(isset($_SESSION['userId'])){
        return true;
    }

    if(isset($_COOKIE['authToken'])){
        list($selector, $token) = explode(':', $_COOKIE['authToken']);
        $result = Database::query('SELECT * FROM logins WHERE login_selector = ?', array($selector))[0];    

        if($result){
            if(!hash_equals($result['login_token'], hash('sha256', base64_decode($token)))){
                return self::notLogged();
            }else{
                self::relogUser($result['login_userId']);
                return true;
            }
        }else{
            return self::notLogged();   
        }
    }else{
        return self::notLogged();
    }
}

I'm looking for advice on how to refactor it to make it more clean and easy to read.

EDIT: I tried using a series of returns but it didn't work because one failed and blocked the rest (obviously).

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

One of the problems you're facing is the pyramid of doom - deeply nested control structures that make for hard to read code.

You can simplify your code by inverting the conditions in your if-statements. For example, instead of

if(isset($_COOKIE['authToken'])) {
    // do something with authToken
    // ...
    // (potentially) lots of code
    // ...         
} else {
    // throw an error / return
}

you could write

if(!isset($_COOKIE['authToken'])) {
    return;
}

// do something with authToken
// ...

By applying this to your the isLoggedIn function, you get a flatter, more easily readable equivalent:

public static function isLoggedIn(){
    Session::openSession();

    if(isset($_SESSION['userId'])){
        return true;
    }

    if(!isset($_COOKIE['authToken'])){
      return self::notLogged();
    }

    list($selector, $token) = explode(':', $_COOKIE['authToken']);
    $result = Database::query('SELECT * FROM logins WHERE login_selector = ?', array($selector))[0];    

    if(!$result) {
      return self::notLogged(); 
    }

    if(hash_equals($result['login_token'], hash('sha256', base64_decode($token)))){
      self::relogUser($result['login_userId']);
      return true;        
    }

    return self::notLogged();    
}

Also, the method naming seems a little off. The name isLoggedIn implies that the function just checks if a user is logged in or not and nothing else. What the method actually does is to check if a user logged in and if not, try to log them in. If that fails, it calls the method self::notLogged.

The next step might be to tear apart the function into smaller functions that actually tell you what they do from their name alone - this will avoid confusion. See Single responsibility principle - this applies to functions as well - a function should ideally not do more than one thing.

In particular, the part of your code that performs the authentication and/or login-checking should not be concerned with what happens when authentication has succeeded/failed. Instead, they should signal their respective results to the calling function, either by returning a value or by throwing an Exception.

Exceptions have the added benefit of explicitly signaling to developers working with the code that a function has somehow failed to do what it was meant to. Better yet, they give you the freedom to express what exactly went wrong and how.

All in all, you might end up with something like this:

public static function authenticate() {
    if (self::isLoggedIn()) {
        // user is logged in already - all done!
        return;
    }

    // try to login user using authToken
    if(!isset($_COOKIE['authToken'])){
        throw new \Exception('Authentication failed! No authToken provided');
    }

    list($selector, $token) = explode(':', $_COOKIE['authToken']);
    $result = Database::query('SELECT * FROM logins WHERE login_selector = ?', array($selector))[0];    

    if(!$result) {
        throw new \Exception('Authentication failed! Invalid authToken: login not found');
    }

    if(!hash_equals($result['login_token'], hash('sha256', base64_decode($token)))) {
        throw new \Exception('Authentication failed! Invalid authToken: key mismatch');
    } 

    self::relogUser($result['login_userId']);
}

public static function isLoggedIn(){
    Session::openSession();
    return isset($_SESSION['userId']);
}

public static function handleAuthentication() {
    try {
        self::authenticate();
    } catch (\Exception $e) {
        self::notLogged();
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for all the tips, yeah the "pyramid" shape was what I was trying to avoid. Also, I used your new structure with those functions but I don't get how handleAuthentication is called? I should, after checking if the user is logged in and return false, call handleAuthentication right? \$\endgroup\$ – nick Sep 14 '17 at 13:11
  • \$\begingroup\$ The idea is that handleAuthentication is now roughly equivalent to your old isLoggedIn function, except for the fact that it does not return a value - I assumed that this was what you were going for, since, judging from the code you provided alone it looked like all you would do in case authentication failed was to call notLogged. \$\endgroup\$ – ced Sep 14 '17 at 17:53
  • \$\begingroup\$ Also, in my experience, all you really need to do in case of failed authentication in a web application is to maybe log it somehow and then send a 401 response to the client. It all depends on what you're trying to do though. \$\endgroup\$ – ced Sep 14 '17 at 18:08
1
\$\begingroup\$
        if($result){
            if(!hash_equals($result['login_token'], hash('sha256', base64_decode($token)))){
                return self::notLogged();
            }else{
                self::relogUser($result['login_userId']);
                return true;
            }
        }else{
            return self::notLogged();   
        }
    }else{
        return self::notLogged();
    }

This could just be

        if (result
         && hash_equals($result['login_token'], hash('sha256', base64_decode($token)))
           )
        {
            self::relogUser($result['login_userId']);
            return true;
        }
    }

    return self::notLogged();

You don't need to return self::notLogged() three times. It can just fall through. It's the default action.

I prefer not to say if ! else regardless. And in this case, it simplifies the logic to swap the two clauses.

We can combine the $result check with the hash_equals check, saving us an if. It would also save us an else, but we already got rid of those.

Now we have the function returning three results.

  1. Logged in. Just return true.
  2. Authorized but not logged in. Register the login and then return true.
  3. Not authorized. Return whatever self::notLogged() returns.

And it returns in three places. Presumably the first and third options are the most likely, and they are the quickest to reach in the normal process.

I prefer to have spaces surrounding parentheses and curly brackets. The compiler/interpreter won't care, but it's easier for a human being to tokenize the line with more whitespace.

\$\endgroup\$
1
\$\begingroup\$

Keep in mind that once you return from a function, nothing else in that function will be executed. This eliminates the need for else entirely since anything "else" will only be executed if the function didn't already return. The else is implicit.

Anyway, this is how I would have done it:

public static function isLoggedIn(){
    Session::openSession();

    if(isset($_SESSION['userId'])) return true;

    if(!isset($_COOKIE['authToken'])) return self::notLogged();

    list($selector, $token) = explode(':', $_COOKIE['authToken']);
    $result = Database::query('SELECT * FROM logins WHERE login_selector = ?', array($selector))[0];

    if(!$result) return self::notLogged();

    if(!hash_equals($result['login_token'], hash('sha256', base64_decode($token)))) return self::notLogged();

    self::relogUser($result['login_userId']);
    return true;
}
\$\endgroup\$

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.