contao / core

Contao 3 → see contao/contao for Contao 4
GNU Lesser General Public License v3.0
491 stars 213 forks source link

Undefined variable in branch 3.x #8868

Closed lindesbs closed 6 years ago

lindesbs commented 6 years ago

Beginning of branch 3.x the variable blnRecordExists is used without initialized with data. This bug results in cleaning autologin data on logout()

The variable blnRecordExists in branch 2.11 Initialization in https://github.com/contao/core/blob/2.11/system/libraries/Model.php#L77

Defined when row exists https://github.com/contao/core/blob/2.11/system/libraries/Model.php#L178

In the branch 3.x it is used in https://github.com/contao/core/blob/3.5/system/modules/core/classes/FrontendUser.php#L244

But this variable will never be defined

leofeyer commented 6 years ago

That is true, however I don't see how this can result in cleaning the data? Since the property is never initialized, it should be null and thus result in not cleaning the data. Did you mean this?

lindesbs commented 6 years ago

Yes. Users which has been self logout manually, will be relogedin automatically.

leofeyer commented 6 years ago

@contao/developers I guess we can just remove the if condition entirely, can't we?

lindesbs commented 6 years ago

Our fix is replacing with this: if ($this->autologin)

aschempp commented 6 years ago

Very interesting find. The variable is undefined because the User class is no longer based on Model since Contao 3. I don't understand why the check for blnRecordExists is necessary at all though...

aschempp commented 6 years ago

Well it is possible to do a FrontendUser::getInstance() call and call logout, which would be an unexpected result. We should probably check for $this->intId > 0.

lindesbs commented 6 years ago

There is an additional problem with ModuleCloseAccount. We have to check, if the MemberRow still exists. The entry will be deleted afterwards it will be logout() https://github.com/contao/core/blob/3.5/system/modules/core/modules/ModuleCloseAccount.php#L135

leofeyer commented 6 years ago

IMHO, $blnRecordExists should simply be replaced with \Config::get('autologin') > 0 (see FrontendUser::login().

@aschempp What is the case in which we have to check $this->intId > 0?

aschempp commented 6 years ago

@aschempp What is the case in which we have to check $this->intId > 0?

We're using that in Contao 4 to determine if a user is loaded.

leofeyer commented 6 years ago

@lindesbs Does this issue occur in Contao 4 as well?

lindesbs commented 6 years ago

Not yet tested

leofeyer commented 6 years ago

@lindesbs Do you have time to check? If it occurs in Contao 4 as well, we have to fix it.

lindesbs commented 6 years ago

Obviously no further impact

leofeyer commented 6 years ago

I still think we should fix this if we implement #8888.

leofeyer commented 6 years ago

Fixed in 148cfdecd7dad2961be160f61752ad4093231fc0.