Merge branch 'feature/40' into develop

module/Core/src/Action/RedirectAction.php

@@ -85,7 +85,7 @@ class RedirectAction implements MiddlewareInterface
             }
 
             // Track visit to this short code
-            $this->visitTracker->track($shortCode);
+            $this->visitTracker->track($shortCode, $request);
 
             // Return a redirect response to the long URL.
             // Use a temporary redirect to make sure browsers always hit the server for analytics purposes

module/Core/src/Service/VisitsTracker.php

@@ -3,6 +3,7 @@ namespace Shlinkio\Shlink\Core\Service;
 
 use Acelaya\ZsmAnnotatedServices\Annotation\Inject;
 use Doctrine\ORM\EntityManagerInterface;
+use Psr\Http\Message\ServerRequestInterface;
 use Shlinkio\Shlink\Common\Exception\InvalidArgumentException;
 use Shlinkio\Shlink\Common\Util\DateRange;
 use Shlinkio\Shlink\Core\Entity\ShortUrl;
@@ -31,12 +32,10 @@ class VisitsTracker implements VisitsTrackerInterface
      * Tracks a new visit to provided short code, using an array of data to look up information
      *
      * @param string $shortCode
-     * @param array $visitorData Defaults to global $_SERVER
+     * @param ServerRequestInterface $request
      */
-    public function track($shortCode, array $visitorData = null)
+    public function track($shortCode, ServerRequestInterface $request)
     {
-        $visitorData = $visitorData ?: $_SERVER;
-
         /** @var ShortUrl $shortUrl */
         $shortUrl = $this->em->getRepository(ShortUrl::class)->findOneBy([
             'shortCode' => $shortCode,
@@ -44,22 +43,27 @@ class VisitsTracker implements VisitsTrackerInterface
 
         $visit = new Visit();
         $visit->setShortUrl($shortUrl)
-              ->setUserAgent($this->getArrayValue($visitorData, 'HTTP_USER_AGENT'))
-              ->setReferer($this->getArrayValue($visitorData, 'HTTP_REFERER'))
-              ->setRemoteAddr($this->getArrayValue($visitorData, 'REMOTE_ADDR'));
+              ->setUserAgent($request->getHeaderLine('User-Agent'))
+              ->setReferer($request->getHeaderLine('Referer'))
+              ->setRemoteAddr($this->findOutRemoteAddr($request));
         $this->em->persist($visit);
         $this->em->flush();
     }
 
     /**
-     * @param array $array
-     * @param $key
-     * @param null $default
-     * @return mixed|null
+     * @param ServerRequestInterface $request
+     * @return string
      */
-    protected function getArrayValue(array $array, $key, $default = null)
+    protected function findOutRemoteAddr(ServerRequestInterface $request)
     {
-        return isset($array[$key]) ? $array[$key] : $default;
+        $forwardedFor = $request->getHeaderLine('X-Forwarded-For');
+        if (empty($forwardedFor)) {
+            $serverParams = $request->getServerParams();
+            return isset($serverParams['REMOTE_ADDR']) ? $serverParams['REMOTE_ADDR'] : null;
+        }
+
+        $ips = explode(',', $forwardedFor);
+        return $ips[0];
     }
 
     /**

module/Core/src/Service/VisitsTrackerInterface.php

@@ -1,6 +1,7 @@
 <?php
 namespace Shlinkio\Shlink\Core\Service;
 
+use Psr\Http\Message\ServerRequestInterface;
 use Shlinkio\Shlink\Common\Util\DateRange;
 use Shlinkio\Shlink\Core\Entity\Visit;
 
@@ -10,9 +11,10 @@ interface VisitsTrackerInterface
      * Tracks a new visit to provided short code, using an array of data to look up information
      *
      * @param string $shortCode
-     * @param array $visitorData Defaults to global $_SERVER
+     * @param ServerRequestInterface $request
+     * @return
      */
-    public function track($shortCode, array $visitorData = null);
+    public function track($shortCode, ServerRequestInterface $request);
 
     /**
      * Returns the visits on certain short code

module/Core/test/Service/VisitsTrackerTest.php

@@ -10,6 +10,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Repository\VisitRepository;
 use Shlinkio\Shlink\Core\Service\VisitsTracker;
+use Zend\Diactoros\ServerRequestFactory;
 
 class VisitsTrackerTest extends TestCase
 {
@@ -41,7 +42,30 @@ class VisitsTrackerTest extends TestCase
         $this->em->persist(Argument::any())->shouldBeCalledTimes(1);
         $this->em->flush()->shouldBeCalledTimes(1);
 
-        $this->visitsTracker->track($shortCode);
+        $this->visitsTracker->track($shortCode, ServerRequestFactory::fromGlobals());
+    }
+
+    /**
+     * @test
+     */
+    public function trackUsesForwardedForHeaderIfPresent()
+    {
+        $shortCode = '123ABC';
+        $test = $this;
+        $repo = $this->prophesize(EntityRepository::class);
+        $repo->findOneBy(['shortCode' => $shortCode])->willReturn(new ShortUrl());
+
+        $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledTimes(1);
+        $this->em->persist(Argument::any())->will(function ($args) use ($test) {
+            /** @var Visit $visit */
+            $visit = $args[0];
+            $test->assertEquals('4.3.2.1', $visit->getRemoteAddr());
+        })->shouldBeCalledTimes(1);
+        $this->em->flush()->shouldBeCalledTimes(1);
+
+        $this->visitsTracker->track($shortCode, ServerRequestFactory::fromGlobals(
+            ['REMOTE_ADDR' => '1.2.3.4']
+        )->withHeader('X-Forwarded-For', '4.3.2.1,99.99.99.99'));
     }
 
     /**