[!!!][TASK] Re-organize variable initialization for User Authentication 75/60875/11
authorBenni Mack <benni@typo3.org>
Tue, 4 Jun 2019 11:00:44 +0000 (13:00 +0200)
committerGeorg Ringer <georg.ringer@gmail.com>
Fri, 14 Jun 2019 19:32:49 +0000 (21:32 +0200)
There are a lot of places where AbstractUserAuthentication
and the dependents (BE/FE user auth) set various settings,
both in the constructor and the start() method.

All possible settings are now moved to the constructor,
or to dependent properties in subclasses.

Some changes are now in place:
- UserAuth->loginType must be set now (which was previously
  in start()). This is now checked in the constructor.
- Most of the variables (sessionTimeout/sessionDateLifetime) are
  now set and evaluated inside the constructor, making start()
  much simpler to understand and read.

Resolves: #88527
Releases: master
Change-Id: Ie03b8b93f869f5bafae8f660d6c983bec308f2fa
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/60875
Reviewed-by: Markus Klein <markus.klein@typo3.org>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Georg Ringer <georg.ringer@gmail.com>
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Tested-by: Georg Ringer <georg.ringer@gmail.com>
typo3/sysext/core/Classes/Authentication/AbstractUserAuthentication.php
typo3/sysext/core/Classes/Authentication/BackendUserAuthentication.php
typo3/sysext/core/Classes/Authentication/CommandLineUserAuthentication.php
typo3/sysext/core/Documentation/Changelog/master/Breaking-88527-OverridingCustomValuesInUserAuthenticationDerivatives.rst [new file with mode: 0644]
typo3/sysext/core/Tests/Unit/Authentication/AbstractUserAuthenticationTest.php
typo3/sysext/frontend/Classes/Authentication/FrontendUserAuthentication.php

index 3e84d05..5e5dbc4 100644 (file)
@@ -162,10 +162,10 @@ abstract class AbstractUserAuthentication implements LoggerAwareInterface
     /**
      * GarbageCollection
      * Purge all server session data older than $gc_time seconds.
-     * 0 = default to $this->sessionTimeout or use 86400 seconds (1 day) if $this->sessionTimeout == 0
+     * if $this->sessionTimeout > 0, then the session timeout is used instead.
      * @var int
      */
-    public $gc_time = 0;
+    public $gc_time = 86400;
 
     /**
      * Probability for garbage collection to be run (in percent)
@@ -317,11 +317,20 @@ abstract class AbstractUserAuthentication implements LoggerAwareInterface
 
     /**
      * Initialize some important variables
+     *
+     * @throws Exception
      */
     public function __construct()
     {
-        // This function has to stay even if it's empty
-        // Implementations of that abstract class might call parent::__construct();
+        $this->svConfig = $GLOBALS['TYPO3_CONF_VARS']['SVCONF']['auth'] ?? [];
+        // If there is a custom session timeout, use this instead of the 1d default gc time.
+        if ($this->sessionTimeout > 0) {
+            $this->gc_time = $this->sessionTimeout;
+        }
+        // Backend or frontend login - used for auth services
+        if (empty($this->loginType)) {
+            throw new Exception('No loginType defined, must be set explicitly by subclass', 1476045345);
+        }
     }
 
     /**
@@ -332,33 +341,22 @@ abstract class AbstractUserAuthentication implements LoggerAwareInterface
      * c) Lookup a session attached to a user and check timeout etc.
      * d) Garbage collection, setting of no-cache headers.
      * If a user is authenticated the database record of the user (array) will be set in the ->user internal variable.
-     *
-     * @throws Exception
      */
     public function start()
     {
-        // Backend or frontend login - used for auth services
-        if (empty($this->loginType)) {
-            throw new Exception('No loginType defined, should be set explicitly by subclass', 1476045345);
-        }
         $this->logger->debug('## Beginning of auth logging.');
         $this->newSessionID = false;
-        // $id is set to ses_id if cookie is present. Otherwise a new session will start
-        $id = $this->getCookie($this->name);
-        $this->svConfig = $GLOBALS['TYPO3_CONF_VARS']['SVCONF']['auth'] ?? [];
+        // Make certain that NO user is set initially
+        $this->user = null;
+        // sessionID is set to ses_id if cookie is present. Otherwise a new session will start
+        $this->id = $this->getCookie($this->name);
 
         // If new session or client tries to fix session...
-        if (!$id || !$this->isExistingSessionRecord($id)) {
-            // New random session-$id is made
-            $id = $this->createSessionId();
-            // New session
+        if (!$this->isExistingSessionRecord($this->id)) {
+            $this->id = $this->createSessionId();
             $this->newSessionID = true;
         }
-        // Internal var 'id' is set
-        $this->id = $id;
 
-        // Make certain that NO user is set initially
-        $this->user = null;
         // Set all possible headers that could ensure that the script is not cached on the client-side
         $this->sendHttpHeaders();
         // Load user session, check to see if anyone has submitted login-information and if so authenticate
@@ -375,11 +373,6 @@ abstract class AbstractUserAuthentication implements LoggerAwareInterface
             ];
             GeneralUtility::callUserFunction($funcName, $_params, $this);
         }
