Entities: Reviewed TODO notes

Author: ssddanbrown

app/Access/Mfa/MfaSession.php

@@ -11,7 +11,6 @@ class MfaSession
      */
     public function isRequiredForUser(User $user): bool
     {
-        // TODO - Test both these cases
         return $user->mfaValues()->exists() || $this->userRoleEnforcesMfa($user);
     }
 

app/Entities/Models/Entity.php

@@ -214,8 +214,6 @@ public function matchesOrContains(self $entity): bool
      */
     public function activity(): MorphMany
     {
-        // TODO - Ensure this is scoped to entity type properly.
-        //   Add test if not.
         return $this->morphMany(Activity::class, 'loggable')
             ->orderBy('created_at', 'desc');
     }

app/Entities/Queries/ChapterQueries.php

@@ -61,7 +61,6 @@ public function usingSlugs(string $bookSlug, string $chapterSlug): Builder
 
     public function visibleForList(): Builder
     {
-        // TODO - Review this is working as expected
         return $this->start()
             ->scopes('visible')
             ->select(array_merge(static::$listAttributes, ['book_slug' => function ($builder) {

app/Entities/Queries/PageQueries.php

@@ -120,7 +120,6 @@ public function visibleTemplates(bool $includeContents = false): Builder
 
     protected function mergeBookSlugForSelect(array $columns): array
     {
-        // TODO - Review this is working as expected
         return array_merge($columns, ['book_slug' => function ($builder) {
             $builder->select('slug')
                 ->from('entities as books')

app/Entities/Tools/TrashCan.php

@@ -220,7 +220,9 @@ protected function destroyPage(Page $page): int
         //   But does that present visibility/permission issues if they used to retain their old
         //   unused ID?
         //   If so, might be better to leave them as-is like before, but ensure the maintenance
-        //   cleanup command/action can find these "orged" images and delete them.
+        //   cleanup command/action can find these "orphaned" images and delete them.
+        //   But that would leave potential attachment to new pages on increment reset scenarios.
+        //   Need to review permission scenarios for null field values relative to storage options.
 
         $page->forceDelete();
 
@@ -401,11 +403,11 @@ protected function destroyCommonRelations(Entity $entity)
         $entity->referencesTo()->delete();
         $entity->referencesFrom()->delete();
 
-        // TODO - Update
-
         if ($entity instanceof HasCoverInterface && $entity->coverInfo()->exists()) {
             $imageService = app()->make(ImageService::class);
             $imageService->destroy($entity->coverInfo()->getImage());
         }
+
+        $entity->relatedData()->delete();
     }
 }

tests/Auth/MfaConfigurationTest.php

@@ -6,6 +6,7 @@
 use BookStack\Activity\ActivityType;
 use BookStack\Users\Models\Role;
 use BookStack\Users\Models\User;
+use Illuminate\Support\Facades\Hash;
 use PragmaRX\Google2FA\Google2FA;
 use Tests\TestCase;
 
@@ -166,6 +167,36 @@ public function test_remove_mfa_method()
         $this->assertEquals(0, $admin->mfaValues()->count());
     }
 
+    public function test_mfa_required_if_set_on_role()
+    {
+        $user = $this->users->viewer();
+        $user->password = Hash::make('password');
+        $user->save();
+        /** @var Role $role */
+        $role = $user->roles()->first();
+        $role->mfa_enforced = true;
+        $role->save();
+
+        $resp = $this->post('/login', ['email' => $user->email, 'password' => 'password']);
+        $this->assertFalse(auth()->check());
+        $resp->assertRedirect('/mfa/verify');
+    }
+
+    public function test_mfa_required_if_mfa_option_configured()
+    {
+        $user = $this->users->viewer();
+        $user->password = Hash::make('password');
+        $user->save();
+        $user->mfaValues()->create([
+            'method' => MfaValue::METHOD_TOTP,
+            'value'  => 'test',
+        ]);
+
+        $resp = $this->post('/login', ['email' => $user->email, 'password' => 'password']);
+        $this->assertFalse(auth()->check());
+        $resp->assertRedirect('/mfa/verify');
+    }
+
     public function test_totp_setup_url_shows_correct_user_when_setup_forced_upon_login()
     {
         $admin = $this->users->admin();

tests/Settings/RecycleBinTest.php

@@ -84,9 +84,11 @@ public function test_recycle_bin_empty()
 
         $this->assertTrue(Deletion::query()->count() === 0);
         $this->assertDatabaseMissing('entities', ['id' => $book->id, 'type' => 'book']);
-        $this->assertDatabaseMissing('entities', ['id' => $page->id, 'type' => 'page']);
+        $this->assertDatabaseMissing('entity_container_data', ['entity_id' => $book->id, 'entity_type' => 'book']);
         $this->assertDatabaseMissing('entities', ['id' => $book->pages->first()->id, 'type' => 'page']);
+        $this->assertDatabaseMissing('entity_page_data', ['page_id' => $book->pages->first()->id]);
         $this->assertDatabaseMissing('entities', ['id' => $book->chapters->first()->id, 'type' => 'chapter']);
+        $this->assertDatabaseMissing('entity_container_data', ['entity_id' => $book->chapters->first()->id, 'entity_type' => 'chapter']);
 
         $itemCount = 2 + $book->pages->count() + $book->chapters->count();
         $redirectReq = $this->get('/settings/recycle-bin');
@@ -125,8 +127,11 @@ public function test_permanent_delete()
         $this->assertTrue(Deletion::query()->count() === 0);
 
         $this->assertDatabaseMissing('entities', ['id' => $book->id, 'type' => 'book']);
+        $this->assertDatabaseMissing('entity_container_data', ['entity_id' => $book->id, 'entity_type' => 'book']);
         $this->assertDatabaseMissing('entities', ['id' => $book->pages->first()->id, 'type' => 'page']);
+        $this->assertDatabaseMissing('entity_page_data', ['page_id' => $book->pages->first()->id]);
         $this->assertDatabaseMissing('entities', ['id' => $book->chapters->first()->id, 'type' => 'chapter']);
+        $this->assertDatabaseMissing('entity_container_data', ['entity_id' => $book->chapters->first()->id, 'entity_type' => 'chapter']);
 
         $itemCount = 1 + $book->pages->count() + $book->chapters->count();
         $redirectReq = $this->get('/settings/recycle-bin');