Merge pull request #7851 from wallabag/remove-site-config-authenticator-extension-point

Remove site config authenticator extension point

src/Guzzle/AuthenticatorSubscriber.php

@@ -9,7 +9,7 @@ use GuzzleHttp\Message\RequestInterface;
 use Psr\Log\LoggerAwareInterface;
 use Psr\Log\LoggerInterface;
 use Psr\Log\NullLogger;
-use Wallabag\SiteConfig\Authenticator\Factory;
+use Wallabag\SiteConfig\LoginFormAuthenticator;
 use Wallabag\SiteConfig\SiteConfig;
 use Wallabag\SiteConfig\SiteConfigBuilder;
 
@@ -23,8 +23,8 @@ class AuthenticatorSubscriber implements SubscriberInterface, LoggerAwareInterfa
     /** @var SiteConfigBuilder */
     private $configBuilder;
 
-    /** @var Factory */
+    /** @var LoginFormAuthenticator */
-    private $authenticatorFactory;
+    private $authenticator;
 
     /** @var LoggerInterface */
     private $logger;
@@ -32,10 +32,10 @@ class AuthenticatorSubscriber implements SubscriberInterface, LoggerAwareInterfa
     /**
      * AuthenticatorSubscriber constructor.
      */
-    public function __construct(SiteConfigBuilder $configBuilder, Factory $authenticatorFactory)
+    public function __construct(SiteConfigBuilder $configBuilder, LoginFormAuthenticator $authenticator)
     {
         $this->configBuilder = $configBuilder;
-        $this->authenticatorFactory = $authenticatorFactory;
+        $this->authenticator = $authenticator;
         $this->logger = new NullLogger();
     }
 
@@ -62,14 +62,13 @@ class AuthenticatorSubscriber implements SubscriberInterface, LoggerAwareInterfa
         }
 
         $client = $event->getClient();
-        $authenticator = $this->authenticatorFactory->buildFromSiteConfig($config);
 
-        if (!$authenticator->isLoggedIn($client)) {
+        if (!$this->authenticator->isLoggedIn($config, $client)) {
             $this->logger->debug('loginIfRequired> user is not logged in, attach authenticator');
 
             $emitter = $client->getEmitter();
             $emitter->detach($this);
-            $authenticator->login($client);
+            $this->authenticator->login($config, $client);
             $emitter->attach($this);
         }
     }
@@ -94,8 +93,7 @@ class AuthenticatorSubscriber implements SubscriberInterface, LoggerAwareInterfa
             return;
         }
 
-        $authenticator = $this->authenticatorFactory->buildFromSiteConfig($config);
+        $isLoginRequired = $this->authenticator->isLoginRequired($config, $body);
-        $isLoginRequired = $authenticator->isLoginRequired($body);
 
         $this->logger->debug('loginIfRequested> retry #' . $this->retries . ' with login ' . ($isLoginRequired ? '' : 'not ') . 'required');
 
@@ -104,7 +102,7 @@ class AuthenticatorSubscriber implements SubscriberInterface, LoggerAwareInterfa
 
             $emitter = $client->getEmitter();
             $emitter->detach($this);
-            $authenticator->login($client);
+            $this->authenticator->login($config, $client);
             $emitter->attach($this);
 
             $event->retry();

src/SiteConfig/Authenticator/Authenticator.php

@@ -1,31 +0,0 @@
-<?php
-
-namespace Wallabag\SiteConfig\Authenticator;
-
-use GuzzleHttp\ClientInterface;
-
-interface Authenticator
-{
-    /**
-     * Logs the configured user on the given Guzzle client.
-     *
-     * @return self
-     */
-    public function login(ClientInterface $guzzle);
-
-    /**
-     * Checks if we are logged into the site, but without calling the server (e.g. do we have a Cookie).
-     *
-     * @return bool
-     */
-    public function isLoggedIn(ClientInterface $guzzle);
-
-    /**
-     * Checks from the HTML of a page if authentication is requested by a grabbed page.
-     *
-     * @param string $html
-     *
-     * @return bool
-     */
-    public function isLoginRequired($html);
-}

