Skip to content

Commit

Permalink
Add user and better structure to session json (#1443)
Browse files Browse the repository at this point in the history
* Improve `SessionController::init()`
* Remove obsolete defines
* Return updated user object
* Fix security attributes of admin user
  • Loading branch information
nagmat84 authored Sep 5, 2022
1 parent 3ec71f1 commit 1c8eedb
Show file tree
Hide file tree
Showing 13 changed files with 148 additions and 153 deletions.
2 changes: 2 additions & 0 deletions app/Actions/Settings/SetLogin.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ public function do(string $username, string $password): User
$adminUser = User::query()->findOrFail(0);
$adminUser->username = $username;
$adminUser->password = Hash::make($password);
$adminUser->is_locked = false;
$adminUser->may_upload = true;
$adminUser->save();

return $adminUser;
Expand Down
2 changes: 2 additions & 0 deletions app/Actions/User/ResetAdmin.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ public function do(): void
$user->id = 0;
$user->username = '';
$user->password = '';
$user->is_locked = false;
$user->may_upload = true;
$user->save();
}
}
16 changes: 10 additions & 6 deletions app/Http/Controllers/Administration/SettingsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use App\Http\Requests\Settings\ChangeLoginRequest;
use App\Http\Requests\Settings\SetSortingRequest;
use App\Models\Configs;
use App\Models\User;
use App\Rules\LicenseRule;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Eloquent\ModelNotFoundException;
Expand All @@ -27,17 +28,17 @@ class SettingsController extends Controller
{
/**
* Set the Login information for the admin user (id = 0)
* when the later is not initialized.
* when the latter is not initialized.
*
* @param SetAdminLoginRequest $request
* @param SetLogin $setLogin
*
* @return void
* @return User
*
* @throws LycheeException
* @throws ModelNotFoundException
*/
public function setLogin(SetAdminLoginRequest $request, SetLogin $setLogin): void
public function setLogin(SetAdminLoginRequest $request, SetLogin $setLogin): User
{
$adminUser = $setLogin->do(
$request->username(),
Expand All @@ -47,6 +48,8 @@ public function setLogin(SetAdminLoginRequest $request, SetLogin $setLogin): voi
// Otherwise, the session is out-of-sync and falsely assumes the user
// to be unauthenticated upon the next request.
Auth::login($adminUser);

return $adminUser;
}

/**
Expand All @@ -55,12 +58,11 @@ public function setLogin(SetAdminLoginRequest $request, SetLogin $setLogin): voi
* @param ChangeLoginRequest $request
* @param UpdateLogin $updateLogin
*
* @return void
* @return User
*
* @throws LycheeException
* @throws ModelNotFoundException
*/
public function updateLogin(ChangeLoginRequest $request, UpdateLogin $updateLogin): void
public function updateLogin(ChangeLoginRequest $request, UpdateLogin $updateLogin): User
{
$currentUser = $updateLogin->do(
$request->username(),
Expand All @@ -72,6 +74,8 @@ public function updateLogin(ChangeLoginRequest $request, UpdateLogin $updateLogi
// Otherwise, the session is out-of-sync and falsely assumes the user
// to be unauthenticated upon the next request.
Auth::login($currentUser);

return $currentUser;
}

/**
Expand Down
85 changes: 43 additions & 42 deletions app/Http/Controllers/SessionController.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@

namespace App\Http\Controllers;

use App\Contracts\LycheeException;
use App\DTO\AlbumSortingCriterion;
use App\DTO\PhotoSortingCriterion;
use App\Exceptions\ConfigurationKeyMissingException;
use App\Exceptions\Internal\FrameworkException;
use App\Exceptions\Internal\InvalidOrderDirectionException;
use App\Exceptions\ModelDBException;
use App\Exceptions\UnauthenticatedException;
use App\Exceptions\VersionControlException;
use App\Facades\Helpers;
use App\Facades\Lang;
use App\Http\Requests\Session\LoginRequest;
Expand All @@ -21,7 +23,6 @@
use Illuminate\Contracts\Container\BindingResolutionException;
use Illuminate\Routing\Controller;
use Illuminate\Support\Facades\Auth;
use Illuminate\Support\Facades\Config;
use Illuminate\Support\Facades\Gate;
use Illuminate\Support\Facades\Session;

Expand All @@ -43,60 +44,57 @@ public function __construct(ConfigFunctions $configFunctions, GitHubFunctions $g
/**
* First function being called via AJAX.
*
* TODO: Remove attribute `status`.
* TODO: Add nullable attribute `user` with a proper user object.
* TODO: Merge attributes `is_admin`, `may_upload`, `username`, and `is_locked` into user object.
*
* `status === 0 ` (i.e. "no config") is legacy and does not occur.
*
* `status === {1|2}` indicates whether a user is authenticated or not.
* But we should return a nullable attribute `user` which either holds the
* currently authenticated user object or `null` if no user is
* authenticated.
*
* The user-related attributes (`is_admin`, etc.) should be part of that
* user object.
*
* @return array
*
* @throws LycheeException
* @throws VersionControlException
* @throws ConfigurationKeyMissingException
* @throws FrameworkException
* @throws ModelDBException
* @throws InvalidOrderDirectionException
*/
public function init(): array
{
try {
// Return settings
$return = [];

// Check if login credentials exist and login if they don't
if (Auth::check() || AdminAuthentication::loginAsAdminIfNotRegistered()) {
if (Gate::check(UserPolicy::IS_ADMIN)) {
$return['status'] = Config::get('defines.status.LYCHEE_STATUS_LOGGEDIN');
$return['admin'] = true;
$return['may_upload'] = true; // not necessary
$return['config'] = $this->configFunctions->admin();
$return['config']['location'] = base_path('public/');
} else {
/** @var User $user */
$user = Auth::user() ?? throw new UnauthenticatedException();

$return['status'] = Config::get('defines.status.LYCHEE_STATUS_LOGGEDIN');
$return['config'] = $this->configFunctions->public();
$return['is_locked'] = $user->is_locked; // may user change their password?
$return['may_upload'] = $user->may_upload; // may user upload?
$return['username'] = $user->username;
}
if (AdminAuthentication::loginAsAdminIfNotRegistered()) {
// TODO: Remove this legacy stuff after creating the admin user has become part of the installation routine.
// If the session is unauthenticated ('user' === null), but grants admin rights nonetheless,
// the front-end shows the dialog to create an admin account.
$return['user'] = null;
$return['rights'] = [
'is_admin' => true,
'is_locked' => false,
'may_upload' => true,
];
} else {
/** @var User|null $user */
$user = Auth::user();
$return['user'] = $user?->toArray();
$return['rights'] = [
'is_admin' => Gate::check(UserPolicy::IS_ADMIN),
'is_locked' => $user?->is_locked ?? true,
'may_upload' => $user?->may_upload ?? false,
];
}

// here we say whether we logged in because there is no login/password or if we actually entered a login/password
// TODO: Refactor this. At least, rename the flag `login` to something more understandable, like `isAdminUserConfigured`, but rather re-factor the whole logic, i.e. creating the initial user should be part of the installation routine.
$return['config']['login'] = !AdminAuthentication::isAdminNotRegistered();
// Load configuration settings acc. to authentication status
if ($return['rights']['is_admin'] === true) {
// Admin rights (either properly authenticated or not registered)
$return['config'] = $this->configFunctions->admin();
$return['config']['location'] = base_path('public/');
$return['config']['lang_available'] = Lang::get_lang_available();
} elseif ($return['user'] !== null) {
// Authenticated as non-admin
$return['config'] = $this->configFunctions->public();
$return['config']['lang_available'] = Lang::get_lang_available();
} else {
// Logged out
// Unauthenticated
$return['config'] = $this->configFunctions->public();
if (Configs::getValueAsBool('hide_version_number')) {
$return['config']['version'] = '';
}
$return['status'] = Config::get('defines.status.LYCHEE_STATUS_LOGGEDOUT');
}

// Consolidate sorting attributes
Expand All @@ -119,8 +117,11 @@ public function init(): array
$return['update_available'] = false;

return array_merge($return, $this->gitHubFunctions->checkUpdates());
} catch (BindingResolutionException) {
throw new FrameworkException('Laravel\'s path component');
} catch (ModelDBException $e) {
$this->logout();
throw $e;
} catch (BindingResolutionException $e) {
throw new FrameworkException('Laravel\'s container component', $e);
}
}

Expand Down
2 changes: 1 addition & 1 deletion app/Legacy/AdminAuthentication.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public static function isAdminNotRegistered(): bool
if ($adminUser !== null) {
return $adminUser->password === '' || $adminUser->username === '';
}
resolve(ResetAdmin::class)->do();
(new ResetAdmin())->do();

return true;
}
Expand Down
9 changes: 0 additions & 9 deletions config/defines.php

This file was deleted.

17 changes: 7 additions & 10 deletions database/migrations/2020_12_12_203153_migrate_admin_user.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

use App\Exceptions\ModelDBException;
use App\Models\Configs;
use App\Models\User;
use Illuminate\Database\Migrations\Migration;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Schema;
Expand All @@ -18,15 +17,13 @@ class MigrateAdminUser extends Migration
*/
public function up(): void
{
$user = new User();
$user->username = Configs::getValueAsString('username', '');
$user->password = Configs::getValueAsString('password', '');
$user->save();

// User will have an ID which is NOT 0.
// We want this user to have an ID of 0 as it is the ADMIN ID.
$user->id = 0;
$user->save();
DB::table('users')->insert([
'id' => 0,
'username' => Configs::getValueAsString('username', ''),
'password' => Configs::getValueAsString('password', ''),
'lock' => false,
'upload' => true,
]);
}

/**
Expand Down
11 changes: 4 additions & 7 deletions public/dist/frame.js

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions public/dist/landing.js

Large diffs are not rendered by default.

133 changes: 69 additions & 64 deletions public/dist/main.js

Large diffs are not rendered by default.

13 changes: 5 additions & 8 deletions public/dist/view.js

Large diffs are not rendered by default.

5 changes: 2 additions & 3 deletions tests/Feature/Lib/SessionUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public function logout(): TestResponse
public function set_admin(
string $login,
string $password,
int $expectedStatusCode = 204,
int $expectedStatusCode = 200,
?string $assertSee = null
): TestResponse {
$response = $this->testCase->postJson('/api/Settings::setLogin', [
Expand All @@ -117,7 +117,6 @@ public function set_admin(
*
* @param string $login
* @param string $password
* @param string $oldUsername
* @param string $oldPassword
* @param int $expectedStatusCode
* @param string|null $assertSee
Expand All @@ -128,7 +127,7 @@ public function update_login(
string $login,
string $password,
string $oldPassword,
int $expectedStatusCode = 204,
int $expectedStatusCode = 200,
?string $assertSee = null
): TestResponse {
$response = $this->testCase->postJson('/api/Settings::updateLogin', [
Expand Down

0 comments on commit 1c8eedb

Please sign in to comment.