Return to Answer

In addition to what @janos said:

authenticateUser

Personally, I find these kind of fall-through nested if statements hard to read. It takes a while to see that false will be returned if the actual password_verify returns false.

If you write it like this, it would be clearer, and you would get rid of one level of nesting:

public function authenticateUser($email, $password, $persist = false)
{
 $user = $this->userMapper->findByEmail($email);
 if (!$user) {
 // Prevent timing attacks.
 password_verify('password', '2ドルy12ドル$liVAn/nttQIipJI3LagCAOehLX0iamDjIn2WlX38ZEKS/dDHR3e1W'); // you could also return here and change else-if to if.
 } else if (password_verify($password, $user->getPassword())) {
 // [... session code ...]
 return true;
 }
 return false;
}

I like that you thought about defending against email harvesting timing attacks (of course, if you have a sign-up page they can be harvested there, but it's still good).

Comments

@return User|void|null The User entity instance if found.

I would be interested in when this returns void, and when null.

True for authentication persistence, false if not. (optional)

I would also mention what the default option is ((optional; false by default)

// Issue a data with this data equivalent to the client.

This doesn't sound quite right. Issue a Cookie?

Naming

• you could use id instead of identifier. It is well understood and shorter, thus might be easier to read (findByIdentifier -> findById, $identifier -> $id, etc).
• getPassword of user doesn't really express what it does, as it returns a hash. getPasswordHash might be a better fit.
• $data is very generic, $cookieData might be better.
• it might be a bit confusing to have authenticateUser in two classes for different activities. I don't have good alternatives, but maybe authenticateUserByCookie? But in that case it doesn't fit in with the rest of the methods. You could change the whole naming schema of AuthenticationStorage to fix this (this class mainly deals with managing cookies, but the naming doesn't reflect this).

Misc

• some of your lines are too long. I would aim for 80 characters per line, you could aim for more, but 195 characters don't fit on a lot of monitors.
• I would probably use a bit less newlines. In AuthenticationStorage:authenticateUser() before the return false;, and generally in AuthenticationService.
• your importing Authentication in AuthenticationStorage but are not using it.

In addition to what @janos said:

authenticateUser

Personally, I find these kind of fall-through nested if statements hard to read. It takes a while to see that false will be returned if the actual password_verify returns false.

If you write it like this, it would be clearer, and you would get rid of one level of nesting:

public function authenticateUser($email, $password, $persist = false)
{
 $user = $this->userMapper->findByEmail($email);
 if (!$user) {
 // Prevent timing attacks.
 password_verify('password', '2ドルy12ドル$liVAn/nttQIipJI3LagCAOehLX0iamDjIn2WlX38ZEKS/dDHR3e1W'); // you could also return here and change else-if to if.
 } else if (password_verify($password, $user->getPassword())) {
 // [... session code ...]
 return true;
 }
 return false;
}

I like that you thought about defending against email harvesting timing attacks (of course, if you have a sign-up page they can be harvested there, but it's still good).

Comments

@return User|void|null The User entity instance if found.

I would be interested in when this returns void, and when null.

True for authentication persistence, false if not. (optional)

I would also mention what the default option is ((optional; false by default)

// Issue a data with this data equivalent to the client.

This doesn't sound quite right. Issue a Cookie?

Naming

• you could use id instead of identifier. It is well understood and shorter, thus might be easier to read (findByIdentifier -> findById, $identifier -> $id, etc).
• getPassword of user doesn't really express what it does, as it returns a hash. getPasswordHash might be a better fit.
• $data is very generic, $cookieData might be better.
• it might be a bit confusing to have authenticateUser in two classes for different activities. I don't have good alternatives, but maybe authenticateUserByCookie? But in that case it doesn't fit in with the rest of the methods. You could change the whole naming schema of AuthenticationStorage to fix this (this class mainly deals with managing cookies, but the naming doesn't reflect this).

Misc

• some of your lines are too long. I would aim for 80 characters per line, you could aim for more, but 195 characters don't fit on a lot of monitors.
• I would probably use a bit less newlines. In AuthenticationStorage:authenticateUser() before the return false;, and generally in AuthenticationService.
• your importing Authentication in AuthenticationStorage but are not using it.

In addition to what @jonas said:

authenticateUser

Personally, I find these kind of fall-through nested if statements hard to read. It takes a while to see that false will be returned if the actual password_verify returns false.

If you write it like this, it would be clearer, and you would get rid of one level of nesting:

public function authenticateUser($email, $password, $persist = false)
{
 $user = $this->userMapper->findByEmail($email);
 if (!$user) {
 // Prevent timing attacks.
 password_verify('password', '2ドルy12ドル$liVAn/nttQIipJI3LagCAOehLX0iamDjIn2WlX38ZEKS/dDHR3e1W'); // you could also return here and change else-if to if.
 } else if (password_verify($password, $user->getPassword())) {
 // [... session code ...]
 return true;
 }
 return false;
}

I like that you thought about defending against email harvesting timing attacks (of course, if you have a sign-up page they can be harvested there, but it's still good).

Comments

@return User|void|null The User entity instance if found.

I would be interested in when this returns void, and when null.

True for authentication persistence, false if not. (optional)

I would also mention what the default option is ((optional; false by default)

// Issue a data with this data equivalent to the client.

This doesn't sound quite right. Issue a Cookie?

Naming

• you could use id instead of identifier. It is well understood and shorter, thus might be easier to read (findByIdentifier -> findById, $identifier -> $id, etc).
• getPassword of user doesn't really express what it does, as it returns a hash. getPasswordHash might be a better fit.
• $data is very generic, $cookieData might be better.
• it might be a bit confusing to have authenticateUser in two classes for different activities. I don't have good alternatives, but maybe authenticateUserByCookie? But in that case it doesn't fit in with the rest of the methods. You could change the whole naming schema of AuthenticationStorage to fix this (this class mainly deals with managing cookies, but the naming doesn't reflect this).

Misc

• some of your lines are too long. I would aim for 80 characters per line, you could aim for more, but 195 characters don't fit on a lot of monitors.
• I would probably use a bit less newlines. In AuthenticationStorage:authenticateUser() before the return false;, and generally in AuthenticationService.
• your importing Authentication in AuthenticationStorage but are not using it.

In addition to what @janos said:

authenticateUser

Personally, I find these kind of fall-through nested if statements hard to read. It takes a while to see that false will be returned if the actual password_verify returns false.

If you write it like this, it would be clearer, and you would get rid of one level of nesting:

public function authenticateUser($email, $password, $persist = false)
{
 $user = $this->userMapper->findByEmail($email);
 if (!$user) {
 // Prevent timing attacks.
 password_verify('password', '2ドルy12ドル$liVAn/nttQIipJI3LagCAOehLX0iamDjIn2WlX38ZEKS/dDHR3e1W'); // you could also return here and change else-if to if.
 } else if (password_verify($password, $user->getPassword())) {
 // [... session code ...]
 return true;
 }
 return false;
}

I like that you thought about defending against email harvesting timing attacks (of course, if you have a sign-up page they can be harvested there, but it's still good).

Comments

@return User|void|null The User entity instance if found.

I would be interested in when this returns void, and when null.

True for authentication persistence, false if not. (optional)

I would also mention what the default option is ((optional; false by default)

// Issue a data with this data equivalent to the client.

This doesn't sound quite right. Issue a Cookie?

Naming

• you could use id instead of identifier. It is well understood and shorter, thus might be easier to read (findByIdentifier -> findById, $identifier -> $id, etc).
• getPassword of user doesn't really express what it does, as it returns a hash. getPasswordHash might be a better fit.
• $data is very generic, $cookieData might be better.
• it might be a bit confusing to have authenticateUser in two classes for different activities. I don't have good alternatives, but maybe authenticateUserByCookie? But in that case it doesn't fit in with the rest of the methods. You could change the whole naming schema of AuthenticationStorage to fix this (this class mainly deals with managing cookies, but the naming doesn't reflect this).

Misc

• some of your lines are too long. I would aim for 80 characters per line, you could aim for more, but 195 characters don't fit on a lot of monitors.
• I would probably use a bit less newlines. In AuthenticationStorage:authenticateUser() before the return false;, and generally in AuthenticationService.
• your importing Authentication in AuthenticationStorage but are not using it.

In addition to what @jonas said:

authenticateUser

Personally, I find these kind of fall-through nested if statements hard to read. It takes a while to see that false will be returned if the actual password_verify returns false.

If you write it like this, it would be clearer, and you would get rid of one level of nesting:

public function authenticateUser($email, $password, $persist = false)
{
 $user = $this->userMapper->findByEmail($email);
 if (!$user) {
 // Prevent timing attacks.
 password_verify('password', '2ドルy12ドル$liVAn/nttQIipJI3LagCAOehLX0iamDjIn2WlX38ZEKS/dDHR3e1W'); // you could also return here and change else-if to if.
 } else if (password_verify($password, $user->getPassword())) {
 // [... session code ...]
 return true;
 }
 return false;
}

I like that you thought about defending against email harvesting timing attacks (of course, if you have a sign-up page they can be harvested there, but it's still good).

Comments

@return User|void|null The User entity instance if found.

I would be interested in when this returns void, and when null.

True for authentication persistence, false if not. (optional)

I would also mention what the default option is ((optional; false by default)

// Issue a data with this data equivalent to the client.

This doesn't sound quite right. Issue a Cookie?

Misc

• some of your lines are too long. I would aim for 80 characters per line, you could aim for more, but 195 characters don't fit on a lot of monitors.
• I would probably use a bit less newlines. In AuthenticationStorage:authenticateUser() before the return false;, and generally in AuthenticationService.
• your importing Authentication in AuthenticationStorage but are not using it.

In addition to what @jonas said:

authenticateUser

Personally, I find these kind of fall-through nested if statements hard to read. It takes a while to see that false will be returned if the actual password_verify returns false.

If you write it like this, it would be clearer, and you would get rid of one level of nesting:

public function authenticateUser($email, $password, $persist = false)
{
 $user = $this->userMapper->findByEmail($email);
 if (!$user) {
 // Prevent timing attacks.
 password_verify('password', '2ドルy12ドル$liVAn/nttQIipJI3LagCAOehLX0iamDjIn2WlX38ZEKS/dDHR3e1W'); // you could also return here and change else-if to if.
 } else if (password_verify($password, $user->getPassword())) {
 // [... session code ...]
 return true;
 }
 return false;
}

I like that you thought about defending against email harvesting timing attacks (of course, if you have a sign-up page they can be harvested there, but it's still good).

Comments

@return User|void|null The User entity instance if found.

I would be interested in when this returns void, and when null.

True for authentication persistence, false if not. (optional)

I would also mention what the default option is ((optional; false by default)

// Issue a data with this data equivalent to the client.

This doesn't sound quite right. Issue a Cookie?

Naming

• you could use id instead of identifier. It is well understood and shorter, thus might be easier to read (findByIdentifier -> findById, $identifier -> $id, etc).
• getPassword of user doesn't really express what it does, as it returns a hash. getPasswordHash might be a better fit.
• $data is very generic, $cookieData might be better.
• it might be a bit confusing to have authenticateUser in two classes for different activities. I don't have good alternatives, but maybe authenticateUserByCookie? But in that case it doesn't fit in with the rest of the methods. You could change the whole naming schema of AuthenticationStorage to fix this (this class mainly deals with managing cookies, but the naming doesn't reflect this).

Misc

• some of your lines are too long. I would aim for 80 characters per line, you could aim for more, but 195 characters don't fit on a lot of monitors.
• I would probably use a bit less newlines. In AuthenticationStorage:authenticateUser() before the return false;, and generally in AuthenticationService.
• your importing Authentication in AuthenticationStorage but are not using it.