From 8f80cde5b75fc6a18856675f68e371f7f57c9b96 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Sat, 3 Dec 2022 22:15:34 +0100 Subject: [PATCH] feat: allow using utf8 characters in tag slugs (#3588) * feat: allow using utf8 characters in slugs url-encoded slugs are not read by the backend. * chore: use as a slug driver * chore: refactor tests to use data provider * Apply fixes from StyleCI * fix: wrong resource used * fix: forgotten slug from slug manager in serializer * chore(review): adapt tag slug suggestions on the UI * chore: introduce modes for slugging * chore: `yarn format` Signed-off-by: Sami Mazouz --- extensions/tags/extend.php | 4 + .../src/Api/Controller/ShowTagController.php | 22 ++- .../tags/src/Api/Serializer/TagSerializer.php | 19 ++- extensions/tags/src/Content/Tag.php | 22 ++- extensions/tags/src/Query/TagFilterGambit.php | 29 ++-- extensions/tags/src/TagRepository.php | 12 +- extensions/tags/src/Utf8SlugDriver.php | 53 ++++++ .../integration/api/discussions/ListTest.php | 152 ++++++++++-------- .../tests/integration/api/tags/ListTest.php | 4 +- .../tests/integration/api/tags/ShowTest.php | 44 +++++ framework/core/js/src/common/utils/string.ts | 35 +++- 11 files changed, 288 insertions(+), 108 deletions(-) create mode 100644 extensions/tags/src/Utf8SlugDriver.php diff --git a/extensions/tags/extend.php b/extensions/tags/extend.php index e4360e121..97a626358 100644 --- a/extensions/tags/extend.php +++ b/extensions/tags/extend.php @@ -30,6 +30,7 @@ use Flarum\Tags\LoadForumTagsRelationship; use Flarum\Tags\Post\DiscussionTaggedPost; use Flarum\Tags\Query\TagFilterGambit; use Flarum\Tags\Tag; +use Flarum\Tags\Utf8SlugDriver; use Psr\Http\Message\ServerRequestInterface; $eagerLoadTagState = function ($query, ?ServerRequestInterface $request, array $relations) { @@ -133,4 +134,7 @@ return [ (new Extend\SimpleFlarumSearch(DiscussionSearcher::class)) ->addGambit(TagFilterGambit::class), + + (new Extend\ModelUrl(Tag::class)) + ->addSlugDriver('default', Utf8SlugDriver::class), ]; diff --git a/extensions/tags/src/Api/Controller/ShowTagController.php b/extensions/tags/src/Api/Controller/ShowTagController.php index 19f23fda3..dd8ae3824 100644 --- a/extensions/tags/src/Api/Controller/ShowTagController.php +++ b/extensions/tags/src/Api/Controller/ShowTagController.php @@ -11,7 +11,9 @@ namespace Flarum\Tags\Api\Controller; use Flarum\Api\Controller\AbstractShowController; use Flarum\Http\RequestUtil; +use Flarum\Http\SlugManager; use Flarum\Tags\Api\Serializer\TagSerializer; +use Flarum\Tags\Tag; use Flarum\Tags\TagRepository; use Illuminate\Support\Arr; use Psr\Http\Message\ServerRequestInterface; @@ -34,11 +36,17 @@ class ShowTagController extends AbstractShowController /** * @var TagRepository */ - private $tags; + protected $tags; - public function __construct(TagRepository $tags) + /** + * @var SlugManager + */ + protected $slugger; + + public function __construct(TagRepository $tags, SlugManager $slugger) { $this->tags = $tags; + $this->slugger = $slugger; } /** @@ -57,11 +65,11 @@ class ShowTagController extends AbstractShowController $include = array_unique(array_diff($include, ['parent.children.parent'])); } - $tag = $this->tags - ->with($include, $actor) - ->whereVisibleTo($actor) - ->where('slug', $slug) - ->firstOrFail(); + $tag = $this->slugger + ->forResource(Tag::class) + ->fromSlug($slug, $actor); + + $tag->load($this->tags->getAuthorizedRelations($include, $actor)); if ($setParentOnChildren && $tag->parent) { foreach ($tag->parent->children as $child) { diff --git a/extensions/tags/src/Api/Serializer/TagSerializer.php b/extensions/tags/src/Api/Serializer/TagSerializer.php index 23ac89c71..7316ae511 100644 --- a/extensions/tags/src/Api/Serializer/TagSerializer.php +++ b/extensions/tags/src/Api/Serializer/TagSerializer.php @@ -11,6 +11,8 @@ namespace Flarum\Tags\Api\Serializer; use Flarum\Api\Serializer\AbstractSerializer; use Flarum\Api\Serializer\DiscussionSerializer; +use Flarum\Http\SlugManager; +use Flarum\Tags\Tag; class TagSerializer extends AbstractSerializer { @@ -20,14 +22,27 @@ class TagSerializer extends AbstractSerializer protected $type = 'tags'; /** - * {@inheritdoc} + * @var SlugManager + */ + protected $slugManager; + + public function __construct(SlugManager $slugManager) + { + $this->slugManager = $slugManager; + } + + /** + * Get the default set of serialized attributes for a model. + * + * @param Tag $tag + * @return array */ protected function getDefaultAttributes($tag) { $attributes = [ 'name' => $tag->name, 'description' => $tag->description, - 'slug' => $tag->slug, + 'slug' => $this->slugManager->forResource(Tag::class)->toSlug($tag), 'color' => $tag->color, 'backgroundUrl' => $tag->background_path, 'backgroundMode' => $tag->background_mode, diff --git a/extensions/tags/src/Content/Tag.php b/extensions/tags/src/Content/Tag.php index 7e6e54890..8451a0f76 100644 --- a/extensions/tags/src/Content/Tag.php +++ b/extensions/tags/src/Content/Tag.php @@ -12,6 +12,8 @@ namespace Flarum\Tags\Content; use Flarum\Api\Client; use Flarum\Frontend\Document; use Flarum\Http\RequestUtil; +use Flarum\Http\SlugManager; +use Flarum\Tags\Tag as TagModel; use Flarum\Tags\TagRepository; use Illuminate\Contracts\View\Factory; use Illuminate\Support\Arr; @@ -41,17 +43,22 @@ class Tag protected $translator; /** - * @param Client $api - * @param Factory $view - * @param TagRepository $tags - * @param TranslatorInterface $translator + * @var SlugManager */ - public function __construct(Client $api, Factory $view, TagRepository $tags, TranslatorInterface $translator) - { + protected $slugger; + + public function __construct( + Client $api, + Factory $view, + TagRepository $tags, + TranslatorInterface $translator, + SlugManager $slugger + ) { $this->api = $api; $this->view = $view; $this->tags = $tags; $this->translator = $translator; + $this->slugger = $slugger; } public function __invoke(Document $document, Request $request) @@ -67,8 +74,7 @@ class Tag $sortMap = $this->getSortMap(); - $tagId = $this->tags->getIdForSlug($slug); - $tag = $this->tags->findOrFail($tagId, $actor); + $tag = $this->slugger->forResource(TagModel::class)->fromSlug($slug, $actor); $params = [ 'sort' => $sort && isset($sortMap[$sort]) ? $sortMap[$sort] : '', diff --git a/extensions/tags/src/Query/TagFilterGambit.php b/extensions/tags/src/Query/TagFilterGambit.php index 0ef0d1e7f..1dea31f16 100644 --- a/extensions/tags/src/Query/TagFilterGambit.php +++ b/extensions/tags/src/Query/TagFilterGambit.php @@ -11,24 +11,24 @@ namespace Flarum\Tags\Query; use Flarum\Filter\FilterInterface; use Flarum\Filter\FilterState; +use Flarum\Http\SlugManager; use Flarum\Search\AbstractRegexGambit; use Flarum\Search\SearchState; -use Flarum\Tags\TagRepository; +use Flarum\Tags\Tag; +use Flarum\User\User; +use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Database\Query\Builder; class TagFilterGambit extends AbstractRegexGambit implements FilterInterface { /** - * @var TagRepository + * @var SlugManager */ - protected $tags; + protected $slugger; - /** - * @param TagRepository $tags - */ - public function __construct(TagRepository $tags) + public function __construct(SlugManager $slugger) { - $this->tags = $tags; + $this->slugger = $slugger; } protected function getGambitPattern() @@ -48,14 +48,14 @@ class TagFilterGambit extends AbstractRegexGambit implements FilterInterface public function filter(FilterState $filterState, string $filterValue, bool $negate) { - $this->constrain($filterState->getQuery(), $filterValue, $negate); + $this->constrain($filterState->getQuery(), $filterValue, $negate, $filterState->getActor()); } - protected function constrain(Builder $query, $rawSlugs, $negate) + protected function constrain(Builder $query, $rawSlugs, $negate, User $actor) { $slugs = explode(',', trim($rawSlugs, '"')); - $query->where(function (Builder $query) use ($slugs, $negate) { + $query->where(function (Builder $query) use ($slugs, $negate, $actor) { foreach ($slugs as $slug) { if ($slug === 'untagged') { $query->whereIn('discussions.id', function (Builder $query) { @@ -63,7 +63,12 @@ class TagFilterGambit extends AbstractRegexGambit implements FilterInterface ->from('discussion_tag'); }, 'or', ! $negate); } else { - $id = $this->tags->getIdForSlug($slug); + // @TODO: grab all IDs first instead of multiple queries. + try { + $id = $this->slugger->forResource(Tag::class)->fromSlug($slug, $actor)->id; + } catch (ModelNotFoundException $e) { + $id = null; + } $query->whereIn('discussions.id', function (Builder $query) use ($id) { $query->select('discussion_id') diff --git a/extensions/tags/src/TagRepository.php b/extensions/tags/src/TagRepository.php index 89a0b06ff..0ae910eaf 100644 --- a/extensions/tags/src/TagRepository.php +++ b/extensions/tags/src/TagRepository.php @@ -32,6 +32,16 @@ class TagRepository * @return Builder */ public function with($relations, User $actor): Builder + { + return $this->query()->with($this->getAuthorizedRelations($relations, $actor)); + } + + /** + * @param array|string $relations + * @param User $actor + * @return array + */ + public function getAuthorizedRelations($relations, User $actor): array { $relations = is_string($relations) ? explode(',', $relations) : $relations; $relationsArray = []; @@ -46,7 +56,7 @@ class TagRepository } } - return $this->query()->with($relationsArray); + return $relationsArray; } /** diff --git a/extensions/tags/src/Utf8SlugDriver.php b/extensions/tags/src/Utf8SlugDriver.php new file mode 100644 index 000000000..aef92f4fe --- /dev/null +++ b/extensions/tags/src/Utf8SlugDriver.php @@ -0,0 +1,53 @@ + + */ +class Utf8SlugDriver implements SlugDriverInterface +{ + /** + * @var TagRepository + */ + protected $repository; + + public function __construct(TagRepository $repository) + { + $this->repository = $repository; + } + + /** + * @param Tag $instance + * @return string + */ + public function toSlug(AbstractModel $instance): string + { + return $instance->slug; + } + + /** + * @param string $slug + * @param User $actor + * @return Tag + */ + public function fromSlug(string $slug, User $actor): AbstractModel + { + return $this->repository + ->query() + ->where('slug', urldecode($slug)) + ->whereVisibleTo($actor) + ->firstOrFail(); + } +} diff --git a/extensions/tags/tests/integration/api/discussions/ListTest.php b/extensions/tags/tests/integration/api/discussions/ListTest.php index 1a70409eb..4fe5eb221 100644 --- a/extensions/tags/tests/integration/api/discussions/ListTest.php +++ b/extensions/tags/tests/integration/api/discussions/ListTest.php @@ -9,7 +9,6 @@ namespace Flarum\Tags\Tests\integration\api\discussions; -use Flarum\Group\Group; use Flarum\Tags\Tests\integration\RetrievesRepresentativeTags; use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; @@ -33,12 +32,25 @@ class ListTest extends TestCase 'tags' => $this->tags(), 'users' => [ $this->normalUser(), + [ + 'id' => 3, + 'username' => 'normal3', + 'password' => '$2y$10$LO59tiT7uggl6Oe23o/O6.utnF6ipngYjvMvaxo1TciKqBttDNKim', // BCrypt hash for "too-obscure" + 'email' => 'normal3@machine.local', + 'is_email_confirmed' => 1, + ] + ], + 'groups' => [ + ['id' => 100, 'name_singular' => 'acme', 'name_plural' => 'acme'] + ], + 'group_user' => [ + ['group_id' => 100, 'user_id' => 2] ], 'group_permission' => [ - ['group_id' => Group::MEMBER_ID, 'permission' => 'tag5.viewForum'], - ['group_id' => Group::MEMBER_ID, 'permission' => 'tag8.viewForum'], - ['group_id' => Group::MEMBER_ID, 'permission' => 'tag11.viewForum'], - ['group_id' => Group::MEMBER_ID, 'permission' => 'tag13.viewForum'], + ['group_id' => 100, 'permission' => 'tag5.viewForum'], + ['group_id' => 100, 'permission' => 'tag8.viewForum'], + ['group_id' => 100, 'permission' => 'tag11.viewForum'], + ['group_id' => 100, 'permission' => 'tag13.viewForum'], ], 'discussions' => [ ['id' => 1, 'title' => 'no tags', 'user_id' => 1, 'comment_count' => 1], @@ -149,59 +161,22 @@ class ListTest extends TestCase } /** + * @dataProvider seeWhereAllowedWhenMoreTagsAreRequiredThanAvailableDataProvider * @test */ - public function admin_can_see_where_allowed_when_more_primary_tags_are_required_than_available() + public function actor_can_see_where_allowed_when_more_tags_are_required_than_available(string $type, int $actorId, array $expectedDiscussions) { - $this->actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(1, ['1', '2', '3', '4', '5', '6']); - } - - /** - * @test - */ - public function user_can_see_where_allowed_when_more_primary_tags_are_required_than_available() - { - $this->actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(2, ['1', '2', '3', '4']); - } - - /** - * @test - */ - public function guest_can_see_where_allowed_when_more_primary_tags_are_required_than_available() - { - $this->actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(0, ['1', '2']); - } - - /** - * @test - */ - public function admin_can_see_where_allowed_when_more_secondary_tags_are_required_than_available() - { - $this->actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(1, ['1', '2', '3', '4', '5', '6']); - } - - /** - * @test - */ - public function user_can_see_where_allowed_when_more_secondary_tags_are_required_than_available() - { - $this->actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(2, ['1', '2', '3', '4']); - } - - /** - * @test - */ - public function guest_can_see_where_allowed_when_more_secondary_tags_are_required_than_available() - { - $this->actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(0, ['1', '2']); - } - - public function actor_can_see_where_allowed_when_more_primary_tags_are_required_than_available(int $actorId, array $expectedDiscussions) - { - $this->setting('flarum-tags.min_primary_tags', 100); - $this->database()->table('tags') - ->where('position', '!=', null) - ->update(['position' => null]); + if ($type === 'secondary') { + $this->setting('flarum-tags.min_secondary_tags', 1); + $this->database()->table('tags') + ->where(['position' => null, 'parent_id' => null]) + ->update(['position' => 200]); + } elseif ($type === 'primary') { + $this->setting('flarum-tags.min_primary_tags', 100); + $this->database()->table('tags') + ->where('position', '!=', null) + ->update(['position' => null]); + } if ($actorId) { $reqParams = [ @@ -221,21 +196,37 @@ class ListTest extends TestCase $this->assertEqualsCanonicalizing($expectedDiscussions, $ids); } - public function actor_can_see_where_allowed_when_more_secondary_tags_are_required_than_available(int $actorId, array $expectedDiscussions) + public function seeWhereAllowedWhenMoreTagsAreRequiredThanAvailableDataProvider(): array { - $this->setting('flarum-tags.min_secondary_tags', 1); - $this->database()->table('tags') - ->where(['position' => null, 'parent_id' => null]) - ->update(['position' => 200]); - - if ($actorId) { - $reqParams = [ - 'authenticatedAs' => $actorId - ]; - } + return [ + // admin_can_see_where_allowed_when_more_primary_tags_are_required_than_available + ['primary', 1, ['1', '2', '3', '4', '5', '6']], + // user_can_see_where_allowed_when_more_primary_tags_are_required_than_available + ['primary', 2, ['1', '2', '3', '4']], + // guest_can_see_where_allowed_when_more_primary_tags_are_required_than_available + ['primary', 0, ['1', '2']], + // admin_can_see_where_allowed_when_more_secondary_tags_are_required_than_available + ['secondary', 1, ['1', '2', '3', '4', '5', '6']], + // user_can_see_where_allowed_when_more_secondary_tags_are_required_than_available + ['secondary', 2, ['1', '2', '3', '4']], + // guest_can_see_where_allowed_when_more_secondary_tags_are_required_than_available + ['secondary', 0, ['1', '2']], + ]; + } + /** + * @dataProvider filterByTagsDataProvider + * @test + */ + public function can_filter_by_authorized_tags(int $authenticatedAs, string $tags, array $expectedDiscussionIds) + { $response = $this->send( - $this->request('GET', '/api/discussions', $reqParams ?? []) + $this->request('GET', '/api/discussions', compact('authenticatedAs')) + ->withQueryParams([ + 'filter' => [ + 'tag' => $tags + ] + ]) ); $this->assertEquals(200, $response->getStatusCode()); @@ -243,6 +234,31 @@ class ListTest extends TestCase $data = json_decode($response->getBody()->getContents(), true)['data']; $ids = Arr::pluck($data, 'id'); - $this->assertEqualsCanonicalizing($expectedDiscussions, $ids); + $this->assertEqualsCanonicalizing($expectedDiscussionIds, array_map('intval', $ids)); + } + + public function filterByTagsDataProvider(): array + { + return [ + // Admin can filter by any tag. + [1, 'primary-1,primary-2', [2, 3, 4]], + [1, 'primary-1,primary-2,primary-restricted', [2, 3, 4, 5]], + [1, 'primary-2-restricted-child-1', [6]], + [1, 'untagged', [1]], + + // Normal user can only filter by tags he has access to. + [3, 'primary-2-restricted-child-1', []], + [3, 'primary-1', [2]], + [3, 'primary-1,primary-2', [2]], + [3, 'untagged', [1]], + + // Authorized User can only filter by tags he has access to. + [2, 'primary-2-restricted-child-1', []], + [2, 'primary-2-restricted-child-1,primary-restricted-child-restricted', []], + [2, 'primary-1', [2, 4]], + [2, 'primary-1,primary-2', [2, 3, 4]], + [2, 'secondary-restricted', [4]], + [2, 'untagged', [1]], + ]; } } diff --git a/extensions/tags/tests/integration/api/tags/ListTest.php b/extensions/tags/tests/integration/api/tags/ListTest.php index 6fad63158..cbe4c1f8f 100644 --- a/extensions/tags/tests/integration/api/tags/ListTest.php +++ b/extensions/tags/tests/integration/api/tags/ListTest.php @@ -83,7 +83,7 @@ class ListTest extends TestCase } /** - * @dataProvider listTagsIncludes + * @dataProvider listTagsIncludesDataProvider * @test */ public function user_sees_where_allowed_with_included_tags(string $include, array $expectedIncludes) @@ -127,7 +127,7 @@ class ListTest extends TestCase $this->assertEquals(['1', '2', '3', '4', '9', '10'], $ids); } - public function listTagsIncludes(): array + public function listTagsIncludesDataProvider(): array { return [ ['children', ['3', '4']], diff --git a/extensions/tags/tests/integration/api/tags/ShowTest.php b/extensions/tags/tests/integration/api/tags/ShowTest.php index 959c99a08..043ce78bc 100644 --- a/extensions/tags/tests/integration/api/tags/ShowTest.php +++ b/extensions/tags/tests/integration/api/tags/ShowTest.php @@ -41,6 +41,50 @@ class ShowTest extends TestCase ]); } + /** @test */ + public function can_show_tag_with_url_decoded_utf8_slug() + { + $this->prepareDatabase([ + 'tags' => [ + ['id' => 155, 'name' => '测试', 'slug' => '测试', 'position' => 0, 'parent_id' => null] + ] + ]); + + $response = $this->send( + $this->request('GET', '/api/tags/测试') + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $response2 = $this->send( + $this->request('GET', '/t/测试') + ); + + $this->assertEquals(200, $response2->getStatusCode()); + } + + /** @test */ + public function can_show_tag_with_url_encoded_utf8_slug() + { + $this->prepareDatabase([ + 'tags' => [ + ['id' => 155, 'name' => '测试', 'slug' => '测试', 'position' => 0, 'parent_id' => null] + ] + ]); + + $response = $this->send( + $this->request('GET', '/api/tags/'.urlencode('测试')) + ); + + $this->assertEquals(200, $response->getStatusCode()); + + $response2 = $this->send( + $this->request('GET', '/t/'.urlencode('测试')) + ); + + $this->assertEquals(200, $response2->getStatusCode()); + } + /** * @dataProvider showTagIncludes * @test diff --git a/framework/core/js/src/common/utils/string.ts b/framework/core/js/src/common/utils/string.ts index df66716d4..84ca81ff7 100644 --- a/framework/core/js/src/common/utils/string.ts +++ b/framework/core/js/src/common/utils/string.ts @@ -6,19 +6,38 @@ export function truncate(string: string, length: number, start: number = 0): str } /** - * Create a slug out of the given string. Non-alphanumeric characters are - * converted to hyphens. + * Create a slug out of the given string depending on the selected mode. + * Invalid characters are converted to hyphens. * * NOTE: This method does not use the comparably sophisticated transliteration * mechanism that is employed in the backend. Therefore, it should only be used * to *suggest* slugs that can be overridden by the user. */ -export function slug(string: string): string { - return string - .toLowerCase() - .replace(/[^a-z0-9]/gi, '-') - .replace(/-+/g, '-') - .replace(/-$|^-/g, ''); +export function slug(string: string, mode: SluggingMode = SluggingMode.ALPHANUMERIC): string { + switch (mode) { + case SluggingMode.UTF8: + return ( + string + .toLowerCase() + // Match non-word characters (take UTF8 into consideration) and replace with a dash. + .replace(/[^\p{L}\p{N}\p{M}]/giu, '-') + .replace(/-+/g, '-') + .replace(/-$|^-/g, '') + ); + + case SluggingMode.ALPHANUMERIC: + default: + return string + .toLowerCase() + .replace(/[^a-z0-9]/gi, '-') + .replace(/-+/g, '-') + .replace(/-$|^-/g, ''); + } +} + +enum SluggingMode { + ALPHANUMERIC = 'alphanum', + UTF8 = 'utf8', } /**