Skip to content

Commit

Permalink
Merge pull request #40766 from nextcloud/enh/noid/oauth2-code-expiration
Browse files Browse the repository at this point in the history
Make OAuth2 authorization code expire
  • Loading branch information
julien-nc authored Oct 5, 2023
2 parents f3f2d9b + d56950a commit b4fec29
Show file tree
Hide file tree
Showing 13 changed files with 362 additions and 30 deletions.
6 changes: 5 additions & 1 deletion apps/oauth2/appinfo/info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<name>OAuth 2.0</name>
<summary>Allows OAuth2 compatible authentication from other web applications.</summary>
<description>The OAuth2 app allows administrators to configure the built-in authentication workflow to also allow OAuth2 compatible authentication from other web applications.</description>
<version>1.16.2</version>
<version>1.16.3</version>
<licence>agpl</licence>
<author>Lukas Reschke</author>
<namespace>OAuth2</namespace>
Expand All @@ -19,6 +19,10 @@
<nextcloud min-version="28" max-version="28"/>
</dependencies>

<background-jobs>
<job>OCA\OAuth2\BackgroundJob\CleanupExpiredAuthorizationCode</job>
</background-jobs>

<repair-steps>
<post-migration>
<step>OCA\OAuth2\Migration\SetTokenExpiration</step>
Expand Down
2 changes: 2 additions & 0 deletions apps/oauth2/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

return array(
'Composer\\InstalledVersions' => $vendorDir . '/composer/InstalledVersions.php',
'OCA\\OAuth2\\BackgroundJob\\CleanupExpiredAuthorizationCode' => $baseDir . '/../lib/BackgroundJob/CleanupExpiredAuthorizationCode.php',
'OCA\\OAuth2\\Controller\\LoginRedirectorController' => $baseDir . '/../lib/Controller/LoginRedirectorController.php',
'OCA\\OAuth2\\Controller\\OauthApiController' => $baseDir . '/../lib/Controller/OauthApiController.php',
'OCA\\OAuth2\\Controller\\SettingsController' => $baseDir . '/../lib/Controller/SettingsController.php',
Expand All @@ -21,5 +22,6 @@
'OCA\\OAuth2\\Migration\\Version010402Date20190107124745' => $baseDir . '/../lib/Migration/Version010402Date20190107124745.php',
'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => $baseDir . '/../lib/Migration/Version011601Date20230522143227.php',
'OCA\\OAuth2\\Migration\\Version011602Date20230613160650' => $baseDir . '/../lib/Migration/Version011602Date20230613160650.php',
'OCA\\OAuth2\\Migration\\Version011603Date20230620111039' => $baseDir . '/../lib/Migration/Version011603Date20230620111039.php',
'OCA\\OAuth2\\Settings\\Admin' => $baseDir . '/../lib/Settings/Admin.php',
);
2 changes: 2 additions & 0 deletions apps/oauth2/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class ComposerStaticInitOAuth2

public static $classMap = array (
'Composer\\InstalledVersions' => __DIR__ . '/..' . '/composer/InstalledVersions.php',
'OCA\\OAuth2\\BackgroundJob\\CleanupExpiredAuthorizationCode' => __DIR__ . '/..' . '/../lib/BackgroundJob/CleanupExpiredAuthorizationCode.php',
'OCA\\OAuth2\\Controller\\LoginRedirectorController' => __DIR__ . '/..' . '/../lib/Controller/LoginRedirectorController.php',
'OCA\\OAuth2\\Controller\\OauthApiController' => __DIR__ . '/..' . '/../lib/Controller/OauthApiController.php',
'OCA\\OAuth2\\Controller\\SettingsController' => __DIR__ . '/..' . '/../lib/Controller/SettingsController.php',
Expand All @@ -36,6 +37,7 @@ class ComposerStaticInitOAuth2
'OCA\\OAuth2\\Migration\\Version010402Date20190107124745' => __DIR__ . '/..' . '/../lib/Migration/Version010402Date20190107124745.php',
'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => __DIR__ . '/..' . '/../lib/Migration/Version011601Date20230522143227.php',
'OCA\\OAuth2\\Migration\\Version011602Date20230613160650' => __DIR__ . '/..' . '/../lib/Migration/Version011602Date20230613160650.php',
'OCA\\OAuth2\\Migration\\Version011603Date20230620111039' => __DIR__ . '/..' . '/../lib/Migration/Version011603Date20230620111039.php',
'OCA\\OAuth2\\Settings\\Admin' => __DIR__ . '/..' . '/../lib/Settings/Admin.php',
);

Expand Down
61 changes: 61 additions & 0 deletions apps/oauth2/lib/BackgroundJob/CleanupExpiredAuthorizationCode.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2023 Julien Veyssier <julien-nc@posteo.net>
*
* @author Julien Veyssier <julien-nc@posteo.net>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/


