Reviewed PR to add import user avatars va LDAP

- Reduced options to single new configuration paramter instead of two.
- Moved more logic into UserAvatars class.
- Updated LDAP avatar import to also run on login when no image is
  currently set.
- Added thumbnail fetching to search requests.
- Added testing to cover.

Related to PR #2320, and issue #1161

app/Auth/Access/Guards/LdapSessionGuard.php

@@ -90,6 +90,11 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
             $this->ldapService->syncGroups($user, $username);
         }
 
+        // Attach avatar if non-existent
+        if (is_null($user->avatar)) {
+            $this->ldapService->saveAndAttachAvatar($user, $userDetails);
+        }
+
         $this->login($user, $remember);
         return true;
     }
@@ -116,15 +121,7 @@ class LdapSessionGuard extends ExternalBaseSessionGuard
         ];
 
         $user = $this->registrationService->registerUser($details, null, false);
-
+        $this->ldapService->saveAndAttachAvatar($user, $ldapUserDetails);
-        if (config('services.ldap.import_thumbnail_photos')) {
-            $imageService = app()->make(ImageService::class);
-            $image = $imageService->saveNewFromBase64Uri('data:image/jpg;base64,'.base64_encode($ldapUserDetails['avatar']), $ldapUserDetails['uid'].'.jpg', 'user');
-
-            $user['image_id'] = $image->id;
-            $user->save();
-        }
-
         return $user;
     }
 }

app/Auth/Access/LdapService.php

@@ -3,7 +3,9 @@
 use BookStack\Auth\User;
 use BookStack\Exceptions\JsonDebugException;
 use BookStack\Exceptions\LdapException;
+use BookStack\Uploads\UserAvatars;
 use ErrorException;
+use Illuminate\Support\Facades\Log;
 
 /**
  * Class LdapService
@@ -14,15 +16,17 @@ class LdapService extends ExternalAuthService
 
     protected $ldap;
     protected $ldapConnection;
+    protected $userAvatars;
     protected $config;
     protected $enabled;
 
     /**
      * LdapService constructor.
      */
-    public function __construct(Ldap $ldap)
+    public function __construct(Ldap $ldap, UserAvatars $userAvatars)
     {
         $this->ldap = $ldap;
+        $this->userAvatars = $userAvatars;
         $this->config = config('services.ldap');
         $this->enabled = config('auth.method') === 'ldap';
     }
@@ -78,9 +82,11 @@ class LdapService extends ExternalAuthService
         $displayNameAttr = $this->config['display_name_attribute'];
         $thumbnailAttr = $this->config['thumbnail_attribute'];
 
-        $user = $this->getUserWithAttributes($userName, ['cn', 'dn', $idAttr, $emailAttr, $displayNameAttr]);
+        $user = $this->getUserWithAttributes($userName, array_filter([
+            'cn', 'dn', $idAttr, $emailAttr, $displayNameAttr, $thumbnailAttr,
+        ]));
 
-        if ($user === null) {
+        if (is_null($user)) {
             return null;
         }
 
@@ -90,7 +96,7 @@ class LdapService extends ExternalAuthService
             'name' => $this->getUserResponseProperty($user, $displayNameAttr, $userCn),
             'dn' => $user['dn'],
             'email' => $this->getUserResponseProperty($user, $emailAttr, null),
-            'avatar'=> $this->getUserResponseProperty($user, $thumbnailAttr, null),
+            'avatar'=> $thumbnailAttr ? $this->getUserResponseProperty($user, $thumbnailAttr, null) : null,
         ];
 
         if ($this->config['dump_user_details']) {
@@ -352,4 +358,22 @@ class LdapService extends ExternalAuthService
         $userLdapGroups = $this->getUserGroups($username);
         $this->syncWithGroups($user, $userLdapGroups);
     }
