Merge pull request #5626 from BookStackApp/rubentalstra-development

Review of #5429, OIDC avatar fetching

app/Access/Oidc/OidcService.php

@@ -11,6 +11,7 @@ use BookStack\Exceptions\UserRegistrationException;
 use BookStack\Facades\Theme;
 use BookStack\Http\HttpRequestService;
 use BookStack\Theming\ThemeEvents;
+use BookStack\Uploads\UserAvatars;
 use BookStack\Users\Models\User;
 use Illuminate\Support\Facades\Cache;
 use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider;
@@ -26,7 +27,8 @@ class OidcService
         protected RegistrationService $registrationService,
         protected LoginService $loginService,
         protected HttpRequestService $http,
-        protected GroupSyncService $groupService
+        protected GroupSyncService $groupService,
+        protected UserAvatars $userAvatars
     ) {
     }
 
@@ -220,6 +222,10 @@ class OidcService
             throw new OidcException($exception->getMessage());
         }
 
+        if ($this->config()['fetch_avatar'] && !$user->avatar()->exists() && $userDetails->picture) {
+            $this->userAvatars->assignToUserFromUrl($user, $userDetails->picture);
+        }
+
         if ($this->shouldSyncGroups()) {
             $detachExisting = $this->config()['remove_from_groups'];
             $this->groupService->syncUserWithFoundGroups($user, $userDetails->groups ?? [], $detachExisting);

app/Access/Oidc/OidcUserDetails.php

@@ -11,6 +11,7 @@ class OidcUserDetails
         public ?string $email = null,
         public ?string $name = null,
         public ?array $groups = null,
+        public ?string $picture = null,
     ) {
     }
 
@@ -40,15 +41,16 @@ class OidcUserDetails
         $this->email = $claims->getClaim('email') ?? $this->email;
         $this->name = static::getUserDisplayName($displayNameClaims, $claims) ?? $this->name;
         $this->groups = static::getUserGroups($groupsClaim, $claims) ?? $this->groups;
+        $this->picture = static::getPicture($claims) ?: $this->picture;
     }
 
-    protected static function getUserDisplayName(string $displayNameClaims, ProvidesClaims $token): string
+    protected static function getUserDisplayName(string $displayNameClaims, ProvidesClaims $claims): string
     {
         $displayNameClaimParts = explode('|', $displayNameClaims);
 
         $displayName = [];
         foreach ($displayNameClaimParts as $claim) {
-            $component = $token->getClaim(trim($claim)) ?? '';
+            $component = $claims->getClaim(trim($claim)) ?? '';
             if ($component !== '') {
                 $displayName[] = $component;
             }
@@ -57,13 +59,13 @@ class OidcUserDetails
         return implode(' ', $displayName);
     }
 
-    protected static function getUserGroups(string $groupsClaim, ProvidesClaims $token): ?array
+    protected static function getUserGroups(string $groupsClaim, ProvidesClaims $claims): ?array
     {
         if (empty($groupsClaim)) {
             return null;
         }
 
-        $groupsList = Arr::get($token->getAllClaims(), $groupsClaim);
+        $groupsList = Arr::get($claims->getAllClaims(), $groupsClaim);
         if (!is_array($groupsList)) {
             return null;
         }
@@ -72,4 +74,14 @@ class OidcUserDetails
             return is_string($val);
         }));
     }
+
+    protected static function getPicture(ProvidesClaims $claims): ?string
+    {
+        $picture = $claims->getClaim('picture');
+        if (is_string($picture) && str_starts_with($picture, 'http')) {
+            return $picture;
+        }
+
+        return null;
+    }
 }

app/Config/oidc.php

@@ -47,6 +47,12 @@ return [
     // Multiple values can be provided comma seperated.
     'additional_scopes' => env('OIDC_ADDITIONAL_SCOPES', null),
 
+    // Enable fetching of the user's avatar from the 'picture' claim on login.
+    // Will only be fetched if the user doesn't already have an avatar image assigned.
+    // This can be a security risk due to performing server-side fetching (with up to 3 redirects) of
+    // data from external URLs. Only enable if you trust the OIDC auth provider to provide safe URLs for user images.
+    'fetch_avatar' => env('OIDC_FETCH_AVATAR', false),
+
     // Group sync options
     // Enable syncing, upon login, of OIDC groups to BookStack roles
     'user_to_groups' => env('OIDC_USER_TO_GROUPS', false),

app/Uploads/UserAvatars.php

@@ -5,6 +5,7 @@ namespace BookStack\Uploads;
 use BookStack\Exceptions\HttpFetchException;
 use BookStack\Http\HttpRequestService;
 use BookStack\Users\Models\User;
+use BookStack\Util\WebSafeMimeSniffer;
 use Exception;
 use GuzzleHttp\Psr7\Request;
 use Illuminate\Support\Facades\Log;
@@ -53,6 +54,33 @@ class UserAvatars
         }
     }
 
