From 94b232bbb8de4699911a6446a1a96f75370cab50 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Tue, 9 May 2017 22:25:18 +0200 Subject: [PATCH] Skip auth when no credentials are found MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit If we can’t find a credential for the current host, even if it required login, we won’t add them and website will be fetched without any login. --- composer.json | 2 +- .../GrabySiteConfigBuilder.php | 36 ++++++++++++----- .../CoreBundle/Helper/HttpClientFactory.php | 1 + .../CoreBundle/Resources/config/services.yml | 3 ++ .../GrabySiteConfigBuilderTest.php | 40 +++++++++++++------ 5 files changed, 59 insertions(+), 23 deletions(-) diff --git a/composer.json b/composer.json index bfd42339..9b69a12c 100644 --- a/composer.json +++ b/composer.json @@ -84,7 +84,7 @@ "javibravo/simpleue": "^1.0", "symfony/dom-crawler": "^3.1", "friendsofsymfony/jsrouting-bundle": "^1.6", - "bdunogier/guzzle-site-authenticator": "dev-master" + "bdunogier/guzzle-site-authenticator": "dev-callback_forms" }, "require-dev": { "doctrine/doctrine-fixtures-bundle": "~2.2", diff --git a/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php b/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php index 1c866f17..a16ed49d 100644 --- a/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php +++ b/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php @@ -6,28 +6,35 @@ use BD\GuzzleSiteAuthenticator\SiteConfig\SiteConfig; use BD\GuzzleSiteAuthenticator\SiteConfig\SiteConfigBuilder; use Graby\SiteConfig\ConfigBuilder; use OutOfRangeException; +use Psr\Log\LoggerInterface; class GrabySiteConfigBuilder implements SiteConfigBuilder { /** - * @var \Graby\SiteConfig\ConfigBuilder + * @var ConfigBuilder */ private $grabyConfigBuilder; /** * @var array */ private $credentials; + /** + * @var LoggerInterface + */ + private $logger; /** * GrabySiteConfigBuilder constructor. * - * @param \Graby\SiteConfig\ConfigBuilder $grabyConfigBuilder - * @param array $credentials + * @param ConfigBuilder $grabyConfigBuilder + * @param array $credentials + * @param LoggerInterface $logger */ - public function __construct(ConfigBuilder $grabyConfigBuilder, array $credentials = []) + public function __construct(ConfigBuilder $grabyConfigBuilder, array $credentials, LoggerInterface $logger) { $this->grabyConfigBuilder = $grabyConfigBuilder; $this->credentials = $credentials; + $this->logger = $logger; } /** @@ -47,6 +54,12 @@ class GrabySiteConfigBuilder implements SiteConfigBuilder $host = substr($host, 4); } + if (!isset($this->credentials[$host])) { + $this->logger->debug('Auth: no credentials available for host.', ['host' => $host]); + + return false; + } + $config = $this->grabyConfigBuilder->buildForHost($host); $parameters = [ 'host' => $host, @@ -56,14 +69,18 @@ class GrabySiteConfigBuilder implements SiteConfigBuilder 'passwordField' => $config->login_password_field ?: null, 'extraFields' => $this->processExtraFields($config->login_extra_fields), 'notLoggedInXpath' => $config->not_logged_in_xpath ?: null, + 'username' => $this->credentials[$host]['username'], + 'password' => $this->credentials[$host]['password'], ]; - if (isset($this->credentials[$host])) { - $parameters['username'] = $this->credentials[$host]['username']; - $parameters['password'] = $this->credentials[$host]['password']; - } + $config = new SiteConfig($parameters); + + // do not leak password in log + $parameters['password'] = '**masked**'; - return new SiteConfig($parameters); + $this->logger->debug('Auth: add parameters.', ['host' => $host, 'parameters' => $parameters]); + + return $config; } /** @@ -85,6 +102,7 @@ class GrabySiteConfigBuilder implements SiteConfigBuilder if (strpos($extraField, '=') === false) { continue; } + list($fieldName, $fieldValue) = explode('=', $extraField, 2); $extraFields[$fieldName] = $fieldValue; } diff --git a/src/Wallabag/CoreBundle/Helper/HttpClientFactory.php b/src/Wallabag/CoreBundle/Helper/HttpClientFactory.php index 11ef26d8..43f5b119 100644 --- a/src/Wallabag/CoreBundle/Helper/HttpClientFactory.php +++ b/src/Wallabag/CoreBundle/Helper/HttpClientFactory.php @@ -51,6 +51,7 @@ class HttpClientFactory $this->cookieJar->clear(); // need to set the (shared) cookie jar $client = new Client(['handler' => new SafeCurlHandler(), 'defaults' => ['cookies' => $this->cookieJar]]); + foreach ($this->subscribers as $subscriber) { $client->getEmitter()->attach($subscriber); } diff --git a/src/Wallabag/CoreBundle/Resources/config/services.yml b/src/Wallabag/CoreBundle/Resources/config/services.yml index 68f900a1..6c9195ce 100644 --- a/src/Wallabag/CoreBundle/Resources/config/services.yml +++ b/src/Wallabag/CoreBundle/Resources/config/services.yml @@ -63,6 +63,9 @@ services: arguments: - "@wallabag_core.graby.config_builder" - "%sites_credentials%" + - '@logger' + tags: + - { name: monolog.logger, channel: graby } # service alias override bd_guzzle_site_authenticator.site_config_builder: diff --git a/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php b/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php index 8341b11f..8b50bce9 100644 --- a/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php +++ b/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php @@ -2,6 +2,8 @@ namespace Tests\Wallabag\CoreBundle\GuzzleSiteAuthenticator; +use Monolog\Handler\TestHandler; +use Monolog\Logger; use BD\GuzzleSiteAuthenticator\SiteConfig\SiteConfig; use Graby\SiteConfig\SiteConfig as GrabySiteConfig; use PHPUnit_Framework_TestCase; @@ -32,14 +34,19 @@ class GrabySiteConfigBuilderTest extends PHPUnit_Framework_TestCase ->with('example.com') ->will($this->returnValue($grabySiteConfig)); + $logger = new Logger('foo'); + $handler = new TestHandler(); + $logger->pushHandler($handler); + $this->builder = new GrabySiteConfigBuilder( $grabyConfigBuilderMock, - ['example.com' => ['username' => 'foo', 'password' => 'bar']] + ['example.com' => ['username' => 'foo', 'password' => 'bar']], + $logger ); $config = $this->builder->buildForHost('example.com'); - self::assertEquals( + $this->assertEquals( new SiteConfig([ 'host' => 'example.com', 'requiresLogin' => true, @@ -53,6 +60,10 @@ class GrabySiteConfigBuilderTest extends PHPUnit_Framework_TestCase ]), $config ); + + $records = $handler->getRecords(); + + $this->assertCount(1, $records, 'One log was recorded'); } public function testBuildConfigDoesntExist() @@ -67,19 +78,22 @@ class GrabySiteConfigBuilderTest extends PHPUnit_Framework_TestCase ->with('unknown.com') ->will($this->returnValue(new GrabySiteConfig())); - $this->builder = new GrabySiteConfigBuilder($grabyConfigBuilderMock, []); + $logger = new Logger('foo'); + $handler = new TestHandler(); + $logger->pushHandler($handler); + + $this->builder = new GrabySiteConfigBuilder( + $grabyConfigBuilderMock, + [], + $logger + ); $config = $this->builder->buildForHost('unknown.com'); - self::assertEquals( - new SiteConfig([ - 'host' => 'unknown.com', - 'requiresLogin' => false, - 'username' => null, - 'password' => null, - 'extraFields' => [], - ]), - $config - ); + $this->assertFalse($config); + + $records = $handler->getRecords(); + + $this->assertCount(1, $records, 'One log was recorded'); } } -- 2.41.0