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

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 Exception;
 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 @@ class BookmarkFileService implements BookmarkServiceInterface
         }
 
         if ($visibility === null) {
-            $visibility = $this->isLoggedIn ? 'all' : 'public';
+            $visibility = $this->isLoggedIn ? BookmarkFilter::$ALL : BookmarkFilter::$PUBLIC;
         }
 
         $bookmark = $this->bookmarks[$id];
@@ -287,9 +288,13 @@ class BookmarkFileService implements BookmarkServiceInterface
         $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 @@ class BookmarkDefaultFormatter extends BookmarkFormatter
      */
     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 @@ abstract class BookmarkFormatter
      */
     protected function formatTagList($bookmark)
     {
-        return $bookmark->getTags();
+        return $this->filterTagList($bookmark->getTags());
     }
 
     /**
@@ -184,7 +188,7 @@ abstract class BookmarkFormatter
      */
     protected function formatTagString($bookmark)
     {
-        return implode(' ', $bookmark->getTags());
+        return implode(' ', $this->formatTagList($bookmark));
     }
 
     /**
@@ -253,4 +257,29 @@ abstract class BookmarkFormatter
         }
         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 @@ class BookmarkMarkdownFormatter extends BookmarkDefaultFormatter
     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 @@ class FormatterFactory
      *
      * @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 @@ class FormatterFactory
             $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\Bookmark\BookmarkFileService;
 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 '<language>en-en</language>';
     echo '<copyright>'. $pageaddr .'</copyright>'. 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 renderPage($conf, $pluginManager, $bookmarkService, $history, $sessionM
         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 renderPage($conf, $pluginManager, $bookmarkService, $history, $sessionM
         } 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 renderPage($conf, $pluginManager, $bookmarkService, $history, $sessionM
             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 renderPage($conf, $pluginManager, $bookmarkService, $history, $sessionM
                 '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 renderPage($conf, $pluginManager, $bookmarkService, $history, $sessionM
         }
 
         try {
-            $factory = new FormatterFactory($conf);
+            $factory = new FormatterFactory($conf, $loginManager->isLoggedIn());
             $formatter = $factory->getFormatter('raw');
             $PAGE->assign(
                 'links',
@@ -1633,7 +1642,7 @@ function renderPage($conf, $pluginManager, $bookmarkService, $history, $sessionM
         $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 renderPage($conf, $pluginManager, $bookmarkService, $history, $sessionM
  */
 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 ReflectionClass;
 use Shaarli;
 use Shaarli\Bookmark\Exception\BookmarkNotFoundException;
 use Shaarli\Config\ConfigManager;
+use Shaarli\Formatter\BookmarkMarkdownFormatter;
 use Shaarli\History;
 
 /**
@@ -1026,6 +1027,46 @@ class BookmarkFileServiceTest extends TestCase
     }
 
     /**
+     * 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
      *
      * @see
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 @@ class FeedBuilderTest extends \PHPUnit\Framework\TestCase
         $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 @@ class BookmarkDefaultFormatterTest extends TestCase
     {
         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 @@ class BookmarkDefaultFormatterTest extends TestCase
             $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 @@ class BookmarkMarkdownFormatterTest extends TestCase
     {
         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 @@ class BookmarkRawFormatterTest extends TestCase
     {
         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 @@ class FormatterFactoryTest extends TestCase
     {
         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 @@ class BookmarkExportTest extends \PHPUnit\Framework\TestCase
         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');
     }