namespace OCA\OAuth2\BackgroundJob;

use OCA\OAuth2\Db\AccessTokenMapper;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\BackgroundJob\IJob;
use OCP\BackgroundJob\TimedJob;
use OCP\DB\Exception;
use Psr\Log\LoggerInterface;

class CleanupExpiredAuthorizationCode extends TimedJob {

public function __construct(
ITimeFactory $timeFactory,
private AccessTokenMapper $accessTokenMapper,
private LoggerInterface $logger,

) {
parent::__construct($timeFactory);
// 30 days
$this->setInterval(60 * 60 * 24 * 30);
$this->setTimeSensitivity(IJob::TIME_INSENSITIVE);
}

/**
* @param mixed $argument
* @inheritDoc
*/
protected function run($argument): void {
try {
$this->accessTokenMapper->cleanupExpiredAuthorizationCode();
} catch (Exception $e) {
$this->logger->warning('Failed to cleanup tokens with expired authorization code', ['exception' => $e]);
}
}
}
52 changes: 46 additions & 6 deletions apps/oauth2/lib/Controller/OauthApiController.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,16 @@
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\JSONResponse;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\DB\Exception;
use OCP\IRequest;
use OCP\Security\Bruteforce\IThrottler;
use OCP\Security\ICrypto;
use OCP\Security\ISecureRandom;
use Psr\Log\LoggerInterface;

