Skip to content

Commit

Permalink
Fix #19067: Fix constant session regeneration (#19113)
Browse files Browse the repository at this point in the history 
* Fix constant session regeneration
* Add tests for session ID, always regenerating session ID for switching identity
  • Loading branch information
Bizley committed Dec 29, 2021
1 parent 8c86763 commit 12763a1
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 12 deletions.
11 changes: 4 additions & 7 deletions framework/web/User.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,9 +655,7 @@ public function switchIdentity($identity, $duration = 0)
}

$session = Yii::$app->getSession();
if (!YII_ENV_TEST) {
$session->regenerateID(true);
}
$session->regenerateID(true);

This comment has been minimized.

Sign in to view

Copy link
@SamMousa

SamMousa Jan 4, 2022

Contributor

This breaks DbSession in some cases during tests (not sure about prod)

This comment has been minimized.

Sign in to view

Copy link
@bizley

bizley Jan 4, 2022

Member

Could you please create a new issue for this? I'll prepare special version for the mentioned tests since it's wrong to have it like that for all the tests.
$session->remove($this->idParam);
$session->remove($this->authTimeoutParam);
$session->remove($this->authKeyParam);
Expand Down Expand Up @@ -698,6 +696,9 @@ protected function renewAuthStatus()
/* @var $class IdentityInterface */
$class = $this->identityClass;
$identity = $class::findIdentity($id);
if ($identity === null) {
$this->switchIdentity(null);
}
}

if ($identity !== null) {
Expand Down Expand Up @@ -728,10 +729,6 @@ protected function renewAuthStatus()
$this->renewIdentityCookie();
}
}

if ($this->getIdentity(false) === null) {
$this->switchIdentity(null);
}
}

/**
Expand Down
53 changes: 48 additions & 5 deletions tests/framework/web/UserTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ public function testLoginExpires()
'authManager' => [
'class' => PhpManager::className(),
'itemFile' => '@runtime/user_test_rbac_items.php',
'assignmentFile' => '@runtime/user_test_rbac_assignments.php',
'ruleFile' => '@runtime/user_test_rbac_rules.php',
'assignmentFile' => '@runtime/user_test_rbac_assignments.php',
'ruleFile' => '@runtime/user_test_rbac_rules.php',
],
],
];
Expand Down Expand Up @@ -169,23 +169,29 @@ public function testCookieCleanup()
];

$this->mockWebApplication($appConfig);
$id1 = Yii::$app->session->id;
Yii::$app->session->removeAll();

$cookie = new Cookie(Yii::$app->user->identityCookie);
$cookie->value = 'junk';
$cookiesMock->add($cookie);
Yii::$app->user->getIdentity();
$this->assertEquals(strlen($cookiesMock->getValue(Yii::$app->user->identityCookie['name'])), 0);
$this->assertSame($id1, Yii::$app->session->id);

Yii::$app->user->login(UserIdentity::findIdentity('user1'), 3600);
$this->assertFalse(Yii::$app->user->isGuest);
$this->assertSame(Yii::$app->user->id, 'user1');
$this->assertNotEquals(strlen($cookiesMock->getValue(Yii::$app->user->identityCookie['name'])), 0);
$id2 = Yii::$app->session->id;
$this->assertNotSame($id1, $id2);

Yii::$app->user->login(UserIdentity::findIdentity('user2'), 0);
$this->assertFalse(Yii::$app->user->isGuest);
$this->assertSame(Yii::$app->user->id, 'user2');
$this->assertEquals(strlen($cookiesMock->getValue(Yii::$app->user->identityCookie['name'])), 0);
$id3 = Yii::$app->session->id;
$this->assertNotSame($id2, $id3);
}

/**
Expand Down Expand Up @@ -225,7 +231,7 @@ public function testLoginRequired()
],
];
$this->mockWebApplication($appConfig);

$id = Yii::$app->session->id;

$user = Yii::$app->user;

Expand All @@ -234,6 +240,7 @@ public function testLoginRequired()
$user->loginRequired();
$this->assertEquals('normal', $user->getReturnUrl());
$this->assertTrue(Yii::$app->response->getIsRedirection());
$this->assertSame($id, Yii::$app->session->id);


$this->reset();
Expand All @@ -242,33 +249,37 @@ public function testLoginRequired()

$user->loginRequired();
$this->assertEquals(Yii::$app->getHomeUrl(), $user->getReturnUrl());
// AJAX requests don't update returnUrl but they do cause redirection.
// AJAX requests don't update returnUrl, but they do cause redirection.
$this->assertTrue(Yii::$app->response->getIsRedirection());

$user->loginRequired(false);
$this->assertEquals('ajax', $user->getReturnUrl());
$this->assertTrue(Yii::$app->response->getIsRedirection());
$this->assertSame($id, Yii::$app->session->id);

$this->reset();
Yii::$app->request->setUrl('json-only');
$_SERVER['HTTP_ACCEPT'] = 'Accept: text/json, q=0.1';
$user->loginRequired(true, false);
$this->assertEquals('json-only', $user->getReturnUrl());
$this->assertTrue(Yii::$app->response->getIsRedirection());
$this->assertSame($id, Yii::$app->session->id);

$this->reset();
Yii::$app->request->setUrl('json-only');
$_SERVER['HTTP_ACCEPT'] = 'text/json,q=0.1';
$user->loginRequired(true, false);
$this->assertEquals('json-only', $user->getReturnUrl());
$this->assertTrue(Yii::$app->response->getIsRedirection());
$this->assertSame($id, Yii::$app->session->id);

$this->reset();
Yii::$app->request->setUrl('accept-all');
$_SERVER['HTTP_ACCEPT'] = '*/*;q=0.1';
$user->loginRequired();
$this->assertEquals('accept-all', $user->getReturnUrl());
$this->assertTrue(Yii::$app->response->getIsRedirection());
$this->assertSame($id, Yii::$app->session->id);

