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.

Author: ssddanbrown

app/Access/Oidc/OidcService.php

@@ -222,9 +222,7 @@ protected function processAccessTokenCallback(OidcAccessToken $accessToken, Oidc
             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);
         }
 

app/Config/oidc.php

@@ -47,7 +47,8 @@
     // 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),

tests/Auth/OidcTest.php

@@ -5,6 +5,7 @@
 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 @@ public function test_user_avatar_fetched_from_picture_on_first_login_if_enabled(
         $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 @@ public function test_user_avatar_not_fetched_if_image_data_format_unknown()
         $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 @@ public function test_user_avatar_not_fetched_when_user_already_exists()
         ]);
 
         $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()