src/SiteConfig/Authenticator/Factory.php

@@ -1,21 +0,0 @@
-<?php
-
-namespace Wallabag\SiteConfig\Authenticator;
-
-use Wallabag\SiteConfig\SiteConfig;
-
-/**
- * Builds an Authenticator based on a SiteConfig.
- */
-class Factory
-{
-    /**
-     * @return Authenticator
-     *
-     * @throw \OutOfRangeException if there are no credentials for this host
-     */
-    public function buildFromSiteConfig(SiteConfig $siteConfig)
-    {
-        return new LoginFormAuthenticator($siteConfig);
-    }
-}

src/SiteConfig/LoginFormAuthenticator.php

@@ -1,50 +1,47 @@
 <?php
 
-namespace Wallabag\SiteConfig\Authenticator;
+namespace Wallabag\SiteConfig;
 
 use GuzzleHttp\ClientInterface;
 use GuzzleHttp\Cookie\CookieJar;
 use Symfony\Component\DomCrawler\Crawler;
 use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
 use Wallabag\ExpressionLanguage\AuthenticatorProvider;
-use Wallabag\SiteConfig\SiteConfig;
 
-class LoginFormAuthenticator implements Authenticator
+class LoginFormAuthenticator
 {
-    /** @var \GuzzleHttp\Client */
+    /**
-    protected $guzzle;
+     * Logs the configured user on the given Guzzle client.
-
+     *
-    /** @var SiteConfig */
+     * @return self
-    private $siteConfig;
+     */
-
+    public function login(SiteConfig $siteConfig, ClientInterface $guzzle)
-    public function __construct(SiteConfig $siteConfig)
-    {
-        // @todo OptionResolver
-        $this->siteConfig = $siteConfig;
-    }
-
-    public function login(ClientInterface $guzzle)
     {
         $postFields = [
-            $this->siteConfig->getUsernameField() => $this->siteConfig->getUsername(),
+            $siteConfig->getUsernameField() => $siteConfig->getUsername(),
-            $this->siteConfig->getPasswordField() => $this->siteConfig->getPassword(),
+            $siteConfig->getPasswordField() => $siteConfig->getPassword(),
-        ] + $this->getExtraFields($guzzle);
+        ] + $this->getExtraFields($siteConfig, $guzzle);
 
         $guzzle->post(
-            $this->siteConfig->getLoginUri(),
+            $siteConfig->getLoginUri(),
             ['body' => $postFields, 'allow_redirects' => true, 'verify' => false]
         );
 
         return $this;
     }
 
