From ac8b40b8b12d4bf85bdd6b7ab038f0605c8651d9 Mon Sep 17 00:00:00 2001 From: Lionel Elie Mamane Date: Sat, 20 Jun 2020 11:21:41 +0200 Subject: [PATCH 1/2] Return correct loginname in credentials, even when token is invalid or has no password. Returning the uid as loginname is wrong, and leads to problems when these differ. E.g. the getapppassword API was creating app token with the uid as loginname. In a scenario with external authentication (such as LDAP), these tokens were then invalidated next time their underlying password was checked, and systematically ceased to function. Co-authored-by: kesselb for: switch to consistent camelCase Signed-off-by: Lionel Elie Mamane --- .../Authentication/LoginCredentials/Store.php | 2 +- lib/private/Server.php | 4 ++-- lib/private/User/Session.php | 4 +++- lib/public/User/Events/PostLoginEvent.php | 16 +++++++++++++++- 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/lib/private/Authentication/LoginCredentials/Store.php b/lib/private/Authentication/LoginCredentials/Store.php index f4bedd88a18ac..6dd7dc3fb738e 100644 --- a/lib/private/Authentication/LoginCredentials/Store.php +++ b/lib/private/Authentication/LoginCredentials/Store.php @@ -112,7 +112,7 @@ public function getLoginCredentials(): ICredentials { if ($trySession && $this->session->exists('login_credentials')) { $creds = json_decode($this->session->get('login_credentials')); - return new Credentials($creds->uid, $creds->uid, $creds->password); + return new Credentials($creds->uid, $creds->loginName, $creds->password); } // If we reach this line, an exception was thrown. diff --git a/lib/private/Server.php b/lib/private/Server.php index 9b452f21ce178..ff3214fe03011 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -566,9 +566,9 @@ public function __construct($webRoot, \OC\Config $config) { $dispatcher = $this->query(IEventDispatcher::class); $dispatcher->dispatchTyped(new BeforeUserLoggedInEvent($uid, $password)); }); - $userSession->listen('\OC\User', 'postLogin', function ($user, $password, $isTokenLogin) { + $userSession->listen('\OC\User', 'postLogin', function ($user, $loginName, $password, $isTokenLogin) { /** @var \OC\User\User $user */ - \OC_Hook::emit('OC_User', 'post_login', ['run' => true, 'uid' => $user->getUID(), 'password' => $password, 'isTokenLogin' => $isTokenLogin]); + \OC_Hook::emit('OC_User', 'post_login', ['run' => true, 'uid' => $user->getUID(), 'loginName' => $loginName, 'password' => $password, 'isTokenLogin' => $isTokenLogin]); /** @var IEventDispatcher $dispatcher */ $dispatcher = $this->query(IEventDispatcher::class); diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 3996869c69291..176e384bcb632 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -80,7 +80,7 @@ * - preUnassignedUserId(string $uid) * - postUnassignedUserId(string $uid) * - preLogin(string $user, string $password) - * - postLogin(\OC\User\User $user, string $password) + * - postLogin(\OC\User\User $user, string $loginName, string $password, boolean $isTokenLogin) * - preRememberedLogin(string $uid) * - postRememberedLogin(\OC\User\User $user) * - logout() @@ -400,11 +400,13 @@ public function completeLogin(IUser $user, array $loginDetails, $regenerateSessi $this->dispatcher->dispatchTyped(new PostLoginEvent( $user, + $loginDetails['loginName'], $loginDetails['password'], $isToken )); $this->manager->emit('\OC\User', 'postLogin', [ $user, + $loginDetails['loginName'], $loginDetails['password'], $isToken, ]); diff --git a/lib/public/User/Events/PostLoginEvent.php b/lib/public/User/Events/PostLoginEvent.php index 15772bfef1794..76d9a70aac820 100644 --- a/lib/public/User/Events/PostLoginEvent.php +++ b/lib/public/User/Events/PostLoginEvent.php @@ -38,6 +38,12 @@ class PostLoginEvent extends Event { /** @var IUser */ private $user; + /** + * @since 20.0.0 + * @var string + */ + private $loginName; + /** @var string */ private $password; @@ -47,9 +53,10 @@ class PostLoginEvent extends Event { /** * @since 18.0.0 */ - public function __construct(IUser $user, string $password, bool $isTokenLogin) { + public function __construct(IUser $user, string $loginName, string $password, bool $isTokenLogin) { parent::__construct(); $this->user = $user; + $this->loginName = $loginName; $this->password = $password; $this->isTokenLogin = $isTokenLogin; } @@ -61,6 +68,13 @@ public function getUser(): IUser { return $this->user; } + /** + * @since 20.0.0 + */ + public function getLoginName(): string { + return $this->loginName; + } + /** * @since 18.0.0 */ From 2c8e7912f30df1d3f2d1be97eba52ebf3761cf26 Mon Sep 17 00:00:00 2001 From: Lionel Elie Mamane Date: Sun, 7 Jun 2020 15:41:09 +0200 Subject: [PATCH 2/2] adapt testGetLoginCredentialsInvalidTokenLoginCredentials() unit test to uid != loginname Signed-off-by: Lionel Elie Mamane --- tests/lib/Authentication/LoginCredentials/StoreTest.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/lib/Authentication/LoginCredentials/StoreTest.php b/tests/lib/Authentication/LoginCredentials/StoreTest.php index ea490b76b8a0e..67cb0a18297f9 100644 --- a/tests/lib/Authentication/LoginCredentials/StoreTest.php +++ b/tests/lib/Authentication/LoginCredentials/StoreTest.php @@ -141,7 +141,8 @@ public function testGetLoginCredentialsInvalidToken() { } public function testGetLoginCredentialsInvalidTokenLoginCredentials() { - $uid = 'user987'; + $uid = 'id987'; + $user = 'user987'; $password = '7389374'; $this->session->expects($this->once()) @@ -158,8 +159,8 @@ public function testGetLoginCredentialsInvalidTokenLoginCredentials() { $this->session->expects($this->once()) ->method('get') ->with($this->equalTo('login_credentials')) - ->willReturn('{"run":true,"uid":"user987","password":"7389374"}'); - $expected = new Credentials('user987', 'user987', '7389374'); + ->willReturn('{"run":true,"uid":"id987","loginName":"user987","password":"7389374"}'); + $expected = new Credentials($uid, $user, $password); $actual = $this->store->getLoginCredentials();