HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Authentication service

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
serviceauthenticationstackoverflow

Problem

This is the AuthenticationService I made based on Barry Jaspan's design, which is way better than the code of which I asked in a question a couple of months ago.

I would like it mainly reviewed on a couple of aspects:

  • Readability (clean code)



  • Accurate comments



  • Accurate naming (especially for the last class)



AuthenticationService.php

```
namespace Model\Application;

use Http\HttpSession;
use Model\Infrastructure\Mapper\UserMapper;
use Model\Application\AuthenticationStorage;

/**
* An authentication service.
*
* @author John Doe
*/
class AuthenticationService
{
/**
* @var HttpSession $httpSession An HttpSession instance.
*/
private $httpSession;

/**
* @var UserMapper $userMapper A UserMapper instance.
*/
private $userMapper;

/**
* @var AuthenticationStorage $authenticationStorage An AuthenticationStorage instance.
*/
private $authenticationStorage;

/**
* Constructs the object.
*
* @param HttpSession $httpSession An HttpSession instance.
* @param UserMapper $userMapper A UserMapper instance.
* @param AuthenticationStorage $authenticationStorage An AuthenticationStorage instance.
*/
public function __construct(
HttpSession $httpSession,
UserMapper $userMapper,
AuthenticationStorage $authenticationStorage
) {
$this->httpSession = $httpSession;
$this->userMapper = $userMapper;
$this->authenticationStorage = $authenticationStorage;
}

/**
* Finds the authenticated User entity.
*
* @return User|void|null The User entity instance if found.
*/
public function findAuthenticatedUser()
{
if ($identifier = $this->httpSession->findParameter('userIdentifier')) {
return $this->userMapper->findByIdentifier($identifier);
}

if ($user = $this->authenticationStor

Solution

In storeAuthenticationData, these variables are constants:

// Character pool for CryptoRandCharGen::generateString().
$lowerAlpha = 'abcdefghijklmnopqrstuvwxyz';
$upperAlpha = strtoupper($lowerAlpha);
$digits     = '0123456789';
$symbols    = './<>?;"\'`!@#$%^&*()[]{}_+=|\-';
$characters = $lowerAlpha . $upperAlpha . $digits . $symbols;


Instead of rebuilding $characters every time this method is called,
move these outside of the method and do it only once.

This is especially important because you have these duplicated in updateAuthenticationData too.

The logic of joining and splitting cookie values by : appear at multiple places.
It would be better to encapsulate that logic in a dedicated class,
which hides such details as the : separator from the rest of the implementation.

A minor thing, but in a void method, why not simply return; instead of return null; ?

Code Snippets

// Character pool for CryptoRandCharGen::generateString().
$lowerAlpha = 'abcdefghijklmnopqrstuvwxyz';
$upperAlpha = strtoupper($lowerAlpha);
$digits     = '0123456789';
$symbols    = './<>?;"\'`!@#$%^&*()[]{}_+=|\-';
$characters = $lowerAlpha . $upperAlpha . $digits . $symbols;

Context

StackExchange Code Review Q#64087, answer score: 5

Revisions (0)

No revisions yet.