Bug #94088
openWrong permalogin condition in LoginController
0%
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.
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.
Updated by Benni Mack over 3 years ago
- Status changed from New to Needs Feedback
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
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)
Updated by Mathias Schreiber almost 3 years ago
- Sprint Focus set to Remote Sprint