Skip to content

Commit 11eb574

Browse files
committed
Improve handling of logout
- add separate script for handling logout - no longer require old_usr for all authentication methods (this avoids potential information leak) - require valid token for logout Signed-off-by: Michal Čihař <michal@cihar.com>
1 parent 381c80d commit 11eb574

10 files changed

+175
-104
lines changed

Diff for: ChangeLog

+1
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ phpMyAdmin - ChangeLog
1212
- issue #11705 Fix occassional 200 errors on Windows
1313
- issue #12219 Fix locking issues when importing SQL
1414
- issue #12231 Avoid confusing warning when mysql extension is missing
15+
- issue Improve handling of logout
1516

1617
4.6.1 (2016-05-02)
1718
- issue #12120 PMA_Util not found in insert_edit.lib.php

Diff for: libraries/navigation/NavigationHeader.php

+1-2
Original file line numberDiff line numberDiff line change
@@ -167,8 +167,7 @@ private function _links()
167167
if ($GLOBALS['server'] != 0) {
168168
// Logout for advanced authentication
169169
if ($GLOBALS['cfg']['Server']['auth_type'] != 'config') {
170-
$link = 'index.php' . $GLOBALS['url_query'];
171-
$link .= '&amp;old_usr=' . urlencode($GLOBALS['PHP_AUTH_USER']);
170+
$link = 'logout.php' . $GLOBALS['url_query'];
172171
$retval .= PMA\libraries\Util::getNavigationLink(
173172
$link,
174173
$showText,

Diff for: libraries/plugins/AuthenticationPlugin.php

+40
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,46 @@ public function storeUserCredentials()
5252
*/
5353
abstract public function authFails();
5454

55+
/**
56+
* Perform logout
57+
*
58+
* @return void
59+
*/
60+
public function logOut()
61+
{
62+
global $PHP_AUTH_USER, $PHP_AUTH_PW;
63+
64+
/* Obtain redirect URL (before doing logout) */
65+
if (! empty($GLOBALS['cfg']['Server']['LogoutURL'])) {
66+
$redirect_url = $GLOBALS['cfg']['Server']['LogoutURL'];
67+
} else {
68+
$redirect_url = $this->getLoginFormURL();
69+
}
70+
71+
/* Clear credentials */
72+
$PHP_AUTH_USER = '';
73+
$PHP_AUTH_PW = '';
74+
75+
/* delete user's choices that were stored in session */
76+
$_SESSION = array();
77+
if (!defined('TESTSUITE')) {
78+
session_destroy();
79+
}
80+
81+
/* Redirect to login form (or configured URL) */
82+
PMA_sendHeaderLocation($redirect_url);
83+
}
84+
85+
/**
86+
* Returns URL for login form.
87+
*
88+
* @return string
89+
*/
90+
public function getLoginFormURL()
91+
{
92+
return './index.php';
93+
}
94+
5595
/**
5696
* Returns error message for failed authentication.
5797
*

Diff for: libraries/plugins/auth/AuthenticationCookie.php

+26-40
Original file line numberDiff line numberDiff line change
@@ -67,18 +67,6 @@ public function auth()
6767
}
6868
}
6969

70-
/* Perform logout to custom URL */
71-
if (! empty($_REQUEST['old_usr'])
72-
&& ! empty($GLOBALS['cfg']['Server']['LogoutURL'])
73-
) {
74-
PMA_sendHeaderLocation($GLOBALS['cfg']['Server']['LogoutURL']);
75-
if (defined('TESTSUITE')) {
76-
return true;
77-
} else {
78-
exit;
79-
}
80-
}
81-
8270
// No recall if blowfish secret is not configured as it would produce
8371
// garbage
8472
if ($GLOBALS['cfg']['LoginCookieRecall']
@@ -295,34 +283,6 @@ public function authCheck()
295283
}
296284
// END Swekey Integration
297285

298-
if (! empty($_REQUEST['old_usr'])) {
299-
// The user wants to be logged out
300-
// -> delete his choices that were stored in session
301-
302-
// according to the PHP manual we should do this before the destroy:
303-
//$_SESSION = array();
304-
305-
if (! defined('TESTSUITE')) {
306-
session_destroy();
307-
}
308-
// -> delete password cookie(s)
309-
if ($GLOBALS['cfg']['LoginCookieDeleteAll']) {
310-
foreach ($GLOBALS['cfg']['Servers'] as $key => $val) {
311-
$GLOBALS['PMA_Config']->removeCookie('pmaPass-' . $key);
312-
if (isset($_COOKIE['pmaPass-' . $key])) {
313-
unset($_COOKIE['pmaPass-' . $key]);
314-
}
315-
}
316-
} else {
317-
$GLOBALS['PMA_Config']->removeCookie(
318-
'pmaPass-' . $GLOBALS['server']
319-
);
320-
if (isset($_COOKIE['pmaPass-' . $GLOBALS['server']])) {
321-
unset($_COOKIE['pmaPass-' . $GLOBALS['server']]);
322-
}
323-
}
324-
}
325-
326286
if (! empty($_REQUEST['pma_username'])) {
327287

328288
// Verify Captcha if it is required.
@@ -831,4 +791,30 @@ public function handlePasswordChange($password)
831791
{
832792
$this->storePasswordCookie($password);
833793
}
794+
795+
/**
796+
* Perform logout
797+
*
798+
* @return void
799+
*/
800+
public function logOut()
801+
{
802+
// -> delete password cookie(s)
803+
if ($GLOBALS['cfg']['LoginCookieDeleteAll']) {
804+
foreach ($GLOBALS['cfg']['Servers'] as $key => $val) {
805+
$GLOBALS['PMA_Config']->removeCookie('pmaPass-' . $key);
806+
if (isset($_COOKIE['pmaPass-' . $key])) {
807+
unset($_COOKIE['pmaPass-' . $key]);
808+
}
809+
}
810+
} else {
811+
$GLOBALS['PMA_Config']->removeCookie(
812+
'pmaPass-' . $GLOBALS['server']
813+
);
814+
if (isset($_COOKIE['pmaPass-' . $GLOBALS['server']])) {
815+
unset($_COOKIE['pmaPass-' . $GLOBALS['server']]);
816+
}
817+
}
818+
parent::logOut();
819+
}
834820
}

Diff for: libraries/plugins/auth/AuthenticationHttp.php

+10-12
Original file line numberDiff line numberDiff line change
@@ -49,18 +49,6 @@ public function auth()
4949
*/
5050
public function authForm()
5151
{
52-
/* Perform logout to custom URL */
53-
if (!empty($_REQUEST['old_usr'])
54-
&& !empty($GLOBALS['cfg']['Server']['LogoutURL'])
55-
) {
56-
PMA_sendHeaderLocation($GLOBALS['cfg']['Server']['LogoutURL']);
57-
if (!defined('TESTSUITE')) {
58-
exit;
59-
} else {
60-
return false;
61-
}
62-
}
63-
6452
if (empty($GLOBALS['cfg']['Server']['auth_http_realm'])) {
6553
if (empty($GLOBALS['cfg']['Server']['verbose'])) {
6654
$server_message = $GLOBALS['cfg']['Server']['host'];
@@ -262,4 +250,14 @@ public function authFails()
262250

263251
return true;
264252
}
253+
254+
/**
255+
* Returns URL for login form.
256+
*
257+
* @return string
258+
*/
259+
public function getLoginFormURL()
260+
{
261+
return './index.php?old_usr=' . $GLOBALS['PHP_AUTH_USER'];
262+
}
265263
}

Diff for: libraries/plugins/auth/AuthenticationSignon.php

+2-18
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,6 @@ public function auth()
2828
unset($_SESSION['LAST_SIGNON_URL']);
2929
if (empty($GLOBALS['cfg']['Server']['SignonURL'])) {
3030
PMA_fatalError('You must set SignonURL!');
31-
} elseif (!empty($_REQUEST['old_usr'])
32-
&& !empty($GLOBALS['cfg']['Server']['LogoutURL'])
33-
) {
34-
/* Perform logout to custom URL */
35-
PMA_sendHeaderLocation($GLOBALS['cfg']['Server']['LogoutURL']);
3631
} else {
3732
PMA_sendHeaderLocation($GLOBALS['cfg']['Server']['SignonURL']);
3833
}
@@ -82,9 +77,6 @@ public function authCheck()
8277
/* No configuration updates */
8378
$single_signon_cfgupdate = array();
8479

85-
/* Are we requested to do logout? */
86-
$do_logout = !empty($_REQUEST['old_usr']);
87-
8880
/* Handle script based auth */
8981
if (!empty($script_name)) {
9082
if (!file_exists($script_name)) {
@@ -117,18 +109,10 @@ public function authCheck()
117109

118110
/* Grab credentials if they exist */
119111
if (isset($_SESSION['PMA_single_signon_user'])) {
120-
if ($do_logout) {
121-
$PHP_AUTH_USER = '';
122-
} else {
123-
$PHP_AUTH_USER = $_SESSION['PMA_single_signon_user'];
124-
}
112+
$PHP_AUTH_USER = $_SESSION['PMA_single_signon_user'];
125113
}
126114
if (isset($_SESSION['PMA_single_signon_password'])) {
127-
if ($do_logout) {
128-
$PHP_AUTH_PW = '';
129-
} else {
130-
$PHP_AUTH_PW = $_SESSION['PMA_single_signon_password'];
131-
}
115+
$PHP_AUTH_PW = $_SESSION['PMA_single_signon_password'];
132116
}
133117
if (isset($_SESSION['PMA_single_signon_host'])) {
134118
$single_signon_host = $_SESSION['PMA_single_signon_host'];

Diff for: logout.php

+15
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?php
2+
/* vim: set expandtab sw=4 ts=4 sts=4: */
3+
/**
4+
* Logout script
5+
*
6+
* @package PhpMyAdmin
7+
*/
8+
require_once 'libraries/common.inc.php';
9+
10+
if ($token_mismatch) {
11+
PMA_sendHeaderLocation('./index.php');
12+
} else {
13+
$auth_plugin->logOut();
14+
}
15+

Diff for: test/classes/plugin/auth/AuthenticationCookieTest.php

+47-13
Original file line numberDiff line numberDiff line change
@@ -373,18 +373,16 @@ public function testAuthCaptcha()
373373
*/
374374
public function testAuthHeader()
375375
{
376+
$GLOBALS['cfg']['LoginCookieDeleteAll'] = false;
377+
$GLOBALS['cfg']['Servers'] = array(1);
378+
376379
$restoreInstance = PMA\libraries\Response::getInstance();
377380

378381
$mockResponse = $this->getMockBuilder('PMA\libraries\Response')
379382
->disableOriginalConstructor()
380383
->setMethods(array('isAjax', 'headersSent', 'header'))
381384
->getMock();
382385

383-
$mockResponse->expects($this->once())
384-
->method('isAjax')
385-
->with()
386-
->will($this->returnValue(false));
387-
388386
$mockResponse->expects($this->any())
389387
->method('headersSent')
390388
->with()
@@ -398,12 +396,9 @@ public function testAuthHeader()
398396
$attrInstance->setAccessible(true);
399397
$attrInstance->setValue($mockResponse);
400398

401-
$_REQUEST['old_usr'] = 'user1';
402399
$GLOBALS['cfg']['Server']['LogoutURL'] = 'http://www.phpmyadmin.net/logout';
403400

404-
$this->assertTrue(
405-
$this->object->auth()
406-
);
401+
$this->object->logOut();
407402

408403
$attrInstance->setValue($restoreInstance);
409404
}
@@ -454,20 +449,40 @@ public function testAuthCheckCaptcha()
454449
*/
455450
public function testLogoutDelete()
456451
{
452+
$restoreInstance = PMA\libraries\Response::getInstance();
453+
454+
$mockResponse = $this->getMockBuilder('PMA\libraries\Response')
455+
->disableOriginalConstructor()
456+
->setMethods(array('isAjax', 'headersSent', 'header'))
457+
->getMock();
458+
459+
$mockResponse->expects($this->any())
460+
->method('headersSent')
461+
->with()
462+
->will($this->returnValue(false));
463+
464+
$mockResponse->expects($this->once())
465+
->method('header')
466+
->with('Location: ./index.php' . ((SID) ? '?' . SID : ''));
467+
468+
$attrInstance = new ReflectionProperty('PMA\libraries\Response', '_instance');
469+
$attrInstance->setAccessible(true);
470+
$attrInstance->setValue($mockResponse);
471+
457472
$GLOBALS['cfg']['Server']['auth_swekey_config'] = '';
458473
$GLOBALS['cfg']['CaptchaLoginPrivateKey'] = '';
459474
$GLOBALS['cfg']['CaptchaLoginPublicKey'] = '';
460-
$_REQUEST['old_usr'] = 'pmaolduser';
461475
$GLOBALS['cfg']['LoginCookieDeleteAll'] = true;
462476
$GLOBALS['cfg']['Servers'] = array(1);
463477

464478
$_COOKIE['pmaPass-0'] = 'test';
465479

466-
$this->object->authCheck();
480+
$this->object->logOut();
467481

468482
$this->assertFalse(
469483
isset($_COOKIE['pmaPass-0'])
470484
);
485+
$attrInstance->setValue($restoreInstance);
471486
}
472487

473488
/**
@@ -477,21 +492,40 @@ public function testLogoutDelete()
477492
*/
478493
public function testLogout()
479494
{
495+
$restoreInstance = PMA\libraries\Response::getInstance();
496+
497+
$mockResponse = $this->getMockBuilder('PMA\libraries\Response')
498+
->disableOriginalConstructor()
499+
->setMethods(array('isAjax', 'headersSent', 'header'))
500+
->getMock();
501+
502+
$mockResponse->expects($this->any())
503+
->method('headersSent')
504+
->with()
505+
->will($this->returnValue(false));
506+
507+
$mockResponse->expects($this->once())
508+
->method('header')
509+
->with('Location: ./index.php' . ((SID) ? '?' . SID : ''));
510+
511+
$attrInstance = new ReflectionProperty('PMA\libraries\Response', '_instance');
512+
$attrInstance->setAccessible(true);
513+
$attrInstance->setValue($mockResponse);
480514
$GLOBALS['cfg']['Server']['auth_swekey_config'] = '';
481515
$GLOBALS['cfg']['CaptchaLoginPrivateKey'] = '';
482516
$GLOBALS['cfg']['CaptchaLoginPublicKey'] = '';
483-
$_REQUEST['old_usr'] = 'pmaolduser';
484517
$GLOBALS['cfg']['LoginCookieDeleteAll'] = false;
485518
$GLOBALS['cfg']['Servers'] = array(1);
486519
$GLOBALS['server'] = 1;
487520

488521
$_COOKIE['pmaPass-1'] = 'test';
489522

490-
$this->object->authCheck();
523+
$this->object->logOut();
491524

492525
$this->assertFalse(
493526
isset($_COOKIE['pmaPass-1'])
494527
);
528+
$attrInstance->setValue($restoreInstance);
495529
}
496530

497531
/**

Diff for: test/classes/plugin/auth/AuthenticationHttpTest.php

+7-3
Original file line numberDiff line numberDiff line change
@@ -123,9 +123,13 @@ public function doMockResponse($set_minimal, $body_id, $set_title)
123123
call_user_func_array(array($header_method, 'withConsecutive'), $headers);
124124

125125
try {
126-
$this->assertFalse(
127-
$this->object->auth()
128-
);
126+
if (!empty($_REQUEST['old_usr'])) {
127+
$this->object->logOut();
128+
} else {
129+
$this->assertFalse(
130+
$this->object->auth()
131+
);
132+
}
129133
} finally {
130134
$attrInstance->setValue($restoreInstance);
131135
}

0 commit comments

Comments
 (0)