+
+    /**
+     * Save and attach an avatar image, if found in the ldap details, and attach
+     * to the given user model.
+     */
+    public function saveAndAttachAvatar(User $user, array $ldapUserDetails): void
+    {
+        if (is_null(config('services.ldap.thumbnail_attribute')) || is_null($ldapUserDetails['avatar'])) {
+            return;
+        }
+
+        try {
+            $imageData = $ldapUserDetails['avatar'];
+            $this->userAvatars->assignToUserFromExistingData($user, $imageData, 'jpg');
+        } catch (\Exception $exception) {
+            Log::info("Failed to use avatar image from LDAP data for user id {$user->id}");
+        }
+    }
 }

app/Auth/UserRepo.php

@@ -8,13 +8,11 @@ use BookStack\Entities\Models\Chapter;
 use BookStack\Entities\Models\Page;
 use BookStack\Exceptions\NotFoundException;
 use BookStack\Exceptions\UserUpdateException;
-use BookStack\Uploads\Image;
 use BookStack\Uploads\UserAvatars;
 use Exception;
 use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Database\Eloquent\Collection;
 use Illuminate\Pagination\LengthAwarePaginator;
-use Images;
 use Log;
 
 class UserRepo
@@ -188,13 +186,7 @@ class UserRepo
         $user->delete();
         
         // Delete user profile images
-        $profileImages = Image::query()->where('type', '=', 'user')
+        $this->userAvatar->destroyAllForUser($user);
-            ->where('uploaded_to', '=', $user->id)
-            ->get();
-
-        foreach ($profileImages as $image) {
-            Images::destroy($image);
-        }
 
         if (!empty($newOwnerId)) {
             $newOwner = User::query()->find($newOwnerId);

app/Config/app.php

@@ -184,7 +184,6 @@ return [
 
         // Custom BookStack
         'Activity' => BookStack\Facades\Activity::class,
-        'Images'   => BookStack\Facades\Images::class,
         'Permissions' => BookStack\Facades\Permissions::class,
         'Theme'    => BookStack\Facades\Theme::class,
     ],

app/Config/services.php

@@ -133,8 +133,7 @@ return [
         'remove_from_groups' => env('LDAP_REMOVE_FROM_GROUPS', false),
         'tls_insecure' => env('LDAP_TLS_INSECURE', false),
         'start_tls' => env('LDAP_START_TLS', false),
-        'import_thumbnail_photos' => env('LDAP_IMPORT_THUMBNAIL_PHOTOS', false),
+        'thumbnail_attribute' => env('LDAP_THUMBNAIL_ATTRIBUTE', null),
-        'thumbnail_attribute' => env('LDAP_THUMBNAIL_ATTRIBUTE', 'thumbnailPhoto'),
     ],
 
 ];

app/Facades/Images.php

@@ -1,16 +0,0 @@
-<?php namespace BookStack\Facades;
-
-use Illuminate\Support\Facades\Facade;
-
-class Images extends Facade
-{
-    /**
-     * Get the registered name of the component.
-     *
-     * @return string
-     */
-    protected static function getFacadeAccessor()
-    {
-        return 'images';
-    }
-}

app/Uploads/Image.php

@@ -3,7 +3,6 @@
 use BookStack\Entities\Models\Page;
 use BookStack\Model;
 use BookStack\Traits\HasCreatorAndUpdater;
-use Images;
 
 class Image extends Model
 {
@@ -14,23 +13,18 @@ class Image extends Model
 
     /**
      * Get a thumbnail for this image.
-     * @param  int $width
-     * @param  int $height
-     * @param bool|false $keepRatio
-     * @return string
      * @throws \Exception
      */
-    public function getThumb($width, $height, $keepRatio = false)
+    public function getThumb(int $width, int $height, bool $keepRatio = false): string
     {
-        return Images::getThumbnail($this, $width, $height, $keepRatio);
+        return app()->make(ImageService::class)->getThumbnail($this, $width, $height, $keepRatio);
     }
 
     /**
      * Get the page this image has been uploaded to.
      * Only applicable to gallery or drawio image types.
-     * @return Page|null
      */
-    public function getPage()
+    public function getPage(): ?Page
     {
         return $this->belongsTo(Page::class, 'uploaded_to')->first();
     }

app/Uploads/UserAvatars.php

@@ -26,6 +26,7 @@ class UserAvatars
         }
 
         try {
+            $this->destroyAllForUser($user);
             $avatar = $this->saveAvatarImage($user);
             $user->avatar()->associate($avatar);
             $user->save();
@@ -34,6 +35,35 @@ class UserAvatars
         }
     }
 
