patternphpMinor
Complex if statements in authentication method
Viewed 0 times
statementsauthenticationcomplexmethod
Problem
I had asked questions about my entire class which I'll flag for closing soon. It appeared to be a little too broad for 1 post so I chopped it up and will be asking a little more specific questions separately.
In my Application Layer I have a class
I'd appreciate anybody who could review this piece of code and give me suggestions on improving / optimizing the design of it. Any other suggestions are welcome as well.
```
public function findLoggedInUser()
{
if (($userData = $this->httpSession->getParameter('user')) && isset($userData['id'])) {
// User found in current http session
return $this->userMapper->findById($userData['id']);
} elseif ($httpSessionData = $this->httpCookie->getParameter('usess')) {
if (isset($httpSessionData['uid'], $httpSessionData['sno'], $httpSessionData['token'])) {
if ($user = $this->userMapper->findById($httpSessionData['uid'])) {
if ($httpSession = $this->httpSessionMapper->findByUserAndSeriesNo($user, $httpSessionData['sno'])) {
if (password_verify($httpSessionData['token'], (string) $httpSession->getTokenHash()) {
// Generate new http session entity tokens
$token['raw'] = new Token(CryptoCharGen::numeric(60));
$token['hash'] = new TokenHash(password_hash((string) $token['raw'], PASSWORD_BCRYPT, ['cost' => 4]));
// Update and persist http session entity with new token
$httpSession->setTokenHash($token['hash']);
$this->httpSessionMapper->update($httpSession);
// Issue new http cookie usess with updated http session recovery data
$this->httpCookie->setParameter('usess', [
'uid' =
In my Application Layer I have a class
AuthService containing the following method with a code structure that has gotten quite complex with if statements that I'm unable to re-factor a better way.I'd appreciate anybody who could review this piece of code and give me suggestions on improving / optimizing the design of it. Any other suggestions are welcome as well.
```
public function findLoggedInUser()
{
if (($userData = $this->httpSession->getParameter('user')) && isset($userData['id'])) {
// User found in current http session
return $this->userMapper->findById($userData['id']);
} elseif ($httpSessionData = $this->httpCookie->getParameter('usess')) {
if (isset($httpSessionData['uid'], $httpSessionData['sno'], $httpSessionData['token'])) {
if ($user = $this->userMapper->findById($httpSessionData['uid'])) {
if ($httpSession = $this->httpSessionMapper->findByUserAndSeriesNo($user, $httpSessionData['sno'])) {
if (password_verify($httpSessionData['token'], (string) $httpSession->getTokenHash()) {
// Generate new http session entity tokens
$token['raw'] = new Token(CryptoCharGen::numeric(60));
$token['hash'] = new TokenHash(password_hash((string) $token['raw'], PASSWORD_BCRYPT, ['cost' => 4]));
// Update and persist http session entity with new token
$httpSession->setTokenHash($token['hash']);
$this->httpSessionMapper->update($httpSession);
// Issue new http cookie usess with updated http session recovery data
$this->httpCookie->setParameter('usess', [
'uid' =
Solution
Two suggestions on how to keep this code maintainable:
-
use early exits
-
keep conditions atomic (one
Example:
This way, the code remains readable and flexible for future modifications.
-
use early exits
-
keep conditions atomic (one
if checks for one thing) Example:
function findLoggedInUser()
{
if (($userData = $this->httpSession->getParameter('user')) && isset($userData['id'])) {
// User found in current http session
return $this->userMapper->findById($userData['id']);
}
$httpSessionData = $this->httpCookie->getParameter('usess');
if(!$httpSessionData) {
$this->httpCookie->destroy('usess');
return null;
}
if(!isset($httpSessionData['uid'], $httpSessionData['sno'], $httpSessionData['token'])) {
$this->httpCookie->destroy('usess');
return null;
}
$user = $this->userMapper->findById($httpSessionData['uid']);
if(!$user) {
$this->httpCookie->destroy('usess');
return null;
}
$httpSession = $this->httpSessionMapper->findByUserAndSeriesNo($user, $httpSessionData['sno']));
if(!$httpSessionData) {
$this->httpCookie->destroy('usess');
return null;
}
if (!password_verify($httpSessionData['token'], (string) $httpSession->getTokenHash()) {
$this->httpSessionMapper->deleteAllByUser($user);
$this->httpCookie->destroy('usess');
return null;
}
// EVERYTHING'S FINE, DO THE JOB
// Generate new http session entity tokens
$token['raw'] = new Token(CryptoCharGen::numeric(60));
$token['hash'] = new TokenHash(password_hash((string) $token['raw'], PASSWORD_BCRYPT, ['cost' => 4]));
// Update and persist http session entity with new token
$httpSession->setTokenHash($token['hash']);
$this->httpSessionMapper->update($httpSession);
// Issue new http cookie usess with updated http session recovery data
$this->httpCookie->setParameter('usess', [
'uid' => (string) $httpSession->getUser()->getId(),
'sno' => (string) $httpSession->getSeriesNo(),
'token' => (string) $token['raw']
], 60 * 60 * 24 * 90);
// User has been found
return $httpSession->getUser();
}This way, the code remains readable and flexible for future modifications.
Code Snippets
function findLoggedInUser()
{
if (($userData = $this->httpSession->getParameter('user')) && isset($userData['id'])) {
// User found in current http session
return $this->userMapper->findById($userData['id']);
}
$httpSessionData = $this->httpCookie->getParameter('usess');
if(!$httpSessionData) {
$this->httpCookie->destroy('usess');
return null;
}
if(!isset($httpSessionData['uid'], $httpSessionData['sno'], $httpSessionData['token'])) {
$this->httpCookie->destroy('usess');
return null;
}
$user = $this->userMapper->findById($httpSessionData['uid']);
if(!$user) {
$this->httpCookie->destroy('usess');
return null;
}
$httpSession = $this->httpSessionMapper->findByUserAndSeriesNo($user, $httpSessionData['sno']));
if(!$httpSessionData) {
$this->httpCookie->destroy('usess');
return null;
}
if (!password_verify($httpSessionData['token'], (string) $httpSession->getTokenHash()) {
$this->httpSessionMapper->deleteAllByUser($user);
$this->httpCookie->destroy('usess');
return null;
}
// EVERYTHING'S FINE, DO THE JOB
// Generate new http session entity tokens
$token['raw'] = new Token(CryptoCharGen::numeric(60));
$token['hash'] = new TokenHash(password_hash((string) $token['raw'], PASSWORD_BCRYPT, ['cost' => 4]));
// Update and persist http session entity with new token
$httpSession->setTokenHash($token['hash']);
$this->httpSessionMapper->update($httpSession);
// Issue new http cookie usess with updated http session recovery data
$this->httpCookie->setParameter('usess', [
'uid' => (string) $httpSession->getUser()->getId(),
'sno' => (string) $httpSession->getSeriesNo(),
'token' => (string) $token['raw']
], 60 * 60 * 24 * 90);
// User has been found
return $httpSession->getUser();
}Context
StackExchange Code Review Q#60203, answer score: 7
Revisions (0)
No revisions yet.