Merge 67e435b2fe into f583354748

Added specific handling to show relevant error message when user
creation fails due to invite sending errors, while also returning user
to the form with previous input.
Includes test to cover.

For #5195

app/Access/Oidc/OidcService.php

@@ -214,7 +214,8 @@ class OidcService
             $user = $this->registrationService->findOrRegister(
                 $userDetails->name,
                 $userDetails->email,
-                $userDetails->externalId
+                $userDetails->externalId,
+                $userDetails->picture,
             );
         } catch (UserRegistrationException $exception) {
             throw new OidcException($exception->getMessage());

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,6 +41,7 @@ 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 = $claims->getClaim('picture') ?? $this->picture;
     }
 
     protected static function getUserDisplayName(string $displayNameClaims, ProvidesClaims $token): string

app/Access/RegistrationService.php

@@ -50,7 +50,7 @@ class RegistrationService
      *
      * @throws UserRegistrationException
      */
-    public function findOrRegister(string $name, string $email, string $externalId): User
+    public function findOrRegister(string $name, string $email, string $externalId, string $picture = null): User
     {
         $user = User::query()
             ->where('external_auth_id', '=', $externalId)
@@ -64,7 +64,7 @@ class RegistrationService
                 'external_auth_id' => $externalId,
             ];
 
-            $user = $this->registerUser($userData, null, false);
+            $user = $this->registerUser($userData, null, false, $picture);
         }
 
         return $user;
@@ -75,7 +75,7 @@ class RegistrationService
      *
      * @throws UserRegistrationException
      */
-    public function registerUser(array $userData, ?SocialAccount $socialAccount = null, bool $emailConfirmed = false): User
+    public function registerUser(array $userData, ?SocialAccount $socialAccount = null, bool $emailConfirmed = false, string $picture = null): User
     {
         $userEmail = $userData['email'];
         $authSystem = $socialAccount ? $socialAccount->driver : auth()->getDefaultDriver();
@@ -96,7 +96,7 @@ class RegistrationService
         }
 
         // Create the user
-        $newUser = $this->userRepo->createWithoutActivity($userData, $emailConfirmed);
+        $newUser = $this->userRepo->createWithoutActivity($userData, $emailConfirmed, $picture);
         $newUser->attachDefaultRole();
 
         // Assign social account if given

app/Access/UserInviteException.php

@@ -0,0 +1,10 @@
+<?php
+
+namespace BookStack\Access;
+
+use Exception;
+
+class UserInviteException extends Exception
+{
+    //
+}

app/Access/UserInviteService.php

@@ -13,11 +13,17 @@ class UserInviteService extends UserTokenService
     /**
      * Send an invitation to a user to sign into BookStack
      * Removes existing invitation tokens.
+     * @throws UserInviteException
      */
     public function sendInvitation(User $user)
     {
         $this->deleteByUser($user);
         $token = $this->createTokenForUser($user);
-        $user->notify(new UserInviteNotification($token));
+
+        try {
+            $user->notify(new UserInviteNotification($token));
+        } catch (\Exception $exception) {
+            throw new UserInviteException($exception->getMessage(), $exception->getCode(), $exception);
+        }
     }
 }

app/Entities/Repos/PageRepo.php

@@ -11,7 +11,6 @@ use BookStack\Entities\Models\PageRevision;
 use BookStack\Entities\Queries\EntityQueries;
 use BookStack\Entities\Tools\BookContents;
 use BookStack\Entities\Tools\PageContent;
-use BookStack\Entities\Tools\PageEditorData;
 use BookStack\Entities\Tools\PageEditorType;
 use BookStack\Entities\Tools\TrashCan;
 use BookStack\Exceptions\MoveOperationException;
@@ -44,6 +43,7 @@ class PageRepo
             'owned_by'   => user()->id,
             'updated_by' => user()->id,
             'draft'      => true,
+            'editor'     => PageEditorType::getSystemDefault()->value,
         ]);
 
         if ($parent instanceof Chapter) {
@@ -146,8 +146,10 @@ class PageRepo
             $pageContent->setNewHTML($input['html'], user());
         }
 
-        if ($newEditor !== $currentEditor && userCan('editor-change')) {
+        if (($newEditor !== $currentEditor || empty($page->editor)) && userCan('editor-change')) {
             $page->editor = $newEditor->value;
+        } elseif (empty($page->editor)) {
+            $page->editor = $defaultEditor->value;
         }
     }
 

app/Uploads/UserAvatars.php

@@ -22,7 +22,7 @@ class UserAvatars
     /**
      * Fetch and assign an avatar image to the given user.
      */