+    /**
+     * Assign a new avatar image to the given user using the given image data.
+     */
+    public function assignToUserFromExistingData(User $user, string $imageData, string $extension): void
+    {
+        try {
+            $this->destroyAllForUser($user);
+            $avatar = $this->createAvatarImageFromData($user, $imageData, $extension);
+            $user->avatar()->associate($avatar);
+            $user->save();
+        } catch (Exception $e) {
+            Log::error('Failed to save user avatar image');
+        }
+    }
+
+    /**
+     * Destroy all user avatars uploaded to the given user.
+     */
+    public function destroyAllForUser(User $user)
+    {
+        $profileImages = Image::query()->where('type', '=', 'user')
+            ->where('uploaded_to', '=', $user->id)
+            ->get();
+
+        foreach ($profileImages as $image) {
+            $this->imageService->destroy($image);
+        }
+    }
+
     /**
      * Save an avatar image from an external service.
      * @throws Exception
@@ -50,8 +80,16 @@ class UserAvatars
         ];
 
         $userAvatarUrl = strtr($avatarUrl, $replacements);
-        $imageName = str_replace(' ', '-', $user->id . '-avatar.png');
         $imageData = $this->getAvatarImageData($userAvatarUrl);
+        return $this->createAvatarImageFromData($user, $imageData, 'png');
+    }
+
+    /**
+     * Creates a new image instance and saves it in the system as a new user avatar image.
+     */
+    protected function createAvatarImageFromData(User $user, string $imageData, string $extension): Image
+    {
+        $imageName = str_replace(' ', '-', $user->id . '-avatar.' . $extension);
 
         $image = $this->imageService->saveNew($imageName, $imageData, 'user', $user->id);
         $image->created_by = $user->id;

tests/Auth/LdapTest.php

@@ -4,7 +4,6 @@ use BookStack\Auth\Access\LdapService;
 use BookStack\Auth\Role;
 use BookStack\Auth\Access\Ldap;
 use BookStack\Auth\User;
-use BookStack\Exceptions\LdapException;
 use Mockery\MockInterface;
 use Tests\BrowserKitTest;
 
@@ -35,6 +34,7 @@ class LdapTest extends BrowserKitTest
             'services.ldap.user_filter' => '(&(uid=${user}))',
             'services.ldap.follow_referrals' => false,
             'services.ldap.tls_insecure' => false,
+            'services.ldap.thumbnail_attribute' => null,
         ]);
         $this->mockLdap = \Mockery::mock(Ldap::class);
         $this->app[Ldap::class] = $this->mockLdap;
@@ -668,4 +668,28 @@ class LdapTest extends BrowserKitTest
         $this->runFailedAuthLogin();
         $this->assertTrue($log->hasWarningThatContains('Failed login for timmyjenkins'));
     }
+
+    public function test_thumbnail_attribute_used_as_user_avatar_if_configured()
+    {
+        config()->set(['services.ldap.thumbnail_attribute' => 'jpegPhoto']);
+
+        $this->commonLdapMocks(1, 1, 1, 2, 1);
+        $ldapDn = 'cn=test-user,dc=test' . config('services.ldap.base_dn');
+        $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)
+            ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array'))
+            ->andReturn(['count' => 1, 0 => [
+                'cn' => [$this->mockUser->name],
+                'dn' => $ldapDn,
+                'jpegphoto' => [base64_decode('/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8Q
+EBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=')],
+                'mail' => [$this->mockUser->email]
+            ]]);
+
+        $this->mockUserLogin()
+            ->seePageIs('/');
+
+        $user = User::query()->where('email', '=' , $this->mockUser->email)->first();
+        $this->assertNotNull($user->avatar);
+        $this->assertEquals('8c90748342f19b195b9c6b4eff742ded', md5_file(public_path($user->avatar->path)));
+    }
 }