From 5c26897d8815d807cd6ebb8875a9bf45ebbafa47 Mon Sep 17 00:00:00 2001 From: MGatner Date: Mon, 3 Jan 2022 01:20:25 +0000 Subject: [PATCH 1/3] Refactor and optimize tests --- .gitignore | 2 +- UPGRADING.md | 3 +- composer.json | 3 +- src/Config/Registrar.php | 17 ++ src/Controllers/Files.php | 5 +- src/Models/FileModel.php | 6 - tests/_support/FeatureTestCase.php | 5 - tests/_support/Models/UserModel.php | 26 --- tests/_support/TestCase.php | 57 +++++- tests/_support/VFS/{storage => }/image.jpg | Bin tests/feature/DisplayTest.php | 8 +- tests/feature/PermissionsTest.php | 216 +++++---------------- tests/feature/UserTest.php | 21 +- tests/misc/ControllerTest.php | 9 +- tests/misc/EntityTest.php | 7 +- tests/misc/ModelTest.php | 9 +- 16 files changed, 132 insertions(+), 262 deletions(-) delete mode 100644 tests/_support/Models/UserModel.php rename tests/_support/VFS/{storage => }/image.jpg (100%) diff --git a/.gitignore b/.gitignore index 11192f3..4fd0807 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ -vendor/ +/vendor/ build/ phpunit*.xml phpunit diff --git a/UPGRADING.md b/UPGRADING.md index 3a9889a..382828c 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -5,6 +5,7 @@ * Switches to `Tatter\Preferences` for managing persistent settings; read more below * Drops `Tatter\Audits` as a dependency and adds it as a suggestion; read more below +* Access rights are now handled via Config file; see [Tatter\Permits](https://github.com/tattersoftware/codeigniter4-permits) for more information ### `Settings` Migration @@ -29,7 +30,7 @@ php spark migrate --all ``` Example model file in **app/Models/FileModel.php**: -``` +```php + */ + public static function Permits() + { + return [ + 'files' => [ + 'userKey' => 'user_id', + 'pivotKey' => 'file_id', + 'pivotTable' => 'files_users', + ], + ]; + } } diff --git a/src/Controllers/Files.php b/src/Controllers/Files.php index 4b3f18e..3d78bd6 100644 --- a/src/Controllers/Files.php +++ b/src/Controllers/Files.php @@ -23,7 +23,9 @@ class Files extends Controller protected FilesConfig $config; /** - * The model to use, may be a child of this library's. + * The model to use. + * + * @var FileModel */ protected FileModel $model; @@ -156,7 +158,6 @@ public function user($userId = null) } $this->setData([ - 'access' => $this->model->mayAdmin() ? 'manage' : 'display', 'title' => 'User Files', 'userName' => 'User', ]); diff --git a/src/Models/FileModel.php b/src/Models/FileModel.php index d4e08b1..5f1964e 100644 --- a/src/Models/FileModel.php +++ b/src/Models/FileModel.php @@ -34,12 +34,6 @@ class FileModel extends Model 'size' => 'permit_empty|is_natural', ]; - // Permits - protected $mode = 04660; - protected $userKey = 'user_id'; - protected $pivotKey = 'file_id'; - protected $usersPivot = 'files_users'; - //-------------------------------------------------------------------- /** diff --git a/tests/_support/FeatureTestCase.php b/tests/_support/FeatureTestCase.php index 70684bc..2f41f48 100644 --- a/tests/_support/FeatureTestCase.php +++ b/tests/_support/FeatureTestCase.php @@ -2,9 +2,7 @@ namespace Tests\Support; -use CodeIgniter\Config\Factories; use CodeIgniter\Test\FeatureTestTrait; -use Tests\Support\Models\UserModel; /** * @internal @@ -22,9 +20,6 @@ protected function setUp(): void { parent::setUp(); - // Make sure we use the correct UserModel for permissions - Factories::injectMock('models', UserModel::class, new UserModel()); - // Make sure everything is published once $this->publishAll(); } diff --git a/tests/_support/Models/UserModel.php b/tests/_support/Models/UserModel.php deleted file mode 100644 index 152dba4..0000000 --- a/tests/_support/Models/UserModel.php +++ /dev/null @@ -1,26 +0,0 @@ -root); - - // "vendor" gets ignored by .gitignore so rename it after copying to VFS - $path = $this->root->url() . DIRECTORY_SEPARATOR . 'vendor' . DIRECTORY_SEPARATOR; - rename($this->root->url() . '/storage', $path); - $this->testPath = $path . 'image.jpg'; + // Make sure all files are on a single page + $_REQUEST['perPage'] = 200; // Force Files config to the virtual path + $path = self::$root->url() . DIRECTORY_SEPARATOR; $config = config('Files'); $config->setPath($path); Factories::injectMock('config', 'Files', $config); + + // Copy the files to VFS (if necessary) + $this->testPath = $config->getPath() . 'image.jpg'; + if (! is_file($this->testPath)) { + vfsStream::copyFromFileSystem(SUPPORTPATH . 'VFS', self::$root); + } + + // Set up the model + $this->model = model(FileModel::class); // @phpstan-ignore-line + } + + protected function tearDown(): void + { + parent::tearDown(); + + ImposterFactory::reset(); + } + + /** + * Creates a test User with some files. + * + * @return array Tuple of [User, File] + */ + protected function createUserWithFile(): array + { + $user = ImposterFactory::fake(); + $user->id = ImposterFactory::add($user); + + $file = fake(FileModel::class); + $this->model->addToUser($file->id, $user->id); + + return [$user, $file]; } } diff --git a/tests/_support/VFS/storage/image.jpg b/tests/_support/VFS/image.jpg similarity index 100% rename from tests/_support/VFS/storage/image.jpg rename to tests/_support/VFS/image.jpg diff --git a/tests/feature/DisplayTest.php b/tests/feature/DisplayTest.php index 4b2155b..eacd523 100644 --- a/tests/feature/DisplayTest.php +++ b/tests/feature/DisplayTest.php @@ -8,7 +8,8 @@ */ final class DisplayTest extends FeatureTestCase { - protected $refresh = true; + protected $refresh = true; + protected $refreshVfs = true; public function testNoFiles() { @@ -144,8 +145,6 @@ public function testSearches(string $keyword) public function testPages() { - $_REQUEST['perPage'] = 2; - for ($i = 0; $i < 2; $i++) { $file = fake(FileModel::class); } @@ -156,7 +155,8 @@ public function testPages() ]); // Last file should be on the next page - $result = $this->get('files'); + $_REQUEST['perPage'] = 2; + $result = $this->get('files'); $result->assertStatus(200); $result->assertDontSee($file->filename); diff --git a/tests/feature/PermissionsTest.php b/tests/feature/PermissionsTest.php index 70c0760..53af0a3 100644 --- a/tests/feature/PermissionsTest.php +++ b/tests/feature/PermissionsTest.php @@ -1,10 +1,8 @@ model = model(FileModel::class); // @phpstan-ignore-line - - if (! $this->seeded) { - // Set permissions - model(PermitModel::class)->insertBatch([ - [ - 'name' => 'listFiles', - 'user_id' => self::USER_ID, - ], - [ - 'name' => 'listFiles', - 'user_id' => self::PROCTOR_ID, - ], - [ - 'name' => 'listFiles', - 'user_id' => self::SUPER_ID, - ], - [ - 'name' => 'readFiles', - 'user_id' => self::SUPER_ID, - ], - [ - 'name' => 'adminFiles', - 'user_id' => self::ADMIN_ID, - ], - ]); - - foreach ([self::USER_ID, self::SUPER_ID, self::ADMIN_ID, self::PROCTOR_ID] as $userId) { - $this->createUserFiles($userId); - } + $this->model->setPermits([ + 'list' => Permits::ANYBODY, + ]); - $this->seeded = true; - } + [$user, $file] = $this->createUserWithFile(); - // Make sure all files are on a single page - $_REQUEST['perPage'] = 200; + $result = $this->withSession()->get('files'); + $result->assertStatus(200); + $result->assertSee($file->filename); } - /** - * Creates random files for a user. - * - * @param int $userId User ID to own the files - * @param int $count Number of files to create - */ - protected function createUserFiles(int $userId, int $count = 2) + public function testListsOwnFiles() { - // Create files and assign them to the user - for ($i = 0; $i < abs($count); $i++) { - $file = fake(FileModel::class); + $this->model->setPermits([ + 'list' => Permits::NOBODY, + ]); - $this->model->addToUser($file->id, $userId); - } + [$user, $userFile] = $this->createUserWithFile(); + [$owner, $ownerFile] = $this->createUserWithFile(); + service('auth')->login($owner); + + $result = $this->withSession()->get('files'); + $result->assertStatus(200); + $result->assertSee($ownerFile->filename); + $result->assertDontSee($userFile->filename); } - /** - * Injects a permission mode into the shared FileModel. - * - * @param int $mode Octal mode - */ - protected function setMode(int $mode) + public function testAdminListsFiles() { - $this->model->setMode($mode); + $this->model->setPermits([ + 'list' => Permits::NOBODY, + ]); - Factories::injectMock('models', FileModel::class, $this->model); - } + [$user, $userFile] = $this->createUserWithFile(); + [$admin, $adminFile] = $this->createUserWithFile(); + $admin->permissions = ['files.admin']; + service('auth')->login($admin); - //-------------------------------------------------------------------- + $result = $this->withSession()->get('files'); - public function testDefaultAccessListsAllFiles() - { - $this->assertSame(04660, $this->model->getMode()); - $result = $this->get('files'); $result->assertStatus(200); - - $files = $this->model->getForUser(self::SUPER_ID); - $result->assertSee($files[0]->filename); - - $files = $this->model->getForUser(self::ADMIN_ID); - $result->assertSee($files[0]->filename); + $result->assertSee($adminFile->filename); + $result->assertSee($userFile->filename); } public function testDenyListRedirects() { - $this->setMode(00660); + $this->model->setPermits([ + 'list' => Permits::NOBODY, + ]); $result = $this->get('files'); $result->assertStatus(302); @@ -134,17 +70,19 @@ public function testDenyListRedirects() public function testDenyAjaxReturnsError() { - $this->setMode(00660); + $this->model->setPermits([ + 'list' => Permits::NOBODY, + ]); $result = $this->withHeaders(['X-Requested-With' => 'XMLHttpRequest'])->get('files'); $result->assertStatus(403); $result->assertJSONFragment(['error' => lang('Permits.notPermitted')]); } - public function testAuthenticatedAddOnlyEmptyFile() + public function testUploadMissingFile() { - $this->setMode(04664); - service('auth')->login(self::ADMIN_ID); + [$user, ] = $this->createUserWithFile(); + service('auth')->login($user); $result = $this->withSession() ->withHeaders(['X-Requested-With' => 'XMLHttpRequest']) @@ -152,10 +90,10 @@ public function testAuthenticatedAddOnlyEmptyFile() $result->assertStatus(400); } - public function testAuthenticatedAddOnlyWithInvalidFile() + public function testUploadInvalidFile() { - $this->setMode(04664); - service('auth')->login(self::ADMIN_ID); + [$user, ] = $this->createUserWithFile(); + service('auth')->login($user); $_FILES = [ 'file' => [ @@ -170,6 +108,7 @@ public function testAuthenticatedAddOnlyWithInvalidFile() $result = $this->withSession() ->withHeaders(['X-Requested-With' => 'XMLHttpRequest']) ->post('files/upload'); + $result->assertSee('The file uploaded with success.(0)'); } @@ -209,69 +148,4 @@ public function testAuthenticatedAddOnlyWithValidFile() $this->assertSame('', $result->response()->getBody()); } */ - public function testProctorListsAllFiles() - { - $this->createUserFiles(self::ADMIN_ID); - - $this->setMode(00660); - service('auth')->login(self::PROCTOR_ID); - - $result = $this->withSession()->get('files'); - $result->assertStatus(200); - - $files = $this->model->getForUser(self::ADMIN_ID); - $result->assertSee($files[0]->filename); - } - - public function testAuthenticatedListOwnOnly() - { - $this->setMode(00660); - service('auth')->login(self::PROCTOR_ID); - - $fileOwnByProctor = fake(FileModel::class); - $this->model->addToUser($fileOwnByProctor->id, self::PROCTOR_ID); - - $fileOwnByProctor2 = fake(FileModel::class); - $this->model->addToUser($fileOwnByProctor2->id, self::PROCTOR_ID); - - $fileOwnByAdmin = fake(FileModel::class); - $this->model->addToUser($fileOwnByAdmin->id, self::ADMIN_ID); - - $result = $this->withSession()->get('files/user/' . self::PROCTOR_ID); - $result->assertStatus(200); - - $files = $this->model->getForUser(self::PROCTOR_ID); - $result->assertSee($files[0]->filename); - $result->assertSee($files[1]->filename); - - $files = $this->model->getForUser(self::ADMIN_ID); - $result->assertDontSee($files[0]->filename); - } - - /** - * @dataProvider accessProvider - */ - public function testAdminAccess(int $mode) - { - $this->setMode($mode); - service('auth')->login(self::ADMIN_ID); - - $result = $this->withSession()->get('files'); - $result->assertStatus(200); - - $files = $this->model->getForUser(self::ADMIN_ID); - $result->assertSee($files[0]->filename); - $files = $this->model->getForUser(self::USER_ID); - $result->assertSee($files[0]->filename); - } - - public function accessProvider() - { - return [ - ['read' => 00444], - ['write' => 00222], - ['execute' => 00111], - ['read write execute' => 00777], - ]; - } } diff --git a/tests/feature/UserTest.php b/tests/feature/UserTest.php index af913a9..2221a7f 100644 --- a/tests/feature/UserTest.php +++ b/tests/feature/UserTest.php @@ -1,6 +1,5 @@ login($userId); + [$user, $file] = $this->createUserWithFile(); + service('auth')->login($user); - /** @var FileModel $model */ - $model = model(FileModel::class); - $model->addToUser($file->id, $userId); - - $result = $this->get('files/user/' . $userId); + $result = $this->get('files/user/' . $user->id); $result->assertSee('Manage My Files', 'h1'); $result->assertSee($file->filename); } public function testShowsOtherFiles() { - $file = fake(FileModel::class); - $userId = 13; - service('auth')->login($userId); - - /** @var FileModel $model */ - $model = model(FileModel::class); - $model->addToUser($file->id, $userId); + [$user, $file] = $this->createUserWithFile(); + service('auth')->login($user); $result = $this->get('files/user/1000'); $result->assertSee('Browse User Files', 'h1'); diff --git a/tests/misc/ControllerTest.php b/tests/misc/ControllerTest.php index 174ee57..bdba921 100644 --- a/tests/misc/ControllerTest.php +++ b/tests/misc/ControllerTest.php @@ -22,6 +22,8 @@ final class ControllerTest extends TestCase */ protected $controller; + protected $refreshVfs = true; + protected function setUp(): void { parent::setUp(); @@ -29,13 +31,6 @@ protected function setUp(): void $this->controller(Files::class); } - protected function tearDown(): void - { - parent::tearDown(); - - $this->controller = null; - } - public function testSetPreferencesUsesValidInput() { $_REQUEST = [ diff --git a/tests/misc/EntityTest.php b/tests/misc/EntityTest.php index 3202d46..20eceb6 100644 --- a/tests/misc/EntityTest.php +++ b/tests/misc/EntityTest.php @@ -1,7 +1,6 @@ createFromPath($this->testPath); + $file = $this->model->createFromPath($this->testPath); $expected = config('Files')->getPath() . $file->localname; diff --git a/tests/misc/ModelTest.php b/tests/misc/ModelTest.php index 2688817..f45ce1d 100644 --- a/tests/misc/ModelTest.php +++ b/tests/misc/ModelTest.php @@ -9,14 +9,7 @@ */ final class ModelTest extends TestCase { - protected FileModel $model; - - protected function setUp(): void - { - parent::setUp(); - - $this->model = model(FileModel::class); // @phpstan-ignore-line - } + protected $refreshVfs = true; public function testAddToUser() { From 75c0eafb93a621c1c4b5a2d83e77458671bc1e80 Mon Sep 17 00:00:00 2001 From: MGatner Date: Sun, 23 Jan 2022 23:22:21 +0000 Subject: [PATCH 2/3] Fix initialized types --- src/Controllers/Files.php | 2 -- src/Entities/File.php | 2 +- src/Structures/FileObject.php | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Controllers/Files.php b/src/Controllers/Files.php index 3d78bd6..4d7922a 100644 --- a/src/Controllers/Files.php +++ b/src/Controllers/Files.php @@ -24,8 +24,6 @@ class Files extends Controller /** * The model to use. - * - * @var FileModel */ protected FileModel $model; diff --git a/src/Entities/File.php b/src/Entities/File.php index 37bb218..fba67cd 100644 --- a/src/Entities/File.php +++ b/src/Entities/File.php @@ -18,7 +18,7 @@ class File extends Entity /** * Resolved path to the default thumbnail */ - protected static ?string $defaultThumbnail; + protected static ?string $defaultThumbnail = null; /** * Returns the absolute path to the configured default thumbnail diff --git a/src/Structures/FileObject.php b/src/Structures/FileObject.php index 0a620e2..ac4dbb6 100644 --- a/src/Structures/FileObject.php +++ b/src/Structures/FileObject.php @@ -16,7 +16,7 @@ class FileObject extends File /** * Base file name to override disk version */ - protected ?string $basename; + protected ?string $basename = null; /** * Returns the full path to this file From 6005f3901562f5db26523ec4baf997d3cdac8a99 Mon Sep 17 00:00:00 2001 From: MGatner Date: Mon, 24 Jan 2022 01:07:57 +0000 Subject: [PATCH 3/3] Fix workflows --- .github/workflows/rector.yml | 2 +- phpunit.xml.dist | 1 - rector.php | 5 +++++ src/Structures/FileObject.php | 2 +- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/workflows/rector.yml b/.github/workflows/rector.yml index fff0208..ff3bcde 100644 --- a/.github/workflows/rector.yml +++ b/.github/workflows/rector.yml @@ -64,5 +64,5 @@ jobs: - name: Analyze for refactoring run: | - composer global require --dev rector/rector:^0.12.10 + composer global require --dev rector/rector:dev-main rector process --dry-run --no-progress-bar diff --git a/phpunit.xml.dist b/phpunit.xml.dist index ac88cfc..aa9fd3d 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -10,7 +10,6 @@ convertErrorsToExceptions="true" convertNoticesToExceptions="true" convertWarningsToExceptions="true" - executionOrder="random" failOnRisky="true" failOnWarning="true" stopOnError="false" diff --git a/rector.php b/rector.php index 6d997d0..34ad228 100644 --- a/rector.php +++ b/rector.php @@ -77,6 +77,11 @@ __DIR__ . '/tests', ], + // Ignore files that should not be namespaced + NormalizeNamespaceByPSR4ComposerAutoloadRector::class => [ + __DIR__ . '/src/Helpers', + ], + // May load view files directly when detecting classes StringClassNameToClassConstantRector::class, diff --git a/src/Structures/FileObject.php b/src/Structures/FileObject.php index ac4dbb6..ff34377 100644 --- a/src/Structures/FileObject.php +++ b/src/Structures/FileObject.php @@ -35,7 +35,7 @@ public function setBasename(?string $basename = null): self * * @param string $suffix Optional suffix to omit from the base name returned */ - public function getBasename($suffix = null): string + public function getBasename($suffix = ''): string { if ($this->basename) { return basename($this->basename, $suffix);