-    public function fetchAndAssignToUser(User $user): void
+    public function fetchAndAssignToUser(User $user, string $picture = null): void
     {
         if (!$this->avatarFetchEnabled()) {
             return;
@@ -30,7 +30,7 @@ class UserAvatars
 
         try {
             $this->destroyAllForUser($user);
-            $avatar = $this->saveAvatarImage($user);
+            $avatar = $this->saveAvatarImage($user, 500, $picture);
             $user->avatar()->associate($avatar);
             $user->save();
         } catch (Exception $e) {
@@ -72,9 +72,9 @@ class UserAvatars
      *
      * @throws HttpFetchException
      */
-    protected function saveAvatarImage(User $user, int $size = 500): Image
+    protected function saveAvatarImage(User $user, int $size = 500, string $picture = null): Image
     {
-        $avatarUrl = $this->getAvatarUrl();
+        $avatarUrl = $picture ?: $this->getAvatarUrl();
         $email = strtolower(trim($user->email));
 
         $replacements = [

app/Users/Controllers/UserController.php

@@ -3,6 +3,7 @@
 namespace BookStack\Users\Controllers;
 
 use BookStack\Access\SocialDriverManager;
+use BookStack\Access\UserInviteException;
 use BookStack\Exceptions\ImageUploadException;
 use BookStack\Exceptions\UserUpdateException;
 use BookStack\Http\Controller;
@@ -14,6 +15,7 @@ use BookStack\Util\SimpleListOptions;
 use Exception;
 use Illuminate\Http\Request;
 use Illuminate\Support\Facades\DB;
+use Illuminate\Support\Facades\Log;
 use Illuminate\Validation\Rules\Password;
 use Illuminate\Validation\ValidationException;
 
@@ -91,9 +93,15 @@ class UserController extends Controller
 
         $validated = $this->validate($request, array_filter($validationRules));
 
-        DB::transaction(function () use ($validated, $sendInvite) {
-            $this->userRepo->create($validated, $sendInvite);
-        });
+        try {
+            DB::transaction(function () use ($validated, $sendInvite) {
+                $this->userRepo->create($validated, $sendInvite);
+            });
+        } catch (UserInviteException $e) {
+            Log::error("Failed to send user invite with error: {$e->getMessage()}");
+            $this->showErrorNotification(trans('errors.users_could_not_send_invite'));
+            return redirect('/settings/users/create')->withInput();
+        }
 
         return redirect('/settings/users');
     }

app/Users/UserRepo.php

@@ -2,6 +2,7 @@
 
 namespace BookStack\Users;
 
+use BookStack\Access\UserInviteException;
 use BookStack\Access\UserInviteService;
 use BookStack\Activity\ActivityType;
 use BookStack\Entities\EntityProvider;
@@ -54,7 +55,7 @@ class UserRepo
      *
      * @param array{name: string, email: string, password: ?string, external_auth_id: ?string, language: ?string, roles: ?array} $data
      */
-    public function createWithoutActivity(array $data, bool $emailConfirmed = false): User
+    public function createWithoutActivity(array $data, bool $emailConfirmed = false, string $picture = null): User
     {
         $user = new User();
         $user->name = $data['name'];
@@ -74,7 +75,7 @@ class UserRepo
             $this->setUserRoles($user, $data['roles']);
         }
 
-        $this->downloadAndAssignUserAvatar($user);
+        $this->downloadAndAssignUserAvatar($user, $picture);
 
         return $user;
     }
@@ -83,6 +84,7 @@ class UserRepo
      * As per "createWithoutActivity" but records a "create" activity.
      *
      * @param array{name: string, email: string, password: ?string, external_auth_id: ?string, language: ?string, roles: ?array} $data
+     * @throws UserInviteException
      */
     public function create(array $data, bool $sendInvite = false): User
     {
@@ -199,10 +201,10 @@ class UserRepo
      * Get an avatar image for a user and set it as their avatar.
      * Returns early if avatars disabled or not set in config.
      */
-    protected function downloadAndAssignUserAvatar(User $user): void
+    protected function downloadAndAssignUserAvatar(User $user, string $picture = null): void
     {
         try {
-            $this->userAvatar->fetchAndAssignToUser($user);
+            $this->userAvatar->fetchAndAssignToUser($user, $picture);
         } catch (Exception $e) {
             Log::error('Failed to save user avatar image');
         }

database/migrations/2024_09_29_140340_ensure_editor_value_set.php

@@ -0,0 +1,48 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Support\Facades\DB;
+
+return new class extends Migration
+{
+    /**
+     * Run the migrations.
+     */
+    public function up(): void
+    {
+        // Ensure we have an "editor" value set for pages
+
+        // Get default
+        $default = DB::table('settings')
+            ->where('setting_key', '=', 'app-editor')
+            ->first()
+            ->value ?? 'wysiwyg';
+        $default = ($default === 'markdown') ? 'markdown' : 'wysiwyg';
+
+        // We set it to 'markdown' for pages currently with markdown content
+        DB::table('pages')
+            ->where('editor', '=', '')
+            ->where('markdown', '!=', '')
+            ->update(['editor' => 'markdown']);
+
+        // We set it to 'wysiwyg' where we have HTML but no markdown
+        DB::table('pages')
+            ->where('editor', '=', '')
+            ->where('markdown', '=', '')
+            ->where('html', '!=', '')
+            ->update(['editor' => 'wysiwyg']);
+
+        // Otherwise, where still empty, set to the current default
+        DB::table('pages')
+            ->where('editor', '=', '')
+            ->update(['editor' => $default]);
+    }
+
+    /**
+     * Reverse the migrations.
+     */
+    public function down(): void
+    {
+        // Can't reverse due to not knowing what would have been empty before
+    }
+};

lang/en/errors.php

@@ -78,6 +78,7 @@ return [
     // Users
     'users_cannot_delete_only_admin' => 'You cannot delete the only admin',
     'users_cannot_delete_guest' => 'You cannot delete the guest user',
+    'users_could_not_send_invite' => 'Could not create user since invite email failed to send',
 
     // Roles
     'role_cannot_be_edited' => 'This role cannot be edited',

tests/Entity/PageEditorTest.php

@@ -24,6 +24,21 @@ class PageEditorTest extends TestCase
         $this->withHtml($this->followRedirects($resp))->assertElementExists('#html-editor');
     }
 
+    public function test_editor_set_for_new_pages()
+    {
+        $book = $this->page->book;
+
+        $this->asEditor()->get($book->getUrl('/create-page'));
+        $newPage = $book->pages()->orderBy('id', 'desc')->first();
+        $this->assertEquals('wysiwyg', $newPage->editor);
+
+        $this->setSettings(['app-editor' => PageEditorType::Markdown->value]);
+
+        $this->asEditor()->get($book->getUrl('/create-page'));
+        $newPage = $book->pages()->orderBy('id', 'desc')->first();
+        $this->assertEquals('markdown', $newPage->editor);
+    }
+
     public function test_markdown_setting_shows_markdown_editor_for_new_pages()
     {
         $this->setSettings(['app-editor' => PageEditorType::Markdown->value]);

tests/User/UserManagementTest.php

@@ -2,6 +2,7 @@
 
 namespace Tests\User;
 
+use BookStack\Access\UserInviteException;
 use BookStack\Access\UserInviteService;
 use BookStack\Activity\ActivityType;
 use BookStack\Uploads\Image;
@@ -229,7 +230,7 @@ class UserManagementTest extends TestCase
 
         // Simulate an invitation sending failure
         $this->mock(UserInviteService::class, function (MockInterface $mock) {
-            $mock->shouldReceive('sendInvitation')->once()->andThrow(RuntimeException::class);
+            $mock->shouldReceive('sendInvitation')->once()->andThrow(UserInviteException::class);
         });
 
         $this->asAdmin()->post('/settings/users/create', [
@@ -247,22 +248,42 @@ class UserManagementTest extends TestCase
     {
         /** @var User $user */
         $user = User::factory()->make();
-        $adminRole = Role::getRole('admin');
 
         $this->mock(UserInviteService::class, function (MockInterface $mock) {
-            $mock->shouldReceive('sendInvitation')->once()->andThrow(RuntimeException::class);
+            $mock->shouldReceive('sendInvitation')->once()->andThrow(UserInviteException::class);
         });
 
         $this->asAdmin()->post('/settings/users/create', [
             'name'                          => $user->name,
             'email'                         => $user->email,
             'send_invite'                   => 'true',
-            'roles[' . $adminRole->id . ']' => 'true',
         ]);
 
         $this->assertDatabaseMissing('activities', ['type' => 'USER_CREATE']);
     }
 
+    public function test_return_to_form_with_warning_if_the_invitation_sending_fails()
+    {
+        $logger = $this->withTestLogger();
+        /** @var User $user */
+        $user = User::factory()->make();
+
+        $this->mock(UserInviteService::class, function (MockInterface $mock) {
+            $mock->shouldReceive('sendInvitation')->once()->andThrow(UserInviteException::class);
+        });
+
+        $resp = $this->asAdmin()->post('/settings/users/create', [
+            'name'                          => $user->name,
+            'email'                         => $user->email,
+            'send_invite'                   => 'true',
+        ]);
+
+        $resp->assertRedirect('/settings/users/create');
+        $this->assertSessionError('Could not create user since invite email failed to send');
+        $this->assertEquals($user->email, session()->getOldInput('email'));
+        $this->assertTrue($logger->hasErrorThatContains('Failed to send user invite with error:'));
+    }
+
     public function test_user_create_update_fails_if_locale_is_invalid()
     {
         $user = $this->users->editor();