From b90033a73032da8657f1bd3ec3687aa4426d8cc1 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 16 Sep 2023 13:18:35 +0100 Subject: [PATCH 1/2] Guest control: Cleaned methods involved in fetching/handling - Moves guest user caching from User class to app container for simplicity. - Updates test to use simpler $this->users->guest() method for consistency. - Streamlined helpers to avoid function overlap for simplicity. - Extracted user profile dropdown while doing changes. --- app/Activity/Models/View.php | 2 +- app/Activity/Tools/UserEntityWatchOptions.php | 2 +- app/App/Providers/AuthServiceProvider.php | 7 ++ app/App/helpers.php | 20 +---- app/Entities/Queries/RecentlyViewed.php | 2 +- app/Entities/Queries/TopFavourites.php | 2 +- app/Http/Controller.php | 2 +- app/Http/Middleware/ApiAuthenticate.php | 4 +- app/Http/Middleware/Authenticate.php | 2 +- .../PreventAuthenticatedResponseCaching.php | 2 +- app/Settings/SettingController.php | 2 +- app/Settings/SettingService.php | 4 +- app/Translation/LanguageManager.php | 2 +- .../Controllers/UserSearchController.php | 2 +- app/Users/Models/User.php | 31 +++---- resources/views/books/show.blade.php | 2 +- resources/views/chapters/show.blade.php | 2 +- .../views/common/header-user-menu.blade.php | 47 ++++++++++ resources/views/common/header.blade.php | 61 ++----------- resources/views/errors/404.blade.php | 90 +++++++++---------- resources/views/pages/show.blade.php | 2 +- resources/views/search/all.blade.php | 2 +- resources/views/shelves/show.blade.php | 2 +- .../views/users/preferences/index.blade.php | 4 +- tests/Entity/PageRevisionTest.php | 2 +- tests/Helpers/UserRoleProvider.php | 2 +- tests/PublicActionTest.php | 4 +- tests/TestCase.php | 1 - tests/User/UserManagementTest.php | 4 +- tests/User/UserSearchTest.php | 3 +- 30 files changed, 148 insertions(+), 166 deletions(-) create mode 100644 resources/views/common/header-user-menu.blade.php diff --git a/app/Activity/Models/View.php b/app/Activity/Models/View.php index 512e08295..b593a7d27 100644 --- a/app/Activity/Models/View.php +++ b/app/Activity/Models/View.php @@ -41,7 +41,7 @@ class View extends Model public static function incrementFor(Viewable $viewable): int { $user = user(); - if (is_null($user) || $user->isDefault()) { + if (is_null($user) || $user->isGuest()) { return 0; } diff --git a/app/Activity/Tools/UserEntityWatchOptions.php b/app/Activity/Tools/UserEntityWatchOptions.php index 231204fdf..559d7903d 100644 --- a/app/Activity/Tools/UserEntityWatchOptions.php +++ b/app/Activity/Tools/UserEntityWatchOptions.php @@ -22,7 +22,7 @@ class UserEntityWatchOptions public function canWatch(): bool { - return $this->user->can('receive-notifications') && !$this->user->isDefault(); + return $this->user->can('receive-notifications') && !$this->user->isGuest(); } public function getWatchLevel(): string diff --git a/app/App/Providers/AuthServiceProvider.php b/app/App/Providers/AuthServiceProvider.php index dc482ce33..26d180310 100644 --- a/app/App/Providers/AuthServiceProvider.php +++ b/app/App/Providers/AuthServiceProvider.php @@ -9,6 +9,7 @@ use BookStack\Access\LdapService; use BookStack\Access\LoginService; use BookStack\Access\RegistrationService; use BookStack\Api\ApiTokenGuard; +use BookStack\Users\Models\User; use Illuminate\Support\Facades\Auth; use Illuminate\Support\ServiceProvider; use Illuminate\Validation\Rules\Password; @@ -65,5 +66,11 @@ class AuthServiceProvider extends ServiceProvider Auth::provider('external-users', function ($app, array $config) { return new ExternalBaseUserProvider($config['model']); }); + + // Bind and provide the default system user as a singleton to the app instance when needed. + // This effectively "caches" fetching the user at an app-instance level. + $this->app->singleton('users.default', function () { + return User::query()->where('system_name', '=', 'public')->first(); + }); } } diff --git a/app/App/helpers.php b/app/App/helpers.php index 00d3e9243..b59a63448 100644 --- a/app/App/helpers.php +++ b/app/App/helpers.php @@ -35,23 +35,7 @@ function versioned_asset(string $file = ''): string */ function user(): User { - return auth()->user() ?: User::getDefault(); -} - -/** - * Check if current user is a signed in user. - */ -function signedInUser(): bool -{ - return auth()->user() && !auth()->user()->isDefault(); -} - -/** - * Check if the current user has general access. - */ -function hasAppAccess(): bool -{ - return !auth()->guest() || setting('app-public'); + return auth()->user() ?: User::getGuest(); } /** @@ -61,7 +45,7 @@ function hasAppAccess(): bool function userCan(string $permission, Model $ownable = null): bool { if ($ownable === null) { - return user() && user()->can($permission); + return user()->can($permission); } // Check permission on ownable item diff --git a/app/Entities/Queries/RecentlyViewed.php b/app/Entities/Queries/RecentlyViewed.php index ffa346888..5895b97a2 100644 --- a/app/Entities/Queries/RecentlyViewed.php +++ b/app/Entities/Queries/RecentlyViewed.php @@ -10,7 +10,7 @@ class RecentlyViewed extends EntityQuery public function run(int $count, int $page): Collection { $user = user(); - if ($user === null || $user->isDefault()) { + if ($user === null || $user->isGuest()) { return collect(); } diff --git a/app/Entities/Queries/TopFavourites.php b/app/Entities/Queries/TopFavourites.php index cbccf35b0..a2f8d9ea1 100644 --- a/app/Entities/Queries/TopFavourites.php +++ b/app/Entities/Queries/TopFavourites.php @@ -10,7 +10,7 @@ class TopFavourites extends EntityQuery public function run(int $count, int $skip = 0) { $user = user(); - if ($user->isDefault()) { + if ($user->isGuest()) { return collect(); } diff --git a/app/Http/Controller.php b/app/Http/Controller.php index 584cea3aa..6e81dfd65 100644 --- a/app/Http/Controller.php +++ b/app/Http/Controller.php @@ -71,7 +71,7 @@ abstract class Controller extends BaseController */ protected function preventGuestAccess(): void { - if (!signedInUser()) { + if (user()->isGuest()) { $this->showPermissionError(); } } diff --git a/app/Http/Middleware/ApiAuthenticate.php b/app/Http/Middleware/ApiAuthenticate.php index b348473cf..5f3ad3168 100644 --- a/app/Http/Middleware/ApiAuthenticate.php +++ b/app/Http/Middleware/ApiAuthenticate.php @@ -31,7 +31,7 @@ class ApiAuthenticate { // Return if the user is already found to be signed in via session-based auth. // This is to make it easy to browser the API via browser after just logging into the system. - if (signedInUser() || session()->isStarted()) { + if (!user()->isGuest() || session()->isStarted()) { if (!$this->sessionUserHasApiAccess()) { throw new ApiAuthException(trans('errors.api_user_no_api_permission'), 403); } @@ -53,6 +53,6 @@ class ApiAuthenticate { $hasApiPermission = user()->can('access-api'); - return $hasApiPermission && hasAppAccess(); + return $hasApiPermission && user()->hasAppAccess(); } } diff --git a/app/Http/Middleware/Authenticate.php b/app/Http/Middleware/Authenticate.php index a32029112..6a5c6e354 100644 --- a/app/Http/Middleware/Authenticate.php +++ b/app/Http/Middleware/Authenticate.php @@ -12,7 +12,7 @@ class Authenticate */ public function handle(Request $request, Closure $next) { - if (!hasAppAccess()) { + if (!user()->hasAppAccess()) { if ($request->ajax()) { return response('Unauthorized.', 401); } diff --git a/app/Http/Middleware/PreventAuthenticatedResponseCaching.php b/app/Http/Middleware/PreventAuthenticatedResponseCaching.php index 60c913811..0a90ddd9e 100644 --- a/app/Http/Middleware/PreventAuthenticatedResponseCaching.php +++ b/app/Http/Middleware/PreventAuthenticatedResponseCaching.php @@ -20,7 +20,7 @@ class PreventAuthenticatedResponseCaching /** @var Response $response */ $response = $next($request); - if (signedInUser()) { + if (!user()->isGuest()) { $response->headers->set('Cache-Control', 'max-age=0, no-store, private'); $response->headers->set('Pragma', 'no-cache'); $response->headers->set('Expires', 'Sun, 12 Jul 2015 19:01:00 GMT'); diff --git a/app/Settings/SettingController.php b/app/Settings/SettingController.php index bd55222f2..bdbc3c78a 100644 --- a/app/Settings/SettingController.php +++ b/app/Settings/SettingController.php @@ -34,7 +34,7 @@ class SettingController extends Controller return view('settings.' . $category, [ 'category' => $category, 'version' => $version, - 'guestUser' => User::getDefault(), + 'guestUser' => User::getGuest(), ]); } diff --git a/app/Settings/SettingService.php b/app/Settings/SettingService.php index 1c5cbdf5a..31debdaea 100644 --- a/app/Settings/SettingService.php +++ b/app/Settings/SettingService.php @@ -47,7 +47,7 @@ class SettingService $default = config('setting-defaults.user.' . $key, false); } - if ($user->isDefault()) { + if ($user->isGuest()) { return $this->getFromSession($key, $default); } @@ -206,7 +206,7 @@ class SettingService */ public function putUser(User $user, string $key, string $value): bool { - if ($user->isDefault()) { + if ($user->isGuest()) { session()->put($key, $value); return true; diff --git a/app/Translation/LanguageManager.php b/app/Translation/LanguageManager.php index ec116fd70..f3432d038 100644 --- a/app/Translation/LanguageManager.php +++ b/app/Translation/LanguageManager.php @@ -75,7 +75,7 @@ class LanguageManager return $default; } - if ($user->isDefault() && config('app.auto_detect_locale')) { + if ($user->isGuest() && config('app.auto_detect_locale')) { return $this->autoDetectLocale($request, $default); } diff --git a/app/Users/Controllers/UserSearchController.php b/app/Users/Controllers/UserSearchController.php index 1c7786f58..b6f37bce0 100644 --- a/app/Users/Controllers/UserSearchController.php +++ b/app/Users/Controllers/UserSearchController.php @@ -14,7 +14,7 @@ class UserSearchController extends Controller */ public function forSelect(Request $request) { - $hasPermission = signedInUser() && ( + $hasPermission = !user()->isGuest() && ( userCan('users-manage') || userCan('restrictions-manage-own') || userCan('restrictions-manage-all') diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php index 2479521a2..1eeacfe2e 100644 --- a/app/Users/Models/User.php +++ b/app/Users/Models/User.php @@ -88,38 +88,31 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon */ protected string $avatarUrl = ''; - /** - * This holds the default user when loaded. - */ - protected static ?User $defaultUser = null; - /** * Returns the default public user. + * Fetches from the container as a singleton to effectively cache at an app level. */ - public static function getDefault(): self + public static function getGuest(): self { - if (!is_null(static::$defaultUser)) { - return static::$defaultUser; - } - - static::$defaultUser = static::query()->where('system_name', '=', 'public')->first(); - - return static::$defaultUser; - } - - public static function clearDefault(): void - { - static::$defaultUser = null; + return app()->make('users.default'); } /** * Check if the user is the default public user. */ - public function isDefault(): bool + public function isGuest(): bool { return $this->system_name === 'public'; } + /** + * Check if the user has general access to the application. + */ + public function hasAppAccess(): bool + { + return !$this->isGuest() || setting('app-public'); + } + /** * The roles that belong to the user. * diff --git a/resources/views/books/show.blade.php b/resources/views/books/show.blade.php index e3aef845c..9e7df4156 100644 --- a/resources/views/books/show.blade.php +++ b/resources/views/books/show.blade.php @@ -142,7 +142,7 @@ @if($watchOptions->canWatch() && !$watchOptions->isWatching()) @include('entities.watch-action', ['entity' => $book]) @endif - @if(signedInUser()) + @if(!user()->isGuest()) @include('entities.favourite-action', ['entity' => $book]) @endif @if(userCan('content-export')) diff --git a/resources/views/chapters/show.blade.php b/resources/views/chapters/show.blade.php index 3cb512ccc..0e5224d54 100644 --- a/resources/views/chapters/show.blade.php +++ b/resources/views/chapters/show.blade.php @@ -160,7 +160,7 @@ @if($watchOptions->canWatch() && !$watchOptions->isWatching()) @include('entities.watch-action', ['entity' => $chapter]) @endif - @if(signedInUser()) + @if(!user()->isGuest()) @include('entities.favourite-action', ['entity' => $chapter]) @endif @if(userCan('content-export')) diff --git a/resources/views/common/header-user-menu.blade.php b/resources/views/common/header-user-menu.blade.php new file mode 100644 index 000000000..8ba750f95 --- /dev/null +++ b/resources/views/common/header-user-menu.blade.php @@ -0,0 +1,47 @@ +
+ + +
\ No newline at end of file diff --git a/resources/views/common/header.blade.php b/resources/views/common/header.blade.php index 97a411d84..86ad3563d 100644 --- a/resources/views/common/header.blade.php +++ b/resources/views/common/header.blade.php @@ -18,7 +18,7 @@
- @if (hasAppAccess()) + @if (user()->hasAppAccess()) - -
    • -
  • - - @icon('user-preferences') -
    {{ trans('preferences.preferences') }}
    -     -
    • -
  • - @include('common.dark-mode-toggle', ['classes' => 'icon-item']) -
    • - -
    + @if(!user()->isGuest()) + @include('common.header-user-menu', ['user' => user()]) @endif diff --git a/resources/views/errors/404.blade.php b/resources/views/errors/404.blade.php index a4e5a0dd4..27d66b30b 100644 --- a/resources/views/errors/404.blade.php +++ b/resources/views/errors/404.blade.php @@ -1,55 +1,55 @@ @extends('layouts.simple') @section('content') -
    +
    -
    -
    -
    - @include('errors.parts.not-found-text', [ - 'title' => $message ?? trans('errors.404_page_not_found'), - 'subtitle' => $subtitle ?? trans('errors.sorry_page_not_found'), - 'details' => $details ?? trans('errors.sorry_page_not_found_permission_warning'), - ]) -
    -
    - @if(!signedInUser()) - {{ trans('auth.log_in') }} - @endif - {{ trans('errors.return_home') }} +
    +
    +
    + @include('errors.parts.not-found-text', [ + 'title' => $message ?? trans('errors.404_page_not_found'), + 'subtitle' => $subtitle ?? trans('errors.sorry_page_not_found'), + 'details' => $details ?? trans('errors.sorry_page_not_found_permission_warning'), + ]) +
    +
    + @if(user()->isGuest()) + {{ trans('auth.log_in') }} + @endif + {{ trans('errors.return_home') }} +
    +
    + @if (setting('app-public') || !user()->isGuest()) +
    +
    +
    +

    {{ trans('entities.pages_popular') }}

    +
    + @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['page']), 'style' => 'compact']) +
    +
    +
    +
    +
    +

    {{ trans('entities.books_popular') }}

    +
    + @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['book']), 'style' => 'compact']) +
    +
    +
    +
    +
    +

    {{ trans('entities.chapters_popular') }}

    +
    + @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['chapter']), 'style' => 'compact']) +
    +
    +
    +
    + @endif
    - @if (setting('app-public') || !user()->isDefault()) -
    -
    -
    -

    {{ trans('entities.pages_popular') }}

    -
    - @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['page']), 'style' => 'compact']) -
    -
    -
    -
    -
    -

    {{ trans('entities.books_popular') }}

    -
    - @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['book']), 'style' => 'compact']) -
    -
    -
    -
    -
    -

    {{ trans('entities.chapters_popular') }}

    -
    - @include('entities.list', ['entities' => (new \BookStack\Entities\Queries\Popular)->run(10, 0, ['chapter']), 'style' => 'compact']) -
    -
    -
    -
    - @endif -
    - @stop \ No newline at end of file diff --git a/resources/views/pages/show.blade.php b/resources/views/pages/show.blade.php index 1cbb81980..5d70f7c28 100644 --- a/resources/views/pages/show.blade.php +++ b/resources/views/pages/show.blade.php @@ -188,7 +188,7 @@ @if($watchOptions->canWatch() && !$watchOptions->isWatching()) @include('entities.watch-action', ['entity' => $page]) @endif - @if(signedInUser()) + @if(!user()->isGuest()) @include('entities.favourite-action', ['entity' => $page]) @endif @if(userCan('content-export')) diff --git a/resources/views/search/all.blade.php b/resources/views/search/all.blade.php index 96b14f6e5..64325d4c1 100644 --- a/resources/views/search/all.blade.php +++ b/resources/views/search/all.blade.php @@ -32,7 +32,7 @@
    {{ trans('entities.search_tags') }}
    @include('search.parts.term-list', ['type' => 'tags', 'currentList' => $options->tags]) - @if(signedInUser()) + @if(!user()->isGuest())
    {{ trans('entities.search_options') }}
    @component('search.parts.boolean-filter', ['filters' => $options->filters, 'name' => 'viewed_by_me', 'value' => null]) diff --git a/resources/views/shelves/show.blade.php b/resources/views/shelves/show.blade.php index 86dd6326d..8694ce86d 100644 --- a/resources/views/shelves/show.blade.php +++ b/resources/views/shelves/show.blade.php @@ -143,7 +143,7 @@ @endif - @if(signedInUser()) + @if(!user()->isGuest())
    @include('entities.favourite-action', ['entity' => $shelf]) @endif diff --git a/resources/views/users/preferences/index.blade.php b/resources/views/users/preferences/index.blade.php index 712fdb288..f8576ed9e 100644 --- a/resources/views/users/preferences/index.blade.php +++ b/resources/views/users/preferences/index.blade.php @@ -13,7 +13,7 @@
    - @if(signedInUser() && userCan('receive-notifications')) + @if(!user()->isGuest() && userCan('receive-notifications'))

    {{ trans('preferences.notifications') }}

    @@ -25,7 +25,7 @@
    @endif - @if(signedInUser()) + @if(!user()->isGuest())

    {{ trans('settings.users_edit_profile') }}

    diff --git a/tests/Entity/PageRevisionTest.php b/tests/Entity/PageRevisionTest.php index 97d5a6664..a272dc38b 100644 --- a/tests/Entity/PageRevisionTest.php +++ b/tests/Entity/PageRevisionTest.php @@ -136,7 +136,7 @@ class PageRevisionTest extends TestCase $page = $this->entities->page(); $this->createRevisions($page, 2); - $pageView = $this->get($page->getUrl()); + $pageView = $this->asViewer()->get($page->getUrl()); $pageView->assertSee('Revision #' . $page->revision_count); } diff --git a/tests/Helpers/UserRoleProvider.php b/tests/Helpers/UserRoleProvider.php index 3b2da369d..fe19cad4a 100644 --- a/tests/Helpers/UserRoleProvider.php +++ b/tests/Helpers/UserRoleProvider.php @@ -55,7 +55,7 @@ class UserRoleProvider */ public function guest(): User { - return User::getDefault(); + return User::getGuest(); } /** diff --git a/tests/PublicActionTest.php b/tests/PublicActionTest.php index 1e4dcbfb7..875b279a8 100644 --- a/tests/PublicActionTest.php +++ b/tests/PublicActionTest.php @@ -103,7 +103,7 @@ class PublicActionTest extends TestCase $resp = $this->post($chapter->getUrl('/create-guest-page'), ['name' => 'My guest page']); $resp->assertRedirect($chapter->book->getUrl('/page/my-guest-page/edit')); - $user = User::getDefault(); + $user = $this->users->guest(); $this->assertDatabaseHas('pages', [ 'name' => 'My guest page', 'chapter_id' => $chapter->id, @@ -197,7 +197,7 @@ class PublicActionTest extends TestCase public function test_public_view_can_take_on_other_roles() { $this->setSettings(['app-public' => 'true']); - $newRole = $this->users->attachNewRole(User::getDefault(), []); + $newRole = $this->users->attachNewRole($this->users->guest(), []); $page = $this->entities->page(); $this->permissions->disableEntityInheritedPermissions($page); $this->permissions->addEntityPermission($page, ['view', 'update'], $newRole); diff --git a/tests/TestCase.php b/tests/TestCase.php index f8f59977a..c59f843e9 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -42,7 +42,6 @@ abstract class TestCase extends BaseTestCase $this->permissions = new PermissionsProvider($this->users); $this->files = new FileProvider(); - User::clearDefault(); parent::setUp(); // We can uncomment the below to run tests with failings upon deprecations. diff --git a/tests/User/UserManagementTest.php b/tests/User/UserManagementTest.php index df60bede6..a6d869b2f 100644 --- a/tests/User/UserManagementTest.php +++ b/tests/User/UserManagementTest.php @@ -191,7 +191,7 @@ class UserManagementTest extends TestCase public function test_guest_profile_shows_limited_form() { - $guest = User::getDefault(); + $guest = $this->users->guest(); $resp = $this->asAdmin()->get('/settings/users/' . $guest->id); $resp->assertSee('Guest'); $this->withHtml($resp)->assertElementNotExists('#password'); @@ -199,7 +199,7 @@ class UserManagementTest extends TestCase public function test_guest_profile_cannot_be_deleted() { - $guestUser = User::getDefault(); + $guestUser = $this->users->guest(); $resp = $this->asAdmin()->get('/settings/users/' . $guestUser->id . '/delete'); $resp->assertSee('Delete User'); $resp->assertSee('Guest'); diff --git a/tests/User/UserSearchTest.php b/tests/User/UserSearchTest.php index 1387311ce..76efbf4af 100644 --- a/tests/User/UserSearchTest.php +++ b/tests/User/UserSearchTest.php @@ -57,8 +57,7 @@ class UserSearchTest extends TestCase public function test_select_requires_logged_in_user() { $this->setSettings(['app-public' => true]); - $defaultUser = User::getDefault(); - $this->permissions->grantUserRolePermissions($defaultUser, ['users-manage']); + $this->permissions->grantUserRolePermissions($this->users->guest(), ['users-manage']); $resp = $this->get('/search/users/select?search=a'); $this->assertPermissionError($resp); From e16bdf443ce68e6363ea40124d66eb15c05f2e8b Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 16 Sep 2023 13:49:03 +0100 Subject: [PATCH 2/2] Removed redundant null check --- app/Activity/Models/View.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Activity/Models/View.php b/app/Activity/Models/View.php index b593a7d27..30ead1193 100644 --- a/app/Activity/Models/View.php +++ b/app/Activity/Models/View.php @@ -41,7 +41,7 @@ class View extends Model public static function incrementFor(Viewable $viewable): int { $user = user(); - if (is_null($user) || $user->isGuest()) { + if ($user->isGuest()) { return 0; }