From 024924eef38179ecb12ef5cd6d7bcdcb8c15a298 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 22 Nov 2021 23:33:55 +0000 Subject: [PATCH] Applied another round of static analysis updates --- app/Actions/ActivityService.php | 4 +-- app/Auth/Access/LdapService.php | 2 +- app/Auth/Access/Oidc/OidcJwtSigningKey.php | 19 +++++++---- app/Auth/Access/SocialAuthService.php | 3 +- app/Entities/Models/Book.php | 22 ++++--------- app/Entities/Models/Bookshelf.php | 2 +- app/Entities/Models/Chapter.php | 4 ++- app/Entities/Models/Entity.php | 4 +-- app/Entities/Models/PageRevision.php | 4 +-- app/Entities/Repos/PageRepo.php | 5 +-- app/Entities/Tools/PageContent.php | 8 +++-- app/Entities/Tools/SearchIndex.php | 7 ++-- app/Entities/Tools/SearchRunner.php | 4 +-- app/Entities/Tools/TrashCan.php | 18 +++++------ .../Api/BookshelfApiController.php | 2 +- .../Controllers/Api/ChapterApiController.php | 2 +- app/Http/Controllers/BookController.php | 2 +- app/Theming/ThemeService.php | 2 +- app/Uploads/ImageRepo.php | 5 ++- tests/Unit/FrameworkAssumptionTest.php | 32 +++++++++++++++++++ version | 2 +- 21 files changed, 96 insertions(+), 57 deletions(-) create mode 100644 tests/Unit/FrameworkAssumptionTest.php diff --git a/app/Actions/ActivityService.php b/app/Actions/ActivityService.php index 33ed44b32..983c1a603 100644 --- a/app/Actions/ActivityService.php +++ b/app/Actions/ActivityService.php @@ -105,10 +105,10 @@ class ActivityService $queryIds = [$entity->getMorphClass() => [$entity->id]]; if ($entity instanceof Book) { - $queryIds[(new Chapter())->getMorphClass()] = $entity->chapters()->visible()->pluck('id'); + $queryIds[(new Chapter())->getMorphClass()] = $entity->chapters()->scopes('visible')->pluck('id'); } if ($entity instanceof Book || $entity instanceof Chapter) { - $queryIds[(new Page())->getMorphClass()] = $entity->pages()->visible()->pluck('id'); + $queryIds[(new Page())->getMorphClass()] = $entity->pages()->scopes('visible')->pluck('id'); } $query = $this->activity->newQuery(); diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php index e3a38537a..e529b80fd 100644 --- a/app/Auth/Access/LdapService.php +++ b/app/Auth/Access/LdapService.php @@ -165,7 +165,7 @@ class LdapService * Bind the system user to the LDAP connection using the given credentials * otherwise anonymous access is attempted. * - * @param $connection + * @param resource $connection * * @throws LdapException */ diff --git a/app/Auth/Access/Oidc/OidcJwtSigningKey.php b/app/Auth/Access/Oidc/OidcJwtSigningKey.php index 9a5b3833a..a70f3b3c7 100644 --- a/app/Auth/Access/Oidc/OidcJwtSigningKey.php +++ b/app/Auth/Access/Oidc/OidcJwtSigningKey.php @@ -41,16 +41,18 @@ class OidcJwtSigningKey protected function loadFromPath(string $path) { try { - $this->key = PublicKeyLoader::load( + $key = PublicKeyLoader::load( file_get_contents($path) - )->withPadding(RSA::SIGNATURE_PKCS1); + ); } catch (\Exception $exception) { throw new OidcInvalidKeyException("Failed to load key from file path with error: {$exception->getMessage()}"); } - if (!($this->key instanceof RSA)) { + if (!$key instanceof RSA) { throw new OidcInvalidKeyException('Key loaded from file path is not an RSA key as expected'); } + + $this->key = $key->withPadding(RSA::SIGNATURE_PKCS1); } /** @@ -81,14 +83,19 @@ class OidcJwtSigningKey $n = strtr($jwk['n'] ?? '', '-_', '+/'); try { - /** @var RSA $key */ - $this->key = PublicKeyLoader::load([ + $key = PublicKeyLoader::load([ 'e' => new BigInteger(base64_decode($jwk['e']), 256), 'n' => new BigInteger(base64_decode($n), 256), - ])->withPadding(RSA::SIGNATURE_PKCS1); + ]); } catch (\Exception $exception) { throw new OidcInvalidKeyException("Failed to load key from JWK parameters with error: {$exception->getMessage()}"); } + + if (!$key instanceof RSA) { + throw new OidcInvalidKeyException('Key loaded from file path is not an RSA key as expected'); + } + + $this->key = $key->withPadding(RSA::SIGNATURE_PKCS1); } /** diff --git a/app/Auth/Access/SocialAuthService.php b/app/Auth/Access/SocialAuthService.php index 23e95970c..0df5ceb5e 100644 --- a/app/Auth/Access/SocialAuthService.php +++ b/app/Auth/Access/SocialAuthService.php @@ -12,6 +12,7 @@ use Illuminate\Support\Str; use Laravel\Socialite\Contracts\Factory as Socialite; use Laravel\Socialite\Contracts\Provider; use Laravel\Socialite\Contracts\User as SocialUser; +use Laravel\Socialite\Two\GoogleProvider; use SocialiteProviders\Manager\SocialiteWasCalled; use Symfony\Component\HttpFoundation\RedirectResponse; @@ -278,7 +279,7 @@ class SocialAuthService { $driver = $this->socialite->driver($driverName); - if ($driverName === 'google' && config('services.google.select_account')) { + if ($driver instanceof GoogleProvider && config('services.google.select_account')) { $driver->with(['prompt' => 'select_account']); } diff --git a/app/Entities/Models/Book.php b/app/Entities/Models/Book.php index 735d25a99..8217d2cab 100644 --- a/app/Entities/Models/Book.php +++ b/app/Entities/Models/Book.php @@ -79,53 +79,43 @@ class Book extends Entity implements HasCoverImage /** * Get all pages within this book. - * - * @return HasMany */ - public function pages() + public function pages(): HasMany { return $this->hasMany(Page::class); } /** * Get the direct child pages of this book. - * - * @return HasMany */ - public function directPages() + public function directPages(): HasMany { return $this->pages()->where('chapter_id', '=', '0'); } /** * Get all chapters within this book. - * - * @return HasMany */ - public function chapters() + public function chapters(): HasMany { return $this->hasMany(Chapter::class); } /** * Get the shelves this book is contained within. - * - * @return BelongsToMany */ - public function shelves() + public function shelves(): BelongsToMany { return $this->belongsToMany(Bookshelf::class, 'bookshelves_books', 'book_id', 'bookshelf_id'); } /** * Get the direct child items within this book. - * - * @return Collection */ public function getDirectChildren(): Collection { - $pages = $this->directPages()->visible()->get(); - $chapters = $this->chapters()->visible()->get(); + $pages = $this->directPages()->scopes('visible')->get(); + $chapters = $this->chapters()->scopes('visible')->get(); return $pages->concat($chapters)->sortBy('priority')->sortByDesc('draft'); } diff --git a/app/Entities/Models/Bookshelf.php b/app/Entities/Models/Bookshelf.php index e4d9775b7..b9ebab92e 100644 --- a/app/Entities/Models/Bookshelf.php +++ b/app/Entities/Models/Bookshelf.php @@ -37,7 +37,7 @@ class Bookshelf extends Entity implements HasCoverImage */ public function visibleBooks(): BelongsToMany { - return $this->books()->visible(); + return $this->books()->scopes('visible'); } /** diff --git a/app/Entities/Models/Chapter.php b/app/Entities/Models/Chapter.php index 224ded935..8fc2d333d 100644 --- a/app/Entities/Models/Chapter.php +++ b/app/Entities/Models/Chapter.php @@ -23,6 +23,7 @@ class Chapter extends BookChild /** * Get the pages that this chapter contains. + * @return HasMany */ public function pages(string $dir = 'ASC'): HasMany { @@ -50,7 +51,8 @@ class Chapter extends BookChild */ public function getVisiblePages(): Collection { - return $this->pages()->visible() + return $this->pages() + ->scopes('visible') ->orderBy('draft', 'desc') ->orderBy('priority', 'asc') ->get(); diff --git a/app/Entities/Models/Entity.php b/app/Entities/Models/Entity.php index 2b6f85d24..0eb402284 100644 --- a/app/Entities/Models/Entity.php +++ b/app/Entities/Models/Entity.php @@ -121,11 +121,11 @@ abstract class Entity extends Model implements Sluggable, Favouritable, Viewable return true; } - if (($entity->isA('chapter') || $entity->isA('page')) && $this->isA('book')) { + if (($entity instanceof BookChild) && $this instanceof Book) { return $entity->book_id === $this->id; } - if ($entity->isA('page') && $this->isA('chapter')) { + if ($entity instanceof Page && $this instanceof Chapter) { return $entity->chapter_id === $this->id; } diff --git a/app/Entities/Models/PageRevision.php b/app/Entities/Models/PageRevision.php index a3c4379a6..2bfa169f4 100644 --- a/app/Entities/Models/PageRevision.php +++ b/app/Entities/Models/PageRevision.php @@ -63,10 +63,8 @@ class PageRevision extends Model /** * Get the previous revision for the same page if existing. - * - * @return \BookStack\Entities\PageRevision|null */ - public function getPrevious() + public function getPrevious(): ?PageRevision { $id = static::newQuery()->where('page_id', '=', $this->page_id) ->where('id', '<', $this->id) diff --git a/app/Entities/Repos/PageRepo.php b/app/Entities/Repos/PageRepo.php index ed5534f08..24fc1e7dd 100644 --- a/app/Entities/Repos/PageRepo.php +++ b/app/Entities/Repos/PageRepo.php @@ -69,9 +69,10 @@ class PageRepo */ public function getByOldSlug(string $bookSlug, string $pageSlug): ?Page { + /** @var ?PageRevision $revision */ $revision = PageRevision::query() ->whereHas('page', function (Builder $query) { - $query->visible(); + $query->scopes('visible'); }) ->where('slug', '=', $pageSlug) ->where('type', '=', 'version') @@ -80,7 +81,7 @@ class PageRepo ->with('page') ->first(); - return $revision ? $revision->page : null; + return $revision->page ?? null; } /** diff --git a/app/Entities/Tools/PageContent.php b/app/Entities/Tools/PageContent.php index c60cf0311..b95131fce 100644 --- a/app/Entities/Tools/PageContent.php +++ b/app/Entities/Tools/PageContent.php @@ -12,6 +12,8 @@ use BookStack\Uploads\ImageRepo; use BookStack\Uploads\ImageService; use BookStack\Util\HtmlContentFilter; use DOMDocument; +use DOMElement; +use DOMNode; use DOMNodeList; use DOMXPath; use Illuminate\Support\Str; @@ -237,9 +239,9 @@ class PageContent * A map for existing ID's should be passed in to check for current existence. * Returns a pair of strings in the format [old_id, new_id]. */ - protected function setUniqueId(\DOMNode $element, array &$idMap): array + protected function setUniqueId(DOMNode $element, array &$idMap): array { - if (!$element instanceof \DOMElement) { + if (!$element instanceof DOMElement) { return ['', '']; } @@ -321,7 +323,7 @@ class PageContent */ protected function headerNodesToLevelList(DOMNodeList $nodeList): array { - $tree = collect($nodeList)->map(function ($header) { + $tree = collect($nodeList)->map(function (DOMElement $header) { $text = trim(str_replace("\xc2\xa0", '', $header->nodeValue)); $text = mb_substr($text, 0, 100); diff --git a/app/Entities/Tools/SearchIndex.php b/app/Entities/Tools/SearchIndex.php index 79139ec24..d43d98207 100644 --- a/app/Entities/Tools/SearchIndex.php +++ b/app/Entities/Tools/SearchIndex.php @@ -9,6 +9,7 @@ use BookStack\Entities\Models\Page; use BookStack\Entities\Models\SearchTerm; use DOMDocument; use DOMNode; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Support\Collection; class SearchIndex @@ -76,7 +77,9 @@ class SearchIndex foreach ($this->entityProvider->all() as $entityModel) { $indexContentField = $entityModel instanceof Page ? 'html' : 'description'; $selectFields = ['id', 'name', $indexContentField]; - $total = $entityModel->newQuery()->withTrashed()->count(); + /** @var Builder $query */ + $query = $entityModel->newQuery(); + $total = $query->withTrashed()->count(); $chunkSize = 250; $processed = 0; @@ -223,7 +226,7 @@ class SearchIndex if ($entity instanceof Page) { $bodyTermsMap = $this->generateTermScoreMapFromHtml($entity->html); } else { - $bodyTermsMap = $this->generateTermScoreMapFromText($entity->description ?? '', $entity->searchFactor); + $bodyTermsMap = $this->generateTermScoreMapFromText($entity->getAttribute('description') ?? '', $entity->searchFactor); } $mergedScoreMap = $this->mergeTermScoreMaps($nameTermsMap, $bodyTermsMap, $tagTermsMap); diff --git a/app/Entities/Tools/SearchRunner.php b/app/Entities/Tools/SearchRunner.php index a4f131482..a0a44f3a5 100644 --- a/app/Entities/Tools/SearchRunner.php +++ b/app/Entities/Tools/SearchRunner.php @@ -145,13 +145,13 @@ class SearchRunner if ($entityModelInstance instanceof BookChild) { $relations['book'] = function (BelongsTo $query) { - $query->visible(); + $query->scopes('visible'); }; } if ($entityModelInstance instanceof Page) { $relations['chapter'] = function (BelongsTo $query) { - $query->visible(); + $query->scopes('visible'); }; } diff --git a/app/Entities/Tools/TrashCan.php b/app/Entities/Tools/TrashCan.php index fcf933726..ab62165af 100644 --- a/app/Entities/Tools/TrashCan.php +++ b/app/Entities/Tools/TrashCan.php @@ -15,6 +15,7 @@ use BookStack\Facades\Activity; use BookStack\Uploads\AttachmentService; use BookStack\Uploads\ImageService; use Exception; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Support\Carbon; class TrashCan @@ -141,11 +142,9 @@ class TrashCan { $count = 0; $pages = $chapter->pages()->withTrashed()->get(); - if (count($pages)) { - foreach ($pages as $page) { - $this->destroyPage($page); - $count++; - } + foreach ($pages as $page) { + $this->destroyPage($page); + $count++; } $this->destroyCommonRelations($chapter); @@ -183,9 +182,10 @@ class TrashCan { $counts = []; - /** @var Entity $instance */ foreach ((new EntityProvider())->all() as $key => $instance) { - $counts[$key] = $instance->newQuery()->onlyTrashed()->count(); + /** @var Builder $query */ + $query = $instance->newQuery(); + $counts[$key] = $query->onlyTrashed()->count(); } return $counts; @@ -344,9 +344,9 @@ class TrashCan $entity->deletions()->delete(); $entity->favourites()->delete(); - if ($entity instanceof HasCoverImage && $entity->cover) { + if ($entity instanceof HasCoverImage && $entity->cover()->exists()) { $imageService = app()->make(ImageService::class); - $imageService->destroy($entity->cover); + $imageService->destroy($entity->cover()->first()); } } } diff --git a/app/Http/Controllers/Api/BookshelfApiController.php b/app/Http/Controllers/Api/BookshelfApiController.php index de7284e61..bd4f23a10 100644 --- a/app/Http/Controllers/Api/BookshelfApiController.php +++ b/app/Http/Controllers/Api/BookshelfApiController.php @@ -75,7 +75,7 @@ class BookshelfApiController extends ApiController $shelf = Bookshelf::visible()->with([ 'tags', 'cover', 'createdBy', 'updatedBy', 'ownedBy', 'books' => function (BelongsToMany $query) { - $query->visible()->get(['id', 'name', 'slug']); + $query->scopes('visible')->get(['id', 'name', 'slug']); }, ])->findOrFail($id); diff --git a/app/Http/Controllers/Api/ChapterApiController.php b/app/Http/Controllers/Api/ChapterApiController.php index 6b226b5f0..8459b8449 100644 --- a/app/Http/Controllers/Api/ChapterApiController.php +++ b/app/Http/Controllers/Api/ChapterApiController.php @@ -70,7 +70,7 @@ class ChapterApiController extends ApiController public function read(string $id) { $chapter = Chapter::visible()->with(['tags', 'createdBy', 'updatedBy', 'ownedBy', 'pages' => function (HasMany $query) { - $query->visible()->get(['id', 'name', 'slug']); + $query->scopes('visible')->get(['id', 'name', 'slug']); }])->findOrFail($id); return response()->json($chapter); diff --git a/app/Http/Controllers/BookController.php b/app/Http/Controllers/BookController.php index af44a6689..51cba642c 100644 --- a/app/Http/Controllers/BookController.php +++ b/app/Http/Controllers/BookController.php @@ -114,7 +114,7 @@ class BookController extends Controller { $book = $this->bookRepo->getBySlug($slug); $bookChildren = (new BookContents($book))->getTree(true); - $bookParentShelves = $book->shelves()->visible()->get(); + $bookParentShelves = $book->shelves()->scopes('visible')->get(); View::incrementFor($book); if ($request->has('shelf')) { diff --git a/app/Theming/ThemeService.php b/app/Theming/ThemeService.php index 7bc12c5d1..57336ec5f 100644 --- a/app/Theming/ThemeService.php +++ b/app/Theming/ThemeService.php @@ -52,7 +52,7 @@ class ThemeService */ public function registerCommand(Command $command) { - Artisan::starting(function(Application $application) use ($command) { + Artisan::starting(function (Application $application) use ($command) { $application->addCommands([$command]); }); } diff --git a/app/Uploads/ImageRepo.php b/app/Uploads/ImageRepo.php index 494ff3ac0..bfe4b5977 100644 --- a/app/Uploads/ImageRepo.php +++ b/app/Uploads/ImageRepo.php @@ -103,7 +103,10 @@ class ImageRepo if ($filterType === 'page') { $query->where('uploaded_to', '=', $contextPage->id); } elseif ($filterType === 'book') { - $validPageIds = $contextPage->book->pages()->visible()->pluck('id')->toArray(); + $validPageIds = $contextPage->book->pages() + ->scopes('visible') + ->pluck('id') + ->toArray(); $query->whereIn('uploaded_to', $validPageIds); } }; diff --git a/tests/Unit/FrameworkAssumptionTest.php b/tests/Unit/FrameworkAssumptionTest.php new file mode 100644 index 000000000..8ad45612c --- /dev/null +++ b/tests/Unit/FrameworkAssumptionTest.php @@ -0,0 +1,32 @@ +expectException(BadMethodCallException::class); + $this->expectExceptionMessage('Call to undefined method BookStack\Entities\Models\Page::scopeNotfoundscope()'); + Page::query()->scopes('notfoundscope'); + } + + public function test_scopes_applies_upon_existing() + { + // Page has SoftDeletes trait by default, so we apply our custom scope and ensure + // it stacks on the global scope to filter out deleted items. + $query = Page::query()->scopes('visible')->toSql(); + $this->assertStringContainsString('joint_permissions', $query); + $this->assertStringContainsString('`deleted_at` is null', $query); + } +} diff --git a/version b/version index 4a22367ef..8f6af9bee 100644 --- a/version +++ b/version @@ -1 +1 @@ -v21.10-dev +v21.11-dev