diff --git a/app/Auth/Access/Guards/LdapSessionGuard.php b/app/Auth/Access/Guards/LdapSessionGuard.php index 078487224..5a902af76 100644 --- a/app/Auth/Access/Guards/LdapSessionGuard.php +++ b/app/Auth/Access/Guards/LdapSessionGuard.php @@ -84,7 +84,7 @@ class LdapSessionGuard extends ExternalBaseSessionGuard try { $user = $this->createNewFromLdapAndCreds($userDetails, $credentials); } catch (UserRegistrationException $exception) { - throw new LoginAttemptException($exception->message); + throw new LoginAttemptException($exception->getMessage()); } } diff --git a/app/Auth/Access/RegistrationService.php b/app/Auth/Access/RegistrationService.php index dcdb68bd5..6fcb404ee 100644 --- a/app/Auth/Access/RegistrationService.php +++ b/app/Auth/Access/RegistrationService.php @@ -96,7 +96,8 @@ class RegistrationService } // Create the user - $newUser = $this->userRepo->registerNew($userData, $emailConfirmed); + $newUser = $this->userRepo->createWithoutActivity($userData, $emailConfirmed); + $newUser->attachDefaultRole(); // Assign social account if given if ($socialAccount) { diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index cb0c0d2fa..c87fda4c8 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -3,6 +3,7 @@ namespace BookStack\Auth; use BookStack\Actions\ActivityType; +use BookStack\Auth\Access\UserInviteService; use BookStack\Entities\EntityProvider; use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Bookshelf; @@ -18,17 +19,20 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection; use Illuminate\Pagination\LengthAwarePaginator; use Illuminate\Support\Facades\Log; +use Illuminate\Support\Str; class UserRepo { protected $userAvatar; + protected $inviteService; /** * UserRepo constructor. */ - public function __construct(UserAvatars $userAvatar) + public function __construct(UserAvatars $userAvatar, UserInviteService $inviteService) { $this->userAvatar = $userAvatar; + $this->inviteService = $inviteService; } /** @@ -92,18 +96,6 @@ class UserRepo return $query->paginate($count); } - /** - * Creates a new user and attaches a role to them. - */ - public function registerNew(array $data, bool $emailConfirmed = false): User - { - $user = $this->create($data, $emailConfirmed); - $user->attachDefaultRole(); - $this->downloadAndAssignUserAvatar($user); - - return $user; - } - /** * Assign a user to a system-level role. * @@ -166,23 +158,47 @@ class UserRepo } /** - * Create a new basic instance of user. + * Create a new basic instance of user with the given pre-validated data. + * @param array{name: string, email: string, password: ?string, external_auth_id: ?string, language: ?string, roles: ?array} $data */ - public function create(array $data, bool $emailConfirmed = false): User + public function createWithoutActivity(array $data, bool $emailConfirmed = false): User { - $details = [ - 'name' => $data['name'], - 'email' => $data['email'], - 'password' => bcrypt($data['password']), - 'email_confirmed' => $emailConfirmed, - 'external_auth_id' => $data['external_auth_id'] ?? '', - ]; - $user = new User(); - $user->forceFill($details); + $user->name = $data['name']; + $user->email = $data['email']; + $user->password = bcrypt(empty($data['password']) ? Str::random(32) : $data['password']); + $user->email_confirmed = $emailConfirmed; + $user->external_auth_id = $data['external_auth_id'] ?? ''; + $user->refreshSlug(); $user->save(); + if (!empty($data['language'])) { + setting()->putUser($user, 'language', $data['language']); + } + + if (isset($data['roles'])) { + $this->setUserRoles($user, $data['roles']); + } + + $this->downloadAndAssignUserAvatar($user); + + return $user; + } + + /** + * 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 + */ + public function create(array $data, bool $sendInvite = false): User + { + $user = $this->createWithoutActivity($data, false); + + if ($sendInvite) { + $this->inviteService->sendInvitation($user); + } + + Activity::add(ActivityType::USER_CREATE, $user); return $user; } diff --git a/app/Console/Commands/CreateAdmin.php b/app/Console/Commands/CreateAdmin.php index c3faef79c..c571d383e 100644 --- a/app/Console/Commands/CreateAdmin.php +++ b/app/Console/Commands/CreateAdmin.php @@ -84,9 +84,8 @@ class CreateAdmin extends Command return SymfonyCommand::FAILURE; } - $user = $this->userRepo->create($validator->validated()); + $user = $this->userRepo->createWithoutActivity($validator->validated()); $this->userRepo->attachSystemRole($user, 'admin'); - $this->userRepo->downloadAndAssignUserAvatar($user); $user->email_confirmed = true; $user->save(); diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index 7ec502525..317b011d8 100644 --- a/app/Exceptions/Handler.php +++ b/app/Exceptions/Handler.php @@ -101,6 +101,10 @@ class Handler extends ExceptionHandler $code = $e->status; } + if (method_exists($e, 'getStatus')) { + $code = $e->getStatus(); + } + $responseData['error']['code'] = $code; return new JsonResponse($responseData, $code, $headers); diff --git a/app/Exceptions/NotifyException.php b/app/Exceptions/NotifyException.php index 8e748a21d..e09247208 100644 --- a/app/Exceptions/NotifyException.php +++ b/app/Exceptions/NotifyException.php @@ -9,17 +9,27 @@ class NotifyException extends Exception implements Responsable { public $message; public $redirectLocation; + protected $status; /** * NotifyException constructor. */ - public function __construct(string $message, string $redirectLocation = '/') + public function __construct(string $message, string $redirectLocation = '/', int $status = 500) { $this->message = $message; $this->redirectLocation = $redirectLocation; + $this->status = $status; parent::__construct(); } + /** + * Get the desired status code for this exception. + */ + public function getStatus(): int + { + return $this->status; + } + /** * Send the response for this type of exception. * diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index 88350e0ea..cd97dead1 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -4,8 +4,10 @@ namespace BookStack\Http\Controllers\Api; use BookStack\Auth\User; use BookStack\Auth\UserRepo; +use BookStack\Exceptions\UserUpdateException; use Closure; use Illuminate\Http\Request; +use Illuminate\Support\Facades\DB; use Illuminate\Validation\Rules\Password; use Illuminate\Validation\Rules\Unique; @@ -20,12 +22,29 @@ class UserApiController extends ApiController public function __construct(UserRepo $userRepo) { $this->userRepo = $userRepo; + + // Checks for all endpoints in this controller + $this->middleware(function ($request, $next) { + $this->checkPermission('users-manage'); + $this->preventAccessInDemoMode(); + return $next($request); + }); } protected function rules(int $userId = null): array { return [ 'create' => [ + 'name' => ['required', 'min:2'], + 'email' => [ + 'required', 'min:2', 'email', new Unique('users', 'email') + ], + 'external_auth_id' => ['string'], + 'language' => ['string'], + 'password' => [Password::default()], + 'roles' => ['array'], + 'roles.*' => ['integer'], + 'send_invite' => ['boolean'], ], 'update' => [ 'name' => ['min:2'], @@ -52,8 +71,6 @@ class UserApiController extends ApiController */ public function list() { - $this->checkPermission('users-manage'); - $users = $this->userRepo->getApiUsersBuilder(); return $this->apiListingResponse($users, [ @@ -62,14 +79,30 @@ class UserApiController extends ApiController ], [Closure::fromCallable([$this, 'listFormatter'])]); } + /** + * Create a new user in the system. + */ + public function create(Request $request) + { + $data = $this->validate($request, $this->rules()['create']); + $sendInvite = ($data['send_invite'] ?? false) === true; + + $user = null; + DB::transaction(function () use ($data, $sendInvite, &$user) { + $user = $this->userRepo->create($data, $sendInvite); + }); + + $this->singleFormatter($user); + + return response()->json($user); + } + /** * View the details of a single user. * Requires permission to manage users. */ public function read(string $id) { - $this->checkPermission('users-manage'); - $user = $this->userRepo->getById($id); $this->singleFormatter($user); @@ -78,12 +111,10 @@ class UserApiController extends ApiController /** * Update an existing user in the system. - * @throws \BookStack\Exceptions\UserUpdateException + * @throws UserUpdateException */ public function update(Request $request, string $id) { - $this->checkPermission('users-manage'); - $data = $this->validate($request, $this->rules($id)['update']); $user = $this->userRepo->getById($id); $this->userRepo->update($user, $data, userCan('users-manage')); @@ -100,8 +131,6 @@ class UserApiController extends ApiController */ public function delete(Request $request, string $id) { - $this->checkPermission('users-manage'); - $user = $this->userRepo->getById($id); $newOwnerId = $request->get('migrate_ownership_id', null); diff --git a/app/Http/Controllers/Controller.php b/app/Http/Controllers/Controller.php index 2c4c2df1e..13a86f6f7 100644 --- a/app/Http/Controllers/Controller.php +++ b/app/Http/Controllers/Controller.php @@ -2,13 +2,13 @@ namespace BookStack\Http\Controllers; +use BookStack\Exceptions\NotifyException; use BookStack\Facades\Activity; use BookStack\Interfaces\Loggable; use BookStack\Model; use BookStack\Util\WebSafeMimeSniffer; use Illuminate\Foundation\Bus\DispatchesJobs; use Illuminate\Foundation\Validation\ValidatesRequests; -use Illuminate\Http\Exceptions\HttpResponseException; use Illuminate\Http\JsonResponse; use Illuminate\Http\Response; use Illuminate\Routing\Controller as BaseController; @@ -53,14 +53,8 @@ abstract class Controller extends BaseController */ protected function showPermissionError() { - if (request()->wantsJson()) { - $response = response()->json(['error' => trans('errors.permissionJson')], 403); - } else { - $response = redirect('/'); - $this->showErrorNotification(trans('errors.permission')); - } - - throw new HttpResponseException($response); + $message = request()->wantsJson() ? trans('errors.permissionJson') : trans('errors.permission'); + throw new NotifyException($message, '/', 403); } /** diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 9e702a1d7..46e858d9b 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -2,9 +2,7 @@ namespace BookStack\Http\Controllers; -use BookStack\Actions\ActivityType; use BookStack\Auth\Access\SocialAuthService; -use BookStack\Auth\Access\UserInviteService; use BookStack\Auth\User; use BookStack\Auth\UserRepo; use BookStack\Exceptions\ImageUploadException; @@ -13,25 +11,20 @@ use BookStack\Uploads\ImageRepo; use Exception; use Illuminate\Http\Request; use Illuminate\Support\Facades\DB; -use Illuminate\Support\Str; use Illuminate\Validation\Rules\Password; use Illuminate\Validation\ValidationException; class UserController extends Controller { - protected $user; protected $userRepo; - protected $inviteService; protected $imageRepo; /** * UserController constructor. */ - public function __construct(User $user, UserRepo $userRepo, UserInviteService $inviteService, ImageRepo $imageRepo) + public function __construct(UserRepo $userRepo, ImageRepo $imageRepo) { - $this->user = $user; $this->userRepo = $userRepo; - $this->inviteService = $inviteService; $this->imageRepo = $imageRepo; } @@ -68,63 +61,34 @@ class UserController extends Controller } /** - * Store a newly created user in storage. + * Store a new user in storage. * - * @throws UserUpdateException * @throws ValidationException */ public function store(Request $request) { $this->checkPermission('users-manage'); - $validationRules = [ - 'name' => ['required'], - 'email' => ['required', 'email', 'unique:users,email'], - 'setting' => ['array'], - ]; $authMethod = config('auth.method'); $sendInvite = ($request->get('send_invite', 'false') === 'true'); + $externalAuth = $authMethod === 'ldap' || $authMethod === 'saml2' || $authMethod === 'oidc'; + $passwordRequired = ($authMethod === 'standard' && !$sendInvite); - if ($authMethod === 'standard' && !$sendInvite) { - $validationRules['password'] = ['required', Password::default()]; - $validationRules['password-confirm'] = ['required', 'same:password']; - } elseif ($authMethod === 'ldap' || $authMethod === 'saml2' || $authMethod === 'openid') { - $validationRules['external_auth_id'] = ['required']; - } - $this->validate($request, $validationRules); + $validationRules = [ + 'name' => ['required'], + 'email' => ['required', 'email', 'unique:users,email'], + 'language' => ['string'], + 'roles' => ['array'], + 'roles.*' => ['integer'], + 'password' => $passwordRequired ? ['required', Password::default()] : null, + 'password-confirm' => $passwordRequired ? ['required', 'same:password'] : null, + 'external_auth_id' => $externalAuth ? ['required'] : null, + ]; - $user = $this->user->fill($request->all()); + $validated = $this->validate($request, array_filter($validationRules)); - if ($authMethod === 'standard') { - $user->password = bcrypt($request->get('password', Str::random(32))); - } elseif ($authMethod === 'ldap' || $authMethod === 'saml2' || $authMethod === 'openid') { - $user->external_auth_id = $request->get('external_auth_id'); - } - - $user->refreshSlug(); - - DB::transaction(function () use ($user, $sendInvite, $request) { - $user->save(); - - // Save user-specific settings - if ($request->filled('setting')) { - foreach ($request->get('setting') as $key => $value) { - setting()->putUser($user, $key, $value); - } - } - - if ($sendInvite) { - $this->inviteService->sendInvitation($user); - } - - if ($request->filled('roles')) { - $roles = $request->get('roles'); - $this->userRepo->setUserRoles($user, $roles); - } - - $this->userRepo->downloadAndAssignUserAvatar($user); - - $this->logActivity(ActivityType::USER_CREATE, $user); + DB::transaction(function () use ($validated, $sendInvite) { + $this->userRepo->create($validated, $sendInvite); }); return redirect('/settings/users'); @@ -138,7 +102,7 @@ class UserController extends Controller $this->checkPermissionOrCurrentUser('users-manage', $id); /** @var User $user */ - $user = $this->user->newQuery()->with(['apiTokens', 'mfaValues'])->findOrFail($id); + $user = User::query()->with(['apiTokens', 'mfaValues'])->findOrFail($id); $authMethod = ($user->system_name) ? 'system' : config('auth.method'); @@ -312,7 +276,7 @@ class UserController extends Controller $newState = $request->get('expand', 'false'); - $user = $this->user->findOrFail($id); + $user = $this->userRepo->getById($id); setting()->putUser($user, 'section_expansion#' . $key, $newState); return response('', 204); @@ -335,7 +299,7 @@ class UserController extends Controller $order = 'asc'; } - $user = $this->user->findOrFail($userId); + $user = $this->userRepo->getById($userId); $sortKey = $listName . '_sort'; $orderKey = $listName . '_sort_order'; setting()->putUser($user, $sortKey, $sort); diff --git a/routes/api.php b/routes/api.php index 0325d7c2a..c7b8887b6 100644 --- a/routes/api.php +++ b/routes/api.php @@ -68,6 +68,7 @@ Route::put('shelves/{id}', [BookshelfApiController::class, 'update']); Route::delete('shelves/{id}', [BookshelfApiController::class, 'delete']); Route::get('users', [UserApiController::class, 'list']); +Route::post('users', [UserApiController::class, 'create']); Route::get('users/{id}', [UserApiController::class, 'read']); Route::put('users/{id}', [UserApiController::class, 'update']); Route::delete('users/{id}', [UserApiController::class, 'delete']); \ No newline at end of file diff --git a/tests/Api/TestsApi.php b/tests/Api/TestsApi.php index 97ca82ea7..0cdd93741 100644 --- a/tests/Api/TestsApi.php +++ b/tests/Api/TestsApi.php @@ -35,6 +35,14 @@ trait TestsApi return ['error' => ['code' => $code, 'message' => $message]]; } + /** + * Get the structure that matches a permission error response. + */ + protected function permissionErrorResponse(): array + { + return $this->errorResponse('You do not have permission to perform the requested action.', 403); + } + /** * Format the given (field_name => ["messages"]) array * into a standard validation response format. diff --git a/tests/Api/UsersApiTest.php b/tests/Api/UsersApiTest.php index 19b7b0adc..e1bcb02d5 100644 --- a/tests/Api/UsersApiTest.php +++ b/tests/Api/UsersApiTest.php @@ -2,10 +2,13 @@ namespace Tests\Api; +use BookStack\Actions\ActivityType; use BookStack\Auth\Role; use BookStack\Auth\User; -use Illuminate\Support\Facades\Auth; +use BookStack\Entities\Models\Entity; +use BookStack\Notifications\UserInvite; use Illuminate\Support\Facades\Hash; +use Illuminate\Support\Facades\Notification; use Tests\TestCase; class UsersApiTest extends TestCase @@ -14,17 +17,34 @@ class UsersApiTest extends TestCase protected $baseEndpoint = '/api/users'; + protected $endpointMap = [ + ['get', '/api/users'], + ['post', '/api/users'], + ['get', '/api/users/1'], + ['put', '/api/users/1'], + ['delete', '/api/users/1'], + ]; + public function test_users_manage_permission_needed_for_all_endpoints() { - // TODO + $this->actingAsApiEditor(); + foreach ($this->endpointMap as [$method, $uri]) { + $resp = $this->json($method, $uri); + $resp->assertStatus(403); + $resp->assertJson($this->permissionErrorResponse()); + } } public function test_no_endpoints_accessible_in_demo_mode() { - // TODO - // $this->preventAccessInDemoMode(); - // Can't use directly in constructor as blocks access to docs - // Maybe via route middleware + config()->set('app.env', 'demo'); + $this->actingAsApiAdmin(); + + foreach ($this->endpointMap as [$method, $uri]) { + $resp = $this->json($method, $uri); + $resp->assertStatus(403); + $resp->assertJson($this->permissionErrorResponse()); + } } public function test_index_endpoint_returns_expected_shelf() @@ -47,6 +67,85 @@ class UsersApiTest extends TestCase ]]); } + public function test_create_endpoint() + { + $this->actingAsApiAdmin(); + /** @var Role $role */ + $role = Role::query()->first(); + + $resp = $this->postJson($this->baseEndpoint, [ + 'name' => 'Benny Boris', + 'email' => 'bboris@example.com', + 'password' => 'mysuperpass', + 'language' => 'it', + 'roles' => [$role->id], + 'send_invite' => false, + ]); + + $resp->assertStatus(200); + $resp->assertJson([ + 'name' => 'Benny Boris', + 'email' => 'bboris@example.com', + 'external_auth_id' => '', + 'roles' => [ + [ + 'id' => $role->id, + 'display_name' => $role->display_name, + ] + ], + ]); + $this->assertDatabaseHas('users', ['email' => 'bboris@example.com']); + + /** @var User $user */ + $user = User::query()->where('email', '=', 'bboris@example.com')->first(); + $this->assertActivityExists(ActivityType::USER_CREATE, null, $user->logDescriptor()); + $this->assertEquals(1, $user->roles()->count()); + $this->assertEquals('it', setting()->getUser($user, 'language')); + } + + public function test_create_with_send_invite() + { + $this->actingAsApiAdmin(); + Notification::fake(); + + $resp = $this->postJson($this->baseEndpoint, [ + 'name' => 'Benny Boris', + 'email' => 'bboris@example.com', + 'send_invite' => true, + ]); + + $resp->assertStatus(200); + /** @var User $user */ + $user = User::query()->where('email', '=', 'bboris@example.com')->first(); + Notification::assertSentTo($user, UserInvite::class); + } + + public function test_create_name_and_email_validation() + { + $this->actingAsApiAdmin(); + /** @var User $existingUser */ + $existingUser = User::query()->first(); + + $resp = $this->postJson($this->baseEndpoint, [ + 'email' => 'bboris@example.com', + ]); + $resp->assertStatus(422); + $resp->assertJson($this->validationResponse(['name' => ['The name field is required.']])); + + $resp = $this->postJson($this->baseEndpoint, [ + 'name' => 'Benny Boris', + ]); + $resp->assertStatus(422); + $resp->assertJson($this->validationResponse(['email' => ['The email field is required.']])); + + $resp = $this->postJson($this->baseEndpoint, [ + 'email' => $existingUser->email, + 'name' => 'Benny Boris', + ]); + $resp->assertStatus(422); + $resp->assertJson($this->validationResponse(['email' => ['The email has already been taken.']])); + } + public function test_read_endpoint() { $this->actingAsApiAdmin(); @@ -133,6 +232,33 @@ class UsersApiTest extends TestCase $this->assertActivityExists('user_delete', null, $user->logDescriptor()); } + public function test_delete_endpoint_with_ownership_migration_user() + { + $this->actingAsApiAdmin(); + /** @var User $user */ + $user = User::query()->where('id', '!=', $this->getAdmin()->id) + ->whereNull('system_name') + ->first(); + $entityChain = $this->createEntityChainBelongingToUser($user); + /** @var User $newOwner */ + $newOwner = User::query()->where('id', '!=', $user->id)->first(); + + /** @var Entity $entity */ + foreach ($entityChain as $entity) { + $this->assertEquals($user->id, $entity->owned_by); + } + + $resp = $this->deleteJson($this->baseEndpoint . "/{$user->id}", [ + 'migrate_ownership_id' => $newOwner->id, + ]); + + $resp->assertStatus(204); + /** @var Entity $entity */ + foreach ($entityChain as $entity) { + $this->assertEquals($newOwner->id, $entity->refresh()->owned_by); + } + } + public function test_delete_endpoint_fails_deleting_only_admin() { $this->actingAsApiAdmin();