fix coding style and some bugs, re #1552

Merge request studip/studip!3764

5 files changed, 150 insertions, 169 deletions

diff --git a/lib/authentication/Manager.php b/lib/authentication/Manager.php
index 64419ca..6055f9f 100644
--- a/lib/authentication/Manager.php
+++ b/lib/authentication/Manager.php
@@ -11,117 +11,137 @@
  */
 namespace Studip\Authentication;
 
+use AccessDeniedException;
+use Config;
+use Metrics;
+use Request;
+use Seminar_Perm;
+use Seminar_User;
+use StudipAuthAbstract;
+use StudipMail;
+use Token;
+use User;
+
 class Manager
 {
-    private $auth = [];
-    public function __construct(private $nobody = false)
-    {
+    private ?array $auth = [];
+
+    public function __construct(
+        private bool $nobody = false
+    ) {
     }
 
-    /**
-     * @return false|mixed
-     */
-    public function getNobody(): mixed
+    public function getNobody(): bool
     {
         return $this->nobody;
     }
 
-    public function setNobody($allow_nobody = false): void
+    public function setNobody(bool $allow_nobody = false): void
     {
         $this->nobody = $allow_nobody;
     }
 
 
-    public function start()
+    public function start(): bool
     {
         $this->auth =& $_SESSION['auth'];
 
         if (!$this->isAuthenticated()) {
             $user = null;
-            if (($provider = \Request::option('sso'))) {
-                \Metrics::increment('core.sso_login.attempted');
+
+            $provider = Request::option('sso');
+
+            if ($provider) {
+                Metrics::increment('core.sso_login.attempted');
                 // then do login
-                $authplugin = \StudipAuthAbstract::GetInstance($provider);
+                $authplugin = StudipAuthAbstract::GetInstance($provider);
                 if ($authplugin) {
                     $authplugin->authenticateUser('', '');
                     if ($authplugin->getUser()) {
                         $user = $authplugin->getStudipUser($authplugin->getUser());
-                        $exp_d = \UserConfig::get($user->id)->EXPIRATION_DATE;
-                        if ($exp_d > 0 && $exp_d < time()) {
-                            throw new \AccessDeniedException(
+                        if ($user->isExpired()) {
+                            throw new AccessDeniedException(
                                 _('Dieses Benutzerkonto ist abgelaufen. Wenden Sie sich bitte an die Administration.')
                             );
                         }
-                        if ($user->locked == 1) {
-                            throw new \AccessDeniedException(
+                        if ($user->locked) {
+                            throw new AccessDeniedException(
                                 _('Dieser Benutzer ist gesperrt! Wenden Sie sich bitte an die Administration.')
                             );
                         }
-                        \Metrics::increment('core.sso_login.succeeded');
+                        Metrics::increment('core.sso_login.succeeded');
+
                         sess()->regenerateId(['auth', '_language', 'phpCAS', 'contrast']);
                     }
                 }
             }
             if (!$user) {
-                if ($this->nobody && !\Request::get('again')) {
-                    $this->setAuthenticatedUser(\User::build(['user_id' => 'nobody', 'perms' => null]));
-                }
-                if (!match_route('dispatch.php/login')) {
+                if ($this->nobody && !Request::get('again')) {
+                    $this->setAuthenticatedUser(User::build(['user_id' => 'nobody', 'perms' => null]));
+                } elseif (!match_route('dispatch.php/login')) {
                     return false;
                 }
             }
         } else {
-            if ($this->auth['uid'] !== 'nobody' && \Request::get('again') && !match_route('dispatch.php/login')) {
+            if (
+                $this->auth['uid'] !== 'nobody'
+                && Request::get('again')
+                && !match_route('dispatch.php/login')
+            ) {
                 return false;
             }
-            $this->setAuthenticatedUser($this->auth['uid'] !== 'nobody' ? \User::find($this->auth['uid']) : \User::build(['user_id' => 'nobody', 'perms' => null]));
+            $this->setAuthenticatedUser($this->auth['uid'] !== 'nobody' ? User::find($this->auth['uid']) : User::build(['user_id' => 'nobody', 'perms' => null]));
         }
         return true;
     }
 
-    public function isAuthenticated()
+    public function isAuthenticated(): string|false
     {
         if (!is_array($this->auth)) {
             $this->auth = [];
         }
-        if (isset($this->auth['uid']) && $this->auth['uid'] === 'nobody' && (!$this->nobody || \Request::option('again'))) {
+        if (
+            isset($this->auth['uid'])
+            && $this->auth['uid'] === 'nobody'
+            && (!$this->nobody || Request::option('again'))
+        ) {
             $this->auth['uid'] = null;
         }
-        $cfg = \Config::GetInstance();
+
+        $maintenance_mode = Config::get()->getValue('MAINTENANCE_MODE');
+
         //check if the user got kicked meanwhile, or if user is locked out
+        $user = null;
         if (!empty($this->auth['uid']) && !in_array($this->auth['uid'], ['nobody'])) {
-            $user = null;
-            if (isset($GLOBALS['user']) && $GLOBALS['user']->id == $this->auth['uid']) {
-                $user = $GLOBALS['user'];
+            if (isset($GLOBALS['user']) && $GLOBALS['user']->id === $this->auth['uid']) {
+                $user = User::findCurrent();
             } else {
-                $user = \User::find($this->auth['uid']);
+                $user = User::find($this->auth['uid']);
             }
-            $exp_d = $user->username ? \UserConfig::get($user->id)->EXPIRATION_DATE : 0;
-            if (!$user->username || $user->locked || ($exp_d > 0 && $exp_d < time())) {
+            if (!$user->username || $user->isBlocked()) {
                 $this->auth = [];
             }
-        } elseif ($cfg->getValue('MAINTENANCE_MODE_ENABLE') && \Request::username('loginname')) {
-            $user = \User::findByUsername(\Request::username('loginname'));
+        } elseif ($maintenance_mode && Request::username('loginname')) {
+            $user = User::findByUsername(Request::username('loginname'));
         }
-        if ($cfg->getValue('MAINTENANCE_MODE_ENABLE') && $user->perms != 'root') {
+        if ($maintenance_mode && $user?->perms !== 'root') {
             $this->auth = [];
-            throw new \AccessDeniedException(_("Das System befindet sich im Wartungsmodus. Zur Zeit ist kein Zugriff möglich."));
+            throw new AccessDeniedException(_("Das System befindet sich im Wartungsmodus. Zur Zeit ist kein Zugriff möglich."));
         }
-        return @$this->auth['uid'] ? : false;
+        return $this->auth['uid'] ?? false;
     }
 
-    public function setAuthenticatedUser(\User $user): void
+    public function setAuthenticatedUser(User $user): void
     {
         $this->auth['uid'] = $user->id;
-        $GLOBALS['user'] = new \Seminar_User($user);
-        $GLOBALS['perm'] = new \Seminar_Perm();
+
+        $GLOBALS['user'] = new Seminar_User($user);
+        $GLOBALS['perm'] = new Seminar_Perm();
     }
 
-    public function sendValidationMail(\User $user = null): void
+    public function sendValidationMail(?User $user = null): void
     {
-        if (is_null($user)) {
-            $user = \User::findCurrent();
-        }
+        $user ??= User::findCurrent();
 
         // template-variables for the include partial
         $Zeit     = date('H:i:s, d.m.Y', $user->mkdate);
@@ -132,9 +152,9 @@ class Manager
 
         // (re-)send the confirmation mail
         $to     = $user->email;
-        $token  = \Token::create(7 * 24 * 60 * 60, $user->id); // Link is valid for 1 week
+        $token  = Token::create(7 * 24 * 60 * 60, $user->id); // Link is valid for 1 week
         $url    = $GLOBALS['ABSOLUTE_URI_STUDIP'] . 'dispatch.php/registration/email_validation?secret=' . $token;
-        $mail   = new \StudipMail();
+        $mail   = new StudipMail();
         $abuse  = $mail->getReplyToEmail();
 
         $lang_path = getUserLanguagePath($user->id);
diff --git a/lib/classes/auth_plugins/StudipAuthAbstract.php b/lib/classes/auth_plugins/StudipAuthAbstract.php
index 65da54b..eea4538 100644
--- a/lib/classes/auth_plugins/StudipAuthAbstract.php
+++ b/lib/classes/auth_plugins/StudipAuthAbstract.php
@@ -111,7 +111,7 @@ class StudipAuthAbstract
      * always use this method to instantiate a plugin object, it will ensure that only one object of each
      * plugin will exist
      * @param string $plugin_name name of plugin, if omitted an array with all plugin objects will be returned
-     * @return   mixed   either a reference to the plugin with the passed name, or an array with references to all plugins
+     * @return static|static[] either a reference to the plugin with the passed name, or an array with references to all plugins
      */
     public static function getInstance($plugin_name = false)
     {
@@ -341,7 +341,7 @@ class StudipAuthAbstract
      * initialize the new user
      * @param string $username the username to check
      * @param string $password the password to check
-     * @return   string  if authentication succeeds the Stud.IP user , else false
+     * @return User|false  if authentication succeeds the Stud.IP user , else false
      */
     public function authenticateUser($username, $password)
     {
diff --git a/lib/session/CacheSessionHandler.php b/lib/session/CacheSessionHandler.php
index c416ceb..6eea688 100644
--- a/lib/session/CacheSessionHandler.php
+++ b/lib/session/CacheSessionHandler.php
@@ -11,80 +11,71 @@
  */
 namespace Studip\Session;
 
-class CacheSessionHandler implements \SessionHandlerInterface, \SessionIdInterface, \SessionUpdateTimestampHandlerInterface
+use SessionHandlerInterface;
+use SessionIdInterface;
+use SessionUpdateTimestampHandlerInterface;
+use Studip\Cache\Cache;
+use Studip\Cache\Factory;
+
+class CacheSessionHandler implements
+    SessionHandlerInterface,
+    SessionIdInterface,
+    SessionUpdateTimestampHandlerInterface
 {
 
-    const CACHE_KEY_PREFIX = 'session_data';
+    private const CACHE_KEY_PREFIX = 'session_data';
 
-    private $session_lifetime = 7200;
+    private int $session_lifetime = 7200;
 
-    private $cache;
+    private Cache $cache;
 
-    public function __construct($session_lifetime = null)
+    public function __construct(?int $session_lifetime = null)
     {
         if ($session_lifetime) {
             $this->session_lifetime = $session_lifetime;
         }
     }
 
-    /**
-     * @inheritDoc
-     */
     public function close(): bool
     {
         return true;
     }
 
-    /**
-     * @inheritDoc
-     */
-    public function destroy($id): bool
+    public function destroy(string $id): bool
     {
         $cache_key = self::CACHE_KEY_PREFIX . '/' . $id;
         $this->cache->expire($cache_key);
         return true;
     }
 
-    /**
-     * @inheritDoc
-     */
-    public function gc($max_lifetime): int|false
+    public function gc(int $max_lifetime): int|false
     {
         return false;
     }
 
-    /**
-     * @inheritDoc
-     */
-    public function open($path, $name): bool
+    public function open(string $path, string $name): bool
     {
-        $this->cache = \Studip\Cache\Factory::getCache();
+        $this->cache = Factory::getCache();
         return true;
     }
 
-    /**
-     * @inheritDoc
-     */
-    public function read($id): string|false
+    public function read(string $id): string|false
     {
         $cache_key = self::CACHE_KEY_PREFIX . '/' . $id;
         return $this->cache->read($cache_key);
     }
 
-    /**
-     * @inheritDoc
-     */
-    public function write($id, $data): bool
+    public function write(string $id, string $data): bool
     {
         $cache_key = self::CACHE_KEY_PREFIX . '/' . $id;
-        return (bool)$this->cache->write($cache_key, $data, $this->session_lifetime);
+        return $this->cache->write($cache_key, $data, $this->session_lifetime);
     }
 
     public function create_sid(): string
     {
         do {
             $new_id = md5(bin2hex(random_bytes(128)));
-        } while (!$this->read($new_id));
+        } while ($this->read($new_id));
         return $new_id;
     }
 
@@ -95,6 +86,6 @@ class CacheSessionHandler implements \SessionHandlerInterface, \SessionIdInterfa
 
     public function validateId(string $id): bool
     {
-        return (bool)$this->read($id);
+        return (bool) $this->read($id);
     }
 }
diff --git a/lib/session/DbSessionHandler.php b/lib/session/DbSessionHandler.php
index 05a31a4..d64fe46 100644
--- a/lib/session/DbSessionHandler.php
+++ b/lib/session/DbSessionHandler.php
@@ -11,70 +11,69 @@
  */
 
 namespace Studip\Session;
-use \DBManager, \Config, \CronjobTask;
+use DBManager;
+use Config;
+use CronjobTask;
+use SessionGcJob;
+use SessionHandlerInterface;
+use SessionIdInterface;
+use SessionUpdateTimestampHandlerInterface;
 
-class DbSessionHandler implements \SessionHandlerInterface, \SessionIdInterface, \SessionUpdateTimestampHandlerInterface
+class DbSessionHandler implements
+    SessionHandlerInterface,
+    SessionIdInterface,
+    SessionUpdateTimestampHandlerInterface
 {
+    private ?string $exists = null;
 
-    private $exists;
-
-    /**
-     * @inheritDoc
-     */
     public function close(): bool
     {
         return true;
     }
 
-    /**
-     * @inheritDoc
-     */
-    public function destroy($id): bool
+    public function destroy(string $id): bool
     {
-        return (bool)DBManager::get()->execute("DELETE FROM session_data WHERE sid = ? LIMIT 1", [$id]);
+        return (bool) DBManager::get()->execute(
+            "DELETE FROM session_data WHERE sid = ? LIMIT 1",
+            [$id]
+        );
     }
 
-    /**
-     * @inheritDoc
-     */
-    public function gc($max_lifetime): false|int
+    public function gc(int $max_lifetime): false|int
     {
-        //bail out if cronjob activated and not called in cli context
-        if (Config::getInstance()->getValue('CRONJOBS_ENABLE')
-            && ($task = array_pop(CronjobTask::findByClass('SessionGcJob')))
+        // bail out if cronjob activated and not called in cli context
+        if (
+            Config::getInstance()->getValue('CRONJOBS_ENABLE')
+            && ($task = CronjobTask::findOneByClass(SessionGcJob::class))
             && count($task->schedules->findBy('active', 1))
             && PHP_SAPI !== 'cli'
         ) {
             return false;
         }
-        return DBManager::get()->execute("DELETE FROM session_data WHERE changed < FROM_UNIXTIME(?) ", [time() - $max_lifetime]);
+        return DBManager::get()->execute(
+            "DELETE FROM session_data WHERE changed < FROM_UNIXTIME(?) ",
+            [time() - $max_lifetime]
+        );
     }
 
-    /**
-     * @inheritDoc
-     */
-    public function open($path, $name): bool
+    public function open(string $path, string $name): bool
     {
         return true;
     }
 
-    /**
-     * @inheritDoc
-     */
-    #[\ReturnTypeWillChange]
-    public function read($id)
+    public function read(string $id): false|string
     {
-        $str = DBManager::get()->fetchColumn("SELECT val FROM session_data where sid  = ?", [$id]);
+        $str = DBManager::get()->fetchColumn(
+            "SELECT val FROM session_data where sid  = ?",
+            [$id]
+        );
         if ($str) {
             $this->exists = $id;
         }
-        return (string)$str;
+        return $str ?: '';
     }
 
-    /**
-     * @inheritDoc
-     */
-    public function write($id, $data): bool
+    public function write(string $id, string $data): bool
     {
         $db = DBManager::get();
         if ($this->exists === $id) {
@@ -85,9 +84,12 @@ class DbSessionHandler implements \SessionHandlerInterface, \SessionIdInterface,
         return (bool) $stmt->execute([$data, $id]);
     }
 
-    public function exists($id)
+    public function exists(string $id): bool
     {
-        return (bool)DBManager::get()->fetchColumn("SELECT 1 FROM session_data where sid  = ?", [$id]);
+        return (bool) DBManager::get()->fetchColumn(
+            "SELECT 1 FROM session_data where sid  = ?",
+            [$id]
+        );
     }
 
     public function create_sid(): string
@@ -107,7 +109,7 @@ class DbSessionHandler implements \SessionHandlerInterface, \SessionIdInterface,
 
     public function validateId(string $id): bool
     {
-        return (bool)$this->exists($id);
+        return $this->exists($id);
     }
 
 
diff --git a/lib/session/Manager.php b/lib/session/Manager.php
index ea95dbe..60246b2 100644
--- a/lib/session/Manager.php
+++ b/lib/session/Manager.php
@@ -14,13 +14,10 @@ namespace Studip\Session;
 
 class Manager
 {
-    /**
-     * @var \SessionHandlerInterface
-     */
-    protected \SessionHandlerInterface $handler;
-    /**
-     * @var array
-     */
+    public const STATE_UNKNOWN = false;
+    public const STATE_AUTHENTICATED = 'authenticated';
+    public const STATE_NOBODY = 'authenticated';
+
     protected array $options = [
         'name' => 'Seminar_Session',
         'lifetime' => 7200,
@@ -31,19 +28,13 @@ class Manager
         'samesite' => 'Lax',
         'cache_limiter' => 'nocache'
     ];
-    /**
-     * @var null
-     */
-    protected $current_session_state = null;
 
+    protected string|false|null $current_session_state = null;
 
-    /**
-     * @param \SessionHandlerInterface $session_handler
-     * @param array $session_options
-     */
-    public function __construct(\SessionHandlerInterface $session_handler, array $session_options = [])
-    {
-        $this->handler = $session_handler;
+    public function __construct(
+        protected \SessionHandlerInterface $handler,
+        array $session_options = []
+    ) {
         $keys = array_keys($this->options);
         foreach ($keys as $key) {
             if (array_key_exists($key, $session_options)) {
@@ -61,16 +52,15 @@ class Manager
     public function start(): void
     {
         if (!$this->isStarted()) {
-
             ini_set('session.use_strict_mode', 1);
             $current = session_get_cookie_params();
 
-            $lifetime = (int)($this->options['lifetime'] ?: $current['lifetime']);
+            $lifetime = (int) ($this->options['lifetime'] ?: $current['lifetime']);
             $path = $this->options['path'] ?: $current['path'];
             $domain = $this->options['domain'] ?: $current['domain'];
             $samesite = $this->options['samesite'] ?: $current['samesite'];
-            $secure = (bool)$this->options['secure'];
-            $httponly = (bool)$this->options['httponly'];
+            $secure = (bool) $this->options['secure'];
+            $httponly = (bool) $this->options['httponly'];
 
             session_set_cookie_params(compact('lifetime', 'path', 'domain', 'secure', 'samesite', 'httponly'));
             session_name($this->options['name']);
@@ -81,18 +71,11 @@ class Manager
         }
     }
 
-    /**
-     * @return bool
-     */
     public function isStarted(): bool
     {
         return session_status() === PHP_SESSION_ACTIVE;
     }
 
-    /**
-     * @param array $keep_session_vars
-     * @return void
-     */
     public function regenerateId(array $keep_session_vars = []): void
     {
         if (!$this->isStarted()) {
@@ -115,17 +98,11 @@ class Manager
         }
     }
 
-    /**
-     * @return string
-     */
     public function getName(): string
     {
         return $this->options['name'];
     }
 
-    /**
-     * @return void
-     */
     public function destroy(): void
     {
         if (!$this->isStarted()) {
@@ -149,9 +126,6 @@ class Manager
         session_destroy();
     }
 
-    /**
-     * @return void
-     */
     public function save() : void
     {
         session_write_close();
@@ -160,13 +134,10 @@ class Manager
     /**
      * Returns true, if the current session is valid and belongs to an
      * authenticated user. Does not start a session.
-     *
-     * @static
-     * @return bool
      */
     public function isCurrentSessionAuthenticated(): bool
     {
-        return self::getCurrentSessionState() === 'authenticated';
+        return $this->getCurrentSessionState() === self::STATE_AUTHENTICATED;
     }
 
     /**
@@ -175,28 +146,25 @@ class Manager
      * 'authenticated' - session is valid and user is authenticated
      * 'nobody' - session is valid, but user is not authenticated
      * false - no valid session
-     *
-     * @static
-     * @return string|false
      */
     public function getCurrentSessionState(): false|string|null
     {
 
-        if (!is_null($this->current_session_state)) {
+        if ($this->current_session_state !== null) {
             return $this->current_session_state;
         }
-        $state = false;
+        $state = self::STATE_UNKNOWN;
         if (isset($GLOBALS['user']) && is_object($GLOBALS['user'])) {
-            $state = in_array($GLOBALS['user']->id, ['nobody', 'form']) ? 'nobody' : 'authenticated';
+            $state = in_array($GLOBALS['user']->id, ['nobody', 'form']) ? self::STATE_NOBODY : self::STATE_AUTHENTICATED;
         } else {
             $sid = $_COOKIE[$this->getName()];
             if ($sid) {
                 $session_vars = $this->getSessionVars($sid);
                 $session_auth = $session_vars['auth'];
                 if ($session_auth['uid'] && !in_array($session_auth['uid'], ['nobody', 'form'])) {
-                    $state = 'authenticated';
+                    $state = self::STATE_AUTHENTICATED;
                 } else {
-                    $state = in_array($session_auth['uid'], ['nobody', 'form']) ? 'nobody' : false;
+                    $state = in_array($session_auth['uid'], ['nobody', 'form']) ? self::STATE_NOBODY : self::STATE_UNKNOWN;
                 }
             }
         }