From 9ed2c94a3a22186a6558dcda71490bd0825c1d0d Mon Sep 17 00:00:00 2001 From: IanM Date: Thu, 19 Sep 2024 15:01:37 +0100 Subject: [PATCH 01/32] chore: refactor to start using model visibility, add first tests --- .github/workflows/backend.yml | 2 +- .gitignore | 1 + composer.json | 26 ++- extend.php | 5 + src/Access/ScopeLinkVisibility.php | 26 +++ src/Link.php | 27 ++- src/LinkRepository.php | 227 +++++++++++-------- src/LoadForumLinksRelationship.php | 18 +- tests/fixtures/.gitkeep | 0 tests/integration/Api/LinkVisibilityTest.php | 119 ++++++++++ tests/integration/setup.php | 16 ++ tests/phpunit.integration.xml | 25 ++ tests/phpunit.unit.xml | 27 +++ tests/unit/.gitkeep | 0 14 files changed, 405 insertions(+), 114 deletions(-) create mode 100644 src/Access/ScopeLinkVisibility.php create mode 100644 tests/fixtures/.gitkeep create mode 100644 tests/integration/Api/LinkVisibilityTest.php create mode 100644 tests/integration/setup.php create mode 100644 tests/phpunit.integration.xml create mode 100644 tests/phpunit.unit.xml create mode 100644 tests/unit/.gitkeep diff --git a/.github/workflows/backend.yml b/.github/workflows/backend.yml index 25de77d..591e1a8 100644 --- a/.github/workflows/backend.yml +++ b/.github/workflows/backend.yml @@ -6,7 +6,7 @@ jobs: run: uses: flarum/framework/.github/workflows/REUSABLE_backend.yml@1.x with: - enable_backend_testing: false + enable_backend_testing: true enable_phpstan: true backend_directory: . diff --git a/.gitignore b/.gitignore index befc0a7..1766afc 100755 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ node_modules composer.lock vendor +.phpunit.result.cache diff --git a/composer.json b/composer.json index 01c203a..5f1afae 100755 --- a/composer.json +++ b/composer.json @@ -51,19 +51,37 @@ }, "flarum-cli": { "modules": { - "githubActions": true + "githubActions": true, + "backendTesting": true } } }, "require-dev": { "flarum/phpstan": "*", - "flarum/tags": "*" + "flarum/tags": "*", + "flarum/testing": "^1.0.0" }, "scripts": { "analyse:phpstan": "phpstan analyse", - "clear-cache:phpstan": "phpstan clear-result-cache" + "clear-cache:phpstan": "phpstan clear-result-cache", + "test": [ + "@test:unit", + "@test:integration" + ], + "test:unit": "phpunit -c tests/phpunit.unit.xml", + "test:integration": "phpunit -c tests/phpunit.integration.xml", + "test:setup": "@php tests/integration/setup.php" }, "scripts-descriptions": { - "analyse:phpstan": "Run static analysis" + "analyse:phpstan": "Run static analysis", + "test": "Runs all tests.", + "test:unit": "Runs all unit tests.", + "test:integration": "Runs all integration tests.", + "test:setup": "Sets up a database for use with integration tests. Execute this only once." + }, + "autoload-dev": { + "psr-4": { + "FoF\\Links\\Tests\\": "tests/" + } } } diff --git a/extend.php b/extend.php index 0dbadeb..c9112bb 100755 --- a/extend.php +++ b/extend.php @@ -9,6 +9,8 @@ * file that was distributed with this source code. */ +namespace FoF\Links; + use Flarum\Api\Controller\ShowForumController; use Flarum\Api\Serializer\ForumSerializer; use Flarum\Extend; @@ -44,4 +46,7 @@ ->registerLessConfigVar('fof-links-show-only-icons-on-mobile', 'fof-links.show_icons_only_on_mobile', function ($value) { return $value ? 'true' : 'false'; }), + + (new Extend\ModelVisibility(Link::class)) + ->scope(Access\ScopeLinkVisibility::class), ]; diff --git a/src/Access/ScopeLinkVisibility.php b/src/Access/ScopeLinkVisibility.php new file mode 100644 index 0000000..0bacbae --- /dev/null +++ b/src/Access/ScopeLinkVisibility.php @@ -0,0 +1,26 @@ +where('visibility', 'everyone'); + + if ($actor->isGuest()) { + $query + ->orWhere('visibility', 'guests'); + } else { + $query + ->orWhere('visibility', 'members'); + } + } +} diff --git a/src/Link.php b/src/Link.php index 3fda2a9..d56b9d1 100755 --- a/src/Link.php +++ b/src/Link.php @@ -12,6 +12,7 @@ namespace FoF\Links; use Flarum\Database\AbstractModel; +use Flarum\Database\ScopeVisibilityTrait; /** * @property int $id @@ -29,6 +30,8 @@ */ class Link extends AbstractModel { + use ScopeVisibilityTrait; + /** * {@inheritdoc} */ @@ -73,21 +76,21 @@ public function parent() return $this->belongsTo(self::class); } - public function save(array $options = []): bool - { - $result = parent::save($options); + // public function save(array $options = []): bool + // { + // $result = parent::save($options); - resolve(LinkRepository::class)->clearLinksCache(); + // resolve(LinkRepository::class)->clearLinksCache(); - return $result; - } + // return $result; + // } - public function delete() - { - $result = parent::delete(); + // public function delete() + // { + // $result = parent::delete(); - resolve(LinkRepository::class)->clearLinksCache(); + // resolve(LinkRepository::class)->clearLinksCache(); - return $result; - } + // return $result; + // } } diff --git a/src/LinkRepository.php b/src/LinkRepository.php index 96ddaa5..01e31a5 100755 --- a/src/LinkRepository.php +++ b/src/LinkRepository.php @@ -13,7 +13,9 @@ use Flarum\User\User; use Illuminate\Contracts\Cache\Store as Cache; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection; +use Illuminate\Support\Collection as SupportCollection; class LinkRepository { @@ -32,128 +34,167 @@ public function __construct(Cache $cache) } /** - * Find a link by ID. - * - * @param int $id - * @param User $actor + * Get a new query builder for the links table. * - * @throws \Illuminate\Database\Eloquent\ModelNotFoundException - * - * @return Link + * @return Builder */ - public function findOrFail($id, User $actor = null) + public function query() { - return Link::where('id', $id)->firstOrFail(); + return Link::query(); } - /** - * Get all links. - */ - public function all() + public function queryVisibleTo(?User $actor = null): Builder { - return Link::query()->get(); + return $this->scopeVisibleTo($this->query(), $actor); } /** - * Gets the cache key for the appropriate links for the given user. - * - * @param User $actor - * - * @return string - */ - public function cacheKey(User $actor): string - { - return self::$cacheKeyPrefix.($actor->isGuest() ? self::$cacheGuestLinksKey : self::$cacheMemberLinksKey); - } - - /** - * Get the links for the given user. + * Find a link by ID. * + * @param int $id * @param User $actor * - * @return Collection - */ - public function getLinks(User $actor): Collection - { - return $actor->isGuest() ? $this->getGuestLinks($actor) : $this->getMemberLinks($actor); - } - - /** - * Get the links for guests. - * - * If the links are cached, they will be returned from the cache, else the cache will be populated from the database. - * - * @param User $actor + * @throws \Illuminate\Database\Eloquent\ModelNotFoundException * - * @return Collection + * @return Link */ - public function getGuestLinks(User $actor): Collection + public function findOrFail($id, User $actor = null) { - if ($links = $this->cache->get($this->cacheKey($actor))) { - return $links; - } else { - $links = $this->getGuestLinksFromDatabase(); - $this->cache->forever($this->cacheKey($actor), $links); + $query = Link::where('id', $id); - return $links; - } + return $this->scopeVisibleTo($query, $actor)->firstOrFail(); } /** - * Get the links for guests from the database. - * - * @return Collection + * Get all links, optionally making sure they are visible to a + * certain user. + * + * @param User|null $user + * @return \Illuminate\Database\Eloquent\Collection */ - protected function getGuestLinksFromDatabase(): Collection + public function all(User $user = null) { - return Link::query() - ->where('visibility', 'guests') - ->orWhere('visibility', 'everyone') - ->get(); + $query = Link::query(); + + return $this->scopeVisibleTo($query, $user)->get(); } /** - * Get the links for members. - * - * If the links are cached, they will be returned from the cache, else the cache will be populated from the database. - * - * @param User $actor + * Scope a query to only include records that are visible to a user. * - * @return Collection + * @param Builder $query + * @param User|null $user + * @return Builder */ - public function getMemberLinks(User $actor): Collection + protected function scopeVisibleTo(Builder $query, ?User $user = null) { - if ($links = $this->cache->get($this->cacheKey($actor))) { - return $links; - } else { - $links = $this->getMemberLinksFromDatabase($actor); - $this->cache->forever($this->cacheKey($actor), $links); - - return $links; + if ($user !== null) { + $query->whereVisibleTo($user); } - } - /** - * Get the links for members from the database. - * - * @param User $actor - * - * @return Collection - */ - protected function getMemberLinksFromDatabase(User $actor): Collection - { - return Link::query() - ->where('visibility', 'members') - ->orWhere('visibility', 'everyone') - ->get(); + return $query; } - /** - * Clear the links cache. - */ - public function clearLinksCache(): void - { - $this->cache->forget(self::$cacheKeyPrefix.self::$cacheGuestLinksKey); - $this->cache->forget(self::$cacheKeyPrefix.self::$cacheMemberLinksKey); - } + // /** + // * Gets the cache key for the appropriate links for the given user. + // * + // * @param User $actor + // * + // * @return string + // */ + // public function cacheKey(User $actor): string + // { + // return self::$cacheKeyPrefix.($actor->isGuest() ? self::$cacheGuestLinksKey : self::$cacheMemberLinksKey); + // } + + // /** + // * Get the links for the given user. + // * + // * @param User $actor + // * + // * @return Collection + // */ + // public function getLinks(User $actor): Collection + // { + // return $actor->isGuest() ? $this->getGuestLinks($actor) : $this->getMemberLinks($actor); + // } + + // /** + // * Get the links for guests. + // * + // * If the links are cached, they will be returned from the cache, else the cache will be populated from the database. + // * + // * @param User $actor + // * + // * @return Collection + // */ + // public function getGuestLinks(User $actor): Collection + // { + // if ($links = $this->cache->get($this->cacheKey($actor))) { + // return $links; + // } else { + // $links = $this->getGuestLinksFromDatabase(); + // $this->cache->forever($this->cacheKey($actor), $links); + + // return $links; + // } + // } + + // /** + // * Get the links for guests from the database. + // * + // * @return Collection + // */ + // protected function getGuestLinksFromDatabase(): Collection + // { + // return Link::query() + // ->where('visibility', 'guests') + // ->orWhere('visibility', 'everyone') + // ->get(); + // } + + // /** + // * Get the links for members. + // * + // * If the links are cached, they will be returned from the cache, else the cache will be populated from the database. + // * + // * @param User $actor + // * + // * @return Collection + // */ + // public function getMemberLinks(User $actor): Collection + // { + // if ($links = $this->cache->get($this->cacheKey($actor))) { + // return $links; + // } else { + // $links = $this->getMemberLinksFromDatabase($actor); + // $this->cache->forever($this->cacheKey($actor), $links); + + // return $links; + // } + // } + + // /** + // * Get the links for members from the database. + // * + // * @param User $actor + // * + // * @return Collection + // */ + // protected function getMemberLinksFromDatabase(User $actor): Collection + // { + // return Link::query() + // ->where('visibility', 'members') + // ->orWhere('visibility', 'everyone') + // ->get(); + // } + + // /** + // * Clear the links cache. + // */ + // public function clearLinksCache(): void + // { + // $this->cache->forget(self::$cacheKeyPrefix.self::$cacheGuestLinksKey); + // $this->cache->forget(self::$cacheKeyPrefix.self::$cacheMemberLinksKey); + // } } diff --git a/src/LoadForumLinksRelationship.php b/src/LoadForumLinksRelationship.php index 241a2e4..3257b75 100644 --- a/src/LoadForumLinksRelationship.php +++ b/src/LoadForumLinksRelationship.php @@ -43,14 +43,24 @@ public function __construct(Config $config, LinkRepository $links) public function __invoke(ShowForumController $controller, &$data, ServerRequestInterface $request) { $actor = RequestUtil::getActor($request); - $adminPath = Arr::get($this->config, 'paths.admin'); // So that admins don't have to see guest only items but can manage them in admin panel, // we only serialize all links if we're visiting the admin panel - if ($actor->isAdmin() && $request->getServerParams()['REQUEST_URI'] === "/$adminPath") { - return $data['links'] = Link::all(); + if ($actor->isAdmin() && $this->isAdminPath($request)) { + return $data['links'] = Link::query()->all(); } - $data['links'] = $this->links->getLinks($actor); + //$data['links'] = $this->links->getLinks($actor); + + $data['links'] = Link::query() + ->whereVisibleTo($actor) + ->get(); + } + + private function isAdminPath(ServerRequestInterface $request): bool + { + $adminPath = Arr::get($this->config, 'paths.admin'); + + return Arr::get($request->getServerParams(), 'REQUEST_URI') === "/$adminPath"; } } diff --git a/tests/fixtures/.gitkeep b/tests/fixtures/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/tests/integration/Api/LinkVisibilityTest.php b/tests/integration/Api/LinkVisibilityTest.php new file mode 100644 index 0000000..48441d1 --- /dev/null +++ b/tests/integration/Api/LinkVisibilityTest.php @@ -0,0 +1,119 @@ +extension('fof-links'); + + $this->prepareDatabase([ + 'users' => [ + $this->normalUser(), + ], + 'links' => [ + ['id' => 1, 'title' => 'Google', 'url' => 'https://google.com', 'visibility' => 'everyone'], + ['id' => 2, 'title' => 'Facebook', 'url' => 'https://facebook.com', 'visibility' => 'guests'], + ['id' => 3, 'title' => 'Twitter', 'url' => 'https://twitter.com', 'visibility' => 'members'], + ['id' => 4, 'title' => 'Reddit', 'url' => 'https://reddit.com', 'visibility' => 'members'] + ] + ]); + } + + public function forumUsersDataProvider(): array + { + return [ + [1], + [2] + ]; + } + + public function extractLinksFromIncluded(array $data): array + { + $this->assertArrayHasKey('included', $data); + $this->assertIsArray($data['included']); + + return array_filter($data['included'], function ($item) { + return $item['type'] === 'links'; + }); + } + + public function searchForLink(array $links, int $id): array + { + $result = array_filter($links, function ($link) use ($id) { + return $link['attributes']['id'] === $id; + }); + + $this->assertCount(1, $result); + + // Return the first item + return reset($result); + } + + /** + * @test + */ + public function guests_see_guest_and_everyone_links() + { + $response = $this->send( + $this->request('GET', '/api') + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $data = json_decode($response->getBody(), true); + + $links = $this->extractLinksFromIncluded($data); + + $this->assertCount(2, $links); + + $link = $this->searchForLink($links, 1); + + $this->assertEquals('Google', $link['attributes']['title']); + + $link = $this->searchForLink($links, 2); + + $this->assertEquals('Facebook', $link['attributes']['title']); + } + + /** + * @test + * @dataProvider forumUsersDataProvider + */ + public function members_see_everyone_and_members_links(int $userId) + { + $response = $this->send( + $this->request('GET', '/api', [ + 'authenticatedAs' => $userId + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $data = json_decode($response->getBody(), true); + + $links = $this->extractLinksFromIncluded($data); + + $this->assertCount(3, $links); + + $link = $this->searchForLink($links, 1); + + $this->assertEquals('Google', $link['attributes']['title']); + + $link = $this->searchForLink($links, 3); + + $this->assertEquals('Twitter', $link['attributes']['title']); + + $link = $this->searchForLink($links, 4); + + $this->assertEquals('Reddit', $link['attributes']['title']); + } +} diff --git a/tests/integration/setup.php b/tests/integration/setup.php new file mode 100644 index 0000000..67039c0 --- /dev/null +++ b/tests/integration/setup.php @@ -0,0 +1,16 @@ +run(); diff --git a/tests/phpunit.integration.xml b/tests/phpunit.integration.xml new file mode 100644 index 0000000..90fbbff --- /dev/null +++ b/tests/phpunit.integration.xml @@ -0,0 +1,25 @@ + + + + + ../src/ + + + + + ./integration + ./integration/tmp + + + diff --git a/tests/phpunit.unit.xml b/tests/phpunit.unit.xml new file mode 100644 index 0000000..d3a4a3e --- /dev/null +++ b/tests/phpunit.unit.xml @@ -0,0 +1,27 @@ + + + + + ../src/ + + + + + ./unit + + + + + + diff --git a/tests/unit/.gitkeep b/tests/unit/.gitkeep new file mode 100644 index 0000000..e69de29 From 8aaac323374c871f1fd8143e2561f68b24a29cc7 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Thu, 19 Sep 2024 14:02:06 +0000 Subject: [PATCH 02/32] Apply fixes from StyleCI --- extend.php | 1 - src/Access/ScopeLinkVisibility.php | 13 +++++++++++-- src/Link.php | 2 +- src/LinkRepository.php | 7 ++++--- tests/integration/Api/LinkVisibilityTest.php | 20 +++++++++++++++----- tests/integration/setup.php | 8 +++++--- 6 files changed, 36 insertions(+), 15 deletions(-) diff --git a/extend.php b/extend.php index c9112bb..76fddee 100755 --- a/extend.php +++ b/extend.php @@ -16,7 +16,6 @@ use Flarum\Extend; use FoF\Links\Api\Controller; use FoF\Links\Api\Serializer\LinkSerializer; -use FoF\Links\LoadForumLinksRelationship; return [ new Extend\Locales(__DIR__.'/locale'), diff --git a/src/Access/ScopeLinkVisibility.php b/src/Access/ScopeLinkVisibility.php index 0bacbae..24abcec 100644 --- a/src/Access/ScopeLinkVisibility.php +++ b/src/Access/ScopeLinkVisibility.php @@ -1,5 +1,14 @@ where('visibility', 'everyone'); - + if ($actor->isGuest()) { $query ->orWhere('visibility', 'guests'); diff --git a/src/Link.php b/src/Link.php index d56b9d1..b50557d 100755 --- a/src/Link.php +++ b/src/Link.php @@ -31,7 +31,7 @@ class Link extends AbstractModel { use ScopeVisibilityTrait; - + /** * {@inheritdoc} */ diff --git a/src/LinkRepository.php b/src/LinkRepository.php index 01e31a5..219dddc 100755 --- a/src/LinkRepository.php +++ b/src/LinkRepository.php @@ -15,7 +15,6 @@ use Illuminate\Contracts\Cache\Store as Cache; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection; -use Illuminate\Support\Collection as SupportCollection; class LinkRepository { @@ -68,8 +67,9 @@ public function findOrFail($id, User $actor = null) /** * Get all links, optionally making sure they are visible to a * certain user. - * + * * @param User|null $user + * * @return \Illuminate\Database\Eloquent\Collection */ public function all(User $user = null) @@ -83,7 +83,8 @@ public function all(User $user = null) * Scope a query to only include records that are visible to a user. * * @param Builder $query - * @param User|null $user + * @param User|null $user + * * @return Builder */ protected function scopeVisibleTo(Builder $query, ?User $user = null) diff --git a/tests/integration/Api/LinkVisibilityTest.php b/tests/integration/Api/LinkVisibilityTest.php index 48441d1..a34f6e4 100644 --- a/tests/integration/Api/LinkVisibilityTest.php +++ b/tests/integration/Api/LinkVisibilityTest.php @@ -1,5 +1,14 @@ 1, 'title' => 'Google', 'url' => 'https://google.com', 'visibility' => 'everyone'], ['id' => 2, 'title' => 'Facebook', 'url' => 'https://facebook.com', 'visibility' => 'guests'], ['id' => 3, 'title' => 'Twitter', 'url' => 'https://twitter.com', 'visibility' => 'members'], - ['id' => 4, 'title' => 'Reddit', 'url' => 'https://reddit.com', 'visibility' => 'members'] - ] + ['id' => 4, 'title' => 'Reddit', 'url' => 'https://reddit.com', 'visibility' => 'members'], + ], ]); } @@ -32,7 +41,7 @@ public function forumUsersDataProvider(): array { return [ [1], - [2] + [2], ]; } @@ -86,13 +95,14 @@ public function guests_see_guest_and_everyone_links() /** * @test + * * @dataProvider forumUsersDataProvider */ public function members_see_everyone_and_members_links(int $userId) { $response = $this->send( $this->request('GET', '/api', [ - 'authenticatedAs' => $userId + 'authenticatedAs' => $userId, ]) ); diff --git a/tests/integration/setup.php b/tests/integration/setup.php index 67039c0..32982f7 100644 --- a/tests/integration/setup.php +++ b/tests/integration/setup.php @@ -1,10 +1,12 @@ Date: Thu, 19 Sep 2024 15:04:41 +0100 Subject: [PATCH 03/32] fix: phpstan error --- src/LoadForumLinksRelationship.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/LoadForumLinksRelationship.php b/src/LoadForumLinksRelationship.php index 3257b75..eb2ceba 100644 --- a/src/LoadForumLinksRelationship.php +++ b/src/LoadForumLinksRelationship.php @@ -47,7 +47,7 @@ public function __invoke(ShowForumController $controller, &$data, ServerRequestI // So that admins don't have to see guest only items but can manage them in admin panel, // we only serialize all links if we're visiting the admin panel if ($actor->isAdmin() && $this->isAdminPath($request)) { - return $data['links'] = Link::query()->all(); + return $data['links'] = Link::all(); } //$data['links'] = $this->links->getLinks($actor); From acf9f1d1b0f24a2b96a37c52bd732a68cb845109 Mon Sep 17 00:00:00 2001 From: IanM Date: Thu, 19 Sep 2024 15:31:54 +0100 Subject: [PATCH 04/32] simply test --- tests/integration/Api/LinkVisibilityTest.php | 56 +++++++------------- 1 file changed, 18 insertions(+), 38 deletions(-) diff --git a/tests/integration/Api/LinkVisibilityTest.php b/tests/integration/Api/LinkVisibilityTest.php index a34f6e4..d4f966f 100644 --- a/tests/integration/Api/LinkVisibilityTest.php +++ b/tests/integration/Api/LinkVisibilityTest.php @@ -13,6 +13,7 @@ use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; +use FoF\Links\Link; class LinkVisibilityTest extends TestCase { @@ -40,8 +41,9 @@ public function setUp(): void public function forumUsersDataProvider(): array { return [ - [1], - [2], + [null, [1, 2]], + [1, [1, 3, 4]], + [2, [1, 3, 4]], ]; } @@ -55,7 +57,7 @@ public function extractLinksFromIncluded(array $data): array }); } - public function searchForLink(array $links, int $id): array + public function getLinkById(int $id, array $links): array { $result = array_filter($links, function ($link) use ($id) { return $link['attributes']['id'] === $id; @@ -69,36 +71,13 @@ public function searchForLink(array $links, int $id): array /** * @test - */ - public function guests_see_guest_and_everyone_links() - { - $response = $this->send( - $this->request('GET', '/api') - ); - - $this->assertEquals(200, $response->getStatusCode()); - - $data = json_decode($response->getBody(), true); - - $links = $this->extractLinksFromIncluded($data); - - $this->assertCount(2, $links); - - $link = $this->searchForLink($links, 1); - - $this->assertEquals('Google', $link['attributes']['title']); - - $link = $this->searchForLink($links, 2); - - $this->assertEquals('Facebook', $link['attributes']['title']); - } - - /** - * @test + * + * @param int|null $userId + * @param array $expectedLinks * * @dataProvider forumUsersDataProvider */ - public function members_see_everyone_and_members_links(int $userId) + public function user_type_sees_expected_links(?int $userId, array $expectedLinks) { $response = $this->send( $this->request('GET', '/api', [ @@ -112,18 +91,19 @@ public function members_see_everyone_and_members_links(int $userId) $links = $this->extractLinksFromIncluded($data); - $this->assertCount(3, $links); - - $link = $this->searchForLink($links, 1); + $this->assertEquals(count($expectedLinks), count($links)); - $this->assertEquals('Google', $link['attributes']['title']); + foreach ($expectedLinks as $expectedLink) { + $link = $this->getLinkById($expectedLink, $links); - $link = $this->searchForLink($links, 3); + $this->assertNotNull($link); - $this->assertEquals('Twitter', $link['attributes']['title']); + $dbLink = Link::find($expectedLink); - $link = $this->searchForLink($links, 4); + $this->assertNotNull($dbLink); - $this->assertEquals('Reddit', $link['attributes']['title']); + $this->assertEquals($dbLink->title, $link['attributes']['title']); + $this->assertEquals($dbLink->url, $link['attributes']['url']); + } } } From 20b7f2e8a9cbdfdab3a8165cc59a549d34d990a2 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Thu, 19 Sep 2024 14:32:04 +0000 Subject: [PATCH 05/32] Apply fixes from StyleCI --- tests/integration/Api/LinkVisibilityTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/Api/LinkVisibilityTest.php b/tests/integration/Api/LinkVisibilityTest.php index d4f966f..8f40be4 100644 --- a/tests/integration/Api/LinkVisibilityTest.php +++ b/tests/integration/Api/LinkVisibilityTest.php @@ -71,8 +71,8 @@ public function getLinkById(int $id, array $links): array /** * @test - * - * @param int|null $userId + * + * @param int|null $userId * @param array $expectedLinks * * @dataProvider forumUsersDataProvider From c5ec2196ce112906d27c786ffa3a4b058f279d2c Mon Sep 17 00:00:00 2001 From: IanM Date: Thu, 19 Sep 2024 16:10:25 +0100 Subject: [PATCH 06/32] caching for guest users only --- src/Link.php | 24 ++-- src/LinkRepository.php | 172 ++++++++++++----------------- src/LoadForumLinksRelationship.php | 6 +- 3 files changed, 82 insertions(+), 120 deletions(-) diff --git a/src/Link.php b/src/Link.php index b50557d..58764df 100755 --- a/src/Link.php +++ b/src/Link.php @@ -76,21 +76,21 @@ public function parent() return $this->belongsTo(self::class); } - // public function save(array $options = []): bool - // { - // $result = parent::save($options); + public function save(array $options = []): bool + { + $result = parent::save($options); - // resolve(LinkRepository::class)->clearLinksCache(); + resolve(LinkRepository::class)->clearLinksCache(); - // return $result; - // } + return $result; + } - // public function delete() - // { - // $result = parent::delete(); + public function delete() + { + $result = parent::delete(); - // resolve(LinkRepository::class)->clearLinksCache(); + resolve(LinkRepository::class)->clearLinksCache(); - // return $result; - // } + return $result; + } } diff --git a/src/LinkRepository.php b/src/LinkRepository.php index 219dddc..b9a994e 100755 --- a/src/LinkRepository.php +++ b/src/LinkRepository.php @@ -20,7 +20,6 @@ class LinkRepository { protected static $cacheKeyPrefix = 'fof-links.links.'; protected static $cacheGuestLinksKey = 'guest'; - protected static $cacheMemberLinksKey = 'member'; /** * @var Cache @@ -96,106 +95,73 @@ protected function scopeVisibleTo(Builder $query, ?User $user = null) return $query; } - // /** - // * Gets the cache key for the appropriate links for the given user. - // * - // * @param User $actor - // * - // * @return string - // */ - // public function cacheKey(User $actor): string - // { - // return self::$cacheKeyPrefix.($actor->isGuest() ? self::$cacheGuestLinksKey : self::$cacheMemberLinksKey); - // } - - // /** - // * Get the links for the given user. - // * - // * @param User $actor - // * - // * @return Collection - // */ - // public function getLinks(User $actor): Collection - // { - // return $actor->isGuest() ? $this->getGuestLinks($actor) : $this->getMemberLinks($actor); - // } - - // /** - // * Get the links for guests. - // * - // * If the links are cached, they will be returned from the cache, else the cache will be populated from the database. - // * - // * @param User $actor - // * - // * @return Collection - // */ - // public function getGuestLinks(User $actor): Collection - // { - // if ($links = $this->cache->get($this->cacheKey($actor))) { - // return $links; - // } else { - // $links = $this->getGuestLinksFromDatabase(); - // $this->cache->forever($this->cacheKey($actor), $links); - - // return $links; - // } - // } - - // /** - // * Get the links for guests from the database. - // * - // * @return Collection - // */ - // protected function getGuestLinksFromDatabase(): Collection - // { - // return Link::query() - // ->where('visibility', 'guests') - // ->orWhere('visibility', 'everyone') - // ->get(); - // } - - // /** - // * Get the links for members. - // * - // * If the links are cached, they will be returned from the cache, else the cache will be populated from the database. - // * - // * @param User $actor - // * - // * @return Collection - // */ - // public function getMemberLinks(User $actor): Collection - // { - // if ($links = $this->cache->get($this->cacheKey($actor))) { - // return $links; - // } else { - // $links = $this->getMemberLinksFromDatabase($actor); - // $this->cache->forever($this->cacheKey($actor), $links); - - // return $links; - // } - // } - - // /** - // * Get the links for members from the database. - // * - // * @param User $actor - // * - // * @return Collection - // */ - // protected function getMemberLinksFromDatabase(User $actor): Collection - // { - // return Link::query() - // ->where('visibility', 'members') - // ->orWhere('visibility', 'everyone') - // ->get(); - // } - - // /** - // * Clear the links cache. - // */ - // public function clearLinksCache(): void - // { - // $this->cache->forget(self::$cacheKeyPrefix.self::$cacheGuestLinksKey); - // $this->cache->forget(self::$cacheKeyPrefix.self::$cacheMemberLinksKey); - // } + /** + * Gets the cache key for the appropriate links for the given user. Only applicable for guests. + * + * @param User $actor + * + * @return string + */ + public function cacheKey(User $actor): string + { + if ($actor->isGuest()) { + return self::$cacheKeyPrefix.self::$cacheGuestLinksKey; + } else { + throw new \InvalidArgumentException('Only guests can have cached links at this time.'); + } + } + + /** + * Get the links for the given user. + * + * @param User $actor + * + * @return Collection + */ + public function getLinks(User $actor): Collection + { + return $actor->isGuest() ? $this->getGuestLinks($actor) : $this->getLinksFromDatabase($actor); + } + + /** + * Get the links for guests. + * + * If the links are cached, they will be returned from the cache, else the cache will be populated from the database. + * + * @param User $actor + * + * @return Collection + */ + public function getGuestLinks(User $actor): Collection + { + if ($links = $this->cache->get($this->cacheKey($actor))) { + return $links; + } else { + $links = $this->getLinksFromDatabase($actor); + $this->cache->forever($this->cacheKey($actor), $links); + + return $links; + } + } + + /** + * Get the links for guests from the database. + * + * @return Collection + */ + protected function getLinksFromDatabase(User $actor): Collection + { + return Link::query() + ->whereVisibleTo($actor) + ->get(); + } + + /** + * Clear the links cache. + */ + public function clearLinksCache(): void + { + $this->cache->forget(self::$cacheKeyPrefix.self::$cacheGuestLinksKey); + //$this->cache->forget(self::$cacheKeyPrefix.self::$cacheMemberLinksKey); + } } diff --git a/src/LoadForumLinksRelationship.php b/src/LoadForumLinksRelationship.php index eb2ceba..16bde59 100644 --- a/src/LoadForumLinksRelationship.php +++ b/src/LoadForumLinksRelationship.php @@ -50,11 +50,7 @@ public function __invoke(ShowForumController $controller, &$data, ServerRequestI return $data['links'] = Link::all(); } - //$data['links'] = $this->links->getLinks($actor); - - $data['links'] = Link::query() - ->whereVisibleTo($actor) - ->get(); + $data['links'] = $this->links->getLinks($actor); } private function isAdminPath(ServerRequestInterface $request): bool From 7e223789d173ecbacda5ab5c469bc77cc93af179 Mon Sep 17 00:00:00 2001 From: IanM Date: Thu, 19 Sep 2024 16:12:23 +0100 Subject: [PATCH 07/32] remove comment --- src/LinkRepository.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/LinkRepository.php b/src/LinkRepository.php index b9a994e..b67214f 100755 --- a/src/LinkRepository.php +++ b/src/LinkRepository.php @@ -162,6 +162,5 @@ protected function getLinksFromDatabase(User $actor): Collection public function clearLinksCache(): void { $this->cache->forget(self::$cacheKeyPrefix.self::$cacheGuestLinksKey); - //$this->cache->forget(self::$cacheKeyPrefix.self::$cacheMemberLinksKey); } } From 92cea203665fee382d6967f899bdc6c69faebb2e Mon Sep 17 00:00:00 2001 From: IanM Date: Thu, 19 Sep 2024 17:49:06 +0100 Subject: [PATCH 08/32] create link test --- extend.php | 3 + src/Access/LinkPolicy.php | 14 ++ src/Api/Controller/CreateLinkController.php | 13 +- tests/integration/Api/CreateLinkTest.php | 138 ++++++++++++++++++++ 4 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 src/Access/LinkPolicy.php create mode 100644 tests/integration/Api/CreateLinkTest.php diff --git a/extend.php b/extend.php index 76fddee..f2e597a 100755 --- a/extend.php +++ b/extend.php @@ -48,4 +48,7 @@ (new Extend\ModelVisibility(Link::class)) ->scope(Access\ScopeLinkVisibility::class), + + (new Extend\Policy()) + ->modelPolicy(Link::class, Access\LinkPolicy::class) ]; diff --git a/src/Access/LinkPolicy.php b/src/Access/LinkPolicy.php new file mode 100644 index 0000000..9123988 --- /dev/null +++ b/src/Access/LinkPolicy.php @@ -0,0 +1,14 @@ +assertAdmin(); + + $data = Arr::get($request->getParsedBody(), 'data'); + + if (!$data) { + throw new ValidationException([ + 'data' => 'Invalid payload', + ]); + } return $this->bus->dispatch( - new CreateLink(RequestUtil::getActor($request), Arr::get($request->getParsedBody(), 'data')) + new CreateLink($actor, $data) ); } } diff --git a/tests/integration/Api/CreateLinkTest.php b/tests/integration/Api/CreateLinkTest.php new file mode 100644 index 0000000..d184fe7 --- /dev/null +++ b/tests/integration/Api/CreateLinkTest.php @@ -0,0 +1,138 @@ +extension('fof-links'); + + $this->prepareDatabase([ + 'users' => [ + $this->normalUser(), + ], + 'links' => [ + ['id' => 1, 'title' => 'Google', 'url' => 'https://google.com', 'visibility' => 'everyone'], + ], + ]); + } + + public function authorizedUsers(): array + { + return [ + [1], + ]; + } + + public function unauthorizedUsers(): array + { + return [ + [null], + [2] + ]; + } + + public function payload(): array + { + return [ + 'data' => [ + 'type' => 'links', + 'attributes' => [ + 'title' => 'Facebook', + 'url' => 'https://facebook.com', + 'icon' => 'fab fa-facebook', + 'visibility' => 'everyone', + ], + ], + ]; + } + + /** + * @test + * @dataProvider authorizedUsers + */ + public function authorized_user_cannot_create_link_with_no_data(int $userId) + { + $response = $this->send( + $this->request('POST', '/api/links', [ + 'authenticatedAs' => $userId, + ]) + ); + + $this->assertEquals(422, $response->getStatusCode()); + + $response = json_decode($response->getBody()->getContents(), true); + + $this->assertArrayHasKey('errors', $response); + } + + /** + * @test + * @dataProvider authorizedUsers + */ + public function authorized_user_can_create_link(int $userId) + { + $response = $this->send( + $this->request('POST', '/api/links', [ + 'authenticatedAs' => $userId, + 'json' => $this->payload(), + ]) + ); + + $this->assertEquals(201, $response->getStatusCode()); + + $response = json_decode($response->getBody()->getContents(), true); + + $this->assertArrayHasKey('data', $response); + $this->assertArrayHasKey('id', $response['data']); + $this->assertEquals('Facebook', $response['data']['attributes']['title']); + $this->assertEquals('https://facebook.com', $response['data']['attributes']['url']); + $this->assertEquals('everyone', $response['data']['attributes']['visibility']); + + $id = $response['data']['id']; + + $link = Link::find($id); + + $this->assertNotNull($link); + $this->assertEquals('Facebook', $link->title); + $this->assertEquals('https://facebook.com', $link->url); + $this->assertEquals('everyone', $link->visibility); + } + + /** + * @test + * @dataProvider unauthorizedUsers + */ + public function unauthorized_cannot_create_link(?int $userId) + { + if (!$userId) { + $this->extend( + (new Extend\Csrf) + ->exemptRoute('links.create') + ); + } + + $response = $this->send( + $this->request('POST', '/api/links', [ + 'authenticatedAs' => $userId, + 'json' => $this->payload(), + ]) + ); + + $this->assertEquals(403, $response->getStatusCode()); + + $link = Link::where('title', 'Facebook')->first(); + + $this->assertNull($link); + } +} From 8f831f7de816b087622cc21fb172548604226a72 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Thu, 19 Sep 2024 16:49:15 +0000 Subject: [PATCH 09/32] Apply fixes from StyleCI --- extend.php | 2 +- src/Access/LinkPolicy.php | 10 ++++++- src/Api/Controller/CreateLinkController.php | 1 + tests/integration/Api/CreateLinkTest.php | 32 ++++++++++++++------- 4 files changed, 33 insertions(+), 12 deletions(-) diff --git a/extend.php b/extend.php index f2e597a..90b68e9 100755 --- a/extend.php +++ b/extend.php @@ -50,5 +50,5 @@ ->scope(Access\ScopeLinkVisibility::class), (new Extend\Policy()) - ->modelPolicy(Link::class, Access\LinkPolicy::class) + ->modelPolicy(Link::class, Access\LinkPolicy::class), ]; diff --git a/src/Access/LinkPolicy.php b/src/Access/LinkPolicy.php index 9123988..79f3b3d 100644 --- a/src/Access/LinkPolicy.php +++ b/src/Access/LinkPolicy.php @@ -1,5 +1,14 @@ 'Invalid payload', ]); } + return $this->bus->dispatch( new CreateLink($actor, $data) ); diff --git a/tests/integration/Api/CreateLinkTest.php b/tests/integration/Api/CreateLinkTest.php index d184fe7..e20312c 100644 --- a/tests/integration/Api/CreateLinkTest.php +++ b/tests/integration/Api/CreateLinkTest.php @@ -1,5 +1,14 @@ [ - 'type' => 'links', + 'type' => 'links', 'attributes' => [ - 'title' => 'Facebook', - 'url' => 'https://facebook.com', - 'icon' => 'fab fa-facebook', + 'title' => 'Facebook', + 'url' => 'https://facebook.com', + 'icon' => 'fab fa-facebook', 'visibility' => 'everyone', ], ], @@ -59,6 +68,7 @@ public function payload(): array /** * @test + * * @dataProvider authorizedUsers */ public function authorized_user_cannot_create_link_with_no_data(int $userId) @@ -78,6 +88,7 @@ public function authorized_user_cannot_create_link_with_no_data(int $userId) /** * @test + * * @dataProvider authorizedUsers */ public function authorized_user_can_create_link(int $userId) @@ -85,7 +96,7 @@ public function authorized_user_can_create_link(int $userId) $response = $this->send( $this->request('POST', '/api/links', [ 'authenticatedAs' => $userId, - 'json' => $this->payload(), + 'json' => $this->payload(), ]) ); @@ -111,21 +122,22 @@ public function authorized_user_can_create_link(int $userId) /** * @test + * * @dataProvider unauthorizedUsers */ public function unauthorized_cannot_create_link(?int $userId) { if (!$userId) { $this->extend( - (new Extend\Csrf) + (new Extend\Csrf()) ->exemptRoute('links.create') ); } - + $response = $this->send( $this->request('POST', '/api/links', [ 'authenticatedAs' => $userId, - 'json' => $this->payload(), + 'json' => $this->payload(), ]) ); From 9b1c5ae5b1608a10f6fb05e9d758c01363d313c4 Mon Sep 17 00:00:00 2001 From: IanM Date: Thu, 19 Sep 2024 18:54:52 +0100 Subject: [PATCH 10/32] more tests --- src/Api/Controller/UpdateLinkController.php | 7 + src/Command/EditLinkHandler.php | 4 +- src/Link.php | 2 +- src/LinkValidator.php | 1 + tests/fixtures/LinkUsersTrait.php | 21 +++ tests/integration/Api/CreateLinkTest.php | 93 ++++++++-- tests/integration/Api/DeleteLinkTest.php | 121 +++++++++++++ tests/integration/Api/EditLinkTest.php | 168 +++++++++++++++++++ tests/integration/Api/LinkVisibilityTest.php | 2 +- 9 files changed, 401 insertions(+), 18 deletions(-) create mode 100644 tests/fixtures/LinkUsersTrait.php create mode 100644 tests/integration/Api/DeleteLinkTest.php create mode 100644 tests/integration/Api/EditLinkTest.php diff --git a/src/Api/Controller/UpdateLinkController.php b/src/Api/Controller/UpdateLinkController.php index ac7623d..344c6e6 100755 --- a/src/Api/Controller/UpdateLinkController.php +++ b/src/Api/Controller/UpdateLinkController.php @@ -12,6 +12,7 @@ namespace FoF\Links\Api\Controller; use Flarum\Api\Controller\AbstractShowController; +use Flarum\Foundation\ValidationException; use Flarum\Http\RequestUtil; use FoF\Links\Api\Serializer\LinkSerializer; use FoF\Links\Command\EditLink; @@ -49,6 +50,12 @@ protected function data(ServerRequestInterface $request, Document $document) $actor = RequestUtil::getActor($request); $data = Arr::get($request->getParsedBody(), 'data'); + if (!$data) { + throw new ValidationException([ + 'data' => 'Invalid payload', + ]); + } + return $this->bus->dispatch( new EditLink($id, $actor, $data) ); diff --git a/src/Command/EditLinkHandler.php b/src/Command/EditLinkHandler.php index 9728a39..9ea085c 100755 --- a/src/Command/EditLinkHandler.php +++ b/src/Command/EditLinkHandler.php @@ -95,7 +95,9 @@ public function handle(EditLink $command) $this->validator->assertValid($link->getDirty()); - $link->save(); + if ($link->isDirty()) { + $link->save(); + } return $link; } diff --git a/src/Link.php b/src/Link.php index 58764df..4a871f7 100755 --- a/src/Link.php +++ b/src/Link.php @@ -23,7 +23,6 @@ * @property bool $is_internal * @property bool $is_newtab * @property bool $use_relme - * @property bool $registered_users_only * @property int $parent_id * @property Link $parent * @property string $visibility @@ -43,6 +42,7 @@ class Link extends AbstractModel protected $casts = [ 'is_internal' => 'boolean', 'is_newtab' => 'boolean', + 'use_relme' => 'boolean', ]; /** diff --git a/src/LinkValidator.php b/src/LinkValidator.php index a79ba08..7083b07 100755 --- a/src/LinkValidator.php +++ b/src/LinkValidator.php @@ -22,5 +22,6 @@ class LinkValidator extends AbstractValidator 'title' => ['required', 'string', 'max:50'], 'url' => ['string', 'max:255'], 'icon' => ['string', 'max:100'], + 'visibility' => ['string', 'in:everyone,guests,members'], ]; } diff --git a/tests/fixtures/LinkUsersTrait.php b/tests/fixtures/LinkUsersTrait.php new file mode 100644 index 0000000..100a389 --- /dev/null +++ b/tests/fixtures/LinkUsersTrait.php @@ -0,0 +1,21 @@ +normalUser(), ], 'links' => [ - ['id' => 1, 'title' => 'Google', 'url' => 'https://google.com', 'visibility' => 'everyone'], + ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'visibility' => 'everyone', 'parent_id' => null], ], ]); } - public function authorizedUsers(): array - { - return [ - [1], - ]; - } - - public function unauthorizedUsers(): array + public function payload(): array { return [ - [null], - [2], + 'data' => [ + 'type' => 'links', + 'attributes' => [ + 'title' => 'Facebook', + 'url' => 'https://facebook.com', + 'icon' => 'fab fa-facebook', + 'visibility' => 'everyone', + 'position' => 0, + 'isInternal' => true, + 'isNewtab' => true, + 'useRelMe' => true, + ], + ], ]; } - public function payload(): array + public function minimalPayload(): array { return [ 'data' => [ 'type' => 'links', 'attributes' => [ - 'title' => 'Facebook', - 'url' => 'https://facebook.com', - 'icon' => 'fab fa-facebook', + 'title' => 'Facebook', + 'url' => 'https://facebook.com', + 'icon' => 'fab fa-facebook', 'visibility' => 'everyone', ], ], @@ -76,6 +82,7 @@ public function authorized_user_cannot_create_link_with_no_data(int $userId) $response = $this->send( $this->request('POST', '/api/links', [ 'authenticatedAs' => $userId, + 'json' => [], ]) ); @@ -109,6 +116,12 @@ public function authorized_user_can_create_link(int $userId) $this->assertEquals('Facebook', $response['data']['attributes']['title']); $this->assertEquals('https://facebook.com', $response['data']['attributes']['url']); $this->assertEquals('everyone', $response['data']['attributes']['visibility']); + $this->assertEquals('fab fa-facebook', $response['data']['attributes']['icon']); + $this->assertEquals(0, $response['data']['attributes']['position']); + $this->assertTrue($response['data']['attributes']['isInternal']); + $this->assertTrue($response['data']['attributes']['isNewtab']); + $this->assertTrue($response['data']['attributes']['useRelMe']); + $this->assertFalse($response['data']['attributes']['isChild']); $id = $response['data']['id']; @@ -118,6 +131,56 @@ public function authorized_user_can_create_link(int $userId) $this->assertEquals('Facebook', $link->title); $this->assertEquals('https://facebook.com', $link->url); $this->assertEquals('everyone', $link->visibility); + $this->assertEquals('fab fa-facebook', $link->icon); + $this->assertEquals(0, $link->position); + $this->assertTrue($link->is_internal); + $this->assertTrue($link->is_newtab); + $this->assertTrue($link->use_relme); + } + + /** + * @test + * + * @dataProvider authorizedUsers + */ + public function authorized_user_can_create_link_with_minimal_data(int $userId) + { + $response = $this->send( + $this->request('POST', '/api/links', [ + 'authenticatedAs' => $userId, + 'json' => $this->minimalPayload(), + ]) + ); + + $this->assertEquals(201, $response->getStatusCode()); + + $response = json_decode($response->getBody()->getContents(), true); + + $this->assertArrayHasKey('data', $response); + $this->assertArrayHasKey('id', $response['data']); + $this->assertEquals('Facebook', $response['data']['attributes']['title']); + $this->assertEquals('https://facebook.com', $response['data']['attributes']['url']); + $this->assertEquals('everyone', $response['data']['attributes']['visibility']); + $this->assertEquals('fab fa-facebook', $response['data']['attributes']['icon']); + + $id = $response['data']['id']; + + $link = Link::find($id); + + $this->assertNotNull($link); + $this->assertEquals('Facebook', $link->title); + $this->assertEquals('https://facebook.com', $link->url); + $this->assertEquals('everyone', $link->visibility); + $this->assertEquals('fab fa-facebook', $link->icon); + $this->assertFalse($response['data']['attributes']['isChild']); + + + // check defaults of optional fields + $this->assertFalse($link->is_internal); + $this->assertFalse($link->is_newtab); + $this->assertFalse($link->use_relme); + $this->assertNull($link->parent_id); + $this->assertNull($link->position); } /** diff --git a/tests/integration/Api/DeleteLinkTest.php b/tests/integration/Api/DeleteLinkTest.php new file mode 100644 index 0000000..a5b3007 --- /dev/null +++ b/tests/integration/Api/DeleteLinkTest.php @@ -0,0 +1,121 @@ +extension('fof-links'); + + $this->prepareDatabase([ + 'users' => [ + $this->normalUser(), + ], + 'links' => [ + ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'visibility' => 'everyone', 'parent_id' => null], + ], + ]); + } + + /** + * @test + * + * @dataProvider authorizedUsers + */ + public function authorized_user_can_delete_link(int $userId) + { + $response = $this->send( + $this->request('DELETE', '/api/links/1', [ + 'authenticatedAs' => $userId, + ]) + ); + + $this->assertEquals(204, $response->getStatusCode()); + + $this->assertNull(Link::find(1)); + $this->assertEquals(0, Link::count()); + } + + /** + * @test + * + * @dataProvider authorizedUsers + */ + public function authorized_user_cannot_delete_nonexistent_link(int $userId) + { + $response = $this->send( + $this->request('DELETE', '/api/links/2', [ + 'authenticatedAs' => $userId, + ]) + ); + + $this->assertEquals(404, $response->getStatusCode()); + + $this->assertNull(Link::find(2)); + $this->assertEquals(1, Link::count()); + } + + /** + * @test + * + * @dataProvider unauthorizedUsers + */ + public function unauthorized_user_cannot_delete_link(?int $userId) + { + if (!$userId) { + $this->extend( + (new Extend\Csrf()) + ->exemptRoute('links.delete') + ); + } + + $response = $this->send( + $this->request('DELETE', '/api/links/1', [ + 'authenticatedAs' => $userId, + ]) + ); + + $this->assertEquals(403, $response->getStatusCode()); + + $this->assertNotNull(Link::find(1)); + $this->assertEquals(1, Link::count()); + } + + /** + * @test + * + * @dataProvider unauthorizedUsers + */ + public function unauthorized_user_cannot_delete_nonexistent_link(?int $userId) + { + if (!$userId) { + $this->extend( + (new Extend\Csrf()) + ->exemptRoute('links.delete') + ); + } + + $response = $this->send( + $this->request('DELETE', '/api/links/2', [ + 'authenticatedAs' => $userId, + ]) + ); + + $this->assertEquals(404, $response->getStatusCode()); + + $this->assertNull(Link::find(2)); + $this->assertEquals(1, Link::count()); + } +} diff --git a/tests/integration/Api/EditLinkTest.php b/tests/integration/Api/EditLinkTest.php new file mode 100644 index 0000000..0fd75a4 --- /dev/null +++ b/tests/integration/Api/EditLinkTest.php @@ -0,0 +1,168 @@ +extension('fof-links'); + + $this->prepareDatabase([ + 'users' => [ + $this->normalUser(), + ], + 'links' => [ + ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'visibility' => 'everyone', 'parent_id' => null], + ], + ]); + } + + /** + * @test + * + * @dataProvider authorizedUsers + */ + public function authorized_user_can_edit_link(int $userId) + { + $response = $this->send( + $this->request('PATCH', '/api/links/1', [ + 'authenticatedAs' => $userId, + 'json' => [ + 'data' => [ + 'type' => 'links', + 'id' => '1', + 'attributes' => [ + 'title' => 'Facebook', + 'url' => 'https://facebook.com', + 'icon' => 'fab fa-facebook', + 'visibility' => 'members', + ], + ], + ], + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $link = json_decode($response->getBody(), true)['data']; + + $this->assertEquals('Facebook', $link['attributes']['title']); + $this->assertEquals('https://facebook.com', $link['attributes']['url']); + $this->assertEquals('fab fa-facebook', $link['attributes']['icon']); + $this->assertEquals('members', $link['attributes']['visibility']); + + $link = Link::find(1); + + $this->assertEquals('Facebook', $link->title); + $this->assertEquals('https://facebook.com', $link->url); + $this->assertEquals('fab fa-facebook', $link->icon); + $this->assertEquals('members', $link->visibility); + } + + /** + * @test + * + * @dataProvider authorizedUsers + */ + public function authorized_user_cannot_edit_link_with_no_data(int $userId) + { + $response = $this->send( + $this->request('PATCH', '/api/links/1', [ + 'authenticatedAs' => $userId, + 'json' => [], + ]) + ); + + $this->assertEquals(422, $response->getStatusCode()); + + $response = json_decode($response->getBody()->getContents(), true); + + $this->assertArrayHasKey('errors', $response); + $this->assertCount(1, $response['errors']); + } + + /** + * @test + * + * @dataProvider authorizedUsers + */ + public function authorized_user_cannot_edit_nonexistent_link(int $userId) + { + $response = $this->send( + $this->request('PATCH', '/api/links/2', [ + 'authenticatedAs' => $userId, + 'json' => [ + 'data' => [ + 'type' => 'links', + 'id' => '2', + 'attributes' => [ + 'title' => 'Facebook', + 'url' => 'https://facebook.com', + 'icon' => 'fab fa-facebook', + 'visibility' => 'members', + ], + ], + ], + ]) + ); + + $this->assertEquals(404, $response->getStatusCode()); + + $this->assertNull(Link::find(2)); + $this->assertEquals(1, Link::count()); + } + + /** + * @test + * + * @dataProvider unauthorizedUsers + */ + public function unauthorized_user_cannot_edit_link(?int $userId) + { + if (!$userId) { + $this->extend( + (new Extend\Csrf()) + ->exemptRoute('links.update') + ); + } + + $response = $this->send( + $this->request('PATCH', '/api/links/1', [ + 'authenticatedAs' => $userId, + 'json' => [ + 'data' => [ + 'type' => 'links', + 'id' => '1', + 'attributes' => [ + 'title' => 'Facebook', + 'url' => 'https://facebook.com', + 'icon' => 'fab fa-facebook', + 'visibility' => 'members', + ], + ], + ], + ]) + ); + + $this->assertEquals(403, $response->getStatusCode()); + + $link = Link::find(1); + + $this->assertEquals('Google', $link->title); + $this->assertEquals('https://google.com', $link->url); + $this->assertEquals('fab fa-google', $link->icon); + $this->assertEquals('everyone', $link->visibility); + } +} diff --git a/tests/integration/Api/LinkVisibilityTest.php b/tests/integration/Api/LinkVisibilityTest.php index 8f40be4..48e010d 100644 --- a/tests/integration/Api/LinkVisibilityTest.php +++ b/tests/integration/Api/LinkVisibilityTest.php @@ -30,7 +30,7 @@ public function setUp(): void $this->normalUser(), ], 'links' => [ - ['id' => 1, 'title' => 'Google', 'url' => 'https://google.com', 'visibility' => 'everyone'], + ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'visibility' => 'everyone', 'parent_id' => null], ['id' => 2, 'title' => 'Facebook', 'url' => 'https://facebook.com', 'visibility' => 'guests'], ['id' => 3, 'title' => 'Twitter', 'url' => 'https://twitter.com', 'visibility' => 'members'], ['id' => 4, 'title' => 'Reddit', 'url' => 'https://reddit.com', 'visibility' => 'members'], From e4b3c47a5b18fa1478233d858e8b8fb2cee2d860 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Thu, 19 Sep 2024 17:55:01 +0000 Subject: [PATCH 11/32] Apply fixes from StyleCI --- src/LinkValidator.php | 6 +++--- tests/fixtures/LinkUsersTrait.php | 11 ++++++++++- tests/integration/Api/CreateLinkTest.php | 11 +++++------ tests/integration/Api/DeleteLinkTest.php | 9 +++++++++ tests/integration/Api/EditLinkTest.php | 17 +++++++++++++---- 5 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/LinkValidator.php b/src/LinkValidator.php index 7083b07..93b77ad 100755 --- a/src/LinkValidator.php +++ b/src/LinkValidator.php @@ -19,9 +19,9 @@ class LinkValidator extends AbstractValidator * {@inheritdoc} */ protected $rules = [ - 'title' => ['required', 'string', 'max:50'], - 'url' => ['string', 'max:255'], - 'icon' => ['string', 'max:100'], + 'title' => ['required', 'string', 'max:50'], + 'url' => ['string', 'max:255'], + 'icon' => ['string', 'max:100'], 'visibility' => ['string', 'in:everyone,guests,members'], ]; } diff --git a/tests/fixtures/LinkUsersTrait.php b/tests/fixtures/LinkUsersTrait.php index 100a389..7e65be9 100644 --- a/tests/fixtures/LinkUsersTrait.php +++ b/tests/fixtures/LinkUsersTrait.php @@ -1,5 +1,14 @@ 'everyone', 'position' => 0, 'isInternal' => true, - 'isNewtab' => true, - 'useRelMe' => true, + 'isNewtab' => true, + 'useRelMe' => true, ], ], ]; @@ -63,9 +63,9 @@ public function minimalPayload(): array 'data' => [ 'type' => 'links', 'attributes' => [ - 'title' => 'Facebook', - 'url' => 'https://facebook.com', - 'icon' => 'fab fa-facebook', + 'title' => 'Facebook', + 'url' => 'https://facebook.com', + 'icon' => 'fab fa-facebook', 'visibility' => 'everyone', ], ], @@ -174,7 +174,6 @@ public function authorized_user_can_create_link_with_minimal_data(int $userId) $this->assertEquals('fab fa-facebook', $link->icon); $this->assertFalse($response['data']['attributes']['isChild']); - // check defaults of optional fields $this->assertFalse($link->is_internal); $this->assertFalse($link->is_newtab); diff --git a/tests/integration/Api/DeleteLinkTest.php b/tests/integration/Api/DeleteLinkTest.php index a5b3007..63c4139 100644 --- a/tests/integration/Api/DeleteLinkTest.php +++ b/tests/integration/Api/DeleteLinkTest.php @@ -1,5 +1,14 @@ send( $this->request('PATCH', '/api/links/1', [ 'authenticatedAs' => $userId, - 'json' => [ + 'json' => [ 'data' => [ 'type' => 'links', 'id' => '1', @@ -81,7 +90,7 @@ public function authorized_user_cannot_edit_link_with_no_data(int $userId) $response = $this->send( $this->request('PATCH', '/api/links/1', [ 'authenticatedAs' => $userId, - 'json' => [], + 'json' => [], ]) ); @@ -103,7 +112,7 @@ public function authorized_user_cannot_edit_nonexistent_link(int $userId) $response = $this->send( $this->request('PATCH', '/api/links/2', [ 'authenticatedAs' => $userId, - 'json' => [ + 'json' => [ 'data' => [ 'type' => 'links', 'id' => '2', @@ -141,7 +150,7 @@ public function unauthorized_user_cannot_edit_link(?int $userId) $response = $this->send( $this->request('PATCH', '/api/links/1', [ 'authenticatedAs' => $userId, - 'json' => [ + 'json' => [ 'data' => [ 'type' => 'links', 'id' => '1', From 4b75ecc9048d1aee245e55d661f9bd8b857fc37d Mon Sep 17 00:00:00 2001 From: IanM Date: Thu, 19 Sep 2024 18:56:36 +0100 Subject: [PATCH 12/32] add minimal db entry --- tests/integration/Api/CreateLinkTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/Api/CreateLinkTest.php b/tests/integration/Api/CreateLinkTest.php index 15739da..100cde9 100644 --- a/tests/integration/Api/CreateLinkTest.php +++ b/tests/integration/Api/CreateLinkTest.php @@ -34,6 +34,7 @@ public function setUp(): void ], 'links' => [ ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'visibility' => 'everyone', 'parent_id' => null], + ['id' => 2, 'title' => 'Minimal'], ], ]); } From 8b4044bbecf87f704923b9e55cd16fa8c28abbc8 Mon Sep 17 00:00:00 2001 From: IanM Date: Thu, 10 Oct 2024 10:51:08 +0100 Subject: [PATCH 13/32] wip --- js/src/admin/components/EditLinkModal.js | 27 ++++++++++++++++++++++++ src/Access/LinkPolicy.php | 14 ++++++++++-- src/Access/ScopeLinkVisibility.php | 4 ++++ 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/js/src/admin/components/EditLinkModal.js b/js/src/admin/components/EditLinkModal.js index d9728f1..33a730b 100755 --- a/js/src/admin/components/EditLinkModal.js +++ b/js/src/admin/components/EditLinkModal.js @@ -9,6 +9,7 @@ import icon from 'flarum/common/helpers/icon'; import withAttr from 'flarum/common/utils/withAttr'; import ItemList from 'flarum/common/utils/ItemList'; import Select from 'flarum/common/components/Select'; +import PermissionDropdown from 'flarum/admin/components/PermissionDropdown'; /** * The `EditlinksModal` component shows a modal dialog which allows the user @@ -185,6 +186,32 @@ export default class EditlinksModal extends Modal { 20 ); + const permissionPriority = 200; + if (this.link.exists) { + items.add( + 'visibility-permission', + [ +
+ +

{app.translator.trans('fof-links.admin.edit_link.permission.help')}

+ +
, + ], + permissionPriority + ); + } else { + items.add( + 'visibility-permission-disabled', + [ +
+ +

{app.translator.trans('fof-links.admin.edit_link.permission.help-disabled')}

+
+ ], + permissionPriority + ); + } + items.add( 'actions', [ diff --git a/src/Access/LinkPolicy.php b/src/Access/LinkPolicy.php index 79f3b3d..286efa5 100644 --- a/src/Access/LinkPolicy.php +++ b/src/Access/LinkPolicy.php @@ -11,12 +11,22 @@ namespace FoF\Links\Access; +use Flarum\User\Access\AbstractPolicy; use Flarum\User\User; use FoF\Links\Link; -class LinkPolicy +class LinkPolicy extends AbstractPolicy { - public function see(User $actor, Link $link) + public function can(User $actor, string $ability, Link $link) { + if ($link->parent_id !== null && ! $actor->can($ability, $link->parent)) { + return $this->deny(); + } + + if ($link->is_restricted) { + $id = $link->id; + + return $actor->hasPermission("link$id.$ability"); + } } } diff --git a/src/Access/ScopeLinkVisibility.php b/src/Access/ScopeLinkVisibility.php index 24abcec..7014ead 100644 --- a/src/Access/ScopeLinkVisibility.php +++ b/src/Access/ScopeLinkVisibility.php @@ -12,6 +12,7 @@ namespace FoF\Links\Access; use Flarum\User\User; +use FoF\Links\Link; use Illuminate\Database\Eloquent\Builder; class ScopeLinkVisibility @@ -31,5 +32,8 @@ public function __invoke(User $actor, Builder $query) $query ->orWhere('visibility', 'members'); } + // $query->whereIn('id', function ($query) use ($actor) { + // Link::query()->setQuery($query->from('links'))->whereHasPermission($actor, 'view')->select('links.id'); + // }); } } From 46cb7b21cc0157158ec6f68fc3ddb354d6c8062a Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Thu, 10 Oct 2024 09:51:22 +0000 Subject: [PATCH 14/32] Apply fixes from StyleCI --- src/Access/LinkPolicy.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Access/LinkPolicy.php b/src/Access/LinkPolicy.php index 286efa5..07e939c 100644 --- a/src/Access/LinkPolicy.php +++ b/src/Access/LinkPolicy.php @@ -19,7 +19,7 @@ class LinkPolicy extends AbstractPolicy { public function can(User $actor, string $ability, Link $link) { - if ($link->parent_id !== null && ! $actor->can($ability, $link->parent)) { + if ($link->parent_id !== null && !$actor->can($ability, $link->parent)) { return $this->deny(); } From bab506a0f3560ca3b49e4f325665bb299a6cf3ec Mon Sep 17 00:00:00 2001 From: IanM Date: Thu, 10 Oct 2024 10:54:58 +0100 Subject: [PATCH 15/32] format --- js/src/admin/components/EditLinkModal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/admin/components/EditLinkModal.js b/js/src/admin/components/EditLinkModal.js index 33a730b..40e3b71 100755 --- a/js/src/admin/components/EditLinkModal.js +++ b/js/src/admin/components/EditLinkModal.js @@ -206,7 +206,7 @@ export default class EditlinksModal extends Modal {

{app.translator.trans('fof-links.admin.edit_link.permission.help-disabled')}

-
+ , ], permissionPriority ); From f51f7bd79f19c405932e1e2d49c420094f91d49e Mon Sep 17 00:00:00 2001 From: IanM Date: Thu, 10 Oct 2024 15:34:12 +0100 Subject: [PATCH 16/32] seems to be working --- extend.php | 8 +- js/src/common/models/Link.ts | 4 + ...00000-add_is_restricted_to_links_table.php | 7 ++ src/Access/LinkPolicy.php | 19 +++- src/Access/ScopeLinkVisibility.php | 28 +++--- .../Controller/SetPermissionController.php | 59 ++++++++++++ src/Api/Serializer/LinkSerializer.php | 8 +- src/Event/PermissionChanged.php | 27 ++++++ src/Link.php | 92 +++++++++++++++++++ src/LinkRepository.php | 3 +- src/Listener/LinkPermissionChanged.php | 41 +++++++++ tests/integration/Api/LinkVisibilityTest.php | 33 +++++-- 12 files changed, 305 insertions(+), 24 deletions(-) create mode 100644 migrations/2024_10_10_000000-add_is_restricted_to_links_table.php create mode 100644 src/Api/Controller/SetPermissionController.php create mode 100644 src/Event/PermissionChanged.php create mode 100644 src/Listener/LinkPermissionChanged.php diff --git a/extend.php b/extend.php index 90b68e9..29635da 100755 --- a/extend.php +++ b/extend.php @@ -16,6 +16,7 @@ use Flarum\Extend; use FoF\Links\Api\Controller; use FoF\Links\Api\Serializer\LinkSerializer; +use FoF\Links\Event\PermissionChanged; return [ new Extend\Locales(__DIR__.'/locale'), @@ -32,11 +33,16 @@ ->post('/links', 'links.create', Controller\CreateLinkController::class) ->post('/links/order', 'links.order', Controller\OrderLinksController::class) ->patch('/links/{id}', 'links.update', Controller\UpdateLinkController::class) - ->delete('/links/{id}', 'links.delete', Controller\DeleteLinkController::class), + ->delete('/links/{id}', 'links.delete', Controller\DeleteLinkController::class) + ->remove('permission') + ->post('/permission', 'permission', Controller\SetPermissionController::class), (new Extend\ApiSerializer(ForumSerializer::class)) ->hasMany('links', LinkSerializer::class), + (new Extend\Event()) + ->listen(PermissionChanged::class, Listener\LinkPermissionChanged::class), + (new Extend\ApiController(ShowForumController::class)) ->addInclude(['links', 'links.parent']) ->prepareDataForSerialization(LoadForumLinksRelationship::class), diff --git a/js/src/common/models/Link.ts b/js/src/common/models/Link.ts index 6483a9e..581ee8b 100644 --- a/js/src/common/models/Link.ts +++ b/js/src/common/models/Link.ts @@ -44,4 +44,8 @@ export default class Link extends Model { visibility() { return Model.attribute('visibility').call(this); } + + isRestricted() { + return Model.attribute('isRestricted').call(this); + } } diff --git a/migrations/2024_10_10_000000-add_is_restricted_to_links_table.php b/migrations/2024_10_10_000000-add_is_restricted_to_links_table.php new file mode 100644 index 0000000..ac9fb18 --- /dev/null +++ b/migrations/2024_10_10_000000-add_is_restricted_to_links_table.php @@ -0,0 +1,7 @@ + ['boolean', 'default' => 0], +]); diff --git a/src/Access/LinkPolicy.php b/src/Access/LinkPolicy.php index 07e939c..deff264 100644 --- a/src/Access/LinkPolicy.php +++ b/src/Access/LinkPolicy.php @@ -17,16 +17,29 @@ class LinkPolicy extends AbstractPolicy { - public function can(User $actor, string $ability, Link $link) + // public function can(User $actor, string $ability, Link $link) + // { + // if ($link->parent_id !== null && !$actor->can($ability, $link->parent)) { + // return $this->deny(); + // } + + // if ($link->is_restricted) { + // $id = $link->id; + + // return $actor->hasPermission("link$id.$ability"); + // } + // } + + public function view(User $actor, Link $link) { - if ($link->parent_id !== null && !$actor->can($ability, $link->parent)) { + if ($link->parent_id !== null && !$actor->can('view', $link->parent)) { return $this->deny(); } if ($link->is_restricted) { $id = $link->id; - return $actor->hasPermission("link$id.$ability"); + return $actor->hasPermission("link$id.view"); } } } diff --git a/src/Access/ScopeLinkVisibility.php b/src/Access/ScopeLinkVisibility.php index 7014ead..9f0d0f8 100644 --- a/src/Access/ScopeLinkVisibility.php +++ b/src/Access/ScopeLinkVisibility.php @@ -14,6 +14,7 @@ use Flarum\User\User; use FoF\Links\Link; use Illuminate\Database\Eloquent\Builder; +use Psr\Log\LoggerInterface; class ScopeLinkVisibility { @@ -23,17 +24,22 @@ class ScopeLinkVisibility */ public function __invoke(User $actor, Builder $query) { - $query->where('visibility', 'everyone'); + $logger = resolve(LoggerInterface::class); + // $query->where('visibility', 'everyone'); - if ($actor->isGuest()) { - $query - ->orWhere('visibility', 'guests'); - } else { - $query - ->orWhere('visibility', 'members'); - } - // $query->whereIn('id', function ($query) use ($actor) { - // Link::query()->setQuery($query->from('links'))->whereHasPermission($actor, 'view')->select('links.id'); - // }); + // if ($actor->isGuest()) { + // $query + // ->orWhere('visibility', 'guests'); + // } else { + // $query + // ->orWhere('visibility', 'members'); + // } + $query->whereIn('id', function ($query) use ($actor) { + Link::query() + ->setQuery($query->from('links')) + ->whereHasPermission($actor, 'view') + ->select('links.id'); + }); + //$logger->info('ScopeLinkVisibility', ['actor' => $actor->id, 'query' => $query->toSql()]); } } diff --git a/src/Api/Controller/SetPermissionController.php b/src/Api/Controller/SetPermissionController.php new file mode 100644 index 0000000..c8a2441 --- /dev/null +++ b/src/Api/Controller/SetPermissionController.php @@ -0,0 +1,59 @@ +events = $events; + } + + /** + * {@inheritdoc} + */ + public function handle(ServerRequestInterface $request): ResponseInterface + { + $actor = RequestUtil::getActor($request); + $actor->assertAdmin(); + + $body = $request->getParsedBody(); + $permission = Arr::get($body, 'permission'); + $groupIds = Arr::get($body, 'groupIds'); + + Permission::where('permission', $permission)->delete(); + + Permission::insert(array_map(function ($groupId) use ($permission) { + return [ + 'permission' => $permission, + 'group_id' => $groupId + ]; + }, $groupIds)); + + $this->events->dispatch(new PermissionChanged($permission, $actor, $body)); + + return new EmptyResponse(204); + } +} diff --git a/src/Api/Serializer/LinkSerializer.php b/src/Api/Serializer/LinkSerializer.php index 99180e1..83875aa 100755 --- a/src/Api/Serializer/LinkSerializer.php +++ b/src/Api/Serializer/LinkSerializer.php @@ -25,7 +25,7 @@ class LinkSerializer extends AbstractSerializer */ protected function getDefaultAttributes($link) { - return [ + $attributes = [ 'id' => $link->id, 'title' => $link->title, 'icon' => $link->icon, @@ -37,6 +37,12 @@ protected function getDefaultAttributes($link) 'isChild' => (bool) $link->parent_id, 'visibility' => $link->visibility, ]; + + if ($this->actor->isAdmin()) { + $attributes['isRestricted'] = (bool) $link->is_restricted; + } + + return $attributes; } /** diff --git a/src/Event/PermissionChanged.php b/src/Event/PermissionChanged.php new file mode 100644 index 0000000..9704e8a --- /dev/null +++ b/src/Event/PermissionChanged.php @@ -0,0 +1,27 @@ +permission = $permission; + $this->actor = $actor; + $this->data = $data; + } +} diff --git a/src/Link.php b/src/Link.php index 4a871f7..d3ddedd 100755 --- a/src/Link.php +++ b/src/Link.php @@ -13,6 +13,9 @@ use Flarum\Database\AbstractModel; use Flarum\Database\ScopeVisibilityTrait; +use Flarum\Group\Permission; +use Flarum\User\User; +use Illuminate\Database\Eloquent\Builder; /** * @property int $id @@ -26,6 +29,7 @@ * @property int $parent_id * @property Link $parent * @property string $visibility + * @property bool $is_restricted */ class Link extends AbstractModel { @@ -43,8 +47,24 @@ class Link extends AbstractModel 'is_internal' => 'boolean', 'is_newtab' => 'boolean', 'use_relme' => 'boolean', + 'is_restricted' => 'boolean', ]; + public static function boot() + { + parent::boot(); + + static::saved(function (self $link) { + if ($link->wasUnrestricted()) { + //$link->deletePermissions(); + } + }); + + static::deleted(function (self $link) { + $link->deletePermissions(); + }); + } + /** * Create a new link. * @@ -93,4 +113,76 @@ public function delete() return $result; } + + public function scopeWhereHasPermission(Builder $query, User $user, string $currPermission): Builder + { + $isAdmin = $user->isAdmin(); + $allPermissions = $user->getPermissions(); + $linkIdsWithPermission = collect($allPermissions) + ->filter(function ($permission) use ($currPermission) { + return substr($permission, 0, 4) === 'link' && strpos($permission, $currPermission) !== false; + }) + ->map(function ($permission) { + $scopeFragment = explode('.', $permission, 2)[0]; + + return substr($scopeFragment, 4); + }) + ->values(); + + resolve('log')->info('linkIdsWithPermission', $linkIdsWithPermission->toArray()); + + return $query + ->where(function ($query) use ($isAdmin, $linkIdsWithPermission) { + $query + ->whereIn('links.id', function ($query) use ($isAdmin, $linkIdsWithPermission) { + static::buildPermissionSubquery($query, $isAdmin, $linkIdsWithPermission); + }) + ->where(function ($query) use ($isAdmin, $linkIdsWithPermission) { + $query + ->whereIn('links.parent_id', function ($query) use ($isAdmin, $linkIdsWithPermission) { + static::buildPermissionSubquery($query, $isAdmin, $linkIdsWithPermission); + }) + ->orWhere('links.parent_id', null); + }); + }); + } + + protected static function buildPermissionSubquery($base, $isAdmin, $linkIdsWithPermission) + { + $base + ->from('links as perm_links') + ->select('perm_links.id'); + + // This needs to be a special case, as `linkIdsWithPermissions` + // won't include admin perms (which are all perms by default). + if ($isAdmin) { + return; + } + + $base->where(function ($query) use ($linkIdsWithPermission) { + $query + ->where('perm_links.is_restricted', true) + ->whereIn('perm_links.id', $linkIdsWithPermission); + }); + + $base->orWhere('perm_links.is_restricted', false); + } + + /** + * Has this link been unrestricted recently? + * + * @return bool + */ + public function wasUnrestricted() + { + return ! $this->is_restricted && $this->wasChanged('is_restricted'); + } + + /** + * Delete all permissions belonging to this link. + */ + public function deletePermissions() + { + Permission::where('permission', 'like', "link{$this->id}.%")->delete(); + } } diff --git a/src/LinkRepository.php b/src/LinkRepository.php index b67214f..cf2fba4 100755 --- a/src/LinkRepository.php +++ b/src/LinkRepository.php @@ -120,7 +120,8 @@ public function cacheKey(User $actor): string */ public function getLinks(User $actor): Collection { - return $actor->isGuest() ? $this->getGuestLinks($actor) : $this->getLinksFromDatabase($actor); + return $this->getLinksFromDatabase($actor); + //return $actor->isGuest() ? $this->getGuestLinks($actor) : $this->getLinksFromDatabase($actor); } /** diff --git a/src/Listener/LinkPermissionChanged.php b/src/Listener/LinkPermissionChanged.php new file mode 100644 index 0000000..cd0ffaf --- /dev/null +++ b/src/Listener/LinkPermissionChanged.php @@ -0,0 +1,41 @@ +permission; + $groupIds = Arr::get($event->data, 'groupIds'); + + // If the permission is of the format `link.` followed by a number, ending with `.view`, + // then we are interested in it. + // Extract the link ID from the permission name. + if (preg_match('/^link\.(\d+)\.view$/', $permission, $matches)) { + $linkId = $matches[1]; + $link = Link::findOrFail($linkId); + + if ($this->isGuestPermission($groupIds)) { + $link->is_restricted = false; + } else { + $link->is_restricted = true; + } + + if ($link->isDirty('is_restricted')) { + $link->save(); + } + } + } + + protected function isGuestPermission(array $groups): bool + { + // If the array contains the value of the guest group ID, then the permission is for guests. + return in_array(Group::GUEST_ID, $groups); + } +} diff --git a/tests/integration/Api/LinkVisibilityTest.php b/tests/integration/Api/LinkVisibilityTest.php index 48e010d..d3cf100 100644 --- a/tests/integration/Api/LinkVisibilityTest.php +++ b/tests/integration/Api/LinkVisibilityTest.php @@ -11,6 +11,7 @@ namespace FoF\Links\Tests\integration\Api; +use Flarum\Group\Group; use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; use FoF\Links\Link; @@ -28,22 +29,40 @@ public function setUp(): void $this->prepareDatabase([ 'users' => [ $this->normalUser(), + ['id' => 3, 'username' => 'moderator', 'email' => 'mod@machine.local', 'is_email_confirmed' => true], + ['id' => 4, 'username' => 'evelated', 'email' => 'elevated@machine.local', 'is_email_confirmed' => true], ], 'links' => [ - ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'visibility' => 'everyone', 'parent_id' => null], - ['id' => 2, 'title' => 'Facebook', 'url' => 'https://facebook.com', 'visibility' => 'guests'], - ['id' => 3, 'title' => 'Twitter', 'url' => 'https://twitter.com', 'visibility' => 'members'], - ['id' => 4, 'title' => 'Reddit', 'url' => 'https://reddit.com', 'visibility' => 'members'], + ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'parent_id' => null, 'is_restricted' => false], + ['id' => 2, 'title' => 'Facebook', 'url' => 'https://facebook.com', 'is_restricted' => true], + ['id' => 3, 'title' => 'Twitter', 'url' => 'https://twitter.com', 'is_restricted' => true], + ['id' => 4, 'title' => 'Reddit', 'url' => 'https://reddit.com', 'is_restricted' => true], ], + 'groups' => [ + ['id' => 5, 'name_singular' => 'FooBar', 'name_plural' => 'FooBars'], + ], + 'group_user' => [ + ['user_id' => 3, 'group_id' => Group::MODERATOR_ID], + ['user_id' => 4, 'group_id' => 5], + ], + 'group_permission' => [ + ['permission' => 'link1.view', 'group_id' => Group::GUEST_ID], + ['permission' => 'link2.view', 'group_id' => 5], + ['permission' => 'link3.view', 'group_id' => Group::MEMBER_ID], + ['permission' => 'link4.view', 'group_id' => Group::MODERATOR_ID], + ['permission' => 'link4.view', 'group_id' => 5], + ] ]); } public function forumUsersDataProvider(): array { return [ - [null, [1, 2]], - [1, [1, 3, 4]], - [2, [1, 3, 4]], + [null, [1]], + [1, [1, 2, 3, 4]], + [2, [1, 3]], + [3, [1, 3, 4]], + [4, [1, 2, 3, 4]], ]; } From d7484b1f1dfdd2026e576f1d9b37998fdc3b32ee Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Thu, 10 Oct 2024 14:34:22 +0000 Subject: [PATCH 17/32] Apply fixes from StyleCI --- ..._10_000000-add_is_restricted_to_links_table.php | 9 +++++++++ src/Api/Controller/SetPermissionController.php | 14 ++++++++------ src/Event/PermissionChanged.php | 11 ++++++++++- src/Link.php | 4 ++-- src/Listener/LinkPermissionChanged.php | 11 ++++++++++- tests/integration/Api/LinkVisibilityTest.php | 2 +- 6 files changed, 40 insertions(+), 11 deletions(-) diff --git a/migrations/2024_10_10_000000-add_is_restricted_to_links_table.php b/migrations/2024_10_10_000000-add_is_restricted_to_links_table.php index ac9fb18..ad19353 100644 --- a/migrations/2024_10_10_000000-add_is_restricted_to_links_table.php +++ b/migrations/2024_10_10_000000-add_is_restricted_to_links_table.php @@ -1,5 +1,14 @@ events = $events; } - + /** * {@inheritdoc} */ @@ -48,7 +50,7 @@ public function handle(ServerRequestInterface $request): ResponseInterface Permission::insert(array_map(function ($groupId) use ($permission) { return [ 'permission' => $permission, - 'group_id' => $groupId + 'group_id' => $groupId, ]; }, $groupIds)); diff --git a/src/Event/PermissionChanged.php b/src/Event/PermissionChanged.php index 9704e8a..bca1dec 100644 --- a/src/Event/PermissionChanged.php +++ b/src/Event/PermissionChanged.php @@ -1,5 +1,14 @@ permission = $permission; diff --git a/src/Link.php b/src/Link.php index d3ddedd..5ff8ac1 100755 --- a/src/Link.php +++ b/src/Link.php @@ -29,7 +29,7 @@ * @property int $parent_id * @property Link $parent * @property string $visibility - * @property bool $is_restricted + * @property bool $is_restricted */ class Link extends AbstractModel { @@ -175,7 +175,7 @@ protected static function buildPermissionSubquery($base, $isAdmin, $linkIdsWithP */ public function wasUnrestricted() { - return ! $this->is_restricted && $this->wasChanged('is_restricted'); + return !$this->is_restricted && $this->wasChanged('is_restricted'); } /** diff --git a/src/Listener/LinkPermissionChanged.php b/src/Listener/LinkPermissionChanged.php index cd0ffaf..3ab4a3b 100644 --- a/src/Listener/LinkPermissionChanged.php +++ b/src/Listener/LinkPermissionChanged.php @@ -1,5 +1,14 @@ isGuestPermission($groupIds)) { $link->is_restricted = false; } else { diff --git a/tests/integration/Api/LinkVisibilityTest.php b/tests/integration/Api/LinkVisibilityTest.php index d3cf100..5dbe4eb 100644 --- a/tests/integration/Api/LinkVisibilityTest.php +++ b/tests/integration/Api/LinkVisibilityTest.php @@ -51,7 +51,7 @@ public function setUp(): void ['permission' => 'link3.view', 'group_id' => Group::MEMBER_ID], ['permission' => 'link4.view', 'group_id' => Group::MODERATOR_ID], ['permission' => 'link4.view', 'group_id' => 5], - ] + ], ]); } From c02e5379683136d732e021c5f8c8d45a867e5a58 Mon Sep 17 00:00:00 2001 From: IanM Date: Fri, 11 Oct 2024 10:14:38 +0100 Subject: [PATCH 18/32] now working correctly --- js/src/admin/components/EditLinkModal.js | 8 ++--- js/src/forum/extendHeader.tsx | 6 ++++ src/Link.php | 2 -- src/Listener/LinkPermissionChanged.php | 4 ++- tests/integration/Api/LinkVisibilityTest.php | 35 ++++++++++++++++---- 5 files changed, 42 insertions(+), 13 deletions(-) diff --git a/js/src/admin/components/EditLinkModal.js b/js/src/admin/components/EditLinkModal.js index 40e3b71..82e739c 100755 --- a/js/src/admin/components/EditLinkModal.js +++ b/js/src/admin/components/EditLinkModal.js @@ -192,8 +192,8 @@ export default class EditlinksModal extends Modal { 'visibility-permission', [
- -

{app.translator.trans('fof-links.admin.edit_link.permission.help')}

+ +

{app.translator.trans('fof-links.admin.edit_link.visibility.help')}

, ], @@ -204,8 +204,8 @@ export default class EditlinksModal extends Modal { 'visibility-permission-disabled', [
- -

{app.translator.trans('fof-links.admin.edit_link.permission.help-disabled')}

+ +

{app.translator.trans('fof-links.admin.edit_link.visibility.help-disabled')}

, ], permissionPriority diff --git a/js/src/forum/extendHeader.tsx b/js/src/forum/extendHeader.tsx index 8dc39c3..df4b243 100644 --- a/js/src/forum/extendHeader.tsx +++ b/js/src/forum/extendHeader.tsx @@ -12,9 +12,15 @@ export default function extendHeader() { extend(HeaderPrimary.prototype, 'items', function (items: ItemList) { const allLinks = app.store.all('links'); const links = allLinks.filter((link) => !link.isChild()); + const addLink = (parent: Link | null | undefined) => { const hasChildren = allLinks.some((link) => link.parent() == parent); + // If the link has no URL and no children, do not display it. + if (!parent?.url() && !hasChildren) { + return; + } + items.add(`link${parent?.id()}`, hasChildren ? LinkDropdown.component({ link: parent }) : LinkItem.component({ link: parent })); }; diff --git a/src/Link.php b/src/Link.php index 5ff8ac1..cb836fa 100755 --- a/src/Link.php +++ b/src/Link.php @@ -129,8 +129,6 @@ public function scopeWhereHasPermission(Builder $query, User $user, string $curr }) ->values(); - resolve('log')->info('linkIdsWithPermission', $linkIdsWithPermission->toArray()); - return $query ->where(function ($query) use ($isAdmin, $linkIdsWithPermission) { $query diff --git a/src/Listener/LinkPermissionChanged.php b/src/Listener/LinkPermissionChanged.php index 3ab4a3b..f48776d 100644 --- a/src/Listener/LinkPermissionChanged.php +++ b/src/Listener/LinkPermissionChanged.php @@ -26,7 +26,9 @@ public function handle(PermissionChanged $event) // If the permission is of the format `link.` followed by a number, ending with `.view`, // then we are interested in it. // Extract the link ID from the permission name. - if (preg_match('/^link\.(\d+)\.view$/', $permission, $matches)) { + // Example permission name: `link1.view` + + if (preg_match('/^link(\d+)\.view$/', $permission, $matches)) { $linkId = $matches[1]; $link = Link::findOrFail($linkId); diff --git a/tests/integration/Api/LinkVisibilityTest.php b/tests/integration/Api/LinkVisibilityTest.php index 5dbe4eb..1e51e64 100644 --- a/tests/integration/Api/LinkVisibilityTest.php +++ b/tests/integration/Api/LinkVisibilityTest.php @@ -31,26 +31,40 @@ public function setUp(): void $this->normalUser(), ['id' => 3, 'username' => 'moderator', 'email' => 'mod@machine.local', 'is_email_confirmed' => true], ['id' => 4, 'username' => 'evelated', 'email' => 'elevated@machine.local', 'is_email_confirmed' => true], + ['id' => 5, 'username' => 'elevatedplus', 'email' => 'elevatedplus@machine.local', 'is_email_confirmed' => true], ], 'links' => [ ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'parent_id' => null, 'is_restricted' => false], ['id' => 2, 'title' => 'Facebook', 'url' => 'https://facebook.com', 'is_restricted' => true], ['id' => 3, 'title' => 'Twitter', 'url' => 'https://twitter.com', 'is_restricted' => true], ['id' => 4, 'title' => 'Reddit', 'url' => 'https://reddit.com', 'is_restricted' => true], + ['id' => 5, 'title' => 'FooBar', 'url' => 'https://foobar.com', 'is_restricted' => true], + ['id' => 6, 'title' => 'BazQux', 'url' => 'https://bazqux.com', 'is_restricted' => true, 'parent_id' => 5, 'position' => 0], + ['id' => 7, 'title' => 'QuuxQuuz', 'url' => 'https://quuxquuz.com', 'is_restricted' => true, 'parent_id' => 5, 'position' => 1], ], 'groups' => [ ['id' => 5, 'name_singular' => 'FooBar', 'name_plural' => 'FooBars'], + ['id' => 6, 'name_singular' => 'BazQux', 'name_plural' => 'BazQuux'], ], 'group_user' => [ ['user_id' => 3, 'group_id' => Group::MODERATOR_ID], ['user_id' => 4, 'group_id' => 5], + ['user_id' => 5, 'group_id' => 6], ], 'group_permission' => [ ['permission' => 'link1.view', 'group_id' => Group::GUEST_ID], ['permission' => 'link2.view', 'group_id' => 5], + ['permission' => 'link2.view', 'group_id' => 6], ['permission' => 'link3.view', 'group_id' => Group::MEMBER_ID], ['permission' => 'link4.view', 'group_id' => Group::MODERATOR_ID], ['permission' => 'link4.view', 'group_id' => 5], + // Parent & child scenarios + ['permission' => 'link5.view', 'group_id' => 4], + ['permission' => 'link5.view', 'group_id' => 5], + ['permission' => 'link5.view', 'group_id' => 6], + ['permission' => 'link6.view', 'group_id' => 5], + ['permission' => 'link6.view', 'group_id' => 6], + ['permission' => 'link7.view', 'group_id' => 6], ], ]); } @@ -59,10 +73,11 @@ public function forumUsersDataProvider(): array { return [ [null, [1]], - [1, [1, 2, 3, 4]], + [1, [1, 2, 3, 4, 5, 6, 7]], [2, [1, 3]], - [3, [1, 3, 4]], - [4, [1, 2, 3, 4]], + [3, [1, 3, 4, 5]], + [4, [1, 2, 3, 4, 5, 6]], + [5, [1, 2, 3, 5, 6, 7]] ]; } @@ -108,17 +123,25 @@ public function user_type_sees_expected_links(?int $userId, array $expectedLinks $data = json_decode($response->getBody(), true); + // Extract links from response data $links = $this->extractLinksFromIncluded($data); - $this->assertEquals(count($expectedLinks), count($links)); + // Extract IDs from the links array + $linkIds = array_column($links, 'id'); + // Sort both arrays to ensure order doesn't matter + sort($linkIds); + sort($expectedLinks); + + // Ensure the arrays contain exactly the same IDs + $this->assertEquals($expectedLinks, $linkIds); + + // Now check for each individual link's data foreach ($expectedLinks as $expectedLink) { $link = $this->getLinkById($expectedLink, $links); - $this->assertNotNull($link); $dbLink = Link::find($expectedLink); - $this->assertNotNull($dbLink); $this->assertEquals($dbLink->title, $link['attributes']['title']); From 952a89c8d4c42fd9aa04d461844859e3a925750e Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Fri, 11 Oct 2024 09:14:55 +0000 Subject: [PATCH 19/32] Apply fixes from StyleCI --- src/Listener/LinkPermissionChanged.php | 2 +- tests/integration/Api/LinkVisibilityTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Listener/LinkPermissionChanged.php b/src/Listener/LinkPermissionChanged.php index f48776d..e1e7339 100644 --- a/src/Listener/LinkPermissionChanged.php +++ b/src/Listener/LinkPermissionChanged.php @@ -27,7 +27,7 @@ public function handle(PermissionChanged $event) // then we are interested in it. // Extract the link ID from the permission name. // Example permission name: `link1.view` - + if (preg_match('/^link(\d+)\.view$/', $permission, $matches)) { $linkId = $matches[1]; $link = Link::findOrFail($linkId); diff --git a/tests/integration/Api/LinkVisibilityTest.php b/tests/integration/Api/LinkVisibilityTest.php index 1e51e64..98a0579 100644 --- a/tests/integration/Api/LinkVisibilityTest.php +++ b/tests/integration/Api/LinkVisibilityTest.php @@ -77,7 +77,7 @@ public function forumUsersDataProvider(): array [2, [1, 3]], [3, [1, 3, 4, 5]], [4, [1, 2, 3, 4, 5, 6]], - [5, [1, 2, 3, 5, 6, 7]] + [5, [1, 2, 3, 5, 6, 7]], ]; } From bfa2aa39602227c99d29d284a5e5e942980bf784 Mon Sep 17 00:00:00 2001 From: IanM Date: Fri, 11 Oct 2024 10:18:20 +0100 Subject: [PATCH 20/32] cleanup, remove commented code, etc --- src/Access/LinkPolicy.php | 13 ------------- src/Access/ScopeLinkVisibility.php | 3 --- src/Link.php | 2 +- src/Listener/LinkPermissionChanged.php | 4 ---- 4 files changed, 1 insertion(+), 21 deletions(-) diff --git a/src/Access/LinkPolicy.php b/src/Access/LinkPolicy.php index deff264..f9b569a 100644 --- a/src/Access/LinkPolicy.php +++ b/src/Access/LinkPolicy.php @@ -17,19 +17,6 @@ class LinkPolicy extends AbstractPolicy { - // public function can(User $actor, string $ability, Link $link) - // { - // if ($link->parent_id !== null && !$actor->can($ability, $link->parent)) { - // return $this->deny(); - // } - - // if ($link->is_restricted) { - // $id = $link->id; - - // return $actor->hasPermission("link$id.$ability"); - // } - // } - public function view(User $actor, Link $link) { if ($link->parent_id !== null && !$actor->can('view', $link->parent)) { diff --git a/src/Access/ScopeLinkVisibility.php b/src/Access/ScopeLinkVisibility.php index 9f0d0f8..a706628 100644 --- a/src/Access/ScopeLinkVisibility.php +++ b/src/Access/ScopeLinkVisibility.php @@ -14,7 +14,6 @@ use Flarum\User\User; use FoF\Links\Link; use Illuminate\Database\Eloquent\Builder; -use Psr\Log\LoggerInterface; class ScopeLinkVisibility { @@ -24,7 +23,6 @@ class ScopeLinkVisibility */ public function __invoke(User $actor, Builder $query) { - $logger = resolve(LoggerInterface::class); // $query->where('visibility', 'everyone'); // if ($actor->isGuest()) { @@ -40,6 +38,5 @@ public function __invoke(User $actor, Builder $query) ->whereHasPermission($actor, 'view') ->select('links.id'); }); - //$logger->info('ScopeLinkVisibility', ['actor' => $actor->id, 'query' => $query->toSql()]); } } diff --git a/src/Link.php b/src/Link.php index cb836fa..1d054ca 100755 --- a/src/Link.php +++ b/src/Link.php @@ -56,7 +56,7 @@ public static function boot() static::saved(function (self $link) { if ($link->wasUnrestricted()) { - //$link->deletePermissions(); + $link->deletePermissions(); } }); diff --git a/src/Listener/LinkPermissionChanged.php b/src/Listener/LinkPermissionChanged.php index e1e7339..bb90fd1 100644 --- a/src/Listener/LinkPermissionChanged.php +++ b/src/Listener/LinkPermissionChanged.php @@ -23,11 +23,7 @@ public function handle(PermissionChanged $event) $permission = $event->permission; $groupIds = Arr::get($event->data, 'groupIds'); - // If the permission is of the format `link.` followed by a number, ending with `.view`, - // then we are interested in it. - // Extract the link ID from the permission name. // Example permission name: `link1.view` - if (preg_match('/^link(\d+)\.view$/', $permission, $matches)) { $linkId = $matches[1]; $link = Link::findOrFail($linkId); From 84a309ab43ae903709cafd931e891279d8cf277c Mon Sep 17 00:00:00 2001 From: IanM Date: Fri, 11 Oct 2024 10:24:34 +0100 Subject: [PATCH 21/32] add translations --- js/src/admin/components/EditLinkModal.js | 31 ++---------------------- locale/en.yml | 8 +++--- 2 files changed, 6 insertions(+), 33 deletions(-) diff --git a/js/src/admin/components/EditLinkModal.js b/js/src/admin/components/EditLinkModal.js index 82e739c..20a710c 100755 --- a/js/src/admin/components/EditLinkModal.js +++ b/js/src/admin/components/EditLinkModal.js @@ -27,7 +27,6 @@ export default class EditlinksModal extends Modal { this.isInternal = Stream(this.link.isInternal() && true); this.isNewtab = Stream(this.link.isNewtab() && true); this.useRelMe = Stream(this.link.useRelMe() && true); - this.visibility = Stream(this.link.visibility() || 'everyone'); if (this.isInternal()) { this.updateInternalUrl(); @@ -171,28 +170,13 @@ export default class EditlinksModal extends Modal { 40 ); - items.add( - 'visibility', - [ -
- - {Select.component({ - value: this.visibility(), - onchange: this.visibility, - options: this.typeOptions(), - })} -
, - ], - 20 - ); - const permissionPriority = 200; if (this.link.exists) { items.add( 'visibility-permission', [
- +

{app.translator.trans('fof-links.admin.edit_link.visibility.help')}

, @@ -204,7 +188,7 @@ export default class EditlinksModal extends Modal { 'visibility-permission-disabled', [
- +

{app.translator.trans('fof-links.admin.edit_link.visibility.help-disabled')}

, ], @@ -239,16 +223,6 @@ export default class EditlinksModal extends Modal { return items; } - typeOptions() { - let opts; - opts = ['everyone', 'members', 'guests'].reduce((o, key) => { - o[key] = app.translator.trans(`fof-links.admin.edit_link.${key}-label`); - - return o; - }, {}); - return opts; - } - submitData() { return { title: this.itemTitle(), @@ -257,7 +231,6 @@ export default class EditlinksModal extends Modal { isInternal: this.isInternal(), isNewtab: this.isNewtab(), useRelMe: this.useRelMe(), - visibility: this.visibility(), }; } diff --git a/locale/en.yml b/locale/en.yml index ee1603b..ba67968 100755 --- a/locale/en.yml +++ b/locale/en.yml @@ -11,10 +11,7 @@ fof-links: edit_link: delete_link_button: Delete Link delete_link_confirmation: "Are you sure you want to delete this link?" - everyone-label: Everyone - guests-label: Guests internal_link: "Is it an internal link?" - members-label: Registered users open_newtab: "Open link in new tab" submit_button: => core.ref.save_changes title: => fof-links.ref.create_link @@ -27,7 +24,10 @@ fof-links: url_label: => fof-links.ref.url url_placeholder: => fof-links.ref.url use_rel_me: Add rel="me" attribute for identity verification on other sites - visibility: Link visibility + visibility: + help: Links by default are visible to everyone (including guests). You can restrict visibility to specific groups. + help-disabled: Save the link before changing visibility settings. + label: Link visibility # These strings are used in the Links page. links: From 2a7e91cbb4d26cfefd55e6042e13ac6e58a362ec Mon Sep 17 00:00:00 2001 From: IanM Date: Fri, 11 Oct 2024 12:28:27 +0100 Subject: [PATCH 22/32] add migrate data migration, drop visibility col --- js/src/common/models/Link.ts | 4 - ...1_000000_add_guest_only_to_links_table.php | 7 ++ ...0001_migrate_visibility_to_permissions.php | 87 +++++++++++++++++++ ...00002_drop_visibility_from_links_table.php | 30 +++++++ src/Access/ScopeLinkVisibility.php | 9 -- src/Api/Serializer/LinkSerializer.php | 1 - src/Command/CreateLinkHandler.php | 1 - src/Command/EditLinkHandler.php | 4 - src/Link.php | 5 +- src/LinkValidator.php | 1 - src/LoadForumLinksRelationship.php | 11 ++- tests/integration/Api/CreateLinkTest.php | 8 +- tests/integration/Api/DeleteLinkTest.php | 2 +- tests/integration/Api/EditLinkTest.php | 8 +- tests/integration/Api/LinkVisibilityTest.php | 15 +++- 15 files changed, 151 insertions(+), 42 deletions(-) create mode 100644 migrations/2024_10_11_000000_add_guest_only_to_links_table.php create mode 100644 migrations/2024_10_11_000001_migrate_visibility_to_permissions.php create mode 100644 migrations/2024_10_11_000002_drop_visibility_from_links_table.php diff --git a/js/src/common/models/Link.ts b/js/src/common/models/Link.ts index 581ee8b..0ef8a80 100644 --- a/js/src/common/models/Link.ts +++ b/js/src/common/models/Link.ts @@ -41,10 +41,6 @@ export default class Link extends Model { return Model.hasOne('parent').call(this); } - visibility() { - return Model.attribute('visibility').call(this); - } - isRestricted() { return Model.attribute('isRestricted').call(this); } diff --git a/migrations/2024_10_11_000000_add_guest_only_to_links_table.php b/migrations/2024_10_11_000000_add_guest_only_to_links_table.php new file mode 100644 index 0000000..97c88f0 --- /dev/null +++ b/migrations/2024_10_11_000000_add_guest_only_to_links_table.php @@ -0,0 +1,7 @@ + ['boolean', 'default' => 0], +]); diff --git a/migrations/2024_10_11_000001_migrate_visibility_to_permissions.php b/migrations/2024_10_11_000001_migrate_visibility_to_permissions.php new file mode 100644 index 0000000..775a885 --- /dev/null +++ b/migrations/2024_10_11_000001_migrate_visibility_to_permissions.php @@ -0,0 +1,87 @@ + function (Builder $schema) { + $connection = $schema->getConnection(); + + // Fetch all links and map the `visibility` column to permissions. + $links = $connection->table('links')->get(); + + // Look over each row, and check the `visibility` column. Map the following values and create entries on the `group_permission` table: + // `X` is a placeholder for the link ID. + // - `everyone` -> ['group_id' = Group::GUEST_ID, 'permission' = 'linkX.view', 'createdAt' = Carbon::now()] + // - `members` -> ['group_id' = Group::MEMBER_ID, 'permission' = 'linkX.view', 'createdAt' = Carbon::now()] + // - `guests` -> ['group_id' = Group::GUEST_ID, 'permission' = 'linkX.view', 'createdAt' = Carbon::now()] + + + foreach ($links as $link) { + $permission = 'link'.$link->id.'.view'; + $createdAt = Carbon::now(); + + switch ($link->visibility) { + case 'everyone': + $connection->table('group_permission')->insert([ + ['group_id' => Group::GUEST_ID, 'permission' => $permission, 'created_at' => $createdAt], + ]); + $connection->table('links')->where('id', $link->id)->update([ + 'is_restricted' => false, + ]); + break; + + case 'members': + $connection->table('group_permission')->insert([ + ['group_id' => Group::MEMBER_ID, 'permission' => $permission, 'created_at' => $createdAt], + ]); + // Also add to the link row the `is_restricted` = true. + $connection->table('links')->where('id', $link->id)->update([ + 'is_restricted' => true, + ]); + break; + + case 'guests': + $connection->table('group_permission')->insert([ + ['group_id' => Group::GUEST_ID, 'permission' => $permission, 'created_at' => $createdAt], + ]); + // Also add to the link row the `is_restricted` = true and `guest_only` = true. + $connection->table('links')->where('id', $link->id)->update([ + 'is_restricted' => false, + 'guest_only' => true, + ]); + break; + } + } + }, + + 'down' => function (Builder $schema) { + $connection = $schema->getConnection(); + + // Remove all entries from `group_permission` that were added in the `up` function. + $links = $connection->table('links')->get(); + + foreach ($links as $link) { + $permission = 'link'.$link->id.'.view'; + + $connection->table('group_permission')->where('permission', $permission)->delete(); + + // Reverse the changes to `is_restricted` and `guest_only`. + $connection->table('links')->where('id', $link->id)->update([ + 'is_restricted' => false, + 'guest_only' => false, + ]); + } + }, +]; diff --git a/migrations/2024_10_11_000002_drop_visibility_from_links_table.php b/migrations/2024_10_11_000002_drop_visibility_from_links_table.php new file mode 100644 index 0000000..aad5e8a --- /dev/null +++ b/migrations/2024_10_11_000002_drop_visibility_from_links_table.php @@ -0,0 +1,30 @@ + function (Builder $schema) { + if ($schema->hasColumn('links', 'visibility')) { + $schema->table('links', function (Blueprint $table) { + $table->dropColumn('visibility'); + }); + } + }, + + 'down' => function (Builder $schema) { + $schema->table('links', function (Blueprint $table) { + $table->enum('visibility', ['everyone', 'members', 'guests'])->default('everyone'); + $table->index(['visibility']); + }); + }, +]; diff --git a/src/Access/ScopeLinkVisibility.php b/src/Access/ScopeLinkVisibility.php index a706628..5c10851 100644 --- a/src/Access/ScopeLinkVisibility.php +++ b/src/Access/ScopeLinkVisibility.php @@ -23,15 +23,6 @@ class ScopeLinkVisibility */ public function __invoke(User $actor, Builder $query) { - // $query->where('visibility', 'everyone'); - - // if ($actor->isGuest()) { - // $query - // ->orWhere('visibility', 'guests'); - // } else { - // $query - // ->orWhere('visibility', 'members'); - // } $query->whereIn('id', function ($query) use ($actor) { Link::query() ->setQuery($query->from('links')) diff --git a/src/Api/Serializer/LinkSerializer.php b/src/Api/Serializer/LinkSerializer.php index 83875aa..fbd434e 100755 --- a/src/Api/Serializer/LinkSerializer.php +++ b/src/Api/Serializer/LinkSerializer.php @@ -35,7 +35,6 @@ protected function getDefaultAttributes($link) 'isNewtab' => $link->is_newtab, 'useRelMe' => $link->use_relme, 'isChild' => (bool) $link->parent_id, - 'visibility' => $link->visibility, ]; if ($this->actor->isAdmin()) { diff --git a/src/Command/CreateLinkHandler.php b/src/Command/CreateLinkHandler.php index 6e5dce3..7282649 100755 --- a/src/Command/CreateLinkHandler.php +++ b/src/Command/CreateLinkHandler.php @@ -57,7 +57,6 @@ public function handle(CreateLink $command) Arr::get($data, 'attributes.url'), Arr::get($data, 'attributes.isInternal'), Arr::get($data, 'attributes.isNewtab'), - Arr::get($data, 'attributes.visibility'), Arr::get($data, 'attributes.useRelMe') ); diff --git a/src/Command/EditLinkHandler.php b/src/Command/EditLinkHandler.php index 9ea085c..747e524 100755 --- a/src/Command/EditLinkHandler.php +++ b/src/Command/EditLinkHandler.php @@ -87,10 +87,6 @@ public function handle(EditLink $command) $link->use_relme = $attributes['useRelMe']; } - if (isset($attributes['visibility'])) { - $link->visibility = $attributes['visibility']; - } - $this->events->dispatch(new Saving($link, $actor, $data)); $this->validator->assertValid($link->getDirty()); diff --git a/src/Link.php b/src/Link.php index 1d054ca..d52c32b 100755 --- a/src/Link.php +++ b/src/Link.php @@ -28,8 +28,8 @@ * @property bool $use_relme * @property int $parent_id * @property Link $parent - * @property string $visibility * @property bool $is_restricted + * @property bool $guest_only */ class Link extends AbstractModel { @@ -76,7 +76,7 @@ public static function boot() * * @return static */ - public static function build($name, $icon, $url, $isInternal, $isNewtab, $visibility, $useRelMe = false) + public static function build($name, $icon, $url, $isInternal, $isNewtab, $useRelMe = false) { $link = new static(); @@ -86,7 +86,6 @@ public static function build($name, $icon, $url, $isInternal, $isNewtab, $visibi $link->is_internal = (bool) $isInternal; $link->is_newtab = (bool) $isNewtab; $link->use_relme = (bool) $useRelMe; - $link->visibility = $visibility; return $link; } diff --git a/src/LinkValidator.php b/src/LinkValidator.php index 93b77ad..9ae1939 100755 --- a/src/LinkValidator.php +++ b/src/LinkValidator.php @@ -22,6 +22,5 @@ class LinkValidator extends AbstractValidator 'title' => ['required', 'string', 'max:50'], 'url' => ['string', 'max:255'], 'icon' => ['string', 'max:100'], - 'visibility' => ['string', 'in:everyone,guests,members'], ]; } diff --git a/src/LoadForumLinksRelationship.php b/src/LoadForumLinksRelationship.php index 16bde59..bdd3feb 100644 --- a/src/LoadForumLinksRelationship.php +++ b/src/LoadForumLinksRelationship.php @@ -50,7 +50,16 @@ public function __invoke(ShowForumController $controller, &$data, ServerRequestI return $data['links'] = Link::all(); } - $data['links'] = $this->links->getLinks($actor); + $links = $this->links->getLinks($actor); + + if (!$actor->isGuest()) { + // If the user is not a guest, and link that has the valued `guests_only` = true should be removed. + $links = $links->reject(function ($link) { + return $link->guest_only; + }); + } + + $data['links'] = $links; } private function isAdminPath(ServerRequestInterface $request): bool diff --git a/tests/integration/Api/CreateLinkTest.php b/tests/integration/Api/CreateLinkTest.php index 100cde9..3664876 100644 --- a/tests/integration/Api/CreateLinkTest.php +++ b/tests/integration/Api/CreateLinkTest.php @@ -33,7 +33,7 @@ public function setUp(): void $this->normalUser(), ], 'links' => [ - ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'visibility' => 'everyone', 'parent_id' => null], + ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'parent_id' => null], ['id' => 2, 'title' => 'Minimal'], ], ]); @@ -48,7 +48,6 @@ public function payload(): array 'title' => 'Facebook', 'url' => 'https://facebook.com', 'icon' => 'fab fa-facebook', - 'visibility' => 'everyone', 'position' => 0, 'isInternal' => true, 'isNewtab' => true, @@ -67,7 +66,6 @@ public function minimalPayload(): array 'title' => 'Facebook', 'url' => 'https://facebook.com', 'icon' => 'fab fa-facebook', - 'visibility' => 'everyone', ], ], ]; @@ -116,7 +114,6 @@ public function authorized_user_can_create_link(int $userId) $this->assertArrayHasKey('id', $response['data']); $this->assertEquals('Facebook', $response['data']['attributes']['title']); $this->assertEquals('https://facebook.com', $response['data']['attributes']['url']); - $this->assertEquals('everyone', $response['data']['attributes']['visibility']); $this->assertEquals('fab fa-facebook', $response['data']['attributes']['icon']); $this->assertEquals(0, $response['data']['attributes']['position']); $this->assertTrue($response['data']['attributes']['isInternal']); @@ -131,7 +128,6 @@ public function authorized_user_can_create_link(int $userId) $this->assertNotNull($link); $this->assertEquals('Facebook', $link->title); $this->assertEquals('https://facebook.com', $link->url); - $this->assertEquals('everyone', $link->visibility); $this->assertEquals('fab fa-facebook', $link->icon); $this->assertEquals(0, $link->position); $this->assertTrue($link->is_internal); @@ -161,7 +157,6 @@ public function authorized_user_can_create_link_with_minimal_data(int $userId) $this->assertArrayHasKey('id', $response['data']); $this->assertEquals('Facebook', $response['data']['attributes']['title']); $this->assertEquals('https://facebook.com', $response['data']['attributes']['url']); - $this->assertEquals('everyone', $response['data']['attributes']['visibility']); $this->assertEquals('fab fa-facebook', $response['data']['attributes']['icon']); $id = $response['data']['id']; @@ -171,7 +166,6 @@ public function authorized_user_can_create_link_with_minimal_data(int $userId) $this->assertNotNull($link); $this->assertEquals('Facebook', $link->title); $this->assertEquals('https://facebook.com', $link->url); - $this->assertEquals('everyone', $link->visibility); $this->assertEquals('fab fa-facebook', $link->icon); $this->assertFalse($response['data']['attributes']['isChild']); diff --git a/tests/integration/Api/DeleteLinkTest.php b/tests/integration/Api/DeleteLinkTest.php index 63c4139..b0b2fb5 100644 --- a/tests/integration/Api/DeleteLinkTest.php +++ b/tests/integration/Api/DeleteLinkTest.php @@ -33,7 +33,7 @@ public function setUp(): void $this->normalUser(), ], 'links' => [ - ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'visibility' => 'everyone', 'parent_id' => null], + ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'parent_id' => null], ], ]); } diff --git a/tests/integration/Api/EditLinkTest.php b/tests/integration/Api/EditLinkTest.php index 643531a..a9cc6c4 100644 --- a/tests/integration/Api/EditLinkTest.php +++ b/tests/integration/Api/EditLinkTest.php @@ -33,7 +33,7 @@ public function setUp(): void $this->normalUser(), ], 'links' => [ - ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'visibility' => 'everyone', 'parent_id' => null], + ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'parent_id' => null], ], ]); } @@ -56,7 +56,6 @@ public function authorized_user_can_edit_link(int $userId) 'title' => 'Facebook', 'url' => 'https://facebook.com', 'icon' => 'fab fa-facebook', - 'visibility' => 'members', ], ], ], @@ -70,14 +69,12 @@ public function authorized_user_can_edit_link(int $userId) $this->assertEquals('Facebook', $link['attributes']['title']); $this->assertEquals('https://facebook.com', $link['attributes']['url']); $this->assertEquals('fab fa-facebook', $link['attributes']['icon']); - $this->assertEquals('members', $link['attributes']['visibility']); $link = Link::find(1); $this->assertEquals('Facebook', $link->title); $this->assertEquals('https://facebook.com', $link->url); $this->assertEquals('fab fa-facebook', $link->icon); - $this->assertEquals('members', $link->visibility); } /** @@ -120,7 +117,6 @@ public function authorized_user_cannot_edit_nonexistent_link(int $userId) 'title' => 'Facebook', 'url' => 'https://facebook.com', 'icon' => 'fab fa-facebook', - 'visibility' => 'members', ], ], ], @@ -158,7 +154,6 @@ public function unauthorized_user_cannot_edit_link(?int $userId) 'title' => 'Facebook', 'url' => 'https://facebook.com', 'icon' => 'fab fa-facebook', - 'visibility' => 'members', ], ], ], @@ -172,6 +167,5 @@ public function unauthorized_user_cannot_edit_link(?int $userId) $this->assertEquals('Google', $link->title); $this->assertEquals('https://google.com', $link->url); $this->assertEquals('fab fa-google', $link->icon); - $this->assertEquals('everyone', $link->visibility); } } diff --git a/tests/integration/Api/LinkVisibilityTest.php b/tests/integration/Api/LinkVisibilityTest.php index 98a0579..9017133 100644 --- a/tests/integration/Api/LinkVisibilityTest.php +++ b/tests/integration/Api/LinkVisibilityTest.php @@ -26,7 +26,12 @@ public function setUp(): void $this->extension('fof-links'); - $this->prepareDatabase([ + $this->prepareDatabase($this->dbData()); + } + + protected function dbData(): array + { + return [ 'users' => [ $this->normalUser(), ['id' => 3, 'username' => 'moderator', 'email' => 'mod@machine.local', 'is_email_confirmed' => true], @@ -41,6 +46,8 @@ public function setUp(): void ['id' => 5, 'title' => 'FooBar', 'url' => 'https://foobar.com', 'is_restricted' => true], ['id' => 6, 'title' => 'BazQux', 'url' => 'https://bazqux.com', 'is_restricted' => true, 'parent_id' => 5, 'position' => 0], ['id' => 7, 'title' => 'QuuxQuuz', 'url' => 'https://quuxquuz.com', 'is_restricted' => true, 'parent_id' => 5, 'position' => 1], + ['id' => 8, 'title' => 'GuestOnly', 'url' => 'https://guestonly.com', 'is_restricted' => false, 'guest_only' => true], + ], 'groups' => [ ['id' => 5, 'name_singular' => 'FooBar', 'name_plural' => 'FooBars'], @@ -65,14 +72,16 @@ public function setUp(): void ['permission' => 'link6.view', 'group_id' => 5], ['permission' => 'link6.view', 'group_id' => 6], ['permission' => 'link7.view', 'group_id' => 6], + // Guest only + ['permission' => 'link8.view', 'group_id' => Group::GUEST_ID], ], - ]); + ]; } public function forumUsersDataProvider(): array { return [ - [null, [1]], + [null, [1, 8]], [1, [1, 2, 3, 4, 5, 6, 7]], [2, [1, 3]], [3, [1, 3, 4, 5]], From 8b6a45a1078276679a0a84b5f75c0f7b301d6bf8 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Fri, 11 Oct 2024 11:28:38 +0000 Subject: [PATCH 23/32] Apply fixes from StyleCI --- ...2024_10_11_000000_add_guest_only_to_links_table.php | 9 +++++++++ ..._10_11_000001_migrate_visibility_to_permissions.php | 10 ++++------ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/migrations/2024_10_11_000000_add_guest_only_to_links_table.php b/migrations/2024_10_11_000000_add_guest_only_to_links_table.php index 97c88f0..607e192 100644 --- a/migrations/2024_10_11_000000_add_guest_only_to_links_table.php +++ b/migrations/2024_10_11_000000_add_guest_only_to_links_table.php @@ -1,5 +1,14 @@ function (Builder $schema) { @@ -27,7 +26,6 @@ // - `members` -> ['group_id' = Group::MEMBER_ID, 'permission' = 'linkX.view', 'createdAt' = Carbon::now()] // - `guests` -> ['group_id' = Group::GUEST_ID, 'permission' = 'linkX.view', 'createdAt' = Carbon::now()] - foreach ($links as $link) { $permission = 'link'.$link->id.'.view'; $createdAt = Carbon::now(); @@ -59,13 +57,13 @@ // Also add to the link row the `is_restricted` = true and `guest_only` = true. $connection->table('links')->where('id', $link->id)->update([ 'is_restricted' => false, - 'guest_only' => true, + 'guest_only' => true, ]); break; } } }, - + 'down' => function (Builder $schema) { $connection = $schema->getConnection(); @@ -80,7 +78,7 @@ // Reverse the changes to `is_restricted` and `guest_only`. $connection->table('links')->where('id', $link->id)->update([ 'is_restricted' => false, - 'guest_only' => false, + 'guest_only' => false, ]); } }, From 883a81670454b4b2503b5d4fbeb2dab7cda40c69 Mon Sep 17 00:00:00 2001 From: IanM Date: Fri, 11 Oct 2024 12:49:10 +0100 Subject: [PATCH 24/32] add guestOnly property --- js/src/admin/components/EditLinkModal.js | 61 ++++++++++++++---------- js/src/common/models/Link.ts | 4 ++ src/Api/Serializer/LinkSerializer.php | 1 + src/Command/CreateLinkHandler.php | 3 +- src/Command/EditLinkHandler.php | 4 ++ src/Link.php | 4 +- tests/integration/Api/CreateLinkTest.php | 11 ++++- tests/integration/Api/EditLinkTest.php | 5 +- 8 files changed, 63 insertions(+), 30 deletions(-) diff --git a/js/src/admin/components/EditLinkModal.js b/js/src/admin/components/EditLinkModal.js index 20a710c..0aa113a 100755 --- a/js/src/admin/components/EditLinkModal.js +++ b/js/src/admin/components/EditLinkModal.js @@ -27,6 +27,7 @@ export default class EditlinksModal extends Modal { this.isInternal = Stream(this.link.isInternal() && true); this.isNewtab = Stream(this.link.isNewtab() && true); this.useRelMe = Stream(this.link.useRelMe() && true); + this.guestOnly = Stream(this.link.guestOnly() && false); if (this.isInternal()) { this.updateInternalUrl(); @@ -68,6 +69,39 @@ export default class EditlinksModal extends Modal { items() { const items = new ItemList(); + const permissionPriority = 200; + if (this.link.exists) { + items.add( + 'visibility-permission', + [ +
+ +

{app.translator.trans('fof-links.admin.edit_link.visibility.help')}

+ +
, +
+ +

{app.translator.trans('fof-links.admin.edit_link.visibility.guest-only.help')}

+
, + ], + permissionPriority + ); + } else { + items.add( + 'visibility-permission-disabled', + [ +
+ +

{app.translator.trans('fof-links.admin.edit_link.visibility.help-disabled')}

+
, + ], + permissionPriority + ); + } + items.add( 'title', [ @@ -170,32 +204,6 @@ export default class EditlinksModal extends Modal { 40 ); - const permissionPriority = 200; - if (this.link.exists) { - items.add( - 'visibility-permission', - [ -
- -

{app.translator.trans('fof-links.admin.edit_link.visibility.help')}

- -
, - ], - permissionPriority - ); - } else { - items.add( - 'visibility-permission-disabled', - [ -
- -

{app.translator.trans('fof-links.admin.edit_link.visibility.help-disabled')}

-
, - ], - permissionPriority - ); - } - items.add( 'actions', [ @@ -231,6 +239,7 @@ export default class EditlinksModal extends Modal { isInternal: this.isInternal(), isNewtab: this.isNewtab(), useRelMe: this.useRelMe(), + guestOnly: this.guestOnly(), }; } diff --git a/js/src/common/models/Link.ts b/js/src/common/models/Link.ts index 0ef8a80..55e46c3 100644 --- a/js/src/common/models/Link.ts +++ b/js/src/common/models/Link.ts @@ -44,4 +44,8 @@ export default class Link extends Model { isRestricted() { return Model.attribute('isRestricted').call(this); } + + guestOnly() { + return Model.attribute('guestOnly').call(this); + } } diff --git a/src/Api/Serializer/LinkSerializer.php b/src/Api/Serializer/LinkSerializer.php index fbd434e..8023dc4 100755 --- a/src/Api/Serializer/LinkSerializer.php +++ b/src/Api/Serializer/LinkSerializer.php @@ -35,6 +35,7 @@ protected function getDefaultAttributes($link) 'isNewtab' => $link->is_newtab, 'useRelMe' => $link->use_relme, 'isChild' => (bool) $link->parent_id, + 'guestOnly' => (bool) $link->guest_only, ]; if ($this->actor->isAdmin()) { diff --git a/src/Command/CreateLinkHandler.php b/src/Command/CreateLinkHandler.php index 7282649..d97a3b4 100755 --- a/src/Command/CreateLinkHandler.php +++ b/src/Command/CreateLinkHandler.php @@ -57,7 +57,8 @@ public function handle(CreateLink $command) Arr::get($data, 'attributes.url'), Arr::get($data, 'attributes.isInternal'), Arr::get($data, 'attributes.isNewtab'), - Arr::get($data, 'attributes.useRelMe') + Arr::get($data, 'attributes.useRelMe'), + Arr::get($data, 'attributes.guestOnly'), ); $parentId = Arr::get($data, 'relationships.parent.data.id'); diff --git a/src/Command/EditLinkHandler.php b/src/Command/EditLinkHandler.php index 747e524..f66e5b1 100755 --- a/src/Command/EditLinkHandler.php +++ b/src/Command/EditLinkHandler.php @@ -87,6 +87,10 @@ public function handle(EditLink $command) $link->use_relme = $attributes['useRelMe']; } + if (isset($attributes['guestOnly'])) { + $link->guest_only = $attributes['guestOnly']; + } + $this->events->dispatch(new Saving($link, $actor, $data)); $this->validator->assertValid($link->getDirty()); diff --git a/src/Link.php b/src/Link.php index d52c32b..19f0196 100755 --- a/src/Link.php +++ b/src/Link.php @@ -48,6 +48,7 @@ class Link extends AbstractModel 'is_newtab' => 'boolean', 'use_relme' => 'boolean', 'is_restricted' => 'boolean', + 'guest_only' => 'boolean', ]; public static function boot() @@ -76,7 +77,7 @@ public static function boot() * * @return static */ - public static function build($name, $icon, $url, $isInternal, $isNewtab, $useRelMe = false) + public static function build($name, $icon, $url, $isInternal, $isNewtab, $useRelMe = false, $guestOnly = false) { $link = new static(); @@ -86,6 +87,7 @@ public static function build($name, $icon, $url, $isInternal, $isNewtab, $useRel $link->is_internal = (bool) $isInternal; $link->is_newtab = (bool) $isNewtab; $link->use_relme = (bool) $useRelMe; + $link->guest_only = (bool) $guestOnly; return $link; } diff --git a/tests/integration/Api/CreateLinkTest.php b/tests/integration/Api/CreateLinkTest.php index 3664876..685bcd6 100644 --- a/tests/integration/Api/CreateLinkTest.php +++ b/tests/integration/Api/CreateLinkTest.php @@ -33,7 +33,7 @@ public function setUp(): void $this->normalUser(), ], 'links' => [ - ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'parent_id' => null], + ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'parent_id' => null, 'is_restricted' => false, 'guest_only' => false], ['id' => 2, 'title' => 'Minimal'], ], ]); @@ -52,6 +52,7 @@ public function payload(): array 'isInternal' => true, 'isNewtab' => true, 'useRelMe' => true, + 'guestOnly' => true, ], ], ]; @@ -120,6 +121,8 @@ public function authorized_user_can_create_link(int $userId) $this->assertTrue($response['data']['attributes']['isNewtab']); $this->assertTrue($response['data']['attributes']['useRelMe']); $this->assertFalse($response['data']['attributes']['isChild']); + $this->assertTrue($response['data']['attributes']['guestOnly']); + $this->assertFalse($response['data']['attributes']['isRestricted']); $id = $response['data']['id']; @@ -133,6 +136,8 @@ public function authorized_user_can_create_link(int $userId) $this->assertTrue($link->is_internal); $this->assertTrue($link->is_newtab); $this->assertTrue($link->use_relme); + $this->assertFalse($link->is_restricted); + $this->assertTrue($link->guest_only); } /** @@ -158,6 +163,8 @@ public function authorized_user_can_create_link_with_minimal_data(int $userId) $this->assertEquals('Facebook', $response['data']['attributes']['title']); $this->assertEquals('https://facebook.com', $response['data']['attributes']['url']); $this->assertEquals('fab fa-facebook', $response['data']['attributes']['icon']); + $this->assertFalse($response['data']['attributes']['isRestricted']); + $this->assertFalse($response['data']['attributes']['guestOnly']); $id = $response['data']['id']; @@ -175,6 +182,8 @@ public function authorized_user_can_create_link_with_minimal_data(int $userId) $this->assertFalse($link->use_relme); $this->assertNull($link->parent_id); $this->assertNull($link->position); + $this->assertFalse($link->is_restricted); + $this->assertFalse($link->guest_only); } /** diff --git a/tests/integration/Api/EditLinkTest.php b/tests/integration/Api/EditLinkTest.php index a9cc6c4..f315513 100644 --- a/tests/integration/Api/EditLinkTest.php +++ b/tests/integration/Api/EditLinkTest.php @@ -33,7 +33,7 @@ public function setUp(): void $this->normalUser(), ], 'links' => [ - ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'parent_id' => null], + ['id' => 1, 'title' => 'Google', 'icon' => 'fab fa-google', 'url' => 'https://google.com', 'position' => null, 'is_internal' => false, 'is_newtab' => true, 'use_relme' => false, 'parent_id' => null, 'is_restricted' => false, 'guest_only' => false], ], ]); } @@ -56,6 +56,7 @@ public function authorized_user_can_edit_link(int $userId) 'title' => 'Facebook', 'url' => 'https://facebook.com', 'icon' => 'fab fa-facebook', + 'guestOnly' => true, ], ], ], @@ -69,12 +70,14 @@ public function authorized_user_can_edit_link(int $userId) $this->assertEquals('Facebook', $link['attributes']['title']); $this->assertEquals('https://facebook.com', $link['attributes']['url']); $this->assertEquals('fab fa-facebook', $link['attributes']['icon']); + $this->assertTrue($link['attributes']['guestOnly']); $link = Link::find(1); $this->assertEquals('Facebook', $link->title); $this->assertEquals('https://facebook.com', $link->url); $this->assertEquals('fab fa-facebook', $link->icon); + $this->assertTrue($link->guest_only); } /** From 94c6d4370b0b054083e3148aafd0572568f63f71 Mon Sep 17 00:00:00 2001 From: IanM Date: Fri, 11 Oct 2024 13:01:37 +0100 Subject: [PATCH 25/32] add translation, clean up --- js/src/admin/components/EditLinkModal.js | 2 +- locale/en.yml | 3 +++ src/Api/Serializer/LinkSerializer.php | 3 ++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/js/src/admin/components/EditLinkModal.js b/js/src/admin/components/EditLinkModal.js index 0aa113a..8d93374 100755 --- a/js/src/admin/components/EditLinkModal.js +++ b/js/src/admin/components/EditLinkModal.js @@ -27,7 +27,7 @@ export default class EditlinksModal extends Modal { this.isInternal = Stream(this.link.isInternal() && true); this.isNewtab = Stream(this.link.isNewtab() && true); this.useRelMe = Stream(this.link.useRelMe() && true); - this.guestOnly = Stream(this.link.guestOnly() && false); + this.guestOnly = Stream(this.link.guestOnly() && true); if (this.isInternal()) { this.updateInternalUrl(); diff --git a/locale/en.yml b/locale/en.yml index ba67968..72f8720 100755 --- a/locale/en.yml +++ b/locale/en.yml @@ -28,6 +28,9 @@ fof-links: help: Links by default are visible to everyone (including guests). You can restrict visibility to specific groups. help-disabled: Save the link before changing visibility settings. label: Link visibility + guest-only: + label: Guests only? + help: "Only guests can see this link. The permission above should be set to 'Everyone'." # These strings are used in the Links page. links: diff --git a/src/Api/Serializer/LinkSerializer.php b/src/Api/Serializer/LinkSerializer.php index 8023dc4..edb5f76 100755 --- a/src/Api/Serializer/LinkSerializer.php +++ b/src/Api/Serializer/LinkSerializer.php @@ -35,11 +35,12 @@ protected function getDefaultAttributes($link) 'isNewtab' => $link->is_newtab, 'useRelMe' => $link->use_relme, 'isChild' => (bool) $link->parent_id, - 'guestOnly' => (bool) $link->guest_only, ]; if ($this->actor->isAdmin()) { $attributes['isRestricted'] = (bool) $link->is_restricted; + $attributes['guestOnly'] = (bool) $link->guest_only; + } return $attributes; From dc975afcca3d7625740c20efe9d1f7ccdd109ae9 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Fri, 11 Oct 2024 12:01:48 +0000 Subject: [PATCH 26/32] Apply fixes from StyleCI --- src/Api/Serializer/LinkSerializer.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Api/Serializer/LinkSerializer.php b/src/Api/Serializer/LinkSerializer.php index edb5f76..d31e0f3 100755 --- a/src/Api/Serializer/LinkSerializer.php +++ b/src/Api/Serializer/LinkSerializer.php @@ -40,7 +40,6 @@ protected function getDefaultAttributes($link) if ($this->actor->isAdmin()) { $attributes['isRestricted'] = (bool) $link->is_restricted; $attributes['guestOnly'] = (bool) $link->guest_only; - } return $attributes; From 45c17c1811acac8978beefbe9c235881bd40be1c Mon Sep 17 00:00:00 2001 From: IanM Date: Fri, 11 Oct 2024 15:04:47 +0100 Subject: [PATCH 27/32] chore: update copy to reflect actual behaiour --- locale/en.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/locale/en.yml b/locale/en.yml index 72f8720..472c5f9 100755 --- a/locale/en.yml +++ b/locale/en.yml @@ -25,7 +25,7 @@ fof-links: url_placeholder: => fof-links.ref.url use_rel_me: Add rel="me" attribute for identity verification on other sites visibility: - help: Links by default are visible to everyone (including guests). You can restrict visibility to specific groups. + help: Links by default are visible to only admin users. Adjust the permissions to specify who can see this link. help-disabled: Save the link before changing visibility settings. label: Link visibility guest-only: From da0c9e172f317496de6039b3553f5a264e663be5 Mon Sep 17 00:00:00 2001 From: IanM <16573496+imorland@users.noreply.github.com> Date: Mon, 14 Oct 2024 07:50:35 +0100 Subject: [PATCH 28/32] Update js/src/admin/components/EditLinkModal.js Co-authored-by: Davide Iadeluca <146922689+DavideIadeluca@users.noreply.github.com> --- js/src/admin/components/EditLinkModal.js | 1 - 1 file changed, 1 deletion(-) diff --git a/js/src/admin/components/EditLinkModal.js b/js/src/admin/components/EditLinkModal.js index 8d93374..52eb6e9 100755 --- a/js/src/admin/components/EditLinkModal.js +++ b/js/src/admin/components/EditLinkModal.js @@ -8,7 +8,6 @@ import Stream from 'flarum/common/utils/Stream'; import icon from 'flarum/common/helpers/icon'; import withAttr from 'flarum/common/utils/withAttr'; import ItemList from 'flarum/common/utils/ItemList'; -import Select from 'flarum/common/components/Select'; import PermissionDropdown from 'flarum/admin/components/PermissionDropdown'; /** From a929984a5b2584e0f3b10df930c49df8490b32f1 Mon Sep 17 00:00:00 2001 From: IanM <16573496+imorland@users.noreply.github.com> Date: Mon, 14 Oct 2024 07:51:03 +0100 Subject: [PATCH 29/32] Update js/src/admin/components/EditLinkModal.js Co-authored-by: Davide Iadeluca <146922689+DavideIadeluca@users.noreply.github.com> --- js/src/admin/components/EditLinkModal.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/js/src/admin/components/EditLinkModal.js b/js/src/admin/components/EditLinkModal.js index 52eb6e9..b693581 100755 --- a/js/src/admin/components/EditLinkModal.js +++ b/js/src/admin/components/EditLinkModal.js @@ -94,7 +94,9 @@ export default class EditlinksModal extends Modal { [
-

{app.translator.trans('fof-links.admin.edit_link.visibility.help-disabled')}

+ + {app.translator.trans('fof-links.admin.edit_link.visibility.help-disabled')} +
, ], permissionPriority From 7af61efef2f86f07ba54a3be9b89b22f8f3dfe87 Mon Sep 17 00:00:00 2001 From: IanM <16573496+imorland@users.noreply.github.com> Date: Mon, 14 Oct 2024 07:51:10 +0100 Subject: [PATCH 30/32] Update js/src/admin/components/EditLinkModal.js Co-authored-by: Davide Iadeluca <146922689+DavideIadeluca@users.noreply.github.com> --- js/src/admin/components/EditLinkModal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/admin/components/EditLinkModal.js b/js/src/admin/components/EditLinkModal.js index b693581..dc259f8 100755 --- a/js/src/admin/components/EditLinkModal.js +++ b/js/src/admin/components/EditLinkModal.js @@ -9,7 +9,7 @@ import icon from 'flarum/common/helpers/icon'; import withAttr from 'flarum/common/utils/withAttr'; import ItemList from 'flarum/common/utils/ItemList'; import PermissionDropdown from 'flarum/admin/components/PermissionDropdown'; - +import Alert from 'flarum/common/components/Alert'; /** * The `EditlinksModal` component shows a modal dialog which allows the user * to create or edit a link. From ee97387be7e2c6c8d3d74273dbd65cad49c78bb0 Mon Sep 17 00:00:00 2001 From: IanM Date: Mon, 14 Oct 2024 08:20:22 +0100 Subject: [PATCH 31/32] use translations where possible --- composer.json | 2 +- js/src/admin/components/EditLinkModal.js | 22 ++++++++++++++++++---- locale/en.yml | 6 +++--- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/composer.json b/composer.json index 5f1afae..31d9faf 100755 --- a/composer.json +++ b/composer.json @@ -19,7 +19,7 @@ } ], "require": { - "flarum/core": "^1.8.3" + "flarum/core": "^1.8.6" }, "authors": [ { diff --git a/js/src/admin/components/EditLinkModal.js b/js/src/admin/components/EditLinkModal.js index dc259f8..0f49f63 100755 --- a/js/src/admin/components/EditLinkModal.js +++ b/js/src/admin/components/EditLinkModal.js @@ -10,6 +10,8 @@ import withAttr from 'flarum/common/utils/withAttr'; import ItemList from 'flarum/common/utils/ItemList'; import PermissionDropdown from 'flarum/admin/components/PermissionDropdown'; import Alert from 'flarum/common/components/Alert'; +import Group from 'flarum/common/models/Group'; +import Link from 'flarum/common/components/Link'; /** * The `EditlinksModal` component shows a modal dialog which allows the user * to create or edit a link. @@ -65,25 +67,35 @@ export default class EditlinksModal extends Modal { ); } + getGroup(id) { + return app.store.getById('groups', id); + } + items() { const items = new ItemList(); const permissionPriority = 200; if (this.link.exists) { + const adminLabel = this.getGroup(Group.ADMINISTRATOR_ID).nameSingular(); + const guestLabel = this.getGroup(Group.GUEST_ID).namePlural(); + const everyoneLabel = app.translator.trans('core.admin.permissions_controls.everyone_button'); + items.add( 'visibility-permission', [
-

{app.translator.trans('fof-links.admin.edit_link.visibility.help')}

+

{app.translator.trans('fof-links.admin.edit_link.visibility.help', { admin: adminLabel })}

,
-

{app.translator.trans('fof-links.admin.edit_link.visibility.guest-only.help')}

+

+ {app.translator.trans('fof-links.admin.edit_link.visibility.guest-only.help', { guest: guestLabel, everyone: everyoneLabel })} +

, ], permissionPriority @@ -121,7 +133,9 @@ export default class EditlinksModal extends Modal {
{app.translator.trans('fof-links.admin.edit_link.icon_text', { - a: , + a: ( + + ), })}
{app.translator.trans('fof-links.admin.edit_link.icon_additional_text')} diff --git a/locale/en.yml b/locale/en.yml index 472c5f9..ceb9d3c 100755 --- a/locale/en.yml +++ b/locale/en.yml @@ -25,12 +25,12 @@ fof-links: url_placeholder: => fof-links.ref.url use_rel_me: Add rel="me" attribute for identity verification on other sites visibility: - help: Links by default are visible to only admin users. Adjust the permissions to specify who can see this link. + help: Links by default are visible to only {admin} users. Adjust the permissions to specify who can see this link. help-disabled: Save the link before changing visibility settings. label: Link visibility guest-only: - label: Guests only? - help: "Only guests can see this link. The permission above should be set to 'Everyone'." + label: "{guest} only?" + help: "Only {guest} can see this link. The permission above should be set to '{everyone}'." # These strings are used in the Links page. links: From 9385875eacdc79193b2caf2d56593dceb2c9f9d6 Mon Sep 17 00:00:00 2001 From: IanM Date: Mon, 14 Oct 2024 09:17:34 +0100 Subject: [PATCH 32/32] fix: check for existence of column --- migrations/2020_03_16_000000_add_child_links.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/migrations/2020_03_16_000000_add_child_links.php b/migrations/2020_03_16_000000_add_child_links.php index 13bd9d2..9054e3c 100644 --- a/migrations/2020_03_16_000000_add_child_links.php +++ b/migrations/2020_03_16_000000_add_child_links.php @@ -25,6 +25,10 @@ }); }, 'down' => function (Builder $schema) { + if (!$schema->hasColumn('links', 'parent_id')) { + return; + } + $schema->table('links', function (Blueprint $table) { $table->dropForeign(['parent_id']);