From b7c7915a653a43f18ffd634cf2bf498408359733 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Tue, 9 Feb 2021 10:05:44 -0500 Subject: [PATCH] Enforce admin rquirements in NCNv1 --- CHANGELOG | 4 +++ .../010_Nextcloud_News.md | 1 - lib/REST/NextcloudNews/V1_2.php | 12 ++++++++ tests/cases/REST/NextcloudNews/TestV1_2.php | 29 +++++++++++++++++++ 4 files changed, 45 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index ba7040e..0e7cdc6 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -13,6 +13,10 @@ Bug fixes: compatibility with RFC 7617 - Accept "t" and "f" as booleans in Tiny Tiny RSS +Changes: +- Administrator account requirements for Nextcloud News functionality are + now enforced + Version 0.8.5 (2020-10-27) ========================== diff --git a/docs/en/030_Supported_Protocols/010_Nextcloud_News.md b/docs/en/030_Supported_Protocols/010_Nextcloud_News.md index a2c34d0..17f5d2d 100644 --- a/docs/en/030_Supported_Protocols/010_Nextcloud_News.md +++ b/docs/en/030_Supported_Protocols/010_Nextcloud_News.md @@ -24,7 +24,6 @@ It allows organizing newsfeeds into single-level folders, and supports a wide ra - When marking articles as starred the feed ID is ignored, as they are not needed to establish uniqueness - The feed updater ignores the `userId` parameter: feeds in The Arsse are deduplicated, and have no owner - The `/feeds/all` route lists only feeds which should be checked for updates, and it also returns all `userId` attributes as empty strings: feeds in The Arsse are deduplicated, and have no owner -- The API's "updater" routes do not require administrator priviledges as The Arsse has no concept of user classes - The "updater" console commands mentioned in the protocol specification are not implemented, as The Arsse does not implement the required Nextcloud subsystems - The `lastLoginTimestamp` attribute of the user metadata is always the current time: The Arsse's implementation of the protocol is fully stateless - Syntactically invalid JSON input will yield a `400 Bad Request` response instead of falling back to GET parameters diff --git a/lib/REST/NextcloudNews/V1_2.php b/lib/REST/NextcloudNews/V1_2.php index 2b14cbd..984491a 100644 --- a/lib/REST/NextcloudNews/V1_2.php +++ b/lib/REST/NextcloudNews/V1_2.php @@ -360,6 +360,9 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { // return list of feeds which should be refreshed protected function feedListStale(array $url, array $data): ResponseInterface { + if (!$this->isAdmin()) { + return new EmptyResponse(403); + } // list stale feeds which should be checked for updates $feeds = Arsse::$db->feedListStale(); $out = []; @@ -372,6 +375,9 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { // refresh a feed protected function feedUpdate(array $url, array $data): ResponseInterface { + if (!$this->isAdmin()) { + return new EmptyResponse(403); + } try { Arsse::$db->feedUpdate($data['feedId']); } catch (ExceptionInput $e) { @@ -667,11 +673,17 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { } protected function cleanupBefore(array $url, array $data): ResponseInterface { + if (!$this->isAdmin()) { + return new EmptyResponse(403); + } Service::cleanupPre(); return new EmptyResponse(204); } protected function cleanupAfter(array $url, array $data): ResponseInterface { + if (!$this->isAdmin()) { + return new EmptyResponse(403); + } Service::cleanupPost(); return new EmptyResponse(204); } diff --git a/tests/cases/REST/NextcloudNews/TestV1_2.php b/tests/cases/REST/NextcloudNews/TestV1_2.php index 7b4ce32..3d5a342 100644 --- a/tests/cases/REST/NextcloudNews/TestV1_2.php +++ b/tests/cases/REST/NextcloudNews/TestV1_2.php @@ -317,6 +317,7 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { // create a mock user manager Arsse::$user = \Phake::mock(User::class); Arsse::$user->id = "john.doe@example.com"; + \Phake::when(Arsse::$user)->propertiesGet->thenReturn(['admin' => true]); // create a mock database interface Arsse::$db = \Phake::mock(Database::class); $this->transaction = \Phake::mock(Transaction::class); @@ -629,6 +630,13 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { $this->assertMessage($exp, $this->req("GET", "/feeds/all")); } + public function testListStaleFeedsWithoutAuthority(): void { + \Phake::when(Arsse::$user)->propertiesGet->thenReturn(['admin' => false]); + $exp = new EmptyResponse(403); + $this->assertMessage($exp, $this->req("GET", "/feeds/all")); + \Phake::verify(Arsse::$db, \Phake::times(0))->feedListStale; + } + public function testUpdateAFeed(): void { $in = [ ['feedId' => 42], // valid @@ -650,6 +658,13 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { $this->assertMessage($exp, $this->req("GET", "/feeds/update", json_encode($in[4]))); } + public function testUpdateAFeedWithoutAuthority(): void { + \Phake::when(Arsse::$user)->propertiesGet->thenReturn(['admin' => false]); + $exp = new EmptyResponse(403); + $this->assertMessage($exp, $this->req("GET", "/feeds/update", ['feedId' => 42])); + \Phake::verify(Arsse::$db, \Phake::times(0))->feedUpdate; + } + /** @dataProvider provideArticleQueries */ public function testListArticles(string $url, array $in, Context $c, $out, ResponseInterface $exp): void { if ($out instanceof \Exception) { @@ -849,6 +864,13 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { \Phake::verify(Arsse::$db)->feedCleanup(); } + public function testCleanUpBeforeUpdateWithoutAuthority(): void { + \Phake::when(Arsse::$user)->propertiesGet->thenReturn(['admin' => false]); + $exp = new EmptyResponse(403); + $this->assertMessage($exp, $this->req("GET", "/cleanup/before-update")); + \Phake::verify(Arsse::$db, \Phake::times(0))->feedCleanup; + } + public function testCleanUpAfterUpdate(): void { \Phake::when(Arsse::$db)->articleCleanup()->thenReturn(true); $exp = new EmptyResponse(204); @@ -856,6 +878,13 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { \Phake::verify(Arsse::$db)->articleCleanup(); } + public function testCleanUpAfterUpdateWithoutAuthority(): void { + \Phake::when(Arsse::$user)->propertiesGet->thenReturn(['admin' => false]); + $exp = new EmptyResponse(403); + $this->assertMessage($exp, $this->req("GET", "/cleanup/after-update")); + \Phake::verify(Arsse::$db, \Phake::times(0))->feedCleanup; + } + public function testQueryTheUserStatus(): void { $act = $this->req("GET", "/user"); $exp = new Response([