From dd29ca1ee88db7dea246a0da0b93ee86a4301eae Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 8 Dec 2016 23:31:27 +1100 Subject: [PATCH 1/2] Correct decoding of JSON response The old version assumed `json_decode` would return an associative array. While it can do so if an extra option is specified, the default behaviour is to return an object. Therefore, a successful password change resulted in an error while parsing the response. The new code is accessing the response as object instead. The method is now covered by a unit test. --- plugins/password/drivers/cpanel_webmail.php | 8 +++++-- plugins/password/tests/Password.php | 25 ++++++++++++++++++++- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/plugins/password/drivers/cpanel_webmail.php b/plugins/password/drivers/cpanel_webmail.php index 77fa0409f..6671c13ed 100644 --- a/plugins/password/drivers/cpanel_webmail.php +++ b/plugins/password/drivers/cpanel_webmail.php @@ -70,13 +70,17 @@ class rcube_cpanel_webmail_password */ public static function decode_response($response) { + if (!$response) { + return PASSWORD_CONNECT_ERROR; + } + $result = json_decode($response); - if ($result['status'] === 1) { + if ($result->status === 1) { return PASSWORD_SUCCESS; } - $errors = $result['errors']; + $errors = $result->errors; if (is_array($errors) && count($errors) > 0) { return array( 'code' => PASSWORD_ERROR, diff --git a/plugins/password/tests/Password.php b/plugins/password/tests/Password.php index b64c6b889..690213d89 100644 --- a/plugins/password/tests/Password.php +++ b/plugins/password/tests/Password.php @@ -19,5 +19,28 @@ class Password_Plugin extends PHPUnit_Framework_TestCase $this->assertInstanceOf('password', $plugin); $this->assertInstanceOf('rcube_plugin', $plugin); } -} + function test_driver_cpanel_webmail() + { + $driver = 'cpanel_webmail'; + include_once __DIR__ . "/../drivers/$driver.php"; + $driver_class = "rcube_${driver}_password"; + $this->assertTrue(class_exists($driver_class)); + + $json_response_fail = '{"data":null,"errors":' + . '["Execution of Email::passwdpop (api version:3) is not ' + . 'permitted inside of webmail"],"status":0,"metadata":{},' + . '"messages":null}'; + $result = $driver_class::decode_response($json_response_fail); + $this->assertTrue(is_array($result)); + $this->assertEquals($result['code'], PASSWORD_ERROR); + $expected_message = 'Execution of Email::passwdpop (api version:3) is' + . ' not permitted inside of webmail'; + $this->assertEquals($result['message'], $expected_message); + + $json_response_success = '{"metadata":{},"data":null,"messages":null,' + . '"errors":null,"status":1}'; + $result = $driver_class::decode_response($json_response_success); + $this->assertEquals($result, PASSWORD_SUCCESS); + } +} From bd5eaf98aaa48fa34c58890338305e1b3fe9d65f Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Sat, 10 Dec 2016 08:45:08 +1100 Subject: [PATCH 2/2] Handle null or non-JSON result of cPanel UAPI More unit testing. More documentation. --- plugins/password/drivers/cpanel_webmail.php | 17 +++++- plugins/password/tests/Password.php | 65 +++++++++++++++------ 2 files changed, 62 insertions(+), 20 deletions(-) diff --git a/plugins/password/drivers/cpanel_webmail.php b/plugins/password/drivers/cpanel_webmail.php index 6671c13ed..674ac870a 100644 --- a/plugins/password/drivers/cpanel_webmail.php +++ b/plugins/password/drivers/cpanel_webmail.php @@ -28,6 +28,15 @@ class rcube_cpanel_webmail_password { + /** + * Changes the user's password. It is called by password.php. + * See "Driver API" README and password.php for the interface details. + * + * @param string $curpas current (old) password + * @param string $newpass new requested password + * @return mixed int code or assoc array with 'code' and 'message', see + * "Driver API" README and password.php + */ public function save($curpas, $newpass) { $user = $_SESSION['username']; @@ -66,7 +75,7 @@ class rcube_cpanel_webmail_password * * @param string $response JSON response by the Cpanel UAPI * - * @return mixed response code or array + * @return mixed response code or array, see save */ public static function decode_response($response) { @@ -74,9 +83,11 @@ class rcube_cpanel_webmail_password return PASSWORD_CONNECT_ERROR; } - $result = json_decode($response); + // $result should be `null` or `stdClass` object + $result = json_decode($response, !JSON_OBJECT_AS_ARRAY); - if ($result->status === 1) { + // The UAPI may return HTML instead of JSON on missing authentication + if (is_object($result) && $result->status === 1) { return PASSWORD_SUCCESS; } diff --git a/plugins/password/tests/Password.php b/plugins/password/tests/Password.php index 690213d89..d59049fee 100644 --- a/plugins/password/tests/Password.php +++ b/plugins/password/tests/Password.php @@ -20,27 +20,58 @@ class Password_Plugin extends PHPUnit_Framework_TestCase $this->assertInstanceOf('rcube_plugin', $plugin); } + /** + * cpanel_webmail driver test + */ function test_driver_cpanel_webmail() { - $driver = 'cpanel_webmail'; + $driver_class = $this->load_driver('cpanel_webmail'); + + $error_result = $driver_class::decode_response(false); + $this->assertEquals($error_result, PASSWORD_CONNECT_ERROR); + + $bad_result = $driver_class::decode_response(null); + $this->assertEquals($bad_result, PASSWORD_CONNECT_ERROR); + + $null_result = $driver_class::decode_response('null'); + $this->assertEquals($null_result, PASSWORD_ERROR); + + $malformed_result = $driver_class::decode_response('random {string]!'); + $this->assertEquals($malformed_result, PASSWORD_ERROR); + + $other_result = $driver_class::decode_response('{"a":"b"}'); + $this->assertEquals($other_result, PASSWORD_ERROR); + + $fail_response = '{"data":null,"errors":["Execution of Email::passwdp' + . 'op (api version:3) is not permitted inside of webmail"],"sta' + . 'tus":0,"metadata":{},"messages":null}'; + $error_message = 'Execution of Email::passwdpop (api version:3) is no' + . 't permitted inside of webmail'; + $expected_result = array( + 'code' => PASSWORD_ERROR, + 'message' => $error_message + ); + $fail_result = $driver_class::decode_response($fail_response); + $this->assertEquals($expected_result, $fail_result); + + $success_response = '{"metadata":{},"data":null,"messages":null,"errors' + . '":null,"status":1}'; + $good_result = $driver_class::decode_response($success_response); + $this->assertEquals($good_result, PASSWORD_SUCCESS); + } + + /** + * Loads a driver's source file, checks that its class exist and returns the + * driver's class name. + * + * @param string $driver driver name, example: "chpasswd" + * @return string driver's class name, example: "rcube_chpasswd_password" + */ + function load_driver($driver) + { include_once __DIR__ . "/../drivers/$driver.php"; $driver_class = "rcube_${driver}_password"; $this->assertTrue(class_exists($driver_class)); - - $json_response_fail = '{"data":null,"errors":' - . '["Execution of Email::passwdpop (api version:3) is not ' - . 'permitted inside of webmail"],"status":0,"metadata":{},' - . '"messages":null}'; - $result = $driver_class::decode_response($json_response_fail); - $this->assertTrue(is_array($result)); - $this->assertEquals($result['code'], PASSWORD_ERROR); - $expected_message = 'Execution of Email::passwdpop (api version:3) is' - . ' not permitted inside of webmail'; - $this->assertEquals($result['message'], $expected_message); - - $json_response_success = '{"metadata":{},"data":null,"messages":null,' - . '"errors":null,"status":1}'; - $result = $driver_class::decode_response($json_response_success); - $this->assertEquals($result, PASSWORD_SUCCESS); + return $driver_class; } }