diff --git a/js/src/common/models/User.js b/js/src/common/models/User.js index 54d8f5071..30bc0567b 100644 --- a/js/src/common/models/User.js +++ b/js/src/common/models/User.js @@ -10,6 +10,7 @@ export default class User extends Model {} Object.assign(User.prototype, { username: Model.attribute('username'), + slug: Model.attribute('slug'), displayName: Model.attribute('displayName'), email: Model.attribute('email'), isEmailConfirmed: Model.attribute('isEmailConfirmed'), diff --git a/js/src/forum/components/DiscussionPage.js b/js/src/forum/components/DiscussionPage.js index c5812bf39..213a5d674 100644 --- a/js/src/forum/components/DiscussionPage.js +++ b/js/src/forum/components/DiscussionPage.js @@ -107,7 +107,7 @@ export default class DiscussionPage extends Page { } else { const params = this.requestParams(); - app.store.find('discussions', m.route.param('id').split('-')[0], params).then(this.show.bind(this)); + app.store.find('discussions', m.route.param('id'), params).then(this.show.bind(this)); } m.redraw(); @@ -121,6 +121,7 @@ export default class DiscussionPage extends Page { */ requestParams() { return { + bySlug: true, page: { near: this.near }, }; } diff --git a/js/src/forum/components/UserPage.js b/js/src/forum/components/UserPage.js index 66bc22db6..56cb9726e 100644 --- a/js/src/forum/components/UserPage.js +++ b/js/src/forum/components/UserPage.js @@ -102,7 +102,7 @@ export default class UserPage extends Page { }); if (!this.user) { - app.store.find('users', username).then(this.show.bind(this)); + app.store.find('users', username, {bySlug: true}).then(this.show.bind(this)); } } diff --git a/js/src/forum/routes.js b/js/src/forum/routes.js index 9615b4ee8..6f10c27c5 100644 --- a/js/src/forum/routes.js +++ b/js/src/forum/routes.js @@ -34,9 +34,8 @@ export default function (app) { * @return {String} */ app.route.discussion = (discussion, near) => { - const slug = discussion.slug(); return app.route(near && near !== 1 ? 'discussion.near' : 'discussion', { - id: discussion.id() + (slug.trim() ? '-' + slug : ''), + id: discussion.slug(), near: near && near !== 1 ? near : undefined, }); }; @@ -59,7 +58,7 @@ export default function (app) { */ app.route.user = (user) => { return app.route('user', { - username: user.username(), + username: user.slug(), }); }; } diff --git a/src/Api/Controller/ShowDiscussionController.php b/src/Api/Controller/ShowDiscussionController.php index b9fb36f7b..a531a29c7 100644 --- a/src/Api/Controller/ShowDiscussionController.php +++ b/src/Api/Controller/ShowDiscussionController.php @@ -12,6 +12,7 @@ namespace Flarum\Api\Controller; use Flarum\Api\Serializer\DiscussionSerializer; use Flarum\Discussion\Discussion; use Flarum\Discussion\DiscussionRepository; +use Flarum\Http\SlugManager; use Flarum\Post\PostRepository; use Flarum\User\User; use Illuminate\Support\Arr; @@ -36,6 +37,11 @@ class ShowDiscussionController extends AbstractShowController */ public $serializer = DiscussionSerializer::class; + /** + * @var SlugManager + */ + protected $slugManager; + /** * {@inheritdoc} */ @@ -62,8 +68,9 @@ class ShowDiscussionController extends AbstractShowController * @param \Flarum\Discussion\DiscussionRepository $discussions * @param \Flarum\Post\PostRepository $posts */ - public function __construct(DiscussionRepository $discussions, PostRepository $posts) + public function __construct(SlugManager $slugManager, DiscussionRepository $discussions, PostRepository $posts) { + $this->slugManager = $slugManager; $this->discussions = $discussions; $this->posts = $posts; } @@ -77,7 +84,11 @@ class ShowDiscussionController extends AbstractShowController $actor = $request->getAttribute('actor'); $include = $this->extractInclude($request); - $discussion = $this->discussions->findOrFail($discussionId, $actor); + if (Arr::get($request->getQueryParams(), 'bySlug', false)) { + $discussion = $this->slugManager->forResource(Discussion::class)->fromSlug($discussionId, $actor); + } else { + $discussion = $this->discussions->findOrFail($discussionId, $actor); + } if (in_array('posts', $include)) { $postRelationships = $this->getPostRelationships($include); diff --git a/src/Api/Controller/ShowUserController.php b/src/Api/Controller/ShowUserController.php index 8f3826e55..60412362a 100644 --- a/src/Api/Controller/ShowUserController.php +++ b/src/Api/Controller/ShowUserController.php @@ -11,6 +11,8 @@ namespace Flarum\Api\Controller; use Flarum\Api\Serializer\CurrentUserSerializer; use Flarum\Api\Serializer\UserSerializer; +use Flarum\Http\SlugManager; +use Flarum\User\User; use Flarum\User\UserRepository; use Illuminate\Support\Arr; use Psr\Http\Message\ServerRequestInterface; @@ -29,15 +31,22 @@ class ShowUserController extends AbstractShowController public $include = ['groups']; /** - * @var \Flarum\User\UserRepository + * @var SlugManager + */ + protected $slugManager; + + /** + * @var UserRepository */ protected $users; /** - * @param \Flarum\User\UserRepository $users + * @param SlugManager $slugManager + * @param UserRepository $users */ - public function __construct(UserRepository $users) + public function __construct(SlugManager $slugManager, UserRepository $users) { + $this->slugManager = $slugManager; $this->users = $users; } @@ -47,17 +56,18 @@ class ShowUserController extends AbstractShowController protected function data(ServerRequestInterface $request, Document $document) { $id = Arr::get($request->getQueryParams(), 'id'); - - if (! is_numeric($id)) { - $id = $this->users->getIdForUsername($id); - } - $actor = $request->getAttribute('actor'); - if ($actor->id == $id) { + if (Arr::get($request->getQueryParams(), 'bySlug', false)) { + $user = $this->slugManager->forResource(User::class)->fromSlug($id, $actor); + } else { + $user = $this->users->findOrFail($id, $actor); + } + + if ($actor->id === $user->id) { $this->serializer = CurrentUserSerializer::class; } - return $this->users->findOrFail($id, $actor); + return $user; } } diff --git a/src/Api/Serializer/BasicDiscussionSerializer.php b/src/Api/Serializer/BasicDiscussionSerializer.php index cdbf491c5..64bbca752 100644 --- a/src/Api/Serializer/BasicDiscussionSerializer.php +++ b/src/Api/Serializer/BasicDiscussionSerializer.php @@ -10,6 +10,7 @@ namespace Flarum\Api\Serializer; use Flarum\Discussion\Discussion; +use Flarum\Http\SlugManager; use InvalidArgumentException; class BasicDiscussionSerializer extends AbstractSerializer @@ -19,6 +20,16 @@ class BasicDiscussionSerializer extends AbstractSerializer */ protected $type = 'discussions'; + /** + * @var SlugManager + */ + protected $slugManager; + + public function __construct(SlugManager $slugManager) + { + $this->slugManager = $slugManager; + } + /** * {@inheritdoc} * @@ -35,7 +46,7 @@ class BasicDiscussionSerializer extends AbstractSerializer return [ 'title' => $discussion->title, - 'slug' => $discussion->slug, + 'slug' => $this->slugManager->forResource(Discussion::class)->toSlug($discussion), ]; } diff --git a/src/Api/Serializer/BasicUserSerializer.php b/src/Api/Serializer/BasicUserSerializer.php index 6fb6c24d0..e024d338d 100644 --- a/src/Api/Serializer/BasicUserSerializer.php +++ b/src/Api/Serializer/BasicUserSerializer.php @@ -9,6 +9,7 @@ namespace Flarum\Api\Serializer; +use Flarum\Http\SlugManager; use Flarum\User\User; use InvalidArgumentException; @@ -19,6 +20,16 @@ class BasicUserSerializer extends AbstractSerializer */ protected $type = 'users'; + /** + * @var SlugManager + */ + protected $slugManager; + + public function __construct(SlugManager $slugManager) + { + $this->slugManager = $slugManager; + } + /** * {@inheritdoc} * @@ -36,7 +47,8 @@ class BasicUserSerializer extends AbstractSerializer return [ 'username' => $user->username, 'displayName' => $user->display_name, - 'avatarUrl' => $user->avatar_url + 'avatarUrl' => $user->avatar_url, + 'slug' => $this->slugManager->forResource(User::class)->toSlug($user) ]; } diff --git a/src/Discussion/IdWithSlugDriver.php b/src/Discussion/IdWithSlugDriver.php new file mode 100644 index 000000000..85690883a --- /dev/null +++ b/src/Discussion/IdWithSlugDriver.php @@ -0,0 +1,27 @@ +id.(trim($instance->slug) ? '-'.$instance->slug : ''); + } + + public function fromSlug(string $slug, User $actor): AbstractModel + { + if (strpos($slug, '-') == true) { + $slug_array = explode('-', $slug); + $slug = $slug_array[0]; + } + return Discussion::where('id', $slug)->whereVisibleTo($actor)->firstOrFail(); + } +} diff --git a/src/Forum/Content/Discussion.php b/src/Forum/Content/Discussion.php index 748027383..780d41631 100644 --- a/src/Forum/Content/Discussion.php +++ b/src/Forum/Content/Discussion.php @@ -10,6 +10,7 @@ namespace Flarum\Forum\Content; use Flarum\Api\Client; +use Flarum\Discussion\Discussion as FlarumDiscussion; use Flarum\Frontend\Document; use Flarum\Http\Exception\RouteNotFoundException; use Flarum\Http\UrlGenerator; @@ -74,9 +75,7 @@ class Discussion unset($newQueryParams['id']); $queryString = http_build_query($newQueryParams); - $idWithSlug = $apiDocument->data->id.(trim($apiDocument->data->attributes->slug) ? '-'.$apiDocument->data->attributes->slug : ''); - - return $this->url->to('forum')->route('discussion', ['id' => $idWithSlug]). + return $this->url->to('forum')->route('discussion', ['id' => $apiDocument->data->attributes->slug]). ($queryString ? '?'.$queryString : ''); }; @@ -106,6 +105,7 @@ class Discussion */ protected function getApiDocument(User $actor, array $params) { + $params['bySlug'] = true; $response = $this->api->send('Flarum\Api\Controller\ShowDiscussionController', $actor, $params); $statusCode = $response->getStatusCode(); diff --git a/src/Forum/Content/User.php b/src/Forum/Content/User.php index 26a6764a6..f1995f03d 100644 --- a/src/Forum/Content/User.php +++ b/src/Forum/Content/User.php @@ -54,7 +54,7 @@ class User $user = $apiDocument->data->attributes; $document->title = $user->displayName; - $document->canonicalUrl = $this->url->to('forum')->route('user', ['username' => $user->username]); + $document->canonicalUrl = $this->url->to('forum')->route('user', ['username' => $user->slug]); $document->payload['apiDocument'] = $apiDocument; return $document; @@ -70,6 +70,7 @@ class User */ protected function getApiDocument(FlarumUser $actor, array $params) { + $params['bySlug'] = true; $response = $this->api->send(ShowUserController::class, $actor, $params); $statusCode = $response->getStatusCode(); diff --git a/src/Http/HttpServiceProvider.php b/src/Http/HttpServiceProvider.php index 4a7d71b24..919f46a16 100644 --- a/src/Http/HttpServiceProvider.php +++ b/src/Http/HttpServiceProvider.php @@ -9,7 +9,14 @@ namespace Flarum\Http; +use Flarum\Discussion\Discussion; +use Flarum\Discussion\IdWithSlugDriver; use Flarum\Foundation\AbstractServiceProvider; +use Flarum\Foundation\Application; +use Flarum\Post\Post; +use Flarum\Settings\SettingsRepositoryInterface; +use Flarum\User\User; +use Flarum\User\UsernameSlugDriver; class HttpServiceProvider extends AbstractServiceProvider { @@ -25,5 +32,61 @@ class HttpServiceProvider extends AbstractServiceProvider $this->app->bind(Middleware\CheckCsrfToken::class, function ($app) { return new Middleware\CheckCsrfToken($app->make('flarum.http.csrfExemptPaths')); }); + + $this->app->singleton('flarum.http.slugDrivers', function() { + return [ + Discussion::class => [ + 'default' => IdWithSlugDriver::class + ], + User::class => [ + 'default' => UsernameSlugDriver::class + ], + ]; + }); + + $this->app->singleton('flarum.http.selectedSlugDrivers', function() { + $settings = $this->app->make(SettingsRepositoryInterface::class); + + $compiledDrivers = []; + + foreach($this->app->make('flarum.http.slugDrivers') as $resourceClass => $resourceDrivers) { + $driverClass = $resourceDrivers[$settings->get("slug_driver_$resourceClass", 'default')]; + $compiledDrivers[$resourceClass] = $this->app->make($driverClass); + } + + return $compiledDrivers; + }); + + $this->app->singleton('flarum.http.resourceUrlGenerators', function() { + $slugManager = $this->app->make(SlugManager::class); + + return [ + Discussion::class => function(UrlGenerator $urlGenerator, Discussion $discussion) use ($slugManager) { + return $urlGenerator->to('forum')->route('discussion', [ + 'id' => $slugManager->toResource(Discussion::class)->toSlug($discussion) + ]); + }, + Post::class => function(UrlGenerator $urlGenerator, Post $post) use ($slugManager) { + return $urlGenerator->to('forum')->route('user', [ + 'id' => $slugManager->toResource(Discussion::class)->toSlug($post->discussion), + 'near' => $post->id, + ]); + }, + User::class => function(UrlGenerator $urlGenerator, User $user) use ($slugManager) { + return $urlGenerator->to('forum')->route('user', [ + 'id' => $slugManager->toResource(User::class)->toSlug($user) + ]); + }, + ]; + }); + $this->app->bind(SlugManager::class, function() { + return new SlugManager($this->app->make('flarum.http.selectedSlugDrivers')); + }); + + $this->app->bind(UrlGenerator::class, function() { + return new UrlGenerator( + $this->app->make(Application::class), + $this->app->make('flarum.http.resourceUrlGenerators')); + }); } } diff --git a/src/Http/SlugDriverInterface.php b/src/Http/SlugDriverInterface.php new file mode 100644 index 000000000..822b3a29a --- /dev/null +++ b/src/Http/SlugDriverInterface.php @@ -0,0 +1,15 @@ +drivers = $drivers; + } + + public function forResource(string $resourceName): SlugDriverInterface + { + return Arr::get($this->drivers, $resourceName, null); + } +} diff --git a/src/Http/UrlGenerator.php b/src/Http/UrlGenerator.php index 4be39ab7d..f05a6a4bf 100644 --- a/src/Http/UrlGenerator.php +++ b/src/Http/UrlGenerator.php @@ -9,6 +9,7 @@ namespace Flarum\Http; +use Flarum\Database\AbstractModel; use Flarum\Foundation\Application; class UrlGenerator @@ -23,12 +24,18 @@ class UrlGenerator */ protected $app; + /** + * @var array + */ + protected $resourceUrlGenerators; + /** * @param Application $app */ - public function __construct(Application $app) + public function __construct(Application $app, array $resourceUrlGenerators) { $this->app = $app; + $this->resourceUrlGenerators = $resourceUrlGenerators; } /** @@ -59,4 +66,19 @@ class UrlGenerator { return $this->routes[$collection]; } + + /** + * Generate a URL to an instance of a resource + * + * @param string $resourceClass + * @param AbstractModel $instance + * @param $args + * @return void + */ + public function toResource(string $resourceClass, AbstractModel $instance, ...$args): string + { + $callback = $this->resourceUrlGenerators[$resourceClass]; + + return $callback($this, $instance, ...$args); + } } diff --git a/src/User/UsernameSlugDriver.php b/src/User/UsernameSlugDriver.php new file mode 100644 index 000000000..25fc644e1 --- /dev/null +++ b/src/User/UsernameSlugDriver.php @@ -0,0 +1,22 @@ +username; + } + + public function fromSlug(string $slug, User $actor): AbstractModel + { + return User::where('username', $slug)->whereVisibleTo($actor)->firstOrFail(); + } +} diff --git a/tests/integration/api/discussions/ShowTest.php b/tests/integration/api/discussions/ShowTest.php index 131cd6776..6f25e2e44 100644 --- a/tests/integration/api/discussions/ShowTest.php +++ b/tests/integration/api/discussions/ShowTest.php @@ -66,6 +66,24 @@ class ShowTest extends TestCase $this->assertEquals(200, $response->getStatusCode()); } + /** + * @test + */ + public function author_can_see_discussion_via_slug() + { + // Note that here, the slug doesn't actually have to match the real slug + // since the default slugging strategy only takes the numerical part into account + $response = $this->send( + $this->request('GET', '/api/discussions/1-fdsafdsajfsakf', [ + 'authenticatedAs' => 2, + ])->withQueryParams([ + "bySlug" => true + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + } + /** * @test */ diff --git a/tests/integration/api/users/CreateTest.php b/tests/integration/api/users/CreateTest.php index c6d7bd198..87100e2c4 100644 --- a/tests/integration/api/users/CreateTest.php +++ b/tests/integration/api/users/CreateTest.php @@ -25,9 +25,10 @@ class CreateTest extends TestCase $this->prepareDatabase([ 'users' => [ $this->adminUser(), + $this->normalUser(), ], 'groups' => [ - $this->adminGroup(), + $this->adminGroup() ], 'group_user' => [ ['user_id' => 1, 'group_id' => 1], @@ -56,7 +57,7 @@ class CreateTest extends TestCase $this->assertEquals(422, $response->getStatusCode()); // The response body should contain details about the failed validation - $body = (string) $response->getBody(); + $body = (string)$response->getBody(); $this->assertJson($body); $this->assertEquals([ 'errors' => [ diff --git a/tests/integration/api/users/ShowTest.php b/tests/integration/api/users/ShowTest.php new file mode 100644 index 000000000..788d63909 --- /dev/null +++ b/tests/integration/api/users/ShowTest.php @@ -0,0 +1,182 @@ +prepareDatabase([ + 'users' => [ + $this->adminUser(), + $this->normalUser(), + ], + 'groups' => [ + $this->adminGroup() + ], + 'group_user' => [ + ['user_id' => 1, 'group_id' => 1], + ], + 'settings' => [ + ['key' => 'mail_driver', 'value' => 'log'], + ], + ]); + } + + /** + * @test + */ + public function admin_can_see_user() + { + $response = $this->send( + $this->request('GET', '/api/users/2', [ + 'authenticatedAs' => 1, + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + } + + /** + * @test + */ + public function admin_can_see_user_via_slug() + { + $response = $this->send( + $this->request('GET', '/api/users/normal', [ + 'authenticatedAs' => 1, + ])->withQueryParams([ + "bySlug" => true + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + } + + /** + * @test + */ + public function guest_cannot_see_user() + { + $response = $this->send( + $this->request('GET', '/api/users/2') + ); + + $this->assertEquals(404, $response->getStatusCode()); + } + + /** + * @test + */ + public function guest_cannot_see_user_by_slug() + { + $response = $this->send( + $this->request('GET', '/api/users/2')->withQueryParams([ + "bySlug" => true + ]) + ); + + $this->assertEquals(404, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_can_see_themselves() + { + $response = $this->send( + $this->request('GET', '/api/users/2', [ + 'authenticatedAs' => 2, + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_can_see_themselves_via_slug() + { + $response = $this->send( + $this->request('GET', '/api/users/normal', [ + 'authenticatedAs' => 2, + ])->withQueryParams([ + "bySlug" => true + ]) + ); + + $this->assertEquals(200, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_cant_see_others_by_default() + { + $response = $this->send( + $this->request('GET', '/api/users/1', [ + 'authenticatedAs' => 2, + ]) + ); + + $this->assertEquals(404, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_cant_see_others_by_default_via_slug() + { + $response = $this->send( + $this->request('GET', '/api/users/admin', [ + 'authenticatedAs' => 2, + ])->withQueryParams([ + "bySlug" => true + ]) + ); + + $this->assertEquals(404, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_can_see_others_if_allowed() + { + $this-> + + $response = $this->send( + $this->request('GET', '/api/users/1', [ + 'authenticatedAs' => 2, + ]) + ); + + $this->assertEquals(404, $response->getStatusCode()); + } + + /** + * @test + */ + public function user_can_see_others_if_allowed_via_slug() + { + $response = $this->send( + $this->request('GET', '/api/users/admin', [ + 'authenticatedAs' => 2, + ])->withQueryParams([ + "bySlug" => true + ]) + ); + + $this->assertEquals(404, $response->getStatusCode()); + } +} diff --git a/views/frontend/content/index.blade.php b/views/frontend/content/index.blade.php index 809a3bd3a..af938247c 100644 --- a/views/frontend/content/index.blade.php +++ b/views/frontend/content/index.blade.php @@ -7,7 +7,7 @@ @foreach ($apiDocument->data as $discussion)
  • {{ $discussion->attributes->title }}