+    /**
+     * Assign a new avatar image to the given user by fetching from a remote URL.
+     */
+    public function assignToUserFromUrl(User $user, string $avatarUrl): void
+    {
+        try {
+            $this->destroyAllForUser($user);
+            $imageData = $this->getAvatarImageData($avatarUrl);
+
+            $mime = (new WebSafeMimeSniffer())->sniff($imageData);
+            [$format, $type] = explode('/', $mime, 2);
+            if ($format !== 'image' || !ImageService::isExtensionSupported($type)) {
+                return;
+            }
+
+            $avatar = $this->createAvatarImageFromData($user, $imageData, $type);
+            $user->avatar()->associate($avatar);
+            $user->save();
+        } catch (Exception $e) {
+            Log::error('Failed to save user avatar image from URL', [
+                'exception' => $e->getMessage(),
+                'url'       => $avatarUrl,
+                'user_id'   => $user->id,
+            ]);
+        }
+    }
+
     /**
      * Destroy all user avatars uploaded to the given user.
      */
@@ -105,7 +133,7 @@ class UserAvatars
     }
 
     /**
-     * Gets an image from url and returns it as a string of image data.
+     * Get an image from a URL and return it as a string of image data.
      *
      * @throws HttpFetchException
      */
@@ -113,7 +141,19 @@ class UserAvatars
     {
         try {
             $client = $this->http->buildClient(5);
-            $response = $client->sendRequest(new Request('GET', $url));
+            $responseCount = 0;
+
+            do {
+                $response = $client->sendRequest(new Request('GET', $url));
+                $responseCount++;
+                $isRedirect = ($response->getStatusCode() === 301 || $response->getStatusCode() === 302);
+                $url = $response->getHeader('Location')[0] ?? '';
+            } while ($responseCount < 3 && $isRedirect && is_string($url) && str_starts_with($url, 'http'));
+
+            if ($responseCount === 3) {
+                throw new HttpFetchException("Failed to fetch image, max redirect limit of 3 tries reached. Last fetched URL: {$url}");
+            }
+
             if ($response->getStatusCode() !== 200) {
                 throw new HttpFetchException(trans('errors.cannot_get_image_from_url', ['url' => $url]));
             }

app/Users/Models/User.php

@@ -45,6 +45,7 @@ use Illuminate\Support\Collection;
  * @property string     $system_name
  * @property Collection $roles
  * @property Collection $mfaValues
+ * @property ?Image     $avatar
  */
 class User extends Model implements AuthenticatableContract, CanResetPasswordContract, Loggable, Sluggable
 {

tests/Auth/OidcTest.php

@@ -5,6 +5,7 @@ namespace Tests\Auth;
 use BookStack\Activity\ActivityType;
 use BookStack\Facades\Theme;
 use BookStack\Theming\ThemeEvents;
+use BookStack\Uploads\UserAvatars;
 use BookStack\Users\Models\Role;
 use BookStack\Users\Models\User;
 use GuzzleHttp\Psr7\Response;
@@ -41,6 +42,7 @@ class OidcTest extends TestCase
             'oidc.discover'               => false,
             'oidc.dump_user_details'      => false,
             'oidc.additional_scopes'      => '',
+            'odic.fetch_avatar'           => false,
             'oidc.user_to_groups'         => false,
             'oidc.groups_claim'           => 'group',
             'oidc.remove_from_groups'     => false,
@@ -457,6 +459,105 @@ class OidcTest extends TestCase
         ]);
     }
 
