OIDC: Updated avatar fetching to run on each login

But only where the user does not already have an avatar assigned.
This aligns with the LDAP avatar fetching logic.

diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php
index 38eecdff35c8b4f4df3a17d97e5b310173bea2ab..d6f6ef156e40020ba162b2f2bd00587d7afb74c5 100644 (file)
--- a/app/Access/Oidc/OidcService.php
+++ b/app/Access/Oidc/OidcService.php
@@ -222,9 +222,7 @@ class OidcService
             throw new OidcException($exception->getMessage());
         }
 
-        // TODO - Update this (and tests and config option comments) to actually align with LDAP system
-        //  which syncs whenever on login or registration, where there's no existing avatar.
-        if ($this->config()['fetch_avatar'] && $user->wasRecentlyCreated && $userDetails->picture) {
+        if ($this->config()['fetch_avatar'] && !$user->avatar()->exists() && $userDetails->picture) {
             $this->userAvatars->assignToUserFromUrl($user, $userDetails->picture);
         }
 

diff --git a/app/Config/oidc.php b/app/Config/oidc.php
index 49427cb12fd7d2f518b8295e3de67c1b723cf528..1d3193eeecc3901fec6b71a969c89fc21d992338 100644 (file)
--- a/app/Config/oidc.php
+++ b/app/Config/oidc.php
@@ -47,7 +47,8 @@ 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 initial login.
+    // 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 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),

diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php
index f4d044bf14dcb243c088854d7e42b92e72ba8b8c..71f883ca6c69830def542c959e68faf9d8cf58b4 100644 (file)
--- a/tests/Auth/OidcTest.php
+++ b/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;
@@ -475,6 +476,26 @@ class OidcTest extends TestCase
         $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]);
@@ -492,11 +513,16 @@ class OidcTest extends TestCase
         $this->assertFalse($user->avatar()->exists());
     }
 
-    public function test_user_avatar_not_fetched_when_user_already_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',
@@ -506,7 +532,8 @@ class OidcTest extends TestCase
         ]);
 
         $editor->refresh();
-        $this->assertFalse($editor->avatar()->exists());
+        $newAvatarData = file_get_contents($this->files->relativeToFullPath($editor->avatar->path));
+        $this->assertEquals($originalImageData, $newAvatarData);
     }
 
     public function test_login_group_sync()