Skip to content

Commit

Permalink
Fixed authentication security tests and cs [SLE-193]
Browse files Browse the repository at this point in the history
  • Loading branch information
samuelgfeller committed Oct 17, 2023
1 parent c1aff81 commit 68add78
Show file tree
Hide file tree
Showing 33 changed files with 97 additions and 99 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public function __invoke(ServerRequestInterface $request, ResponseInterface $res
if (isset($queryParams['redirect'])) {
return $this->responder->redirectToUrl($response, $queryParams['redirect']);
}

// Otherwise, go to home page
return $this->responder->redirectToRouteName($response, 'home-page');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ public function __invoke(ServerRequest $request, Response $response): Response
);
$this->logger->error('Invalid or expired token user_verification id: ' . $queryParams['id']);
$newQueryParam = isset($queryParams['redirect']) ? ['redirect' => $queryParams['redirect']] : [];

// Redirect to login page with redirect query param if set
return $this->responder->redirectToRouteName($response, 'login-page', [], $newQueryParam);
} catch (UserAlreadyVerifiedException $uave) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ public function __invoke(ServerRequest $request, Response $response): Response
$this->logger->error(
'Invalid or expired token password reset user_verification id: ' . $parsedBody['id']
);

// Render password
return $this->responder->render($response, 'authentication/login.html.php');
} catch (ValidationException $validationException) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ public function __invoke(ServerRequest $request, Response $response): Response
$flash->add('error', __('Invalid or expired link. Please log in to receive a new link.'));
$this->logger->error('Invalid or expired token user_verification id: ' . $queryParams['id']);
$newQueryParam = isset($queryParams['redirect']) ? ['redirect' => $queryParams['redirect']] : [];

