refactor: fix SonarQube code smells and add AnalyticsCryptoService tests
Reduce cognitive complexity and fix code smells across multiple files: - Extract helper methods in DocuSealService, ForgotPasswordController, WebhookDocuSealController - Reduce MembresController.persistLocalUser from 8 to 3 parameters using typed array - Replace chained if/returns with ROLE_ROUTES map in LoginSuccessHandler - Add 100% test coverage for AnalyticsCryptoService (15 tests) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -85,7 +85,14 @@ class MembresController extends AbstractController
|
||||
if (!$kcResult['created']) {
|
||||
$this->addFlash('error', 'Erreur lors de la creation du compte Keycloak. L\'email existe peut-etre deja.');
|
||||
} else {
|
||||
$user = $this->persistLocalUser($em, $passwordHasher, $email, $firstName, $lastName, $kcResult['keycloakId'], $kcResult['tempPassword'], $groups);
|
||||
$this->persistLocalUser($em, $passwordHasher, [
|
||||
'email' => $email,
|
||||
'firstName' => $firstName,
|
||||
'lastName' => $lastName,
|
||||
'keycloakId' => $kcResult['keycloakId'],
|
||||
'tempPassword' => $kcResult['tempPassword'],
|
||||
'groups' => $groups,
|
||||
]);
|
||||
$this->sendCreationEmail($mailer, $twig, $email, $firstName, $lastName, $kcResult['tempPassword'], $groups);
|
||||
|
||||
$this->addFlash('success', 'Le membre '.$firstName.' '.$lastName.' a ete cree. Un email avec les identifiants lui a ete envoye.');
|
||||
@@ -118,26 +125,21 @@ class MembresController extends AbstractController
|
||||
}
|
||||
|
||||
/**
|
||||
* @param list<string> $groups
|
||||
* @param array{email: string, firstName: string, lastName: string, keycloakId: string, tempPassword: string, groups: list<string>} $data
|
||||
*/
|
||||
private function persistLocalUser(
|
||||
EntityManagerInterface $em,
|
||||
UserPasswordHasherInterface $passwordHasher,
|
||||
string $email,
|
||||
string $firstName,
|
||||
string $lastName,
|
||||
string $keycloakId,
|
||||
string $tempPassword,
|
||||
array $groups,
|
||||
array $data,
|
||||
): User {
|
||||
$user = new User();
|
||||
$user->setEmail($email);
|
||||
$user->setFirstName($firstName);
|
||||
$user->setLastName($lastName);
|
||||
$user->setKeycloakId($keycloakId);
|
||||
$user->setRoles(\in_array('super_admin_asso', $groups, true) ? ['ROLE_ROOT'] : ['ROLE_EMPLOYE']);
|
||||
$user->setPassword($passwordHasher->hashPassword($user, $tempPassword));
|
||||
$user->setTempPassword($tempPassword);
|
||||
$user->setEmail($data['email']);
|
||||
$user->setFirstName($data['firstName']);
|
||||
$user->setLastName($data['lastName']);
|
||||
$user->setKeycloakId($data['keycloakId']);
|
||||
$user->setRoles(\in_array('super_admin_asso', $data['groups'], true) ? ['ROLE_ROOT'] : ['ROLE_EMPLOYE']);
|
||||
$user->setPassword($passwordHasher->hashPassword($user, $data['tempPassword']));
|
||||
$user->setTempPassword($data['tempPassword']);
|
||||
|
||||
$em->persist($user);
|
||||
$em->flush();
|
||||
|
||||
@@ -94,18 +94,9 @@ class ForgotPasswordController extends AbstractController
|
||||
$sessionEmail = $session->get('reset_email');
|
||||
$expires = $session->get('reset_expires', 0);
|
||||
|
||||
if (time() > $expires) {
|
||||
$this->clearResetSession($session);
|
||||
|
||||
return ['redirect' => false, 'step' => 'email', 'error' => 'Le code a expire. Veuillez recommencer.'];
|
||||
}
|
||||
|
||||
if (!hash_equals($sessionCode, $inputCode)) {
|
||||
return ['redirect' => false, 'step' => 'code', 'error' => 'Le code est incorrect.'];
|
||||
}
|
||||
|
||||
if (\strlen($newPassword) < 8) {
|
||||
return ['redirect' => false, 'step' => 'code', 'error' => 'Le mot de passe doit contenir au moins 8 caracteres.'];
|
||||
$error = $this->validateResetInput($session, $inputCode, $newPassword, $sessionCode, $expires);
|
||||
if (null !== $error) {
|
||||
return $error;
|
||||
}
|
||||
|
||||
$user = $userRepository->findOneBy(['email' => $sessionEmail]);
|
||||
@@ -129,6 +120,28 @@ class ForgotPasswordController extends AbstractController
|
||||
return ['redirect' => true, 'step' => 'email', 'error' => null];
|
||||
}
|
||||
|
||||
/**
|
||||
* @return array{redirect: bool, step: string, error: string}|null
|
||||
*/
|
||||
private function validateResetInput(object $session, string $inputCode, string $newPassword, string $sessionCode, int $expires): ?array
|
||||
{
|
||||
if (time() > $expires) {
|
||||
$this->clearResetSession($session);
|
||||
|
||||
return ['redirect' => false, 'step' => 'email', 'error' => 'Le code a expire. Veuillez recommencer.'];
|
||||
}
|
||||
|
||||
if (!hash_equals($sessionCode, $inputCode)) {
|
||||
return ['redirect' => false, 'step' => 'code', 'error' => 'Le code est incorrect.'];
|
||||
}
|
||||
|
||||
if (\strlen($newPassword) < 8) {
|
||||
return ['redirect' => false, 'step' => 'code', 'error' => 'Le mot de passe doit contenir au moins 8 caracteres.'];
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
private function clearResetSession(object $session): void
|
||||
{
|
||||
$session->remove('reset_step');
|
||||
|
||||
@@ -29,14 +29,9 @@ class WebhookDocuSealController extends AbstractController
|
||||
#[Autowire(env: 'DOCUSEAL_WEBHOOKS_SECRET')] string $secret,
|
||||
#[Autowire('%kernel.project_dir%')] string $projectDir,
|
||||
): Response {
|
||||
if (!$this->verifySecret($request, $secretHeader, $secret)) {
|
||||
return new JsonResponse(['error' => 'Unauthorized'], Response::HTTP_UNAUTHORIZED);
|
||||
}
|
||||
|
||||
$payload = json_decode($request->getContent(), true);
|
||||
|
||||
if (null === $payload) {
|
||||
return new JsonResponse(['error' => 'Invalid payload'], Response::HTTP_BAD_REQUEST);
|
||||
$payload = $this->parseAndValidate($request, $secretHeader, $secret);
|
||||
if ($payload instanceof Response) {
|
||||
return $payload;
|
||||
}
|
||||
|
||||
$eventType = $payload['event_type'] ?? '';
|
||||
@@ -60,6 +55,24 @@ class WebhookDocuSealController extends AbstractController
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* @return array<string, mixed>|Response
|
||||
*/
|
||||
private function parseAndValidate(Request $request, string $secretHeader, string $secret): array|Response
|
||||
{
|
||||
if (!$this->verifySecret($request, $secretHeader, $secret)) {
|
||||
return new JsonResponse(['error' => 'Unauthorized'], Response::HTTP_UNAUTHORIZED);
|
||||
}
|
||||
|
||||
$payload = json_decode($request->getContent(), true);
|
||||
|
||||
if (null === $payload) {
|
||||
return new JsonResponse(['error' => 'Invalid payload'], Response::HTTP_BAD_REQUEST);
|
||||
}
|
||||
|
||||
return $payload;
|
||||
}
|
||||
|
||||
private function verifySecret(Request $request, string $secretHeader, string $secret): bool
|
||||
{
|
||||
if ('' === $secret) {
|
||||
@@ -77,6 +90,7 @@ class WebhookDocuSealController extends AbstractController
|
||||
return '' !== $bodySecret && hash_equals($secret, $bodySecret);
|
||||
}
|
||||
|
||||
/** @param array<string, mixed> $metadata */
|
||||
private function syncSubmitterIdFromMetadata(?string $docType, mixed $submitterId, array $metadata, AttestationRepository $attestationRepository, EntityManagerInterface $em): void
|
||||
{
|
||||
if ('attestation' !== $docType || null === $submitterId || null !== $this->findAttestation($submitterId, $attestationRepository)) {
|
||||
|
||||
@@ -165,6 +165,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface, EmailTw
|
||||
|
||||
public function eraseCredentials(): void
|
||||
{
|
||||
// Required by UserInterface — no sensitive data to clear in this implementation
|
||||
}
|
||||
|
||||
/** @return array<string, mixed> */
|
||||
@@ -264,7 +265,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface, EmailTw
|
||||
|
||||
public function getEmailAuthRecipient(): string
|
||||
{
|
||||
return $this->email;
|
||||
return $this->getUserIdentifier();
|
||||
}
|
||||
|
||||
public function getEmailAuthCode(): ?string
|
||||
@@ -293,7 +294,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface, EmailTw
|
||||
|
||||
public function getGoogleAuthenticatorUsername(): string
|
||||
{
|
||||
return $this->email;
|
||||
return $this->getUserIdentifier();
|
||||
}
|
||||
|
||||
public function getGoogleAuthenticatorSecret(): ?string
|
||||
|
||||
@@ -11,6 +11,12 @@ use Symfony\Component\Security\Http\Authentication\AuthenticationSuccessHandlerI
|
||||
|
||||
class LoginSuccessHandler implements AuthenticationSuccessHandlerInterface
|
||||
{
|
||||
private const ROLE_ROUTES = [
|
||||
'ROLE_EMPLOYE' => 'app_admin_dashboard',
|
||||
'ROLE_REVENDEUR' => 'app_espace_prestataire_index',
|
||||
'ROLE_CUSTOMER' => 'app_espace_client_index',
|
||||
];
|
||||
|
||||
public function __construct(
|
||||
private RouterInterface $router,
|
||||
private AuthorizationCheckerInterface $authorizationChecker,
|
||||
@@ -19,16 +25,10 @@ class LoginSuccessHandler implements AuthenticationSuccessHandlerInterface
|
||||
|
||||
public function onAuthenticationSuccess(Request $request, TokenInterface $token): RedirectResponse
|
||||
{
|
||||
if ($this->authorizationChecker->isGranted('ROLE_EMPLOYE')) {
|
||||
return new RedirectResponse($this->router->generate('app_admin_dashboard'));
|
||||
}
|
||||
|
||||
if ($this->authorizationChecker->isGranted('ROLE_REVENDEUR')) {
|
||||
return new RedirectResponse($this->router->generate('app_espace_prestataire_index'));
|
||||
}
|
||||
|
||||
if ($this->authorizationChecker->isGranted('ROLE_CUSTOMER')) {
|
||||
return new RedirectResponse($this->router->generate('app_espace_client_index'));
|
||||
foreach (self::ROLE_ROUTES as $role => $route) {
|
||||
if ($this->authorizationChecker->isGranted($role)) {
|
||||
return new RedirectResponse($this->router->generate($route));
|
||||
}
|
||||
}
|
||||
|
||||
return new RedirectResponse($this->router->generate('app_home'));
|
||||
|
||||
@@ -92,51 +92,71 @@ class DocuSealService
|
||||
public function downloadSignedDocuments(Attestation $attestation): bool
|
||||
{
|
||||
$submitterId = $attestation->getSubmitterId();
|
||||
$success = false;
|
||||
|
||||
if (null !== $submitterId) {
|
||||
try {
|
||||
$submitter = $this->api->getSubmitter($submitterId);
|
||||
$documents = $submitter['documents'] ?? [];
|
||||
|
||||
if ([] !== $documents) {
|
||||
$dir = $this->projectDir.'/var/rgpd/signed';
|
||||
if (!is_dir($dir)) {
|
||||
mkdir($dir, 0775, true);
|
||||
}
|
||||
|
||||
$ref = $attestation->getReference();
|
||||
|
||||
// PDF signe
|
||||
$pdfUrl = $documents[0]['url'] ?? null;
|
||||
if (null !== $pdfUrl) {
|
||||
$signedPath = $dir.'/'.$ref.'-signed.pdf';
|
||||
$content = file_get_contents($pdfUrl);
|
||||
if (false !== $content && str_starts_with($content, '%PDF')) {
|
||||
file_put_contents($signedPath, $content);
|
||||
$attestation->setPdfFileSigned($signedPath);
|
||||
}
|
||||
}
|
||||
|
||||
// Certificat d'audit
|
||||
$auditUrl = $submitter['audit_log_url'] ?? null;
|
||||
if (null !== $auditUrl) {
|
||||
$certPath = $dir.'/'.$ref.'-certificat.pdf';
|
||||
$content = file_get_contents($auditUrl);
|
||||
if (false !== $content) {
|
||||
file_put_contents($certPath, $content);
|
||||
$attestation->setPdfFileCertificate($certPath);
|
||||
}
|
||||
}
|
||||
|
||||
$this->em->flush();
|
||||
$success = null !== $attestation->getPdfFileSigned();
|
||||
}
|
||||
} catch (\Throwable) {
|
||||
$success = false;
|
||||
}
|
||||
if (null === $submitterId) {
|
||||
return false;
|
||||
}
|
||||
|
||||
return $success;
|
||||
try {
|
||||
$submitter = $this->api->getSubmitter($submitterId);
|
||||
$documents = $submitter['documents'] ?? [];
|
||||
|
||||
if ([] === $documents) {
|
||||
return false;
|
||||
}
|
||||
|
||||
$dir = $this->ensureSignedDirectory();
|
||||
$ref = $attestation->getReference();
|
||||
|
||||
$this->downloadSignedPdf($documents, $dir, $ref, $attestation);
|
||||
$this->downloadAuditCertificate($submitter, $dir, $ref, $attestation);
|
||||
|
||||
$this->em->flush();
|
||||
|
||||
return null !== $attestation->getPdfFileSigned();
|
||||
} catch (\Throwable) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
private function ensureSignedDirectory(): string
|
||||
{
|
||||
$dir = $this->projectDir.'/var/rgpd/signed';
|
||||
if (!is_dir($dir)) {
|
||||
mkdir($dir, 0775, true);
|
||||
}
|
||||
|
||||
return $dir;
|
||||
}
|
||||
|
||||
/** @param list<array<string, mixed>> $documents */
|
||||
private function downloadSignedPdf(array $documents, string $dir, string $ref, Attestation $attestation): void
|
||||
{
|
||||
$pdfUrl = $documents[0]['url'] ?? null;
|
||||
if (null === $pdfUrl) {
|
||||
return;
|
||||
}
|
||||
|
||||
$content = @file_get_contents($pdfUrl);
|
||||
if (false !== $content && str_starts_with($content, '%PDF')) {
|
||||
$path = $dir.'/'.$ref.'-signed.pdf';
|
||||
file_put_contents($path, $content);
|
||||
$attestation->setPdfFileSigned($path);
|
||||
}
|
||||
}
|
||||
|
||||
/** @param array<string, mixed> $submitter */
|
||||
private function downloadAuditCertificate(array $submitter, string $dir, string $ref, Attestation $attestation): void
|
||||
{
|
||||
$auditUrl = $submitter['audit_log_url'] ?? null;
|
||||
if (null === $auditUrl) {
|
||||
return;
|
||||
}
|
||||
|
||||
$content = @file_get_contents($auditUrl);
|
||||
if (false !== $content) {
|
||||
$path = $dir.'/'.$ref.'-certificat.pdf';
|
||||
file_put_contents($path, $content);
|
||||
$attestation->setPdfFileCertificate($path);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
150
tests/Service/AnalyticsCryptoServiceTest.php
Normal file
150
tests/Service/AnalyticsCryptoServiceTest.php
Normal file
@@ -0,0 +1,150 @@
|
||||
<?php
|
||||
|
||||
namespace App\Tests\Service;
|
||||
|
||||
use App\Service\AnalyticsCryptoService;
|
||||
use PHPUnit\Framework\TestCase;
|
||||
|
||||
class AnalyticsCryptoServiceTest extends TestCase
|
||||
{
|
||||
private AnalyticsCryptoService $service;
|
||||
|
||||
protected function setUp(): void
|
||||
{
|
||||
$this->service = new AnalyticsCryptoService('test-secret-key');
|
||||
}
|
||||
|
||||
public function testEncryptReturnsBase64String(): void
|
||||
{
|
||||
$data = ['page' => '/home', 'visits' => 42];
|
||||
|
||||
$encrypted = $this->service->encrypt($data);
|
||||
|
||||
$this->assertNotEmpty($encrypted);
|
||||
$this->assertNotFalse(base64_decode($encrypted, true));
|
||||
}
|
||||
|
||||
public function testEncryptThenDecryptReturnsOriginalData(): void
|
||||
{
|
||||
$data = ['page' => '/stats', 'count' => 10, 'nested' => ['a' => 1]];
|
||||
|
||||
$encrypted = $this->service->encrypt($data);
|
||||
$decrypted = $this->service->decrypt($encrypted);
|
||||
|
||||
$this->assertSame($data, $decrypted);
|
||||
}
|
||||
|
||||
public function testDecryptInvalidBase64ReturnsNull(): void
|
||||
{
|
||||
$this->assertNull($this->service->decrypt('!!!not-base64!!!'));
|
||||
}
|
||||
|
||||
public function testDecryptTooShortPayloadReturnsNull(): void
|
||||
{
|
||||
// Less than 28 bytes after decode
|
||||
$this->assertNull($this->service->decrypt(base64_encode('short')));
|
||||
}
|
||||
|
||||
public function testDecryptCorruptedPayloadReturnsNull(): void
|
||||
{
|
||||
$data = ['test' => true];
|
||||
$encrypted = $this->service->encrypt($data);
|
||||
|
||||
// Corrupt the payload by flipping bytes
|
||||
$raw = base64_decode($encrypted, true);
|
||||
$corrupted = $raw;
|
||||
$corrupted[20] = chr(ord($corrupted[20]) ^ 0xFF);
|
||||
$corrupted[21] = chr(ord($corrupted[21]) ^ 0xFF);
|
||||
|
||||
$this->assertNull($this->service->decrypt(base64_encode($corrupted)));
|
||||
}
|
||||
|
||||
public function testDecryptJsFormatWithShortCiphertextReturnsNull(): void
|
||||
{
|
||||
// 28 bytes of zeros — neither JS nor PHP format can decrypt
|
||||
$raw = str_repeat("\x00", 28);
|
||||
|
||||
$this->assertNull($this->service->decrypt(base64_encode($raw)));
|
||||
}
|
||||
|
||||
public function testTryDecryptJsFormatWithTooShortInput(): void
|
||||
{
|
||||
$method = new \ReflectionMethod(AnalyticsCryptoService::class, 'tryDecryptJsFormat');
|
||||
|
||||
$result = $method->invoke($this->service, str_repeat("\x00", 12), 'short');
|
||||
|
||||
$this->assertNull($result);
|
||||
}
|
||||
|
||||
public function testDecryptPhpFormatFallback(): void
|
||||
{
|
||||
// Build a PHP-format payload: iv (12) + tag (16) + ciphertext
|
||||
$data = ['fallback' => true];
|
||||
$json = json_encode($data);
|
||||
$key = substr(hash('sha256', 'test-secret-key', true), 0, 32);
|
||||
$iv = random_bytes(12);
|
||||
$encrypted = openssl_encrypt($json, 'aes-256-gcm', $key, \OPENSSL_RAW_DATA, $iv, $tag, '', 16);
|
||||
|
||||
// PHP format: iv + tag + ciphertext
|
||||
$phpFormat = base64_encode($iv . $tag . $encrypted);
|
||||
|
||||
$this->assertSame($data, $this->service->decrypt($phpFormat));
|
||||
}
|
||||
|
||||
public function testGenerateVisitorHashReturnsDeterministicHash(): void
|
||||
{
|
||||
$hash1 = $this->service->generateVisitorHash('user-123');
|
||||
$hash2 = $this->service->generateVisitorHash('user-123');
|
||||
|
||||
$this->assertSame($hash1, $hash2);
|
||||
$this->assertSame(64, \strlen($hash1)); // SHA-256 hex = 64 chars
|
||||
}
|
||||
|
||||
public function testGenerateVisitorHashDiffersForDifferentUids(): void
|
||||
{
|
||||
$hash1 = $this->service->generateVisitorHash('user-123');
|
||||
$hash2 = $this->service->generateVisitorHash('user-456');
|
||||
|
||||
$this->assertNotSame($hash1, $hash2);
|
||||
}
|
||||
|
||||
public function testVerifyVisitorHashReturnsTrueForValidHash(): void
|
||||
{
|
||||
$uid = 'visitor-abc';
|
||||
$hash = $this->service->generateVisitorHash($uid);
|
||||
|
||||
$this->assertTrue($this->service->verifyVisitorHash($uid, $hash));
|
||||
}
|
||||
|
||||
public function testVerifyVisitorHashReturnsFalseForInvalidHash(): void
|
||||
{
|
||||
$this->assertFalse($this->service->verifyVisitorHash('visitor-abc', 'wrong-hash'));
|
||||
}
|
||||
|
||||
public function testGetKeyForJsReturnsBase64EncodedKey(): void
|
||||
{
|
||||
$jsKey = $this->service->getKeyForJs();
|
||||
|
||||
$this->assertNotEmpty($jsKey);
|
||||
$decoded = base64_decode($jsKey, true);
|
||||
$this->assertNotFalse($decoded);
|
||||
$this->assertSame(32, \strlen($decoded)); // AES-256 key = 32 bytes
|
||||
}
|
||||
|
||||
public function testDifferentSecretsProduceDifferentKeys(): void
|
||||
{
|
||||
$service2 = new AnalyticsCryptoService('different-secret');
|
||||
|
||||
$this->assertNotSame($this->service->getKeyForJs(), $service2->getKeyForJs());
|
||||
}
|
||||
|
||||
public function testEncryptedByOneServiceCannotBeDecryptedByAnother(): void
|
||||
{
|
||||
$service2 = new AnalyticsCryptoService('different-secret');
|
||||
$data = ['secret' => 'data'];
|
||||
|
||||
$encrypted = $this->service->encrypt($data);
|
||||
|
||||
$this->assertNull($service2->decrypt($encrypted));
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user