From b64c9b31d51b5d15972c6cb37cfdca37db80d28a Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 24 May 2025 14:36:36 +0100
Subject: [PATCH] OIDC: Added testing coverage for picture fetching

---
 app/Access/Oidc/OidcService.php |   2 ++
 app/Uploads/UserAvatars.php     |   2 +-
 app/Users/Models/User.php       |   1 +
 tests/Auth/OidcTest.php         |  52 ++++++++++++++++++++++++++++++++
 tests/Helpers/FileProvider.php  |   8 +++++
 tests/test-data/test-image.jpg  | Bin 0 -> 268 bytes
 6 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100644 tests/test-data/test-image.jpg

diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php
index 452fda3d8..38eecdff3 100644
--- a/app/Access/Oidc/OidcService.php
+++ b/app/Access/Oidc/OidcService.php
@@ -222,6 +222,8 @@ 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) {
             $this->userAvatars->assignToUserFromUrl($user, $userDetails->picture);
         }
diff --git a/app/Uploads/UserAvatars.php b/app/Uploads/UserAvatars.php
index 1763dcbbd..2c9f7a848 100644
--- a/app/Uploads/UserAvatars.php
+++ b/app/Uploads/UserAvatars.php
@@ -65,7 +65,7 @@ class UserAvatars
 
             $mime = (new WebSafeMimeSniffer())->sniff($imageData);
             [$format, $type] = explode('/', $mime, 2);
-            if ($format !== 'image' || ImageService::isExtensionSupported($type)) {
+            if ($format !== 'image' || !ImageService::isExtensionSupported($type)) {
                 return;
             }
 
diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php
index 3797e7cb0..0d437418b 100644
--- a/app/Users/Models/User.php
+++ b/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
 {
diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php
index 205f75a4d..f4d044bf1 100644
--- a/tests/Auth/OidcTest.php
+++ b/tests/Auth/OidcTest.php
@@ -41,6 +41,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 +458,57 @@ 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_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_user_already_exists()
+    {
+        config()->set(['oidc.fetch_avatar' => true]);
+        $editor = $this->users->editor();
+        $editor->external_auth_id = 'benny509';
+
+        $this->runLogin([
+            'picture' => 'https://example.com/my-avatar.jpg',
+            'sub' => 'benny509',
+        ], [
+            new Response(200, ['Content-Type' => 'image/jpeg'], $this->files->jpegImageData())
+        ]);
+
+        $editor->refresh();
+        $this->assertFalse($editor->avatar()->exists());
+    }
+
     public function test_login_group_sync()
     {
         config()->set([
diff --git a/tests/Helpers/FileProvider.php b/tests/Helpers/FileProvider.php
index 442e036ff..a455e0fb4 100644
--- a/tests/Helpers/FileProvider.php
+++ b/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.
      */
diff --git a/tests/test-data/test-image.jpg b/tests/test-data/test-image.jpg
new file mode 100644
index 0000000000000000000000000000000000000000..9bb74ff04833b969a41dd25c05144a7930ec4791
GIT binary patch
literal 268
zcmex=<NpH&0WUXCHwH#VMg|WcWcYuZ!I|Mdr2uFMBc*2iKgl4-!QjN;!px||z$D1X
zEXer(3<DQ~fB-9_fFSGtTMRtRj6exyK?Zw<=hw>h+pFW<7#R=#a_3=8ej9ot%hR(f
wrhv<c$CmSij*T$KGU+QN!Cdz?zQ0jr%3E+vW%b^Rci+!ADRxd#o#+2e0B6vxbpQYW

literal 0
HcmV?d00001