cloudyr / limer

A LimeSurvey R Client
MIT License
67 stars 37 forks source link

get_session_key.R: fix PHP 8.1 major flaw #62

Closed Jan-E closed 1 year ago

Jan-E commented 1 year ago

TL;DR: param 'admin' for get_session_key succeeds in PHP 7.4, but fails in PHP 8.1. The param should be 'username'.

Limer sends 2 params for get_session_key: 'admin' and 'password'

body.json = list(method = "get_session_key",
                   id = " ",
                   params = list(admin = 'report',
                                 password = 'verysecret'))

https://github.com/cloudyr/limer/blob/master/R/get_session_key.R#L15-L18

Limesurvey expects params 'username' and 'password'

    /**
     * Set username and password by event
     *
     * @return null
     */
    public function remoteControlLogin()
    {
        $event = $this->getEvent();
        $this->setUsername($event->get('username'));
        $this->setPassword($event->get('password'));
    }

https://github.com/LimeSurvey/LimeSurvey/blob/971b1a72b5d3e62f299ef0a3829862f3d8ee5966/application/libraries/PluginManager/AuthPluginBase.php#L73-L83

I tested the fix in this PR against the same Limesurvey 5.3 instance, running PHP 7.4 and (symlinked) also PHP 8.1.13. See the comments under the commit: https://github.com/Jan-E/limer/commit/bc807c61fb834cb0034c376d814e80f126fc796c#commitcomment-94178825

Only afterwards I found that migliorati on the Limesurvey forum also suggested renaming 'admin' in 'username': https://forums.limesurvey.org/forum/installation-a-update-issues/127953-remotecontrol-doesn-t-work-after-update-to-5-3

Please review and merge.

Jan-E commented 1 year ago

@andrewheiss Could you merge this PR, please?

Shnoulle commented 1 year ago

Limesurvey expects params 'username' and 'password'

Not really : https://github.com/LimeSurvey/LimeSurvey/blob/3909cf4bd2db67b3b0c6704d5daa99bedc880cee/application/helpers/remotecontrol/remotecontrol_handle.php#L3714-L3715

call here : https://github.com/LimeSurvey/LimeSurvey/blob/3909cf4bd2db67b3b0c6704d5daa99bedc880cee/application/helpers/remotecontrol/remotecontrol_handle.php#L47

It's event, not getRequest

Stranget it was an issue here …

Jan-E commented 1 year ago

Really, Limesurvey does expect params 'username' and 'password' in the body.json. https://github.com/LimeSurvey/LimeSurvey/blob/3909cf4bd2db67b3b0c6704d5daa99bedc880cee/application/controllers/admin/Authentication.php#L151-L153

            // Call the function afterLoginFormSubmit of the plugin.
            // For Authdb, it calls AuthPluginBase::afterLoginFormSubmit()
            // which set the plugin's private variables _username and _password with the POST information if it's a POST request else it does nothing

get_session_key() works using a POST request which invokes the afterLoginFormSubmit event in the Authdb plugin.

https://github.com/LimeSurvey/LimeSurvey/blob/3909cf4bd2db67b3b0c6704d5daa99bedc880cee/application/libraries/PluginManager/AuthPluginBase.php#L73-L83

    /**
     * Set username and password by event
     *
     * @return null
     */
    public function remoteControlLogin()
    {
        $event = $this->getEvent();
        $this->setUsername($event->get('username'));
        $this->setPassword($event->get('password'));
    }

This apparently will not set the _username to a valid value if no param 'username' is in the body.json. Probably $_username will stay 'null'. PHP 8.1 stumbles over this: https://php.watch/versions/8.1/internal-func-non-nullable-null-deprecation

PHP 7 will not stumble over a null private variable '_username' in the Authdb plugin, but I should not trust the results of a afterLoginFormSubmit event when the $event->get('username') and the variable '_username' in the plugin Authdb are not set.

Note: edited after further testing.

Shnoulle commented 1 year ago

I test a lot remoteControlLogin event because before only AuthDB work for plugin. Today i have Auth plugin working

I do my test with AuthLDAP : https://github.com/LimeSurvey/LimeSurvey/commit/98c5293a01eb92cd4cc4930c9e667c3daa50cd44

If it work before (seems you have to fix limer only now) : something was update and broke system.

Please report the issue, and check with previous version of LimeSurvey.

If LS really wait for username then it broke with 7.4 …

RemoteControl don't use afterLoginFormSubmit but remoteControlLogin

if afterLoginFormSubmit event happen during Auth process for remoteControl it's a LimeSurvey issue.

Shnoulle commented 1 year ago

get_session_key()works using a POST request which invokes the afterLoginFormSubmit event in the Authdb plugin.

get_session_key don't call afterLoginFormSubmit event.

Only remoteControlLogin

Jan-E commented 1 year ago

Thanks a lot

r0bis commented 1 year ago

Thanks a lot for sorting this. Very good to see that limer gets fixed/updated.

Shnoulle commented 1 year ago

Think we found the origin of the issue : https://stackoverflow.com/a/71241345/2239406

PHP8 broke jsonRPCv1