From a39acb2518f272df8a601af72c13eabe2719dcb8 Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Sat, 18 Jan 2020 11:33:23 +0100 Subject: [PATCH] Fix an issue with private tags and fix nomarkdown tag The new bookmark service wasn't handling private tags properly. nomarkdown tag is now shown only for logged in user in bookmarks, and hidden for everyone in tag clouds/lists. Fixes #726 --- application/bookmark/BookmarkFileService.php | 9 +++- .../formatter/BookmarkDefaultFormatter.php | 2 +- application/formatter/BookmarkFormatter.php | 35 ++++++++++++++-- .../formatter/BookmarkMarkdownFormatter.php | 8 ++-- application/formatter/FormatterFactory.php | 11 +++-- index.php | 39 +++++++++++------- tests/bookmark/BookmarkFileServiceTest.php | 41 +++++++++++++++++++ tests/feed/FeedBuilderTest.php | 2 +- .../BookmarkDefaultFormatterTest.php | 23 ++++++++++- .../BookmarkMarkdownFormatterTest.php | 2 +- tests/formatter/BookmarkRawFormatterTest.php | 2 +- tests/formatter/FormatterFactoryTest.php | 2 +- tests/netscape/BookmarkExportTest.php | 2 +- 13 files changed, 145 insertions(+), 33 deletions(-) diff --git a/application/bookmark/BookmarkFileService.php b/application/bookmark/BookmarkFileService.php index a56cc92b..9c59e139 100644 --- a/application/bookmark/BookmarkFileService.php +++ b/application/bookmark/BookmarkFileService.php @@ -8,6 +8,7 @@ use Shaarli\Bookmark\Exception\BookmarkNotFoundException; use Shaarli\Bookmark\Exception\EmptyDataStoreException; use Shaarli\Config\ConfigManager; +use Shaarli\Formatter\BookmarkMarkdownFormatter; use Shaarli\History; use Shaarli\Legacy\LegacyLinkDB; use Shaarli\Legacy\LegacyUpdater; @@ -130,7 +131,7 @@ public function get($id, $visibility = null) } if ($visibility === null) { - $visibility = $this->isLoggedIn ? 'all' : 'public'; + $visibility = $this->isLoggedIn ? BookmarkFilter::$ALL : BookmarkFilter::$PUBLIC; } $bookmark = $this->bookmarks[$id]; @@ -287,9 +288,13 @@ public function bookmarksCountPerTag($filteringTags = [], $visibility = null) $caseMapping = []; foreach ($bookmarks as $bookmark) { foreach ($bookmark->getTags() as $tag) { - if (empty($tag) || (! $this->isLoggedIn && startsWith($tag, '.'))) { + if (empty($tag) + || (! $this->isLoggedIn && startsWith($tag, '.')) + || $tag === BookmarkMarkdownFormatter::NO_MD_TAG + ) { continue; } + // The first case found will be displayed. if (!isset($caseMapping[strtolower($tag)])) { $caseMapping[strtolower($tag)] = $tag; diff --git a/application/formatter/BookmarkDefaultFormatter.php b/application/formatter/BookmarkDefaultFormatter.php index 7550c556..c6c59064 100644 --- a/application/formatter/BookmarkDefaultFormatter.php +++ b/application/formatter/BookmarkDefaultFormatter.php @@ -34,7 +34,7 @@ public function formatDescription($bookmark) */ protected function formatTagList($bookmark) { - return escape($bookmark->getTags()); + return escape(parent::formatTagList($bookmark)); } /** diff --git a/application/formatter/BookmarkFormatter.php b/application/formatter/BookmarkFormatter.php index c82c3452..a80d83fc 100644 --- a/application/formatter/BookmarkFormatter.php +++ b/application/formatter/BookmarkFormatter.php @@ -20,6 +20,9 @@ abstract class BookmarkFormatter */ protected $conf; + /** @var bool */ + protected $isLoggedIn; + /** * @var array Additional parameters than can be used for specific formatting * e.g. index_url for Feed formatting @@ -30,9 +33,10 @@ abstract class BookmarkFormatter * LinkDefaultFormatter constructor. * @param ConfigManager $conf */ - public function __construct(ConfigManager $conf) + public function __construct(ConfigManager $conf, bool $isLoggedIn) { $this->conf = $conf; + $this->isLoggedIn = $isLoggedIn; } /** @@ -172,7 +176,7 @@ protected function formatThumbnail($bookmark) */ protected function formatTagList($bookmark) { - return $bookmark->getTags(); + return $this->filterTagList($bookmark->getTags()); } /** @@ -184,7 +188,7 @@ protected function formatTagList($bookmark) */ protected function formatTagString($bookmark) { - return implode(' ', $bookmark->getTags()); + return implode(' ', $this->formatTagList($bookmark)); } /** @@ -253,4 +257,29 @@ protected function formatUpdatedTimestamp(Bookmark $bookmark) } return 0; } + + /** + * Format tag list, e.g. remove private tags if the user is not logged in. + * + * @param array $tags + * + * @return array + */ + protected function filterTagList(array $tags): array + { + if ($this->isLoggedIn === true) { + return $tags; + } + + $out = []; + foreach ($tags as $tag) { + if (strpos($tag, '.') === 0) { + continue; + } + + $out[] = $tag; + } + + return $out; + } } diff --git a/application/formatter/BookmarkMarkdownFormatter.php b/application/formatter/BookmarkMarkdownFormatter.php index 7797bfbf..077e5312 100644 --- a/application/formatter/BookmarkMarkdownFormatter.php +++ b/application/formatter/BookmarkMarkdownFormatter.php @@ -36,10 +36,12 @@ class BookmarkMarkdownFormatter extends BookmarkDefaultFormatter * LinkMarkdownFormatter constructor. * * @param ConfigManager $conf instance + * @param bool $isLoggedIn */ - public function __construct(ConfigManager $conf) + public function __construct(ConfigManager $conf, bool $isLoggedIn) { - parent::__construct($conf); + parent::__construct($conf, $isLoggedIn); + $this->parsedown = new \Parsedown(); $this->escape = $conf->get('security.markdown_escape', true); $this->allowedProtocols = $conf->get('security.allowed_protocols', []); @@ -79,7 +81,7 @@ public function formatDescription($bookmark) protected function formatTagList($bookmark) { $out = parent::formatTagList($bookmark); - if (($pos = array_search(self::NO_MD_TAG, $out)) !== false) { + if ($this->isLoggedIn === false && ($pos = array_search(self::NO_MD_TAG, $out)) !== false) { unset($out[$pos]); return array_values($out); } diff --git a/application/formatter/FormatterFactory.php b/application/formatter/FormatterFactory.php index 0d2c0466..5f282f68 100644 --- a/application/formatter/FormatterFactory.php +++ b/application/formatter/FormatterFactory.php @@ -16,14 +16,19 @@ class FormatterFactory /** @var ConfigManager instance */ protected $conf; + /** @var bool */ + protected $isLoggedIn; + /** * FormatterFactory constructor. * * @param ConfigManager $conf + * @param bool $isLoggedIn */ - public function __construct(ConfigManager $conf) + public function __construct(ConfigManager $conf, bool $isLoggedIn) { $this->conf = $conf; + $this->isLoggedIn = $isLoggedIn; } /** @@ -33,7 +38,7 @@ public function __construct(ConfigManager $conf) * * @return BookmarkFormatter instance. */ - public function getFormatter($type = null) + public function getFormatter(string $type = null) { $type = $type ? $type : $this->conf->get('formatter', 'default'); $className = '\\Shaarli\\Formatter\\Bookmark'. ucfirst($type) .'Formatter'; @@ -41,6 +46,6 @@ public function getFormatter($type = null) $className = '\\Shaarli\\Formatter\\BookmarkDefaultFormatter'; } - return new $className($this->conf); + return new $className($this->conf, $this->isLoggedIn); } } diff --git a/index.php b/index.php index 2dd003f0..bf75f7c9 100644 --- a/index.php +++ b/index.php @@ -70,6 +70,7 @@ use \Shaarli\Config\ConfigManager; use \Shaarli\Feed\CachedPage; use \Shaarli\Feed\FeedBuilder; +use Shaarli\Formatter\BookmarkMarkdownFormatter; use Shaarli\Formatter\FormatterFactory; use \Shaarli\History; use \Shaarli\Languages; @@ -351,7 +352,7 @@ function showDailyRSS($bookmarkService, $conf, $loginManager) echo 'en-en'; echo ''. $pageaddr .''. PHP_EOL; - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); $formatter = $factory->getFormatter(); $formatter->addContextData('index_url', index_url($_SERVER)); // For each day. @@ -441,7 +442,7 @@ function showDaily($pageBuilder, $bookmarkService, $conf, $pluginManager, $login $linksToDisplay = []; } - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); $formatter = $factory->getFormatter(); // We pre-format some fields for proper output. foreach ($linksToDisplay as $key => $bookmark) { @@ -630,7 +631,7 @@ function renderPage($conf, $pluginManager, $bookmarkService, $history, $sessionM // Get only bookmarks which have a thumbnail. // Note: we do not retrieve thumbnails here, the request is too heavy. - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); $formatter = $factory->getFormatter(); foreach ($links as $key => $link) { if ($link->getThumbnail() !== false) { @@ -753,7 +754,7 @@ function renderPage($conf, $pluginManager, $bookmarkService, $history, $sessionM exit; } - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); // Generate data. $feedGenerator = new FeedBuilder( $bookmarkService, @@ -1183,7 +1184,7 @@ function renderPage($conf, $pluginManager, $bookmarkService, $history, $sessionM $bookmarkService->addOrSet($bookmark, false); // To preserve backward compatibility with 3rd parties, plugins still use arrays - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); $formatter = $factory->getFormatter('raw'); $data = $formatter->format($bookmark); $pluginManager->executeHooks('save_link', $data); @@ -1230,7 +1231,7 @@ function ($item) { if (!count($ids)) { die('no id provided'); } - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); $formatter = $factory->getFormatter('raw'); foreach ($ids as $id) { $id = (int) escape($id); @@ -1286,7 +1287,7 @@ function ($item) { } else { $private = $_GET['newVisibility'] === 'private'; } - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); $formatter = $factory->getFormatter('raw'); foreach ($ids as $id) { $id = (int) escape($id); @@ -1324,14 +1325,18 @@ function ($item) { exit; } - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); $formatter = $factory->getFormatter('raw'); $formattedLink = $formatter->format($link); + $tags = $bookmarkService->bookmarksCountPerTag(); + if ($conf->get('formatter') === 'markdown') { + $tags[BookmarkMarkdownFormatter::NO_MD_TAG] = 1; + } $data = array( 'link' => $formattedLink, 'link_is_new' => false, 'http_referer' => (isset($_SERVER['HTTP_REFERER']) ? escape($_SERVER['HTTP_REFERER']) : ''), - 'tags' => $bookmarkService->bookmarksCountPerTag(), + 'tags' => $tags, ); $pluginManager->executeHooks('render_editlink', $data); @@ -1391,17 +1396,21 @@ function ($item) { 'private' => $private, ]; } else { - $factory = new FormatterFactory($conf); - $formatter = $factory->getFormatter('raw'); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); + $formatter = $factory->getFormatter('raw'); $link = $formatter->format($bookmark); } + $tags = $bookmarkService->bookmarksCountPerTag(); + if ($conf->get('formatter') === 'markdown') { + $tags[BookmarkMarkdownFormatter::NO_MD_TAG] = 1; + } $data = [ 'link' => $link, 'link_is_new' => $link_is_new, 'http_referer' => (isset($_SERVER['HTTP_REFERER']) ? escape($_SERVER['HTTP_REFERER']) : ''), 'source' => (isset($_GET['source']) ? $_GET['source'] : ''), - 'tags' => $bookmarkService->bookmarksCountPerTag(), + 'tags' => $tags, 'default_private_links' => $conf->get('privacy.default_private_links', false), ]; $pluginManager->executeHooks('render_editlink', $data); @@ -1451,7 +1460,7 @@ function ($item) { } try { - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); $formatter = $factory->getFormatter('raw'); $PAGE->assign( 'links', @@ -1633,7 +1642,7 @@ function ($a, $b) { $bookmark->setThumbnail($thumbnailer->get($bookmark->getUrl())); $bookmarkService->set($bookmark); - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); echo json_encode($factory->getFormatter('raw')->format($bookmark)); exit; } @@ -1655,7 +1664,7 @@ function ($a, $b) { */ function buildLinkList($PAGE, $linkDb, $conf, $pluginManager, $loginManager) { - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, $loginManager->isLoggedIn()); $formatter = $factory->getFormatter(); // Used in templates diff --git a/tests/bookmark/BookmarkFileServiceTest.php b/tests/bookmark/BookmarkFileServiceTest.php index 1b438a7f..4900d41d 100644 --- a/tests/bookmark/BookmarkFileServiceTest.php +++ b/tests/bookmark/BookmarkFileServiceTest.php @@ -12,6 +12,7 @@ use Shaarli; use Shaarli\Bookmark\Exception\BookmarkNotFoundException; use Shaarli\Config\ConfigManager; +use Shaarli\Formatter\BookmarkMarkdownFormatter; use Shaarli\History; /** @@ -1025,6 +1026,46 @@ public function testCountLinkPerTagPrivateWithFilter() $this->assertEquals($expected, $tags, var_export($tags, true)); } + /** + * Test linksCountPerTag public tags with filter. + * Equal occurrences should be sorted alphabetically. + */ + public function testCountTagsNoMarkdown() + { + $expected = [ + 'cartoon' => 3, + 'dev' => 2, + 'tag1' => 1, + 'tag2' => 1, + 'tag3' => 1, + 'tag4' => 1, + 'web' => 4, + 'gnu' => 2, + 'hashtag' => 2, + 'sTuff' => 2, + '-exclude' => 1, + '.hidden' => 1, + 'Mercurial' => 1, + 'css' => 1, + 'free' => 1, + 'html' => 1, + 'media' => 1, + 'newTagToCount' => 1, + 'samba' => 1, + 'software' => 1, + 'stallman' => 1, + 'ut' => 1, + 'w3c' => 1, + ]; + $bookmark = new Bookmark(); + $bookmark->setTags(['newTagToCount', BookmarkMarkdownFormatter::NO_MD_TAG]); + $this->privateLinkDB->add($bookmark); + + $tags = $this->privateLinkDB->bookmarksCountPerTag(); + + $this->assertEquals($expected, $tags, var_export($tags, true)); + } + /** * Allows to test LinkDB's private methods * diff --git a/tests/feed/FeedBuilderTest.php b/tests/feed/FeedBuilderTest.php index a43ff672..54671891 100644 --- a/tests/feed/FeedBuilderTest.php +++ b/tests/feed/FeedBuilderTest.php @@ -51,7 +51,7 @@ public static function setUpBeforeClass() $refLinkDB = new \ReferenceLinkDB(); $refLinkDB->write(self::$testDatastore); $history = new History('sandbox/history.php'); - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, true); self::$formatter = $factory->getFormatter(); self::$bookmarkService = new BookmarkFileService($conf, $history, true); diff --git a/tests/formatter/BookmarkDefaultFormatterTest.php b/tests/formatter/BookmarkDefaultFormatterTest.php index fe42a208..382a560e 100644 --- a/tests/formatter/BookmarkDefaultFormatterTest.php +++ b/tests/formatter/BookmarkDefaultFormatterTest.php @@ -29,7 +29,7 @@ public function setUp() { copy('tests/utils/config/configJson.json.php', self::$testConf .'.json.php'); $this->conf = new ConfigManager(self::$testConf); - $this->formatter = new BookmarkDefaultFormatter($this->conf); + $this->formatter = new BookmarkDefaultFormatter($this->conf, true); } /** @@ -153,4 +153,25 @@ public function testFormatNoteWithIndexUrl() $link['description'] ); } + + /** + * Make sure that private tags are properly filtered out when the user is logged out. + */ + public function testFormatTagListRemovePrivate(): void + { + $this->formatter = new BookmarkDefaultFormatter($this->conf, false); + + $bookmark = new Bookmark(); + $bookmark->setId($id = 11); + $bookmark->setTags($tags = ['bookmark', '.private', 'othertag']); + + $link = $this->formatter->format($bookmark); + + unset($tags[1]); + $tags = array_values($tags); + + $this->assertSame(11, $link['id']); + $this->assertSame($tags, $link['taglist']); + $this->assertSame(implode(' ', $tags), $link['tags']); + } } diff --git a/tests/formatter/BookmarkMarkdownFormatterTest.php b/tests/formatter/BookmarkMarkdownFormatterTest.php index 0ca7f802..f1f12c04 100644 --- a/tests/formatter/BookmarkMarkdownFormatterTest.php +++ b/tests/formatter/BookmarkMarkdownFormatterTest.php @@ -29,7 +29,7 @@ public function setUp() { copy('tests/utils/config/configJson.json.php', self::$testConf .'.json.php'); $this->conf = new ConfigManager(self::$testConf); - $this->formatter = new BookmarkMarkdownFormatter($this->conf); + $this->formatter = new BookmarkMarkdownFormatter($this->conf, true); } /** diff --git a/tests/formatter/BookmarkRawFormatterTest.php b/tests/formatter/BookmarkRawFormatterTest.php index ceb6fb73..4491b035 100644 --- a/tests/formatter/BookmarkRawFormatterTest.php +++ b/tests/formatter/BookmarkRawFormatterTest.php @@ -29,7 +29,7 @@ public function setUp() { copy('tests/utils/config/configJson.json.php', self::$testConf .'.json.php'); $this->conf = new ConfigManager(self::$testConf); - $this->formatter = new BookmarkRawFormatter($this->conf); + $this->formatter = new BookmarkRawFormatter($this->conf, true); } /** diff --git a/tests/formatter/FormatterFactoryTest.php b/tests/formatter/FormatterFactoryTest.php index 317c0b2d..5adf3ffd 100644 --- a/tests/formatter/FormatterFactoryTest.php +++ b/tests/formatter/FormatterFactoryTest.php @@ -28,7 +28,7 @@ public function setUp() { copy('tests/utils/config/configJson.json.php', self::$testConf .'.json.php'); $this->conf = new ConfigManager(self::$testConf); - $this->factory = new FormatterFactory($this->conf); + $this->factory = new FormatterFactory($this->conf, true); } /** diff --git a/tests/netscape/BookmarkExportTest.php b/tests/netscape/BookmarkExportTest.php index 011d19ac..6c948bba 100644 --- a/tests/netscape/BookmarkExportTest.php +++ b/tests/netscape/BookmarkExportTest.php @@ -46,7 +46,7 @@ public static function setUpBeforeClass() self::$refDb->write(self::$testDatastore); $history = new History('sandbox/history.php'); self::$bookmarkService = new BookmarkFileService($conf, $history, true); - $factory = new FormatterFactory($conf); + $factory = new FormatterFactory($conf, true); self::$formatter = $factory->getFormatter('raw'); }