finalize feature (tests, lint, doc)

doc/internal_requests.md

@@ -21,6 +21,18 @@ Ainsi, si l'on prend l'exemple d'une requête `/internal/download/123` sur ap2i
 * Un utilisateur hors VPN, même s'il connaissait le token, recevra une erreur 500, car n'ayant pas une ip autorisée
 * Une requête issue de la V1 sera autorisée sans authentification
 
+### Ip internes 
+
+Les ips considérées comme interne sont :
+
+- Le localhost (`127.0.0.[0-1]`)
+- Les adresses venant de l'intérieur du VPN (`10.8.0.[0-255]`)
+- Les adresses publiques des serveurs Opentalent (`141.94.117.[33-61]`)
+- Les adresses privées des serveurs Opentalent (`172.16.0.[0-255]`)
+- Les adresses des autres containers docker (`172.20.[0-255].[0-255]`)
+
+> Plus d'infos ici : https://ressources.opentalent.fr/display/SI/Infrastructure+et+reseau
+
 
 ### Mise en oeuvre
 

phpunit.xml.dist

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!-- https://phpunit.readthedocs.io/en/latest/configuration.html -->
 <phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd" backupGlobals="false" colors="true" bootstrap="tests/bootstrap.php">
-  <coverage processUncoveredFiles="true">
+  <coverage pathCoverage="true" includeUncoveredFiles="true">
     <include>
       <directory suffix=".php">src</directory>
     </include>

src/Service/File/FileManager.php

@@ -51,7 +51,6 @@ class FileManager
      * Lit le fichier et retourne son contenu
      *
      * @param File $file
-     * @param TokenInterface|null $token  Used to read files from API v1 with a switch account
      * @return string
      * @throws FileNotFoundException
      */

src/Service/Security/InternalRequestsService.php