// Redirect to login page with redirect query param if set
return $this->responder->redirectToRouteName($response, 'login-page', [], $newQueryParam);
} catch (UserAlreadyVerifiedException $uave) {
Expand Down
1 change: 1 addition & 0 deletions src/Application/Actions/Client/Ajax/ClientDeleteAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public function __invoke(
$this->logger->notice(
'403 Forbidden, user ' . $loggedInUserId . ' tried to delete other client with id: ' . $clientId
);

// Not throwing HttpForbiddenException as it's a json request and response should be json too
return $this->responder->respondWithJson(
$response,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ public function __invoke(
IntlDateFormatter::LONG,
IntlDateFormatter::SHORT
);

// camelCase according to Google recommendation
return $this->responder->respondWithJson($response, [
'status' => 'success',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ public function __invoke(
$this->logger->notice(
'403 Forbidden, user ' . $loggedInUserId . ' tried to delete other note with id: ' . $noteId
);

// Not throwing HttpForbiddenException as it's a json request and response should be json too
return $this->responder->respondWithJson(
$response,
Expand Down
1 change: 1 addition & 0 deletions src/Application/Actions/Note/Page/NoteReadPageAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public function __invoke(
$flash = $this->session->getFlash();
// If not existing note, inform user
$flash->add('error', __('The note was not not found.'));

// When note does not exist link to client list page
return $this->responder->redirectToRouteName($response, 'client-list-page');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public function __invoke(
if ($updated) {
return $this->responder->respondWithJson($response, ['status' => 'success', 'data' => null]);
}

// If for example values didn't change
return $this->responder->respondWithJson(
$response,
Expand Down
1 change: 1 addition & 0 deletions src/Application/Middleware/CorsMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface

return $response;
}

// If no allowOrigin url, handle return response
return $handler->handle($request);
}
Expand Down
1 change: 1 addition & 0 deletions src/Application/Validation/MalformedRequestBodyChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public function requestBodyHasValidKeys(?array $parsedBody, array $requiredKeys,
return false;
}
}

// Check that all required keys are set plus the additional ones if there were some
return count($parsedBody ?? []) === $amount;
}
Expand Down
1 change: 1 addition & 0 deletions src/Common/LocaleHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ public function getLanguageCodeForPath(): string
$localeCode = setlocale(LC_ALL, 0);
// Available locales keys are language codes ('en', 'de') and values are locale codes ('en_US', 'de_CH')
$langCode = array_search($localeCode, $this->localeSettings['available'], true) ?: '';

// If language code is 'en' return empty string as default email templates are in english and not in a sub folder
// If language code is not empty, add a slash to complete the path it will be inserted into
return $langCode === 'en' || $langCode === '' ? '' : $langCode . '/';
Expand Down
22 changes: 14 additions & 8 deletions src/Domain/Authentication/Service/LoginNonActiveUserHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,15 @@ public function __construct(
* Handles the login attempt from a non-active user.
* Sends an email to the user with information and a link to activate his account.
*
* @param UserData $dbUser The user data from the database.
* @param array $queryParams The query parameters.
* @param UserData $dbUser the user data from the database
* @param array $queryParams the query parameters
* @param ?string $captcha
*
* @throws TransportExceptionInterface if there is an exception while sending an email
* @throws \RuntimeException if there is an invalid status in the database
* @throws UnableToLoginStatusNotActiveException thrown in all cases as the user status is not active
*
* @return void
* @throws TransportExceptionInterface If there is an exception while sending an email.
* @throws \RuntimeException If there is an invalid status in the database.
* @throws UnableToLoginStatusNotActiveException Thrown in all cases as the user status is not active.
*/
public function handleLoginAttemptFromNonActiveUser(
UserData $dbUser,
Expand Down Expand Up @@ -109,8 +112,9 @@ public function handleLoginAttemptFromNonActiveUser(
* @param UserData $user
* @param array $queryParams
*
* @return void
* @throws TransportExceptionInterface
*
* @return void
*/
private function handleUnverifiedUserLoginAttempt(UserData $user, array $queryParams = []): void
{
Expand All @@ -127,8 +131,9 @@ private function handleUnverifiedUserLoginAttempt(UserData $user, array $queryPa
*
* @param UserData $user
*
* @return void
* @throws TransportExceptionInterface
*
* @return void
*/
private function handleSuspendedUserLoginAttempt(UserData $user): void
{
Expand All @@ -146,8 +151,9 @@ private function handleSuspendedUserLoginAttempt(UserData $user): void
* @param UserData $user
* @param array $queryParams existing query params like redirect
*
* @return void
* @throws TransportExceptionInterface
*
* @return void
*/
private function handleLockedUserLoginAttempt(UserData $user, array $queryParams = []): void
{
Expand Down
5 changes: 3 additions & 2 deletions src/Domain/Authentication/Service/LoginVerifier.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

class LoginVerifier
{

public function __construct(
private readonly UserValidator $userValidator,
private readonly SecurityLoginChecker $loginSecurityChecker,
Expand All @@ -36,8 +35,9 @@ public function __construct(
* @param string|null $captcha user captcha response if filled out
* @param array $queryParams
*
* @return int id
* @throws TransportExceptionInterface
*
* @return int id
*/
public function getUserIdIfAllowedToLogin(
array $userLoginValues,
Expand Down Expand Up @@ -71,6 +71,7 @@ public function getUserIdIfAllowedToLogin(
['login'],
$dbUser->id
);

// Return id (not sure if it's better to regenerate session here in service or in action)
return $dbUser->id;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ public function sendPasswordRecoveryEmail(array $userValues, ?string $captcha =
$this->mailer->send($this->email);
// Reset locale
$this->localeHelper->setLanguage($originalLocale);

// User activity entry is done when user verification token is created
return;
}
Expand Down
1 change: 1 addition & 0 deletions src/Domain/Client/Service/ClientFinder.php
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ public function findAllClientsFromUser(int $userId): ClientResultDataCollection
{
$clientResultCollection = new ClientResultDataCollection();
$clientResultCollection->clients = $this->clientFinderRepository->findAllClientsByUserId($userId);

// $this->clientUserRightSetter->defineUserRightsOnClients($allClients);
return $clientResultCollection;
}
Expand Down
1 change: 1 addition & 0 deletions src/Domain/Note/Service/NoteFilterFinder.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public function findNotesWithFilter(array $params): array
// Exception message tested in NoteFilterProvider.php
throw new InvalidNoteFilterException('Value has to be numeric.');
}

// Get notes from user and return them
return $this->noteFinder->findAllNotesExceptMainFromUser((int)$params['user']);
}
Expand Down
2 changes: 2 additions & 0 deletions src/Domain/Security/Service/EmailRequestFinder.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public function findEmailAmountInSetTimespan(string $email): int
* Returns the very last EMAIL request from actual ip or given email.
*
* @param string $email
*
* @return int
*/
public function findLastEmailRequestTimestamp(string $email): int
Expand All @@ -45,6 +46,7 @@ public function findLastEmailRequestTimestamp(string $email): int
if ($createdAt) {
return (new \DateTime($createdAt))->format('U');
}

return 0;
}
}
1 change: 1 addition & 0 deletions src/Domain/Security/Service/LoginRequestFinder.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ public function findLatestLoginRequestTimestamp(string $email): int
if ($createdAt) {
return (new \DateTime($createdAt))->format('U');
}

return 0;
}
}
1 change: 0 additions & 1 deletion src/Domain/Security/Service/SecurityEmailChecker.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ public function performEmailAbuseCheck(string $email, ?string $reCaptchaResponse
*
* @param int $emailsAmount amount of emails sent in the last timespan
* @param string $email
*
*/
private function performEmailRequestsCheck(
int $emailsAmount,
Expand Down
5 changes: 4 additions & 1 deletion src/Domain/Utility/Mailer.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace App\Domain\Utility;

use App\Application\Data\UserNetworkSessionData;
use App\Infrastructure\SecurityLogging\EmailLoggerRepository;
use Slim\Views\PhpRenderer;
use Symfony\Component\Mailer\Exception\TransportExceptionInterface;
Expand All @@ -26,6 +27,7 @@ public function __construct(
private readonly MailerInterface $mailer,
private readonly PhpRenderer $phpRenderer,
private readonly EmailLoggerRepository $emailLoggerRepository,
private readonly UserNetworkSessionData $userNetworkSessionData
) {
}

Expand Down Expand Up @@ -56,9 +58,9 @@ public function getContentFromTemplate(string $templatePath, array $templateData
*
* @param Email $email
*
* @return void
* @throws TransportExceptionInterface
*
* @return void
*/
public function send(Email $email): void
{
Expand All @@ -71,6 +73,7 @@ public function send(Email $email): void
$email->getFrom()[0]->getAddress(),
$email->getTo()[0]->getAddress(),
$email->getSubject(),
$this->userNetworkSessionData->userId
);
}
}
3 changes: 3 additions & 0 deletions src/Infrastructure/Client/ClientFinderRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public function findClientsWithResultAggregate(array $whereArray = ['client.dele
->where($whereArray);
$sql = $query->sql();
$resultRows = $query->execute()->fetchAll('assoc') ?: [];

// Convert to list of Post objects with associated User info
return $this->hydrator->hydrate($resultRows, ClientResultData::class);
}
Expand Down Expand Up @@ -135,6 +136,7 @@ public function findClientAggregateByIdIncludingDeleted(int $id): ClientResultDa
);

$resultRows = $query->execute()->fetch('assoc') ?: [];

// Instantiate UserPost DTO
return new ClientResultData($resultRows);
}
Expand All @@ -158,6 +160,7 @@ public function findAllClientsByUserId(int $userId): array
['client.user_id' => $userId, 'client.deleted_at IS' => null]
);
$resultRows = $query->execute()->fetchAll('assoc') ?: [];

// Convert to list of Post objects with aggregate
return $this->hydrator->hydrate($resultRows, ClientResultData::class);
}
Expand Down
6 changes: 6 additions & 0 deletions src/Infrastructure/Note/NoteFinderRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public function findAllNotesWithUsers(): array
->join(['table' => 'user', 'conditions' => 'note.user_id = user.id'])
->andWhere(['note.deleted_at IS' => null]);
$resultRows = $query->execute()->fetchAll('assoc') ?: [];

// Convert to list of Note objects with associated User info
return $this->hydrator->hydrate($resultRows, NoteResultData::class);
}
Expand Down Expand Up @@ -81,6 +82,7 @@ public function findNoteWithUserById(int $id): NoteResultData
->join(['table' => 'user', 'conditions' => 'note.user_id = user.id'])
->andWhere(['note.id' => $id, 'note.deleted_at IS' => null]);
$resultRows = $query->execute()->fetch('assoc') ?: [];

// Instantiate UserNote DTO
return new NoteResultData($resultRows);
}
Expand Down Expand Up @@ -130,6 +132,7 @@ public function findAllNotesExceptMainByUserId(int $userId): array
'note.deleted_at IS' => null,
]);
$resultRows = $query->execute()->fetchAll('assoc') ?: [];

// Convert to list of Note objects with associated User info
return $this->hydrator->hydrate($resultRows, NoteResultData::class);
}
Expand Down Expand Up @@ -163,6 +166,7 @@ public function findAllNotesExceptMainWithUserByClientId(int $clientId): array
]
)->orderDesc('note.created_at');
$resultRows = $query->execute()->fetchAll('assoc') ?: [];

// Convert to list of Note objects with associated User info
return $this->hydrator->hydrate($resultRows, NoteResultData::class);
}
Expand Down Expand Up @@ -198,6 +202,7 @@ public function findMostRecentNotes(int $notesAmount): array
->orderDesc('note.updated_at')->limit($notesAmount);

$resultRows = $query->execute()->fetchAll('assoc') ?: [];

// Convert to list of Note objects with associated User info
return $this->hydrator->hydrate($resultRows, NoteResultData::class);
}
Expand All @@ -223,6 +228,7 @@ public function findClientNotesAmount(int $clientId): int
'n.deleted_at IS' => null,
]
);