$this->reset();
Yii::$app->request->setUrl('json-and-accept-all');
Expand All @@ -278,34 +289,39 @@ public function testLoginRequired()
} catch (ForbiddenHttpException $e) {
}
$this->assertFalse(Yii::$app->response->getIsRedirection());
$this->assertSame($id, Yii::$app->session->id);

$this->reset();
Yii::$app->request->setUrl('accept-html-json');
$_SERVER['HTTP_ACCEPT'] = 'text/json; q=1, text/html; q=0.1';
$user->loginRequired();
$this->assertEquals('accept-html-json', $user->getReturnUrl());
$this->assertTrue(Yii::$app->response->getIsRedirection());
$this->assertSame($id, Yii::$app->session->id);

$this->reset();
Yii::$app->request->setUrl('accept-html-json');
$_SERVER['HTTP_ACCEPT'] = 'text/json;q=1,application/xhtml+xml;q=0.1';
$user->loginRequired();
$this->assertEquals('accept-html-json', $user->getReturnUrl());
$this->assertTrue(Yii::$app->response->getIsRedirection());
$this->assertSame($id, Yii::$app->session->id);

$this->reset();
$_SERVER['REQUEST_METHOD'] = 'POST';
Yii::$app->request->setUrl('dont-set-return-url-on-post-request');
Yii::$app->getSession()->set($user->returnUrlParam, null);
$user->loginRequired();
$this->assertNull(Yii::$app->getSession()->get($user->returnUrlParam));
$this->assertSame($id, Yii::$app->session->id);

$this->reset();
$_SERVER['REQUEST_METHOD'] = 'GET';
Yii::$app->request->setUrl('set-return-url-on-get-request');
Yii::$app->getSession()->set($user->returnUrlParam, null);
$user->loginRequired();
$this->assertEquals('set-return-url-on-get-request', Yii::$app->getSession()->get($user->returnUrlParam));
$this->assertSame($id, Yii::$app->session->id);

// Confirm that returnUrl is not set.
$this->reset();
Expand All @@ -316,11 +332,13 @@ public function testLoginRequired()
} catch (ForbiddenHttpException $e) {
}
$this->assertNotEquals('json-only', $user->getReturnUrl());
$this->assertSame($id, Yii::$app->session->id);

$this->reset();
$_SERVER['HTTP_ACCEPT'] = 'text/json;q=0.1';
$this->expectException('yii\\web\\ForbiddenHttpException');
$user->loginRequired();
$this->assertSame($id, Yii::$app->session->id);
}

public function testLoginRequiredException1()
Expand Down Expand Up @@ -482,8 +500,10 @@ public function testSessionAuthWithMissingKey()
$this->mockWebApplication($appConfig);

Yii::$app->session->set('__id', 'user1');
$id = Yii::$app->session->id;

$this->assertNotNull(Yii::$app->user->getIdentity());
$this->assertSame($id, Yii::$app->session->id);
}

public function testSessionAuthWithInvalidKey()
Expand All @@ -497,12 +517,13 @@ public function testSessionAuthWithInvalidKey()
];

$this->mockWebApplication($appConfig);
$id = Yii::$app->session->id;

Yii::$app->session->set('__id', 'user1');
Yii::$app->session->set('__authKey', 'invalid');


$this->assertNull(Yii::$app->user->getIdentity());
$this->assertSame($id, Yii::$app->session->id);
}

public function testSessionAuthWithValidKey()
Expand All @@ -516,11 +537,33 @@ public function testSessionAuthWithValidKey()
];

$this->mockWebApplication($appConfig);
$id = Yii::$app->session->id;

Yii::$app->session->set('__id', 'user1');
Yii::$app->session->set('__authKey', 'ABCD1234');

$this->assertNotNull(Yii::$app->user->getIdentity());
$this->assertSame($id, Yii::$app->session->id);
}

public function testSessionAuthWhenIdentityReturnsNull()
{
$appConfig = [
'components' => [
'user' => [
'identityClass' => UserIdentity::className(),
],
],
];

$this->mockWebApplication($appConfig);
$id = Yii::$app->session->id;

Yii::$app->session->set('__id', 'user999999');
Yii::$app->session->set('__authKey', 'ABCD1234');

$this->assertNull(Yii::$app->user->getIdentity());
$this->assertNotSame($id, Yii::$app->session->id);
}
}

Expand Down

0 comments on commit 12763a1

Please sign in to comment.