+    public function test_user_avatar_fetched_from_picture_on_first_login_if_enabled()
+    {
+        config()->set(['oidc.fetch_avatar' => true]);
+
+        $this->runLogin([
+            'email' => 'avatar@example.com',
+            'picture' => 'https://example.com/my-avatar.jpg',
+        ], [
+            new Response(200, ['Content-Type' => 'image/jpeg'], $this->files->jpegImageData())
+        ]);
+
+        $user = User::query()->where('email', '=', 'avatar@example.com')->first();
+        $this->assertNotNull($user);
+
+        $this->assertTrue($user->avatar()->exists());
+    }
+
+    public function test_user_avatar_fetched_for_existing_user_when_no_avatar_already_assigned()
+    {
+        config()->set(['oidc.fetch_avatar' => true]);
+        $editor = $this->users->editor();
+        $editor->external_auth_id = 'benny509';
+        $editor->save();
+
+        $this->assertFalse($editor->avatar()->exists());
+
+        $this->runLogin([
+            'picture' => 'https://example.com/my-avatar.jpg',
+            'sub' => 'benny509',
+        ], [
+            new Response(200, ['Content-Type' => 'image/jpeg'], $this->files->jpegImageData())
+        ]);
+
+        $editor->refresh();
+        $this->assertTrue($editor->avatar()->exists());
+    }
+
+    public function test_user_avatar_not_fetched_if_image_data_format_unknown()
+    {
+        config()->set(['oidc.fetch_avatar' => true]);
+
+        $this->runLogin([
+            'email' => 'avatar-format@example.com',
+            'picture' => 'https://example.com/my-avatar.jpg',
+        ], [
+            new Response(200, ['Content-Type' => 'image/jpeg'], str_repeat('abc123', 5))
+        ]);
+
+        $user = User::query()->where('email', '=', 'avatar-format@example.com')->first();
+        $this->assertNotNull($user);
+
+        $this->assertFalse($user->avatar()->exists());
+    }
+
+    public function test_user_avatar_not_fetched_when_avatar_already_assigned()
+    {
+        config()->set(['oidc.fetch_avatar' => true]);
+        $editor = $this->users->editor();
+        $editor->external_auth_id = 'benny509';
+        $editor->save();
+
+        $avatars = $this->app->make(UserAvatars::class);
+        $originalImageData = $this->files->pngImageData();
+        $avatars->assignToUserFromExistingData($editor, $originalImageData, 'png');
+
+        $this->runLogin([
+            'picture' => 'https://example.com/my-avatar.jpg',
+            'sub' => 'benny509',
+        ], [
+            new Response(200, ['Content-Type' => 'image/jpeg'], $this->files->jpegImageData())
+        ]);
+
+        $editor->refresh();
+        $newAvatarData = file_get_contents($this->files->relativeToFullPath($editor->avatar->path));
+        $this->assertEquals($originalImageData, $newAvatarData);
+    }
+
+    public function test_user_avatar_fetch_follows_up_to_three_redirects()
+    {
+        config()->set(['oidc.fetch_avatar' => true]);
+
+        $logger = $this->withTestLogger();
+
+        $this->runLogin([
+            'email' => 'avatar@example.com',
+            'picture' => 'https://example.com/my-avatar.jpg',
+        ], [
+            new Response(302, ['Location' => 'https://example.com/a']),
+            new Response(302, ['Location' => 'https://example.com/b']),
+            new Response(302, ['Location' => 'https://example.com/c']),
+            new Response(302, ['Location' => 'https://example.com/d']),
+        ]);
+
+        $user = User::query()->where('email', '=', 'avatar@example.com')->first();
+        $this->assertFalse($user->avatar()->exists());
+
+        $this->assertStringContainsString('"Failed to fetch image, max redirect limit of 3 tries reached. Last fetched URL: https://example.com/c"', $logger->getRecords()[0]->formatted);
+    }
+
     public function test_login_group_sync()
     {
         config()->set([

tests/Helpers/FileProvider.php

@@ -60,6 +60,14 @@ class FileProvider
         return file_get_contents($this->testFilePath('test-image.png'));
     }
 
+    /**
+     * Get raw data for a Jpeg image test file.
+     */
+    public function jpegImageData(): string
+    {
+        return file_get_contents($this->testFilePath('test-image.jpg'));
+    }
+
     /**
      * Get the expected relative path for an uploaded image of the given type and filename.
      */

tests/Permissions/EntityPermissionsTest.php

@@ -184,7 +184,7 @@ class EntityPermissionsTest extends TestCase
 
         $this->get($bookUrl . '/edit')->assertRedirect('/');
         $this->get('/')->assertSee('You do not have permission');
-        $this->get($bookPage->getUrl() . '/edit')->assertRedirect('/');
+        $this->get($bookPage->getUrl() . '/edit')->assertRedirect($bookPage->getUrl());
         $this->get('/')->assertSee('You do not have permission');
         $this->get($bookChapter->getUrl() . '/edit')->assertRedirect('/');
         $this->get('/')->assertSee('You do not have permission');
@@ -282,7 +282,7 @@ class EntityPermissionsTest extends TestCase
 
         $this->get($chapterUrl . '/edit')->assertRedirect('/');
         $this->get('/')->assertSee('You do not have permission');
-        $this->get($chapterPage->getUrl() . '/edit')->assertRedirect('/');
+        $this->get($chapterPage->getUrl() . '/edit')->assertRedirect($chapterPage->getUrl());
         $this->get('/')->assertSee('You do not have permission');
 
         $this->setRestrictionsForTestRoles($chapter, ['view', 'update']);
@@ -341,7 +341,7 @@ class EntityPermissionsTest extends TestCase
 
         $this->setRestrictionsForTestRoles($page, ['view', 'delete']);
 
-        $this->get($pageUrl . '/edit')->assertRedirect('/');
+        $this->get($pageUrl . '/edit')->assertRedirect($pageUrl);
         $this->get('/')->assertSee('You do not have permission');
 
         $this->setRestrictionsForTestRoles($page, ['view', 'update']);
@@ -565,7 +565,7 @@ class EntityPermissionsTest extends TestCase
 
         $this->get($bookUrl . '/edit')->assertRedirect('/');
         $this->get('/')->assertSee('You do not have permission');
-        $this->get($bookPage->getUrl() . '/edit')->assertRedirect('/');
+        $this->get($bookPage->getUrl() . '/edit')->assertRedirect($bookPage->getUrl());
         $this->get('/')->assertSee('You do not have permission');
         $this->get($bookChapter->getUrl() . '/edit')->assertRedirect('/');
         $this->get('/')->assertSee('You do not have permission');

tests/Permissions/RolePermissionsTest.php

@@ -2,7 +2,6 @@
 
 namespace Tests\Permissions;
 
-use BookStack\Activity\ActivityType;
 use BookStack\Activity\Models\Comment;
 use BookStack\Entities\Models\Book;
 use BookStack\Entities\Models\Bookshelf;
@@ -10,7 +9,6 @@ use BookStack\Entities\Models\Chapter;
 use BookStack\Entities\Models\Entity;
 use BookStack\Entities\Models\Page;
 use BookStack\Uploads\Image;
-use BookStack\Users\Models\Role;
 use BookStack\Users\Models\User;
 use Illuminate\Testing\TestResponse;
 use Tests\TestCase;
@@ -152,10 +150,14 @@ class RolePermissionsTest extends TestCase
     /**
      * Check a standard entity access permission.
      */
-    private function checkAccessPermission(string $permission, array $accessUrls = [], array $visibles = [])
+    private function checkAccessPermission(
-    {
+        string $permission,
+        array $accessUrls = [],
+        array $visibles = [],
+        string $expectedRedirectUri = '/',
+    ) {
         foreach ($accessUrls as $url) {
-            $this->actingAs($this->user)->get($url)->assertRedirect('/');
+            $this->actingAs($this->user)->get($url)->assertRedirect($expectedRedirectUri);
         }
 
         foreach ($visibles as $url => $text) {
@@ -535,11 +537,11 @@ class RolePermissionsTest extends TestCase
             $ownPage->getUrl() . '/edit',
         ], [
             $ownPage->getUrl() => 'Edit',
-        ]);
+        ], $ownPage->getUrl());
 
         $resp = $this->get($otherPage->getUrl());
         $this->withHtml($resp)->assertElementNotContains('.action-buttons', 'Edit');
-        $this->get($otherPage->getUrl() . '/edit')->assertRedirect('/');
+        $this->get($otherPage->getUrl() . '/edit')->assertRedirect($otherPage->getUrl());
     }
 
     public function test_page_edit_all_permission()
@@ -550,7 +552,7 @@ class RolePermissionsTest extends TestCase
             $otherPage->getUrl('/edit'),
         ], [
             $otherPage->getUrl() => 'Edit',
-        ]);
+        ], $otherPage->getUrl());
     }
 
     public function test_page_delete_own_permission()