Refactor legacy User model (#1802)

We currently have two user models: a legacy model, and an Eloquent model. The reflection approach used to access legacy model members through the Eloquent model is not conducive to static analysis, and is a pain to work with. This PR moves a significant portion of the legacy model logic to the Eloquent model. Additionally, I have improved the User<->Password model relationship.
Kitware · Nov 17, 2023 · d78f908 · d78f908
1 parent cc80bb2
commit d78f908
Show file tree

Hide file tree

Showing 31 changed files with 164 additions and 361 deletions.
diff --git a/app/Console/Commands/SaveUser.php b/app/Console/Commands/SaveUser.php
@@ -46,7 +46,7 @@ public function handle()
 
         // Are we creating a new user or updating an existing one?
         $user = \App\Models\User::where('email', $email)->first();
-        if (!$user) {
+        if ($user === null) {
             $user = new User();
             $user->email = $email;
             $create_new_user = true;
@@ -72,7 +72,7 @@ public function handle()
 
         // Handle admin flag.
         if ($this->option('admin')) {
-            $user->admin = $this->option('admin');
+            $user->admin = (bool) $this->option('admin');
         }
 
         $user->save();

diff --git a/app/Http/Controllers/AdminController.php b/app/Http/Controllers/AdminController.php
@@ -919,18 +919,13 @@ public function install(): View
                             }
                         }
 
-                        $passwordHash = User::PasswordHash($admin_password);
-                        if ($passwordHash === false) {
-                            $xml .= '<alert>Failed to hash password</alert>';
-                        } else {
-                            $user = new \CDash\Model\User();
-                            $user->Email = $admin_email;
-                            $user->Password = $passwordHash;
-                            $user->FirstName = 'administrator';
-                            $user->Institution = 'Kitware Inc.';
-                            $user->Admin = 1;
-                            $user->Save();
-                        }
+                        $user = new \CDash\Model\User();
+                        $user->Email = $admin_email;
+                        $user->Password = password_hash($admin_password, PASSWORD_DEFAULT);
+                        $user->FirstName = 'administrator';
+                        $user->Institution = 'Kitware Inc.';
+                        $user->Admin = 1;
+                        $user->Save();
                         $xml .= '<db_created>1</db_created>';
 
                         // Set the database version

diff --git a/app/Http/Controllers/CoverageController.php b/app/Http/Controllers/CoverageController.php
@@ -88,7 +88,7 @@ public function manageCoverage(): View|RedirectResponse
 
         $sql = 'SELECT id,name FROM project';
         $params = [];
-        if ($User->IsAdmin() == false) {
+        if ($User->admin) {
             $sql .= ' WHERE id IN (SELECT projectid AS id FROM user2project WHERE userid=? AND role>0)';
             $params[] = intval($userid);
         }

diff --git a/app/Http/Controllers/ManageBannerController.php b/app/Http/Controllers/ManageBannerController.php
@@ -26,7 +26,7 @@ public function manageBanner(): View|RedirectResponse
         if (isset($_GET['projectid']) && (int) $_GET['projectid'] > 0) {
             $project->Id = (int) $_GET['projectid'];
             Gate::authorize('edit-project', $project);
-        } elseif ($user->IsAdmin()) {
+        } elseif ($user->admin) {
             // We are able to set the global banner
             $project->Id = (int) ($_GET['projectid'] ?? 0);
         } else {
@@ -37,7 +37,7 @@ public function manageBanner(): View|RedirectResponse
         $available_projects = [];
 
         // If user is admin then we can add a banner for all projects
-        if ($user->IsAdmin()) {
+        if ($user->admin) {
             $root_project = new Project();
             $root_project->Id = 0;
             $root_project->Name = 'All';
@@ -46,7 +46,7 @@ public function manageBanner(): View|RedirectResponse
 
         $sql = 'SELECT id, name FROM project';
         $params = [];
-        if (!$user->IsAdmin()) {
+        if (!$user->admin) {
             $sql .= " WHERE id IN (SELECT projectid AS id FROM user2project WHERE userid=? AND role>0)";
             $params[] = intval(Auth::id());
         }

diff --git a/app/Http/Controllers/ManageProjectRolesController.php b/app/Http/Controllers/ManageProjectRolesController.php
@@ -24,7 +24,7 @@ final class ManageProjectRolesController extends AbstractProjectController
      */
     public function viewPage(): View|RedirectResponse
     {
-        $usersessionid = Auth::id();
+        /** @var User $current_user */
         $current_user = Auth::user();
 
         @$projectid = $_GET['projectid'];
@@ -41,12 +41,12 @@ public function viewPage(): View|RedirectResponse
         if ($projectid > 0) {
             $current_user_project = new UserProject();
             $current_user_project->ProjectId = $projectid;
-            $current_user_project->UserId = $usersessionid;
+            $current_user_project->UserId = Auth::id();
             $current_user_project->FillFromUserId();
             $role = $current_user_project->Role;
         }
 
-        if (!$current_user->IsAdmin() && $role <= 1) {
+        if (!$current_user->admin && $role <= 1) {
             return view('cdash', [
                 'xsl' => true,
                 'xsl_content' => "You don't have the permissions to access this page!",
@@ -95,9 +95,7 @@ public function viewPage(): View|RedirectResponse
                     if (strlen($email) > 0) {
                         $email .= ', ';
                     }
-                    $maintainer = new User();
-                    $maintainer->Id = $maintainerid;
-                    $email .= $maintainer->GetEmail();
+                    $email .= User::findOrFail((int) $maintainerid)->email;
                 }
 
                 $projectname = get_project_name($projectid);
@@ -262,9 +260,9 @@ public function viewPage(): View|RedirectResponse
 
         $sql = 'SELECT id, name FROM project';
         $params = [];
-        if (!$current_user->IsAdmin()) {
+        if (!$current_user->admin) {
             $sql .= ' WHERE id IN (SELECT projectid AS id FROM user2project WHERE userid=? AND role>0)';
-            $params[] = intval($usersessionid);
+            $params[] = intval(Auth::id());
         }
         $sql .= ' ORDER BY name';
         $projects = $db->executePrepared($sql, $params);

diff --git a/app/Http/Controllers/ProjectController.php b/app/Http/Controllers/ProjectController.php
@@ -105,7 +105,7 @@ public function apiCreateProject(): JsonResponse
             $project_response['TestTimeMaxStatus'] = 3;
             $project_response['TestTimeStd'] = 4.0;
             $project_response['TestTimeStdThreshold'] = 1.0;
-            if (!boolval(config('cdash.user_create_projects')) || $User->IsAdmin()) {
+            if (!boolval(config('cdash.user_create_projects')) || $User->admin) {
                 $project_response['UploadQuota'] = 1;
             }
             $project_response['WarningsFilter'] = "";

diff --git a/app/Http/Controllers/SubProjectController.php b/app/Http/Controllers/SubProjectController.php
@@ -47,7 +47,7 @@ public function apiManageSubProject(): JsonResponse
 
         $sql = 'SELECT id, name FROM project';
         $params = [];
-        if (!$user->IsAdmin()) {
+        if (!$user->admin) {
             $sql .= " WHERE id IN (SELECT projectid AS id FROM user2project WHERE userid = ? AND role > 0)";
             $params[] = intval(Auth::id());
         }

diff --git a/app/Http/Controllers/SubscribeProjectController.php b/app/Http/Controllers/SubscribeProjectController.php
@@ -313,7 +313,7 @@ public function subscribeProject(): View|RedirectResponse
 
         $sql = 'SELECT id, name FROM project';
         $params = [];
-        if ($user->IsAdmin() === false) {
+        if (!$user->admin) {
             $sql .= " WHERE public=1 OR id IN (SELECT projectid AS id FROM user2project WHERE userid=? AND role>0)";
             $params[] = $user->id;
         }

diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php
@@ -388,12 +388,6 @@ public function edit(): View
                 $error_msg = "Password must be at least $minimum_length characters.";
             }
 
-            $password_hash = User::PasswordHash($passwd);
-            if ($password_hash === false) {
-                $password_is_good = false;
-                $error_msg = 'Failed to hash password.  Contact an admin.';
-            }
-
             if ($password_is_good && config('cdash.password.expires') > 0) {
                 $query = 'SELECT password FROM password WHERE userid=?';
                 $unique_count = (int) config('cdash.password.unique');
@@ -429,7 +423,7 @@ public function edit(): View
             if (!$password_is_good) {
                 $xml .= "<error>$error_msg</error>";
             } else {
-                $user->Password = $password_hash;
+                $user->Password = password_hash($passwd, PASSWORD_DEFAULT);
                 if ($user->Save()) {
                     $xml .= '<error>Your password has been updated.</error>';
                     if (isset($_SESSION['cdash']['redirect'])) {
@@ -512,8 +506,8 @@ public function recoverPassword(): View
         if (isset($_POST['recover'])) {
             $email = $_POST['email'];
             $user = new \CDash\Model\User();
-            $userid = $user->GetIdFromEmail($email);
-            if (!$userid) {
+            $userid = User::firstWhere('email', $email)?->id;
+            if ($userid === null) {
                 // Don't reveal whether or not this is a valid account.
                 $message = 'A confirmation message has been sent to your inbox.';
             } else {
@@ -531,10 +525,9 @@ public function recoverPassword(): View
 
                 if (cdashmail("$email", 'CDash password recovery', $text)) {
                     // If we can send the email we update the database
-                    $passwordHash = User::PasswordHash($password);
                     $user->Id = $userid;
                     $user->Fill();
-                    $user->Password = $passwordHash;
+                    $user->Password = password_hash($password, PASSWORD_DEFAULT);
                     $user->Save();
                     $message = 'A confirmation message has been sent to your inbox.';
                 } else {

diff --git a/app/Http/Middleware/Admin.php b/app/Http/Middleware/Admin.php
@@ -15,7 +15,8 @@ public function handle(Request $request, Closure $next)
     {
         // We can assume that the user is logged in at this point.  We deliberately want to fail with an
         // exception if this is not the case.
-        if (!Auth::user()->IsAdmin()) {
+        $user = Auth::user();
+        if ($user === null || !$user->admin) {
             abort(403, 'You must be an administrator to access this page.');
         }
 

diff --git a/app/Http/Middleware/PasswordExpired.php b/app/Http/Middleware/PasswordExpired.php
@@ -2,7 +2,8 @@
 
 namespace App\Http\Middleware;
 
-use CDash\Model\User;
+use App\Models\User;
+use Carbon\Carbon;
 use Closure;
 use Illuminate\Support\Facades\Auth;
 use Illuminate\Support\Str;
@@ -23,9 +24,14 @@ public function handle($request, Closure $next)
             if (!Str::contains(url()->current(), $password_expired_path)) {
                 /** @var User $user */
                 $user = Auth::user();
-                if ($user->hasExpiredPassword()) {
-                    $password_expired_uri = "{$password_expired_path}?password_expired=1";
-                    return redirect($password_expired_uri);
+                $current_password = $user->currentPassword ?? null;
+                if ($current_password !== null) {
+                    $password_lifetime = (int) config('cdash.password.expires');
+                    $password_expired = $current_password->date->addDays($password_lifetime) < Carbon::now();
+                    if ($password_lifetime > 0 && $password_expired) {
+                        $password_expired_uri = "{$password_expired_path}?password_expired=1";
+                        return redirect($password_expired_uri);
+                    }
                 }
             }
         }

diff --git a/app/Models/Password.php b/app/Models/Password.php
@@ -2,14 +2,37 @@
 
 namespace App\Models;
 
+use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Database\Eloquent\Model;
+use Illuminate\Support\Carbon;
 
+/**
+ * @property int $id
+ * @property int $userid
+ * @property string $password
+ * @property Carbon $date
+ *
+ * TODO: Determine if created_at and updated_at columns are necessary
+ *
+ * @mixin Builder<Password>
+ */
 class Password extends Model
 {
-    public $incrementing = false;
-
     protected $table = 'password';
-    protected $fillable = ['password', 'date'];
-    protected $hidden = ['password'];
-    protected $primaryKey = null;
+
+    protected $fillable = [
+        'userid',
+        'password',
+        'date',
+    ];
+
+    protected $casts = [
+        'id' => 'integer',
+        'userid' => 'integer',
+        'date' => 'datetime',
+    ];
+
+    protected $hidden = [
+        'password',
+    ];
 }
diff --git a/app/Models/Project.php b/app/Models/Project.php
@@ -161,7 +161,7 @@ public function scopeForUser(Builder $query, ?User $user = null): void
 
         if ($user === null) {
             $query->where('public', self::ACCESS_PUBLIC);
-        } elseif (!$user->IsAdmin()) {
+        } elseif (!$user->admin) {
             $query->whereHas('users', function ($query2) use ($user) {
                 $query2->where('user.id', $user->id);
                 $query2->orWhere('public', self::ACCESS_PUBLIC);

diff --git a/app/Models/User.php b/app/Models/User.php
@@ -3,6 +3,8 @@
 namespace App\Models;
 
 use Illuminate\Database\Eloquent\Builder;
+use Illuminate\Database\Eloquent\Relations\HasMany;
+use Illuminate\Database\Eloquent\Relations\HasOne;
 use Illuminate\Notifications\Notifiable;
 use Illuminate\Contracts\Auth\MustVerifyEmail;
 use Illuminate\Foundation\Auth\User as Authenticatable;
@@ -11,17 +13,19 @@
  * All of these methods are accessed through reflection.  Only the ones currently necessary are
  * listed to encourage future developers to move User logic to this class.
  *
- * @method bool IsAdmin()
  * @method GetEmail()
  * @method int|false GetIdFromName($name)
  *
  * @property int $id
+ * @property bool $admin
  * @property string $firstname
  * @property string $lastname
  * @property string $email
  * @property string $password
  * @property string $institution
  *
+ * @property Password $currentPassword
+ *
  * @mixin Builder<User>
  */
 class User extends Authenticatable implements MustVerifyEmail
@@ -57,9 +61,24 @@ class User extends Authenticatable implements MustVerifyEmail
         'password', 'remember_token',
     ];
 
-    public function passwords()
+    protected $casts = [
+        'admin' => 'boolean',
+    ];
+
+    /**
+     * @return HasMany<Password>
+     */
+    public function passwords(): HasMany
+    {
+        return $this->hasMany(Password::class, 'userid')->orderBy('date', 'desc');
+    }
+
+    /**
+     * @return HasOne<Password>
+     */
+    public function currentPassword(): HasOne
     {
-        return $this->hasMany('App\Models\Password', 'userid')->orderBy('date');
+        return $this->hasOne(Password::class, 'userid')->ofMany('date', 'max');
     }
 
     /**

diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php
@@ -60,7 +60,7 @@ private function defineGates(): void
         });
 
         Gate::define('create-project', function (User $user) {
-            if (!$user->IsAdmin() && !boolval(config('cdash.user_create_projects'))) {
+            if (!$user->admin && !boolval(config('cdash.user_create_projects'))) {
                 return Response::denyWithStatus(403, 'You do not have permission to create new projects.');
             }
             return Response::allow();

diff --git a/app/Services/AuthTokenService.php b/app/Services/AuthTokenService.php
@@ -175,7 +175,7 @@ public static function deleteToken(string $token_hash, int $expected_user_id): b
                     if (
                         $expected_user_id !== $auth_token['userid']
                         && $user2project->Role !== UserProject::PROJECT_ADMIN
-                        && !$user->IsAdmin()
+                        && !$user->admin
                     ) {
                         return false;
                     }
@@ -188,7 +188,7 @@ public static function deleteToken(string $token_hash, int $expected_user_id): b
                 // Full-access tokens can be deleted by:
                 // 1. The user who created them
                 // 2. A system administrator
-                if ($expected_user_id !== $auth_token['userid'] && !$user->IsAdmin()) {
+                if ($expected_user_id !== $auth_token['userid'] && !$user->admin) {
                     return false;
                 }
                 break;

diff --git a/app/Services/ProjectPermissions.php b/app/Services/ProjectPermissions.php
@@ -31,7 +31,7 @@ class ProjectPermissions
     public static function canEditProject(Project $project, User $user): bool
     {
         // Check if this user is a global admin.
-        if ($user->IsAdmin()) {
+        if ($user->admin) {
             return true;
         }
 
@@ -65,7 +65,7 @@ public static function canViewProject(Project $project, ?User $user): bool
         }
 
         // Global admins have access to all projects.
-        if ($user->IsAdmin()) {
+        if ($user->admin) {
             return true;
         }