Project

General

Profile

Actions

Bug #94088

open

Wrong permalogin condition in LoginController

Added by Harald Witt over 3 years ago. Updated over 2 years ago.

Status:
Needs Feedback
Priority:
Should have
Assignee:
-
Category:
felogin
Target version:
Start date:
2021-05-07
Due date:
% Done:

0%

Estimated time:
TYPO3 Version:
10
PHP Version:
Tags:
permalogin felogin
Complexity:
no-brainer
Is Regression:
Sprint Focus:
Remote Sprint

Description

    protected function isPermaloginDisabled(int $permaLogin): bool
    {
        return $permaLogin > 1
               || (int)($this->settings['showPermaLogin'] ?? 0) === 0
               || $GLOBALS['TYPO3_CONF_VARS']['FE']['lifetime'] === 0;
    }

Permalogin is disabled if "$permalogin < 1"!
In most cases $permalogin will be equal to 1. So this bug will have no consequences.

Actions #1

Updated by Harald Witt over 3 years ago

  • Description updated (diff)
Actions #2

Updated by Johannes Seipelt over 3 years ago

For me this does not look like a bug but more of a naming/code style issue. isPermaloginDisabled handles 3 special cases where the permalogin checkbox should not be displayed. Two of them are described in the comment of the method calling isPermaloginDisabled, permalogin=2 (forced) and cache lifetime=0.


    /**
     * The permanent login checkbox should only be shown if permalogin is not deactivated (-1),
     * not forced to be always active (2) and lifetime is greater than 0
     *
     * @return int
     */
    protected function getPermaloginStatus(): int
    {
        $permaLogin = (int)$GLOBALS['TYPO3_CONF_VARS']['FE']['permalogin'];

        return $this->isPermaloginDisabled($permaLogin) ? -1 : $permaLogin;
    }

    protected function isPermaloginDisabled(int $permaLogin): bool
    {
        return $permaLogin > 1
               || (int)($this->settings['showPermaLogin'] ?? 0) === 0
               || $GLOBALS['TYPO3_CONF_VARS']['FE']['lifetime'] === 0;
    }

and additionally there is logic in the fluid template felogin/Resources/Private/Templates/Login/Login.html
I wrote some tests and the logic worked correctly.
Did you had a case where the permalogin checkbox was/wasn't shown as expected? That would be nice so we can reproduce the issue.

Actions #3

Updated by Benni Mack over 3 years ago

  • Status changed from New to Needs Feedback
Actions #4

Updated by Harald Witt over 3 years ago

I work with $permalogin=1. So the condition $permalogin>1 will result false as well as $permalogin<1. So it is not disabled if the other two conditions meet, what is absolutely correct.

But now let's assume $permalogin is forced to be always active (2). The condition $permalogin>1 will give "true" what means isPermaloginDisabled() will surely return true. That means permalogin is disabled, what is the opposite of "forced to be always active"!

Now let's assume the 2'nd and the 3'rd condition result in false. So all depends on the first condition $permalogin>1.
Here are the 4 cases:

                            isPermaloginDisabled() 
$permalogin  $permalogin>1       returns               my comment

    -1          false             false                should be true because -1 means disabled
     0          false             false                should be true because 0 means disabled by default
     1          false             false                THIS IS THE ONLY CORRECT RESULT
     2          true              true                 should be false because 2 means permalogin is forced to be enabled (not disabled)

So if you need a testcase, simply set permalogin to 2 and it will NOT be shown although it means to be forced to.

Let's modify the condition to $permalogin < 1:

    protected function isPermaloginDisabled(int $permaLogin): bool
    {
        return $permaLogin < 1
               || (int)($this->settings['showPermaLogin'] ?? 0) === 0
               || $GLOBALS['TYPO3_CONF_VARS']['FE']['lifetime'] === 0;
    }

This will result in the following cases:
                            isPermaloginDisabled() 
$permalogin  $permalogin<1       returns               my comment

    -1          true              true                 correct
     0          true              true                 correct
     1          false             false                correct
     2          false             false                correct

Actions #5

Updated by Johannes Seipelt over 3 years ago

So to confirm we have an understanding about the issue here: You dont actually had a case where the login form rendered something wrong (permalogin checkbox missing etc.), but the method is not doing what you expect it to do by its name "isPermaloginDisabled" (this is what i was refering to with "naming" issue).

To be more clear: the method is only used once in the typo3 core by the method above "getPermaloginStatus". Which is as far as i can tell correctly doing what the indent is as described by the comment and comparing it to older fe-login versions, as well as tests.

So it only gets problematic if you extend the LoginController and call isPermaloginDisabled (as it is protectet, not private) and it would return an incorrect bolean value for the thing you want to know "is the permalogin disabled?".

So we would be better of rename it correctly, something like "getPermaloginStatusOnlyConsideringTheThreeSettingsIsFeCachetimeZeroIsPermaloginForcedIsShowPermaloginEnabled" (kinda hard to get a right name here), make it private and also create a proper puplic api you could call to get the needed information about if the permalogin is enabled/disabled.
In my opinion this also implicates that the logic currently located in the fluid template needs to refactored into this public api method, because currently its split across three places (Fluid Template -> getPermaloginStatus -> isPermaloginDisabled)

Actions #6

Updated by Mathias Schreiber over 2 years ago

  • Sprint Focus set to Remote Sprint
Actions

Also available in: Atom PDF