@@ -13,9 +13,10 @@ class InternalRequestsService
     const INTERNAL_IPS = [
         '/^127\.0\.0\.[0-1]$/', // Localhost
         '/^localhost$/',  // Localhost
-        '/^10\.8\.0\.\d{1,3}$/', // VPN
-        '/^141\.94\.117\.[33-61]$/',   // opentalent hosts public ips
-        '/^172\.20\.\d{1,3}\.\d{1,3}$/',  // docker
+        '/^10\.8\.0\.\d{1,3}$/', // 10.8.0.[0-255] - VPN
+        '/^141\.94\.117\.((3[3-9])|(4\d)|(5\d)|(6[0-1]))$/',   // 141.94.117.[33-61] - Opentalent hosts public ips
+        '/^172\.16\.0.\d{1,3}$/',   // 172.16.0.[0-255] - Opentalent hosts private ips
+        '/^172\.20\.\d{1,3}\.\d{1,3}$/',  // 172.20.[0-255].[0-255] - Docker
     ];
 
     public function __construct(
@@ -40,12 +41,13 @@ class InternalRequestsService
 
     /**
      * Compare the given token to the expected one, and return true if they are identical
+     * An empty token can not be valid
      *
      * @param string $token
      * @return bool
      */
     protected function tokenIsValid(string $token): bool {
-        return $token === $this->internalRequestsToken;
+        return $token && $token === $this->internalRequestsToken;
     }
 
     /**

src/State/Provider/Core/DownloadRequestProvider.php

@@ -29,7 +29,7 @@ final class DownloadRequestProvider implements ProviderInterface
     /**
      * @param string $resourceClass
      * @param string|null $operationName
-     * @param array $context
+     * @param array<mixed> $context
      * @return bool
      */
     public function supports(string $resourceClass, string $operationName = null, array $context = []): bool
@@ -39,8 +39,8 @@ final class DownloadRequestProvider implements ProviderInterface
 
     /**
      * @param Operation $operation
-     * @param array $uriVariables
-     * @param array $context
+     * @param array<mixed> $uriVariables
+     * @param array<mixed> $context
      * @return Response|RedirectResponse
      * @throws FileNotFoundException
      */

tests/Service/Security/InternalRequestsServiceTest.php

@@ -0,0 +1,93 @@
+<?php
+
+namespace App\Tests\Service\Security;
+
+use App\Service\Security\InternalRequestsService;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+
+class TestableInternalRequestsService extends InternalRequestsService {
+    public function isInternalIp(string $ip): bool { return parent::isInternalIp($ip); }
+    public function tokenIsValid(string $token): bool { return parent::tokenIsValid($token); }
+}
+
+class InternalRequestsServiceTest extends TestCase
+{
+    const internalRequestsToken = 'azerty';
+
+    public function setUp(): void {
+    }
+
+    private function getInternalRequestsServiceMockFor(string $methodName, string $token = null): TestableInternalRequestsService | MockObject {
+        return $this->getMockBuilder(TestableInternalRequestsService::class)
+            ->setConstructorArgs([$token ?? self::internalRequestsToken])
+            ->setMethodsExcept([$methodName])
+            ->getMock();
+    }
+
+    public function testIsInternalIp(): void {
+        $internalRequestsService = $this->getInternalRequestsServiceMockFor('isInternalIp');
+
+        $this->assertTrue($internalRequestsService->isInternalIp('127.0.0.0'));
+        $this->assertTrue($internalRequestsService->isInternalIp('127.0.0.1'));
+
+        $this->assertTrue($internalRequestsService->isInternalIp('10.8.0.1'));
+        $this->assertTrue($internalRequestsService->isInternalIp('10.8.0.255'));
+
+        $this->assertFalse($internalRequestsService->isInternalIp('141.94.117.32'));
+        $this->assertTrue($internalRequestsService->isInternalIp('141.94.117.33'));
+        $this->assertTrue($internalRequestsService->isInternalIp('141.94.117.50'));
+        $this->assertTrue($internalRequestsService->isInternalIp('141.94.117.61'));
+        $this->assertFalse($internalRequestsService->isInternalIp('141.94.117.62'));
+
+        $this->assertTrue($internalRequestsService->isInternalIp('172.20.0.0'));
+        $this->assertTrue($internalRequestsService->isInternalIp('172.20.255.255'));
+    }
+
+    public function testTokenIsValid(): void {
+        $internalRequestsService = $this->getInternalRequestsServiceMockFor('tokenIsValid');
+
+        $this->assertTrue($internalRequestsService->tokenIsValid(self::internalRequestsToken));
+        $this->assertFalse($internalRequestsService->tokenIsValid('foo'));
+    }
+
+    public function testTokenIsValidWithEmptyToken(): void {
+        $internalRequestsService = $this->getInternalRequestsServiceMockFor('tokenIsValid', '');
+
+        // A token can not be valid if it is an empty string, even if it's equal to the internal token
+        $this->assertFalse($internalRequestsService->tokenIsValid(''));
+    }
+
+    public function testIsAllowed(): void {
+        $internalRequestsService = $this->getInternalRequestsServiceMockFor('isAllowed');
+
+        $internalRequestsService->expects(self::once())->method('isInternalIp')->with('128.0.0.1')->willReturn(True);
+        $internalRequestsService->expects(self::once())->method('tokenIsValid')->with('azerty')->willReturn(True);
+
+        $result = $internalRequestsService->isAllowed('128.0.0.1', 'azerty');
+
+        $this->assertTrue($result);
+    }
+
+    public function testIsAllowedInvalidIp(): void {
+        $internalRequestsService = $this->getInternalRequestsServiceMockFor('isAllowed');
+
+        $internalRequestsService->expects(self::once())->method('isInternalIp')->with('128.0.0.1')->willReturn(False);
+        $internalRequestsService->expects(self::never())->method('tokenIsValid');
+
+        $result = $internalRequestsService->isAllowed('128.0.0.1', 'azerty');
+
+        $this->assertFalse($result);
+    }
+
+    public function testIsAllowedInvalidToken(): void {
+        $internalRequestsService = $this->getInternalRequestsServiceMockFor('isAllowed');
+
+        $internalRequestsService->expects(self::once())->method('isInternalIp')->with('128.0.0.1')->willReturn(True);
+        $internalRequestsService->expects(self::once())->method('tokenIsValid')->with('azerty')->willReturn(False);
+
+        $result = $internalRequestsService->isAllowed('128.0.0.1', 'azerty');
+
+        $this->assertFalse($result);
+    }
+}