Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
cleanup access tokens that are still in authorization state and that …
…have expired

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
  • Loading branch information
julien-nc committed Oct 5, 2023
commit 7bba41099753ca3e28ae5ee22f2460e5cd989250
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
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) {
try {
$this->accessTokenMapper->cleanupExpiredAuthorizationCode();
} catch (Exception $e) {
$this->logger->warning('Failed to cleanup tokens with expired authorization code', ['exception' => $e]);
}
}
}
5 changes: 5 additions & 0 deletions apps/oauth2/lib/Controller/OauthApiController.php
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,11 @@ public function getToken(
$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
5 changes: 5 additions & 0 deletions apps/oauth2/lib/Db/AccessToken.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
* @method void setHashedCode(string $token)
* @method int getCreatedAt()
* @method void setCreatedAt(int $createdAt)
* @method int getTokenCount()
* @method void setTokenCount(int $tokenCount)
*/
class AccessToken extends Entity {
/** @var int */
Expand All @@ -48,6 +50,8 @@ class AccessToken extends Entity {
protected $encryptedToken;
/** @var int */
protected $createdAt;
/** @var int */
protected $tokenCount;

Check notice

Code scanning / Psalm

PropertyNotSetInConstructor

Property OCA\OAuth2\Db\AccessToken::$tokenCount is not defined in constructor of OCA\OAuth2\Db\AccessToken or in any methods called in the constructor

public function __construct() {
$this->addType('id', 'int');
Expand All @@ -56,5 +60,6 @@ public function __construct() {
$this->addType('hashedCode', 'string');
$this->addType('encryptedToken', 'string');
$this->addType('created_at', 'int');
$this->addType('token_count', '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('created_at', $qb->createNamedParameter($maxTokenCreationTs, IQueryBuilder::PARAM_INT)));
$qb->executeStatement();
}
}
24 changes: 24 additions & 0 deletions apps/oauth2/lib/Migration/Version011603Date20230620111039.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,16 @@

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,
) {
}

Expand All @@ -43,15 +46,36 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt

if ($schema->hasTable('oauth2_access_tokens')) {
$table = $schema->getTable('oauth2_access_tokens');
$dbChanged = false;
if (!$table->hasColumn('created_at') || !$table->hasColumn('token_count')) {
$dbChanged = true;
}
if (!$table->hasColumn('created_at')) {
$table->addColumn('created_at', Types::BIGINT, [
'notnull' => true,
'default' => 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'default' => 0,
'unsigned' => true,

Default 0 is automatically, unsigned will allow more numbers

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not explicitly setting the default to 0? I didn't know about it, maybe others don't know either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fine 👍🏼

]);
}
if (!$table->hasColumn('token_count')) {
$table->addColumn('token_count', Types::BIGINT, [
'notnull' => true,
'default' => 0,
]);
}
if ($dbChanged) {
return $schema;
}
}

return null;
}

public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) {
// 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();
}
}