// Return amount of notes
return (int)$query->execute()->fetch('assoc')['amount'];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public function __construct(
* @param ?string $ip
* @param bool $success whether login request was a successful login or not
* @param int|null $userId
*
* @return string
*/
public function logLoginRequest(string $email, ?string $ip, bool $success, ?int $userId = null): string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public function __construct(
* @param string $email
* @param int $seconds
* Throws PersistenceRecordNotFoundException if entry not found
*
* @return int
*/
public function getLoggedEmailCountInTimespan(string $email, int $seconds): int
Expand Down Expand Up @@ -44,6 +45,7 @@ public function getLoggedEmailCountInTimespan(string $email, int $seconds): int
* Searches the latest email request concerning a specific email address.
*
* @param string $email
*
* @return string|bool datetime of last email request or false
*/
public function findLatestEmailRequest(string $email): bool|string
Expand Down Expand Up @@ -77,8 +79,6 @@ public function getGlobalSentEmailAmount(int $days): int
'created_at >' => $query->newExpr('DATE_SUB(NOW(), INTERVAL :days DAY)'),
]
)->bind(':days', $days, 'integer');
$val = (int)($query->execute()->fetch('assoc')['sent_email_amount'] ?? 0);

return $val;
return (int)($query->execute()->fetch('assoc')['sent_email_amount'] ?? 0);
}
}
Loading

0 comments on commit 68add78

Please sign in to comment.