From 2c3523f6a1237be491f400340f2f887e2ea1a5a9 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 24 May 2021 12:09:28 +0100 Subject: [PATCH 1/2] Updated image permission setting logic To ensure thhat the visibility is still set on local storage options since the previous recent changes could cause problems where in scenarios where the server user could not read images uploaded by the php process user. Closes #2758 --- app/Uploads/ImageService.php | 11 ++++++----- tests/Uploads/ImageTest.php | 22 +++++++++++----------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php index 7e8eedada..293049f4f 100644 --- a/app/Uploads/ImageService.php +++ b/app/Uploads/ImageService.php @@ -140,12 +140,13 @@ class ImageService { $storage->put($path, $data); - // Set visibility if using s3 without an endpoint set. - // Done since this call can break s3-like services but desired for actual - // AWS s3 usage. Attempting to set ACL during above put request requires - // different permissions hence would technically be a breaking change. + // Set visibility when a non-AWS-s3, s3-like storage option is in use. + // Done since this call can break s3-like services but desired for other image stores. + // Attempting to set ACL during above put request requires different permissions + // hence would technically be a breaking change for actual s3 usage. $usingS3 = strtolower(config('filesystems.images')) === 's3'; - if ($usingS3 && is_null(config('filesystems.disks.s3.endpoint'))) { + $usingS3Like = $usingS3 && !is_null(config('filesystems.disks.s3.endpoint')); + if (!$usingS3Like) { $storage->setVisibility($path, 'public'); } } diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php index c03d15dd7..95332565e 100644 --- a/tests/Uploads/ImageTest.php +++ b/tests/Uploads/ImageTest.php @@ -14,7 +14,7 @@ class ImageTest extends TestCase public function test_image_upload() { - $page = Page::first(); + $page = Page::query()->first(); $admin = $this->getAdmin(); $this->actingAs($admin); @@ -38,7 +38,7 @@ class ImageTest extends TestCase public function test_image_display_thumbnail_generation_does_not_increase_image_size() { - $page = Page::first(); + $page = Page::query()->first(); $admin = $this->getAdmin(); $this->actingAs($admin); @@ -108,7 +108,7 @@ class ImageTest extends TestCase public function test_image_usage() { - $page = Page::first(); + $page = Page::query()->first(); $editor = $this->getEditor(); $this->actingAs($editor); @@ -128,7 +128,7 @@ class ImageTest extends TestCase public function test_php_files_cannot_be_uploaded() { - $page = Page::first(); + $page = Page::query()->first(); $admin = $this->getAdmin(); $this->actingAs($admin); @@ -150,7 +150,7 @@ class ImageTest extends TestCase public function test_php_like_files_cannot_be_uploaded() { - $page = Page::first(); + $page = Page::query()->first(); $admin = $this->getAdmin(); $this->actingAs($admin); @@ -202,7 +202,7 @@ class ImageTest extends TestCase ]; foreach ($badNames as $name) { $galleryFile = $this->getTestImage($name); - $page = Page::first(); + $page = Page::query()->first(); $badPath = $this->getTestImagePath('gallery', $name); $this->deleteImage($badPath); @@ -227,7 +227,7 @@ class ImageTest extends TestCase config()->set('filesystems.images', 'local_secure'); $this->asEditor(); $galleryFile = $this->getTestImage('my-secure-test-upload.png'); - $page = Page::first(); + $page = Page::query()->first(); $expectedPath = storage_path('uploads/images/gallery/' . Date('Y-m') . '/my-secure-test-upload.png'); $upload = $this->call('POST', '/images/gallery', ['uploaded_to' => $page->id], [], ['file' => $galleryFile], []); @@ -245,7 +245,7 @@ class ImageTest extends TestCase config()->set('filesystems.images', 'local_secure'); $this->asEditor(); $galleryFile = $this->getTestImage('my-secure-test-upload.png'); - $page = Page::first(); + $page = Page::query()->first(); $expectedPath = storage_path('uploads/images/gallery/' . Date('Y-m') . '/my-secure-test-upload.png'); $upload = $this->call('POST', '/images/gallery', ['uploaded_to' => $page->id], [], ['file' => $galleryFile], []); @@ -282,7 +282,7 @@ class ImageTest extends TestCase public function test_image_delete() { - $page = Page::first(); + $page = Page::query()->first(); $this->asAdmin(); $imageName = 'first-image.png'; $relPath = $this->getTestImagePath('gallery', $imageName); @@ -304,7 +304,7 @@ class ImageTest extends TestCase public function test_image_delete_does_not_delete_similar_images() { - $page = Page::first(); + $page = Page::query()->first(); $this->asAdmin(); $imageName = 'first-image.png'; @@ -383,7 +383,7 @@ class ImageTest extends TestCase public function test_deleted_unused_images() { - $page = Page::first(); + $page = Page::query()->first(); $admin = $this->getAdmin(); $this->actingAs($admin); From c7322a71f7f8ab35c19ed0286178160b56574914 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 24 May 2021 12:55:45 +0100 Subject: [PATCH 2/2] Added theme add social driver redirect configuration callback Allows someone using the theme system to configure the social driver before a redirect action occurs, by passing a callback as an additional param to the theme 'addSocialDriver' method. --- app/Auth/Access/SocialAuthService.php | 52 +++++++++++++++++++++++---- app/Theming/ThemeService.php | 4 +-- dev/docs/logical-theme-system.md | 14 ++++++++ tests/ThemeTest.php | 25 +++++++++++-- 4 files changed, 84 insertions(+), 11 deletions(-) diff --git a/app/Auth/Access/SocialAuthService.php b/app/Auth/Access/SocialAuthService.php index 7c8b66ea5..a03eb2b1d 100644 --- a/app/Auth/Access/SocialAuthService.php +++ b/app/Auth/Access/SocialAuthService.php @@ -19,10 +19,37 @@ use Symfony\Component\HttpFoundation\RedirectResponse; class SocialAuthService { + /** + * The core socialite library used. + * @var Socialite + */ protected $socialite; - protected $socialAccount; - protected $validSocialDrivers = ['google', 'github', 'facebook', 'slack', 'twitter', 'azure', 'okta', 'gitlab', 'twitch', 'discord']; + /** + * The default built-in social drivers we support. + * @var string[] + */ + protected $validSocialDrivers = [ + 'google', + 'github', + 'facebook', + 'slack', + 'twitter', + 'azure', + 'okta', + 'gitlab', + 'twitch', + 'discord' + ]; + + /** + * Callbacks to run when configuring a social driver + * for an initial redirect action. + * Array is keyed by social driver name. + * Callbacks are passed an instance of the driver. + * @var array + */ + protected $configureForRedirectCallbacks = []; /** * SocialAuthService constructor. @@ -39,7 +66,7 @@ class SocialAuthService public function startLogIn(string $socialDriver): RedirectResponse { $driver = $this->validateDriver($socialDriver); - return $this->getSocialDriver($driver)->redirect(); + return $this->getDriverForRedirect($driver)->redirect(); } /** @@ -49,7 +76,7 @@ class SocialAuthService public function startRegister(string $socialDriver): RedirectResponse { $driver = $this->validateDriver($socialDriver); - return $this->getSocialDriver($driver)->redirect(); + return $this->getDriverForRedirect($driver)->redirect(); } /** @@ -227,7 +254,7 @@ class SocialAuthService /** * Provide redirect options per service for the Laravel Socialite driver */ - public function getSocialDriver(string $driverName): Provider + protected function getDriverForRedirect(string $driverName): Provider { $driver = $this->socialite->driver($driverName); @@ -238,6 +265,10 @@ class SocialAuthService $driver->with(['resource' => 'https://graph.windows.net']); } + if (isset($this->configureForRedirectCallbacks[$driverName])) { + $this->configureForRedirectCallbacks[$driverName]($driver); + } + return $driver; } @@ -248,12 +279,19 @@ class SocialAuthService * within the `Config/services.php` file. * Handler should be a Class@method handler to the SocialiteWasCalled event. */ - public function addSocialDriver(string $driverName, array $config, string $socialiteHandler) - { + public function addSocialDriver( + string $driverName, + array $config, + string $socialiteHandler, + callable $configureForRedirect = null + ) { $this->validSocialDrivers[] = $driverName; config()->set('services.' . $driverName, $config); config()->set('services.' . $driverName . '.redirect', url('/login/service/' . $driverName . '/callback')); config()->set('services.' . $driverName . '.name', $config['name'] ?? $driverName); Event::listen(SocialiteWasCalled::class, $socialiteHandler); + if (!is_null($configureForRedirect)) { + $this->configureForRedirectCallbacks[$driverName] = $configureForRedirect; + } } } diff --git a/app/Theming/ThemeService.php b/app/Theming/ThemeService.php index 54e476ae2..895108e3e 100644 --- a/app/Theming/ThemeService.php +++ b/app/Theming/ThemeService.php @@ -53,9 +53,9 @@ class ThemeService /** * @see SocialAuthService::addSocialDriver */ - public function addSocialDriver(string $driverName, array $config, string $socialiteHandler) + public function addSocialDriver(string $driverName, array $config, string $socialiteHandler, callable $configureForRedirect = null) { $socialAuthService = app()->make(SocialAuthService::class); - $socialAuthService->addSocialDriver($driverName, $config, $socialiteHandler); + $socialAuthService->addSocialDriver($driverName, $config, $socialiteHandler, $configureForRedirect); } } \ No newline at end of file diff --git a/dev/docs/logical-theme-system.md b/dev/docs/logical-theme-system.md index fc8e6646f..b950d7df9 100644 --- a/dev/docs/logical-theme-system.md +++ b/dev/docs/logical-theme-system.md @@ -95,4 +95,18 @@ Theme::listen(ThemeEvents::APP_BOOT, function($app) { 'name' => 'Reddit', ], '\SocialiteProviders\Reddit\RedditExtendSocialite@handle'); }); +``` + +In some cases you may need to customize the driver before it performs a redirect. +This can be done by providing a callback as a fourth parameter like so: + +```php +Theme::addSocialDriver('reddit', [ + 'client_id' => 'abc123', + 'client_secret' => 'def456789', + 'name' => 'Reddit', +], '\SocialiteProviders\Reddit\RedditExtendSocialite@handle', function($driver) { + $driver->with(['prompt' => 'select_account']); + $driver->scopes(['open_id']); +}); ``` \ No newline at end of file diff --git a/tests/ThemeTest.php b/tests/ThemeTest.php index 7a0cd49cb..be3fc4ebd 100644 --- a/tests/ThemeTest.php +++ b/tests/ThemeTest.php @@ -1,6 +1,5 @@ setSettings(['registration-enabled' => 'true']); $user = factory(User::class)->make(); - $this->post('/register', ['email' => $user->email, 'name' => $user->name, 'password' => 'password']); + $this->post('/register', ['email' => $user->email, 'name' => $user->name, 'password' => 'password']); $this->assertCount(2, $args); $this->assertEquals('standard', $args[0]); @@ -184,6 +183,28 @@ class ThemeTest extends TestCase $loginResp->assertSee('Super Cat Name'); } + + public function test_add_social_driver_allows_a_configure_for_redirect_callback_to_be_passed() + { + Theme::addSocialDriver( + 'discord', + [ + 'client_id' => 'abc123', + 'client_secret' => 'def456', + 'name' => 'Super Cat Name', + ], + 'SocialiteProviders\Discord\DiscordExtendSocialite@handle', + function ($driver) { + $driver->with(['donkey' => 'donut']); + } + ); + + $loginResp = $this->get('/login/service/discord'); + $redirect = $loginResp->headers->get('location'); + $this->assertStringContainsString('donkey=donut', $redirect); + } + + protected function usingThemeFolder(callable $callback) { // Create a folder and configure a theme