Skip to content

Commit

Permalink
When verifying the request authentication mechanisms we need to loop …
Browse files Browse the repository at this point in the history 
…over all modules and allow each of them

to verify the headers and in case something is terribly wrong the user will be logged out and the session will be destroyed.

The BasicAuthModule now handles app passwords as well - this was missing before and caused users being locked out of ldap.

Solves OAuth token expiry issue - fixes owncloud/oauth2#103
  • Loading branch information
@DeepDiver1975
DeepDiver1975 committed Feb 12, 2018
1 parent 1c8baeb commit f7194a2
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 42 deletions.
9 changes: 6 additions & 3 deletions lib/private/User/BasicAuthModule.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,13 @@ public function auth(IRequest $request) {
}
// check email and password
$users = $this->manager->getByEmail($request->server['PHP_AUTH_USER']);
if (count($users) !== 1) {
return null;
if (count($users) === 1) {
$user = $this->manager->checkPassword($users[0]->getUID(), $request->server['PHP_AUTH_PW']);
}
if ($user instanceof IUser) {
return $user;
}
return $this->manager->checkPassword($users[0]->getUID(), $request->server['PHP_AUTH_PW']);
throw new \Exception('Invalid credentials');
}

/**
Expand Down
60 changes: 35 additions & 25 deletions lib/private/User/Session.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,15 +283,15 @@ public function setLoginName($loginName) {
public function getLoginName() {
if ($this->activeUser) {
return $this->session->get('loginname');
} else {
$uid = $this->session->get('user_id');
if ($uid) {
$this->activeUser = $this->manager->get($uid);
return $this->session->get('loginname');
} else {
return null;
}
}

$uid = $this->session->get('user_id');
if ($uid) {
$this->activeUser = $this->manager->get($uid);
return $this->session->get('loginname');
}

return null;
}

/**
Expand Down Expand Up @@ -320,6 +320,7 @@ public function login($uid, $password) {
* @param string $user
* @param string $password
* @param IRequest $request
* @throws \InvalidArgumentException
* @throws LoginException
* @throws PasswordLoginForbiddenException
* @return boolean
Expand Down Expand Up @@ -988,31 +989,44 @@ public function updateSessionTokenPassword($password) {
}

public function verifyAuthHeaders($request) {
foreach ($this->getAuthModules(true) as $module) {
$user = $module->auth($request);
if ($user !== null) {
if ($this->isLoggedIn() && $this->getUser()->getUID() !== $user->getUID()) {
// the session is bad -> kill it
$this->logout();
return false;
$shallLogout = false;
try {
$lastUser = null;
foreach ($this->getAuthModules(true) as $module) {
$user = $module->auth($request);
if ($user !== null) {
if ($this->isLoggedIn() && $this->getUser()->getUID() !== $user->getUID()) {
$shallLogout = true;
break;
}
if ($lastUser !== null && $user->getUID() !== $lastUser->getUID()) {
$shallLogout = true;
break;
}
$lastUser = $user;
}
return true;
}
}

// the session is bad -> kill it
$this->logout();
return false;
} catch (Exception $ex) {
$shallLogout = true;
}
if ($shallLogout) {
// the session is bad -> kill it
$this->logout();
return false;
}
return true;
}

/**
* @param $includeBuiltIn
* @return \Generator | IAuthModule[]
* @throws Exception
*/
private function getAuthModules($includeBuiltIn) {
protected function getAuthModules($includeBuiltIn) {
if ($includeBuiltIn) {
yield new TokenAuthModule($this->session, $this->tokenProvider, $this->manager);
yield new BasicAuthModule($this->manager, $this);
}

$modules = $this->serviceLoader->load(['auth-modules']);
Expand All @@ -1023,9 +1037,5 @@ private function getAuthModules($includeBuiltIn) {
continue;
}
}

if ($includeBuiltIn) {
yield new BasicAuthModule($this->manager);
}
}
}
63 changes: 49 additions & 14 deletions lib/private/User/TokenAuthModule.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@
use OC\Authentication\Exceptions\InvalidTokenException;
use OC\Authentication\Exceptions\PasswordlessTokenException;
use OC\Authentication\Token\IProvider;
use OC\Authentication\Token\IToken;
use OCP\Authentication\IAuthModule;
use OCP\IRequest;
use OCP\ISession;
use OCP\IUser;
use OCP\IUserManager;
use OCP\Session\Exceptions\SessionNotAvailableException;