-        // Set $this->gc_time if not explicitly specified
-        if ($this->gc_time === 0) {
-            // Default to 86400 seconds (1 day) if $this->sessionTimeout is 0
-            $this->gc_time = $this->sessionTimeout === 0 ? 86400 : $this->sessionTimeout;
-        }
         // If we're lucky we'll get to clean up old sessions
         if (rand() % 100 <= $this->gc_probability) {
             $this->gc();
@@ -1035,6 +1028,9 @@ abstract class AbstractUserAuthentication implements LoggerAwareInterface
      */
     public function isExistingSessionRecord($id)
     {
+        if (empty($id)) {
+            return false;
+        }
         try {
             $sessionRecord = $this->getSessionBackend()->get($id);
             if (empty($sessionRecord)) {
index 4c69c2c..c5dfa7a 100644 (file)
@@ -285,17 +285,22 @@ class BackendUserAuthentication extends AbstractUserAuthentication
         'resizeTextareas_Flexible' => 0
     ];
 
+    /**
+     * Login type, used for services.
+     * @var string
+     */
+    public $loginType = 'BE';
+
     /**
      * Constructor
      */
     public function __construct()
     {
-        parent::__construct();
         $this->name = self::getCookieName();
-        $this->loginType = 'BE';
         $this->warningEmail = $GLOBALS['TYPO3_CONF_VARS']['BE']['warning_email_addr'];
         $this->lockIP = $GLOBALS['TYPO3_CONF_VARS']['BE']['lockIP'];
         $this->sessionTimeout = (int)$GLOBALS['TYPO3_CONF_VARS']['BE']['sessionTimeout'];
+        parent::__construct();
     }
 
     /**
index cfa4ee4..84718ef 100644 (file)
@@ -55,13 +55,11 @@ class CommandLineUserAuthentication extends BackendUserAuthentication
      * Replacement for AbstactUserAuthentication::start()
      *
      * We do not need support for sessions, cookies, $_GET-modes, the postUserLookup hook or
-     * a database connectiona during CLI Bootstrap
+     * a database connection during CLI Bootstrap
      */
     public function start()
     {
-        $this->logger->debug('## Beginning of auth logging.');
-        // svConfig is unused, but we set it, as the property is public and might be used by extensions
-        $this->svConfig = $GLOBALS['TYPO3_CONF_VARS']['SVCONF']['auth'] ?? [];
+        // do nothing
     }
 
     /**
@@ -71,6 +69,7 @@ class CommandLineUserAuthentication extends BackendUserAuthentication
      */
     public function checkAuthentication()
     {
+        // do nothing
     }
 
     /**
diff --git a/typo3/sysext/core/Documentation/Changelog/master/Breaking-88527-OverridingCustomValuesInUserAuthenticationDerivatives.rst b/typo3/sysext/core/Documentation/Changelog/master/Breaking-88527-OverridingCustomValuesInUserAuthenticationDerivatives.rst
new file mode 100644 (file)
index 0000000..484ecb0
--- /dev/null
@@ -0,0 +1,48 @@
+.. include:: ../../Includes.txt
+
+==============================================================================
+Breaking: #88527 - Overriding custom values in User Authentication derivatives
+==============================================================================
+
+See :issue:`88527`
+
+Description
+===========
+
+Due to some restructuring of :php:`AbstractUserAuthentication` and its direct sub-classes
+:php:`BackendUserAuthentication` (a.k.a. :php:`$BE_USER`) and :php:`FrontendUserAuthentication`,
+various settings are now directly initiated and set in the respective constructor of each PHP class.
+
+Following this, the properties `sessionTimeout`, `gc_time` and `sessionDataLifetime` are set already
+when the constructor is called. Before this was the case when :php:`start()` was called.
+
+In addition, the property `loginType` must be set for any subclass on instantiation. Previously
+this was possible to be set just before :php:`start()` was called.
+
+The previous behavior allowed to override certain parameters to be evaluated just before `start()`.
+
+
+Impact
+======
+
+Setting any global variables between the constructor method and `start()` will have no effect, as
+this is transferred and evaluated at the public properties already when the constructor is called.
+
+Subclassing :php:`AbstractUserAuthentication` without setting `loginType` will trigger an exception
+on instantiation.
+
+
+Affected Installations
+======================
+
+Any TYPO3 installation where a custom UserAuthentication instantiation or sub-class is in place, and the setting
+order was changed between calling the constructor and the method `start()`, which is considered a very rare case.
+
+
+Migration
+=========
+
+Consider using a proper subclass and a custom constructor method, or set all properties properly before
+the constructor is called (default values of class members).
+
+.. index:: PHP-API, NotScanned
\ No newline at end of file
index de3c881..def7e39 100644 (file)
@@ -61,7 +61,9 @@ class AbstractUserAuthenticationTest extends UnitTestCase
         /** @var $mock \TYPO3\CMS\Core\Authentication\AbstractUserAuthentication */
         $mock = $this->getMockBuilder(AbstractUserAuthentication::class)
             ->setMethods(['dummy'])
+            ->disableOriginalConstructor()
             ->getMock();
+        $mock->loginType = 'BE';
         $mock->checkPid = true;
         $mock->checkPid_value = null;
         $mock->user_table = 'be_users';
@@ -76,7 +78,8 @@ class AbstractUserAuthenticationTest extends UnitTestCase
     {
         $_SERVER['HTTP_USER_AGENT'] = 'Mozilla/5.0 (Windows NT 6.2; rv:22.0) Gecko/20130405 Firefox/23.0';
         $_SERVER['HTTPS'] = 'on';
-        $subject = $this->getAccessibleMockForAbstractClass(AbstractUserAuthentication::class);
+        $subject = $this->getAccessibleMockForAbstractClass(AbstractUserAuthentication::class, [], '', false);
+        $subject->_set('loginType', 'BE');
         $result = $subject->_call('getHttpHeaders');
         $this->assertEquals($result['Pragma'], 'no-cache');
     }
@@ -88,7 +91,7 @@ class AbstractUserAuthenticationTest extends UnitTestCase
     {
         $_SERVER['HTTP_USER_AGENT'] = 'Mozilla/5.0 (Windows; U; MSIE 9.0; WIndows NT 9.0; en-US)';
         $_SERVER['HTTPS'] = 'on';
-        $subject = $this->getAccessibleMockForAbstractClass(AbstractUserAuthentication::class);
+        $subject = $this->getAccessibleMockForAbstractClass(AbstractUserAuthentication::class, [], '', false);
         $result = $subject->_call('getHttpHeaders');
         $this->assertEquals($result['Pragma'], 'private');
     }
index fb36868..93706c3 100644 (file)
@@ -25,6 +25,30 @@ use TYPO3\CMS\Core\Utility\GeneralUtility;
  */
 class FrontendUserAuthentication extends AbstractUserAuthentication
 {
+    /**
+     * Login type, used for services.
+     * @var string
+     */
+    public $loginType = 'FE';
+
+    /**
+     * Form field with login-name
+     * @var string
+     */
+    public $formfield_uname = 'user';
+
+    /**
+     * Form field with password
+     * @var string
+     */
+    public $formfield_uident = 'pass';
+
+    /**
+     * Form field with status: *'login', 'logout'. If empty login is not verified.
+     * @var string
+     */
+    public $formfield_status = 'logintype';
+
     /**
      * form field with 0 or 1
      * 1 = permanent login enabled
@@ -49,6 +73,36 @@ class FrontendUserAuthentication extends AbstractUserAuthentication
      */
     public $sessionTimeout = 6000;
 
+    /**
+     * Table in database with user data
+     * @var string
+     */
+    public $user_table = 'fe_users';
+
+    /**
+     * Column for login-name
+     * @var string
+     */
+    public $username_column = 'username';
+
+    /**
+     * Column for password
+     * @var string
+     */
+    public $userident_column = 'password';
+
+    /**
+     * Column for user-id
+     * @var string
+     */
+    public $userid_column = 'uid';
+
+    /**
+     * Column name for last login timestamp
+     * @var string
+     */
+    public $lastLogin_column = 'lastlogin';
+
     /**
      * @var string
      */
@@ -59,6 +113,17 @@ class FrontendUserAuthentication extends AbstractUserAuthentication
      */
     public $usergroup_table = 'fe_groups';
 
+    /**
+     * Enable field columns of user table
+     * @var array
+     */
+    public $enablecolumns = [
+        'deleted' => 'deleted',
+        'disabled' => 'disable',
+        'starttime' => 'starttime',
+        'endtime' => 'endtime'
+    ];
+
     /**
      * @var array
      */
@@ -105,37 +170,35 @@ class FrontendUserAuthentication extends AbstractUserAuthentication
     protected $loginHidden = false;
 
     /**
-     * Default constructor.
+     * Will prevent the setting of the session cookie (takes precedence over forceSetCookie)
+     * Disable cookie by default, will be activated if saveSessionData() is called,
+     * a user is logging-in or an existing session is found
+     * @var bool
      */
-    public function __construct()
-    {
-        parent::__construct();
+    public $dontSetCookie = true;
 
-        // Disable cookie by default, will be activated if saveSessionData() is called,
-        // a user is logging-in or an existing session is found
-        $this->dontSetCookie = true;
+    /**
+     * Send no-cache headers (disabled by default, if no fixed session is there)
+     * @var bool
+     */
+    public $sendNoCacheHeaders = false;
 
+    public function __construct()
+    {
         $this->name = self::getCookieName();
-        $this->loginType = 'FE';
-        $this->user_table = 'fe_users';
-        $this->username_column = 'username';
-        $this->userident_column = 'password';
-        $this->userid_column = 'uid';
-        $this->lastLogin_column = 'lastlogin';
-        $this->enablecolumns = [
-            'deleted' => 'deleted',
-            'disabled' => 'disable',
-            'starttime' => 'starttime',
-            'endtime' => 'endtime'
-        ];
-        $this->formfield_uname = 'user';
-        $this->formfield_uident = 'pass';
-        $this->formfield_status = 'logintype';
-        $this->sendNoCacheHeaders = false;
         $this->lockIP = $GLOBALS['TYPO3_CONF_VARS']['FE']['lockIP'];
         $this->checkPid = $GLOBALS['TYPO3_CONF_VARS']['FE']['checkFeUserPid'];
         $this->lifetime = (int)$GLOBALS['TYPO3_CONF_VARS']['FE']['lifetime'];
         $this->sessionTimeout = (int)$GLOBALS['TYPO3_CONF_VARS']['FE']['sessionTimeout'];
+        if ($this->sessionTimeout > 0 && $this->sessionTimeout < $this->lifetime) {
+            // If server session timeout is non-zero but less than client session timeout: Copy this value instead.
+            $this->sessionTimeout = $this->lifetime;
+        }
+        $this->sessionDataLifetime = (int)$GLOBALS['TYPO3_CONF_VARS']['FE']['sessionDataLifetime'];
+        if ($this->sessionDataLifetime <= 0) {
+            $this->sessionDataLifetime = 86400;
+        }
+        parent::__construct();
     }
 
     /**
@@ -152,24 +215,6 @@ class FrontendUserAuthentication extends AbstractUserAuthentication
         return $configuredCookieName;
     }
 
-    /**
-     * Starts a user session
-     *
-     * @see AbstractUserAuthentication::start()
-     */
-    public function start()
-    {
-        if ($this->sessionTimeout > 0 && $this->sessionTimeout < $this->lifetime) {
-            // If server session timeout is non-zero but less than client session timeout: Copy this value instead.
-            $this->sessionTimeout = $this->lifetime;
-        }
-        $this->sessionDataLifetime = (int)$GLOBALS['TYPO3_CONF_VARS']['FE']['sessionDataLifetime'];
-        if ($this->sessionDataLifetime <= 0) {
-            $this->sessionDataLifetime = 86400;
-        }
-        parent::start();
-    }
-
     /**
      * Returns a new session record for the current user for insertion into the DB.
      *