-    public function isLoggedIn(ClientInterface $guzzle)
+    /**
+     * Checks if we are logged into the site, but without calling the server (e.g. do we have a Cookie).
+     *
+     * @return bool
+     */
+    public function isLoggedIn(SiteConfig $siteConfig, ClientInterface $guzzle)
     {
         if (($cookieJar = $guzzle->getDefaultOption('cookies')) instanceof CookieJar) {
             /** @var \GuzzleHttp\Cookie\SetCookie $cookie */
             foreach ($cookieJar as $cookie) {
                 // check required cookies
-                if ($cookie->getDomain() === $this->siteConfig->getHost()) {
+                if ($cookie->getDomain() === $siteConfig->getHost()) {
                     return true;
                 }
             }
@@ -53,13 +50,20 @@ class LoginFormAuthenticator implements Authenticator
         return false;
     }
 
-    public function isLoginRequired($html)
+    /**
+     * Checks from the HTML of a page if authentication is requested by a grabbed page.
+     *
+     * @param string $html
+     *
+     * @return bool
+     */
+    public function isLoginRequired(SiteConfig $siteConfig, $html)
     {
         // need to check for the login dom element ($options['not_logged_in_xpath']) in the HTML
         try {
             $crawler = new Crawler((string) $html);
 
-            $loggedIn = $crawler->evaluate((string) $this->siteConfig->getNotLoggedInXpath());
+            $loggedIn = $crawler->evaluate((string) $siteConfig->getNotLoggedInXpath());
         } catch (\Throwable $e) {
             return false;
         }
@@ -73,17 +77,17 @@ class LoginFormAuthenticator implements Authenticator
      *
      * @return array
      */
-    private function getExtraFields(ClientInterface $guzzle)
+    private function getExtraFields(SiteConfig $siteConfig, ClientInterface $guzzle)
     {
         $extraFields = [];
 
-        foreach ($this->siteConfig->getExtraFields() as $fieldName => $fieldValue) {
+        foreach ($siteConfig->getExtraFields() as $fieldName => $fieldValue) {
             if ('@=' === substr($fieldValue, 0, 2)) {
                 $expressionLanguage = $this->getExpressionLanguage($guzzle);
                 $fieldValue = $expressionLanguage->evaluate(
                     substr($fieldValue, 2),
                     [
-                        'config' => $this->siteConfig,
+                        'config' => $siteConfig,
                     ]
                 );
             }

tests/Guzzle/AuthenticatorSubscriberTest.php

@@ -14,16 +14,19 @@ use Monolog\Logger;
 use PHPUnit\Framework\TestCase;
 use Wallabag\Guzzle\AuthenticatorSubscriber;
 use Wallabag\SiteConfig\ArraySiteConfigBuilder;
-use Wallabag\SiteConfig\Authenticator\Authenticator;
+use Wallabag\SiteConfig\LoginFormAuthenticator;
-use Wallabag\SiteConfig\Authenticator\Factory;
 
 class AuthenticatorSubscriberTest extends TestCase
 {
     public function testGetEvents()
     {
+        $authenticator = $this->getMockBuilder(LoginFormAuthenticator::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
         $subscriber = new AuthenticatorSubscriber(
             new ArraySiteConfigBuilder(),
-            new Factory()
+            $authenticator
         );
         $events = $subscriber->getEvents();
 
@@ -35,8 +38,12 @@ class AuthenticatorSubscriberTest extends TestCase
 
     public function testLoginIfRequiredNotRequired()
     {
+        $authenticator = $this->getMockBuilder(LoginFormAuthenticator::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
         $builder = new ArraySiteConfigBuilder(['example.com' => []]);
-        $subscriber = new AuthenticatorSubscriber($builder, new Factory());
+        $subscriber = new AuthenticatorSubscriber($builder, $authenticator);
 
         $logger = new Logger('foo');
         $handler = new TestHandler();
@@ -64,7 +71,7 @@ class AuthenticatorSubscriberTest extends TestCase
 
     public function testLoginIfRequiredWithNotLoggedInUser()
     {
-        $authenticator = $this->getMockBuilder(Authenticator::class)
+        $authenticator = $this->getMockBuilder(LoginFormAuthenticator::class)
             ->disableOriginalConstructor()
             ->getMock();
 
@@ -75,16 +82,8 @@ class AuthenticatorSubscriberTest extends TestCase
         $authenticator->expects($this->once())
             ->method('login');
 
-        $factory = $this->getMockBuilder(Factory::class)
-            ->disableOriginalConstructor()
-            ->getMock();
-
-        $factory->expects($this->once())
-            ->method('buildFromSiteConfig')
-            ->willReturn($authenticator);
-
         $builder = new ArraySiteConfigBuilder(['example.com' => ['requiresLogin' => true]]);
-        $subscriber = new AuthenticatorSubscriber($builder, $factory);
+        $subscriber = new AuthenticatorSubscriber($builder, $authenticator);
 
         $logger = new Logger('foo');
         $handler = new TestHandler();
@@ -124,8 +123,12 @@ class AuthenticatorSubscriberTest extends TestCase
 
     public function testLoginIfRequestedNotRequired()
     {
+        $authenticator = $this->getMockBuilder(LoginFormAuthenticator::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
         $builder = new ArraySiteConfigBuilder(['example.com' => []]);
-        $subscriber = new AuthenticatorSubscriber($builder, new Factory());
+        $subscriber = new AuthenticatorSubscriber($builder, $authenticator);
 
         $logger = new Logger('foo');
         $handler = new TestHandler();
@@ -153,7 +156,7 @@ class AuthenticatorSubscriberTest extends TestCase
 
     public function testLoginIfRequestedNotRequested()
     {
-        $authenticator = $this->getMockBuilder(Authenticator::class)
+        $authenticator = $this->getMockBuilder(LoginFormAuthenticator::class)
             ->disableOriginalConstructor()
             ->getMock();
 
@@ -161,19 +164,11 @@ class AuthenticatorSubscriberTest extends TestCase
             ->method('isLoginRequired')
             ->willReturn(false);
 
-        $factory = $this->getMockBuilder(Factory::class)
-            ->disableOriginalConstructor()
-            ->getMock();
-
-        $factory->expects($this->once())
-            ->method('buildFromSiteConfig')
-            ->willReturn($authenticator);
-
         $builder = new ArraySiteConfigBuilder(['example.com' => [
             'requiresLogin' => true,
             'notLoggedInXpath' => '//html',
         ]]);
-        $subscriber = new AuthenticatorSubscriber($builder, $factory);
+        $subscriber = new AuthenticatorSubscriber($builder, $authenticator);
 
         $logger = new Logger('foo');
         $handler = new TestHandler();
@@ -210,7 +205,7 @@ class AuthenticatorSubscriberTest extends TestCase
 
     public function testLoginIfRequestedRequested()
     {
-        $authenticator = $this->getMockBuilder(Authenticator::class)
+        $authenticator = $this->getMockBuilder(LoginFormAuthenticator::class)
             ->disableOriginalConstructor()
             ->getMock();
 
@@ -221,19 +216,11 @@ class AuthenticatorSubscriberTest extends TestCase
         $authenticator->expects($this->once())
             ->method('login');
 
-        $factory = $this->getMockBuilder(Factory::class)
-            ->disableOriginalConstructor()
-            ->getMock();
-
-        $factory->expects($this->once())
-            ->method('buildFromSiteConfig')
-            ->willReturn($authenticator);
-
         $builder = new ArraySiteConfigBuilder(['example.com' => [
             'requiresLogin' => true,
             'notLoggedInXpath' => '//html',
         ]]);
-        $subscriber = new AuthenticatorSubscriber($builder, $factory);
+        $subscriber = new AuthenticatorSubscriber($builder, $authenticator);
 
         $logger = new Logger('foo');
         $handler = new TestHandler();
@@ -276,7 +263,7 @@ class AuthenticatorSubscriberTest extends TestCase
 
     public function testLoginIfRequestedRedirect()
     {
-        $factory = $this->getMockBuilder(Factory::class)
+        $authenticator = $this->getMockBuilder(LoginFormAuthenticator::class)
             ->disableOriginalConstructor()
             ->getMock();
 
@@ -284,7 +271,7 @@ class AuthenticatorSubscriberTest extends TestCase
             'requiresLogin' => true,
             'notLoggedInXpath' => '//html',
         ]]);
-        $subscriber = new AuthenticatorSubscriber($builder, $factory);
+        $subscriber = new AuthenticatorSubscriber($builder, $authenticator);
 
         $logger = new Logger('foo');
         $handler = new TestHandler();

tests/SiteConfig/LoginFormAuthenticatorTest.php

@@ -1,13 +1,13 @@
 <?php
 
-namespace Tests\Wallabag\SiteConfig\Authenticator;
+namespace Tests\Wallabag\SiteConfig;
 
 use GuzzleHttp\Client;
 use GuzzleHttp\Message\Response;
 use GuzzleHttp\Stream\Stream;
 use GuzzleHttp\Subscriber\Mock;
 use PHPUnit\Framework\TestCase;
-use Wallabag\SiteConfig\Authenticator\LoginFormAuthenticator;
+use Wallabag\SiteConfig\LoginFormAuthenticator;
 use Wallabag\SiteConfig\SiteConfig;
 
 class LoginFormAuthenticatorTest extends TestCase
@@ -35,8 +35,8 @@ class LoginFormAuthenticatorTest extends TestCase
             'password' => 'unkn0wn',
         ]);
 
-        $auth = new LoginFormAuthenticator($siteConfig);
+        $auth = new LoginFormAuthenticator();
-        $res = $auth->login($guzzle);
+        $res = $auth->login($siteConfig, $guzzle);
 
         $this->assertInstanceOf(LoginFormAuthenticator::class, $res);
     }
@@ -65,8 +65,8 @@ class LoginFormAuthenticatorTest extends TestCase
             'password' => 'unkn0wn',
         ]);
 
-        $auth = new LoginFormAuthenticator($siteConfig);
+        $auth = new LoginFormAuthenticator();
-        $res = $auth->login($guzzle);
+        $res = $auth->login($siteConfig, $guzzle);
 
         $this->assertInstanceOf(LoginFormAuthenticator::class, $res);
     }
@@ -80,7 +80,7 @@ class LoginFormAuthenticatorTest extends TestCase
 
         $response->expects($this->any())
             ->method('getBody')
-            ->willReturn(file_get_contents(__DIR__ . '/../../fixtures/aoc.media.html'));
+            ->willReturn(file_get_contents(__DIR__ . '/../fixtures/aoc.media.html'));
 
         $response->expects($this->any())
             ->method('getStatusCode')
@@ -128,8 +128,8 @@ class LoginFormAuthenticatorTest extends TestCase
             'password' => 'unkn0wn',
         ]);
 
-        $auth = new LoginFormAuthenticator($siteConfig);
+        $auth = new LoginFormAuthenticator();
-        $res = $auth->login($client);
+        $res = $auth->login($siteConfig, $client);
 
         $this->assertInstanceOf(LoginFormAuthenticator::class, $res);
     }
@@ -142,7 +142,7 @@ class LoginFormAuthenticatorTest extends TestCase
 
         $response->expects($this->any())
             ->method('getBody')
-            ->willReturn(file_get_contents(__DIR__ . '/../../fixtures/nextinpact-login.html'));
+            ->willReturn(file_get_contents(__DIR__ . '/../fixtures/nextinpact-login.html'));
 
         $response->expects($this->any())
             ->method('getStatusCode')
@@ -194,8 +194,8 @@ class LoginFormAuthenticatorTest extends TestCase
             'password' => 'unkn0wn',
         ]);
 
-        $auth = new LoginFormAuthenticator($siteConfig);
+        $auth = new LoginFormAuthenticator();
-        $res = $auth->login($client);
+        $res = $auth->login($siteConfig, $client);
 
         $this->assertInstanceOf(LoginFormAuthenticator::class, $res);
     }
@@ -210,8 +210,8 @@ class LoginFormAuthenticatorTest extends TestCase
             'password' => 'unkn0wn',
         ]);
 
-        $auth = new LoginFormAuthenticator($siteConfig);
+        $auth = new LoginFormAuthenticator();
-        $loginRequired = $auth->isLoginRequired(file_get_contents(__DIR__ . '/../../fixtures/nextinpact-login.html'));
+        $loginRequired = $auth->isLoginRequired($siteConfig, file_get_contents(__DIR__ . '/../fixtures/nextinpact-login.html'));
 
         $this->assertFalse($loginRequired);
     }
@@ -227,8 +227,8 @@ class LoginFormAuthenticatorTest extends TestCase
             'notLoggedInXpath' => '//h2[@class="title_reserve_article"]',
         ]);
 
-        $auth = new LoginFormAuthenticator($siteConfig);
+        $auth = new LoginFormAuthenticator();
-        $loginRequired = $auth->isLoginRequired(file_get_contents(__DIR__ . '/../../fixtures/nextinpact-article.html'));
+        $loginRequired = $auth->isLoginRequired($siteConfig, file_get_contents(__DIR__ . '/../fixtures/nextinpact-article.html'));
 
         $this->assertTrue($loginRequired);
     }