Expand Down Expand Up @@ -56,23 +58,14 @@ public function __construct(ISession $session, IProvider $tokenProvider, IUserMa
* @inheritdoc
*/
public function auth(IRequest $request) {
$authHeader = $request->getHeader('Authorization');
if ($authHeader === null || strpos($authHeader, 'token ') === false) {
// No auth header, let's try session id
try {
$token = $this->session->getId();
} catch (SessionNotAvailableException $ex) {
return null;
}
} else {
$token = substr($authHeader, 6);
$dbToken = $this->getTokenForAppPassword($request, $token);
if ($dbToken === null) {
$dbToken = $this->getToken($request, $token);
}

try {
$dbToken = $this->tokenProvider->getToken($token);
} catch (InvalidTokenException $ex) {
if ($dbToken === null) {
return null;
}

$uid = $dbToken->getUID();

// When logging in with token, the password must be decrypted first before passing to login hook
Expand All @@ -91,4 +84,46 @@ public function auth(IRequest $request) {
public function getUserPassword(IRequest $request) {
return $this->password;
}

/**
* @param IRequest $request
* @return null|IToken
*/
private function getTokenForAppPassword(IRequest $request, &$token) {
if (empty($request->server['PHP_AUTH_USER']) || empty($request->server['PHP_AUTH_PW'])) {
return null;
}
try {
$token = $request->server['PHP_AUTH_PW'];
return $this->tokenProvider->getToken($token);
} catch (InvalidTokenException $ex) {
$token = null;
return null;
}
}

/**
* @param IRequest $request
* @return null|IToken
*/
private function getToken(IRequest $request, &$token) {
$authHeader = $request->getHeader('Authorization');
if ($authHeader === null || strpos($authHeader, 'token ') === false) {
// No auth header, let's try session id
try {
$token = $this->session->getId();
} catch (SessionNotAvailableException $ex) {
return null;
}
} else {
$token = substr($authHeader, 6);
}

try {
return $this->tokenProvider->getToken($token);
} catch (InvalidTokenException $ex) {
$token = null;
return null;
}
}
}
4 changes: 4 additions & 0 deletions tests/lib/User/BasicAuthModuleTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ public function testAuth($expectsUser, $userId) {
'PHP_AUTH_USER' => $userId,
'PHP_AUTH_PW' => '123456',
];
if (!$expectsUser) {
$this->expectException(\Exception::class);
$this->expectExceptionMessage('Invalid credentials');
}
$this->assertEquals($expectsUser ? $this->user : null, $module->auth($this->request));
}

Expand Down
54 changes: 54 additions & 0 deletions tests/lib/User/SessionTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use OC\User\Session;
use OCP\App\IServiceLoader;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\Authentication\IAuthModule;
use OCP\IConfig;
use OCP\IRequest;
use OCP\ISession;
Expand Down Expand Up @@ -1051,5 +1052,58 @@ public function testApacheLogin() {
$this->assertFalse($loginVal);
}

public function providesModules() {
$nullModule = $this->createMock(IAuthModule::class);
$nullModule->expects($this->any())->method('auth')->willReturn(null);
$throwingModule = $this->createMock(IAuthModule::class);
$throwingModule->expects($this->any())->method('auth')->willThrowException(new \Exception('Invalid token'));
$user1 = $this->createMock(IUser::class);
$user1->expects($this->any())->method('getUID')->willReturn('user1');
$user1Module = $this->createMock(IAuthModule::class);
$user1Module->expects($this->any())->method('auth')->willReturn($user1);
$user2 = $this->createMock(IUser::class);
$user2->expects($this->any())->method('getUID')->willReturn('user2');
$user2Module = $this->createMock(IAuthModule::class);
$user2Module->expects($this->any())->method('auth')->willReturn($user2);
return [
'no modules' => [true, []],
'module returning null' => [true, [$nullModule]],
'module returning a user' => [true, [$user1Module]],
'module throwing an exception' => [false, [$throwingModule]],
'modules returning different user' => [false, [$user1Module, $user2Module]],
'logged in user does not match' => [false, [$user2Module], $user1],
];
}

/**
* @dataProvider providesModules
* @param $expectedReturn
* @param array $modules
*/
public function testVerifyAuthHeaders($expectedReturn, array $modules, $loggedInUser = null) {
/** @var IRequest | \PHPUnit_Framework_MockObject_MockObject $request */
$request = $this->createMock(IRequest::class);
/** @var IUserManager | \PHPUnit_Framework_MockObject_MockObject $userManager */
$userManager = $this->createMock(IUserManager::class);
/** @var ISession | \PHPUnit_Framework_MockObject_MockObject $session */
$session = $this->createMock(ISession::class);
/** @var ITimeFactory | \PHPUnit_Framework_MockObject_MockObject $timeFactory */
$timeFactory = $this->createMock(ITimeFactory::class);
/** @var IProvider | \PHPUnit_Framework_MockObject_MockObject $tokenProvider */
$tokenProvider = $this->createMock(IProvider::class);

/** @var Session | \PHPUnit_Framework_MockObject_MockObject $session */
$session = $this->getMockBuilder(Session::class)
->setConstructorArgs([$userManager, $session, $timeFactory, $tokenProvider, $this->config, $this->serviceLoader, $this->userSyncService])
->setMethods(['getAuthModules', 'logout', 'isLoggedIn', 'getUser'])
->getMock();
$session->expects($this->any())->method('getAuthModules')->willReturn($modules);

$session->expects($this->any())->method('isLoggedIn')->willReturn($loggedInUser !== null);
$session->expects($this->any())->method('getUser')->willReturn($loggedInUser);

$session->expects($expectedReturn ? $this->never() : $this->once())->method('logout');
$this->assertEquals( $expectedReturn, $session->verifyAuthHeaders($request));
}

}

0 comments on commit f7194a2

Please sign in to comment.