class OauthApiController extends Controller {
// the authorization code expires after 10 minutes
public const AUTHORIZATION_CODE_EXPIRES_AFTER = 10 * 60;

public function __construct(
string $appName,
Expand All @@ -57,7 +60,8 @@ public function __construct(
private ISecureRandom $secureRandom,
private ITimeFactory $time,
private LoggerInterface $logger,
private IThrottler $throttler
private IThrottler $throttler,
private ITimeFactory $timeFactory,
) {
parent::__construct($appName, $request);
}
Expand All @@ -70,16 +74,20 @@ public function __construct(
* Get a token
*
* @param string $grant_type Token type that should be granted
* @param string $code Code of the flow
* @param string $refresh_token Refresh token
* @param string $client_id Client ID
* @param string $client_secret Client secret
* @param ?string $code Code of the flow
* @param ?string $refresh_token Refresh token
* @param ?string $client_id Client ID
* @param ?string $client_secret Client secret
* @throws Exception
* @return JSONResponse<Http::STATUS_OK, array{access_token: string, token_type: string, expires_in: int, refresh_token: string, user_id: string}, array{}>|JSONResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
*
* 200: Token returned
* 400: Getting token is not possible
*/
public function getToken($grant_type, $code, $refresh_token, $client_id, $client_secret): JSONResponse {
public function getToken(
string $grant_type, ?string $code, ?string $refresh_token,
?string $client_id, ?string $client_secret
): JSONResponse {

// We only handle two types
if ($grant_type !== 'authorization_code' && $grant_type !== 'refresh_token') {
Expand All @@ -105,6 +113,33 @@ public function getToken($grant_type, $code, $refresh_token, $client_id, $client
return $response;
}

if ($grant_type === 'authorization_code') {
// check this token is in authorization code state
$deliveredTokenCount = $accessToken->getTokenCount();
if ($deliveredTokenCount > 0) {
$response = new JSONResponse([
'error' => 'invalid_request',
], Http::STATUS_BAD_REQUEST);
$response->throttle(['invalid_request' => 'authorization_code_received_for_active_token']);
return $response;
}

// check authorization code expiration
$now = $this->timeFactory->now()->getTimestamp();
$codeCreatedAt = $accessToken->getCodeCreatedAt();
if ($codeCreatedAt < $now - self::AUTHORIZATION_CODE_EXPIRES_AFTER) {
// we know this token is not useful anymore
$this->accessTokenMapper->delete($accessToken);

$response = new JSONResponse([
'error' => 'invalid_request',
], Http::STATUS_BAD_REQUEST);
$expiredSince = $now - self::AUTHORIZATION_CODE_EXPIRES_AFTER - $codeCreatedAt;
$response->throttle(['invalid_request' => 'authorization_code_expired', 'expired_since' => $expiredSince]);
return $response;
}
}

try {
$client = $this->clientMapper->getByUid($accessToken->getClientId());
} catch (ClientNotFoundException $e) {
Expand Down Expand Up @@ -172,6 +207,11 @@ public function getToken($grant_type, $code, $refresh_token, $client_id, $client
$newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
$accessToken->setHashedCode(hash('sha512', $newCode));
$accessToken->setEncryptedToken($this->crypto->encrypt($newToken, $newCode));
// increase the number of delivered oauth token
// this helps with cleaning up DB access token when authorization code has expired
// and it never delivered any oauth token
$tokenCount = $accessToken->getTokenCount();
$accessToken->setTokenCount($tokenCount + 1);
$this->accessTokenMapper->update($accessToken);

$this->throttler->resetDelay($this->request->getRemoteAddress(), 'login', ['user' => $appToken->getUID()]);
Expand Down
10 changes: 10 additions & 0 deletions apps/oauth2/lib/Db/AccessToken.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@
* @method void setEncryptedToken(string $token)
* @method string getHashedCode()
* @method void setHashedCode(string $token)
* @method int getCodeCreatedAt()
* @method void setCodeCreatedAt(int $createdAt)
* @method int getTokenCount()
* @method void setTokenCount(int $tokenCount)
*/
class AccessToken extends Entity {
/** @var int */
Expand All @@ -44,12 +48,18 @@ class AccessToken extends Entity {
protected $hashedCode;
/** @var string */
protected $encryptedToken;
/** @var int */
protected $codeCreatedAt;
/** @var int */
protected $tokenCount;

public function __construct() {
$this->addType('id', 'int');
$this->addType('tokenId', 'int');
$this->addType('clientId', 'int');
$this->addType('hashedCode', 'string');
$this->addType('encryptedToken', 'string');
$this->addType('codeCreatedAt', 'int');
$this->addType('tokenCount', 'int');
}
}
31 changes: 27 additions & 4 deletions apps/oauth2/lib/Db/AccessTokenMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,12 @@
*/
namespace OCA\OAuth2\Db;

use OCA\OAuth2\Controller\OauthApiController;
use OCA\OAuth2\Exceptions\AccessTokenNotFoundException;
use OCP\AppFramework\Db\IMapperException;
use OCP\AppFramework\Db\QBMapper;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\DB\Exception;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;

Expand All @@ -39,10 +42,10 @@
*/
class AccessTokenMapper extends QBMapper {

/**
* @param IDBConnection $db
*/
public function __construct(IDBConnection $db) {
public function __construct(
IDBConnection $db,
private ITimeFactory $timeFactory,
) {
parent::__construct($db, 'oauth2_access_tokens');
}

Expand Down Expand Up @@ -79,4 +82,24 @@ public function deleteByClientId(int $id) {
->where($qb->expr()->eq('client_id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)));
$qb->executeStatement();
}

/**
* Delete access tokens that have an expired authorization code
* -> those that are old enough
* and which never delivered any oauth token (still in authorization state)
*
* @return void
* @throws Exception
*/
public function cleanupExpiredAuthorizationCode(): void {
$now = $this->timeFactory->now()->getTimestamp();
$maxTokenCreationTs = $now - OauthApiController::AUTHORIZATION_CODE_EXPIRES_AFTER;

$qb = $this->db->getQueryBuilder();
$qb
->delete($this->tableName)
->where($qb->expr()->eq('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->lt('code_created_at', $qb->createNamedParameter($maxTokenCreationTs, IQueryBuilder::PARAM_INT)));
$qb->executeStatement();
}
}
86 changes: 86 additions & 0 deletions apps/oauth2/lib/Migration/Version011603Date20230620111039.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright 2023, Julien Veyssier <julien-nc@posteo.net>
*
* @author Julien Veyssier <julien-nc@posteo.net>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/
namespace OCA\OAuth2\Migration;

use Closure;
use OCP\DB\ISchemaWrapper;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\DB\Types;
use OCP\IDBConnection;
use OCP\Migration\IOutput;
use OCP\Migration\SimpleMigrationStep;

class Version011603Date20230620111039 extends SimpleMigrationStep {

public function __construct(
private IDBConnection $connection,
) {
}

public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
/** @var ISchemaWrapper $schema */
$schema = $schemaClosure();

if ($schema->hasTable('oauth2_access_tokens')) {
$table = $schema->getTable('oauth2_access_tokens');
$dbChanged = false;
if (!$table->hasColumn('code_created_at')) {
$table->addColumn('code_created_at', Types::BIGINT, [
'notnull' => true,
'default' => 0,
'unsigned' => true,
]);
$dbChanged = true;
}
if (!$table->hasColumn('token_count')) {
$table->addColumn('token_count', Types::BIGINT, [
'notnull' => true,
'default' => 0,
'unsigned' => true,
]);
$dbChanged = true;
}
if (!$table->hasIndex('oauth2_tk_c_created_idx')) {
$table->addIndex(['token_count', 'code_created_at'], 'oauth2_tk_c_created_idx');
$dbChanged = true;
}
if ($dbChanged) {
return $schema;
}
}

return null;
}

public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void {
// we consider that existing access_tokens have already produced at least one oauth token
// which prevents cleaning them up
$qbUpdate = $this->connection->getQueryBuilder();
$qbUpdate->update('oauth2_access_tokens')
->set('token_count', $qbUpdate->createNamedParameter(1, IQueryBuilder::PARAM_INT));
$qbUpdate->executeStatement();
}
}
Loading

0 comments on commit b4fec29

Please sign in to comment.