Skip to content

Commit

Permalink
Merge pull request #210 from astrairidium/delay-failed-verify
Browse files Browse the repository at this point in the history
Delay validating previously failed files.
  • Loading branch information
fancycode committed Sep 6, 2023
2 parents d18c4b7 + 7273bad commit 6cadc24
Show file tree
Hide file tree
Showing 9 changed files with 311 additions and 76 deletions.
16 changes: 16 additions & 0 deletions lib/BackgroundJob/BackgroundVerify.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@
use Psr\Log\LoggerInterface;

class BackgroundVerify extends TimedJob {

// Retry files with errors after 1 hour.
private const VERIFY_ERROR_DELAY_MINUTES = 60;

private LoggerInterface $logger;
private IUserManager $userManager;
private IDBConnection $db;
Expand Down Expand Up @@ -160,13 +164,24 @@ protected function getUserWithAccessToStorage(int $storageId): array {
private function getPendingFiles(): IResult {
$pdfMimeTypeId = $this->mimeTypeLoader->getId('application/pdf');

$since = new \DateTime();
$since->setTimezone(new \DateTimeZone('UTC'));
$since = $since->sub(new \DateInterval('PT' . self::VERIFY_ERROR_DELAY_MINUTES . 'M'));

$query = $this->db->getQueryBuilder();
$query->select('fc.fileid', 'storage')
->from('filecache', 'fc')
->leftJoin('fc', 'c24_file_signatures', 'fs', $query->expr()->eq('fc.fileid', 'fs.file_id'))
->leftJoin('fc', 'c24_verify_failed', 'vf', $query->expr()->eq('fc.fileid', 'vf.file_id'))
->where($query->expr()->isNull('fs.file_id'))
->andWhere($query->expr()->eq('mimetype', $query->expr()->literal($pdfMimeTypeId)))
->andWhere($query->expr()->like('path', $query->expr()->literal('files/%')))
->andWhere(
$query->expr()->orX(
$query->expr()->isNull('vf.updated'),
$query->expr()->lte('vf.updated', $query->createNamedParameter($since, 'datetimetz'))
),
)
->setMaxResults($this->getBatchSize() * 10);

return $query->executeQuery();
Expand All @@ -186,6 +201,7 @@ private function verifyFile(File $file, array $account, string $server) {
$this->logger->error('Error sending request to ' . $server . ' for ' . $file->getPath(), [
'exception' => $e,
]);
$this->verify->storeFailed($file);
return;
}

Expand Down
1 change: 1 addition & 0 deletions lib/Controller/VerifyController.php
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ public function getFileSignatures(int $id, bool $reverify = false): DataResponse
*/
public function clearCache(): DataResponse {
$this->verify->deleteAllFileSignatures();
$this->verify->deleteAllFailed();
return new DataResponse([
'unverified_count' => $this->verify->getUnverifiedCount(),
]);
Expand Down
1 change: 1 addition & 0 deletions lib/DeleteListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public function handle(Event $event): void {

$this->metadata->deleteMetadata($file);
$this->verify->deleteFileSignatures($file);
$this->verify->deleteFailed($file);
$requests = $this->requests->getRequestsForFile($file, true);
foreach ($requests as $request) {
$this->manager->deleteRequest($request, $account);
Expand Down
64 changes: 64 additions & 0 deletions lib/Migration/Version1000Date20230830121523.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php

/**
* @copyright Copyright (c) 2023, struktur AG.
*
* @author Joachim Bauch <bauch@struktur.de>
*
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* 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, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
namespace OCA\Certificate24\Migration;

use Doctrine\DBAL\Types\Types;
use OCP\DB\ISchemaWrapper;
use OCP\Migration\IOutput;
use OCP\Migration\SimpleMigrationStep;

class Version1000Date20230830121523 extends SimpleMigrationStep {
/**
* @param IOutput $output
* @param \Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
* @param array $options
* @return null|ISchemaWrapper
*/
public function changeSchema(IOutput $output, \Closure $schemaClosure, array $options) {
$schema = $schemaClosure();

if (!$schema->hasTable('c24_verify_failed')) {
$table = $schema->createTable('c24_verify_failed');
$table->addColumn('id', Types::BIGINT, [
'autoincrement' => true,
'notnull' => true,
]);
$table->addColumn('file_id', Types::BIGINT, [
'notnull' => true,
'length' => 64,
]);
$table->addColumn('created', Types::DATETIMETZ_MUTABLE, [
'notnull' => true,
]);
$table->addColumn('updated', Types::DATETIMETZ_MUTABLE, [
'notnull' => true,
]);
$table->addColumn('count', Types::INTEGER, [
'notnull' => true,
]);
$table->setPrimaryKey(['id']);
$table->addUniqueIndex(['file_id']);
}
return $schema;
}
}
61 changes: 61 additions & 0 deletions lib/Verify.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,11 @@ public function __construct(LoggerInterface $logger,
public function storeFileSignatures(File $file, ?array $signatures): void {
if (empty($signatures)) {
$this->deleteFileSignatures($file);
$this->deleteFailed($file);
return;
}

$this->deleteFailed($file);
$signatures = json_encode($signatures);
$update = $this->db->getQueryBuilder();
$update->update('c24_file_signatures')
Expand Down Expand Up @@ -144,4 +146,63 @@ public function getUnverifiedCount(): ?int {

return $row['count'];
}

public function deleteFailed(File $file): void {
$query = $this->db->getQueryBuilder();
$query->delete('c24_verify_failed')
->where($query->expr()->eq('file_id', $query->createNamedParameter($file->getId(), IQueryBuilder::PARAM_INT)));
$query->executeStatement();
}

public function getFailedCount(File $file): int {
$query = $this->db->getQueryBuilder();
$query->select('*')
->from('c24_verify_failed')
->where($query->expr()->eq('file_id', $query->createNamedParameter($file->getId(), IQueryBuilder::PARAM_INT)));
$result = $query->executeQuery();
$row = $result->fetch();
$result->closeCursor();
if ($row === false) {
return 0;
}

return (int) $row['count'];
}

public function storeFailed(File $file): void {
$update = $this->db->getQueryBuilder();
$update->update('c24_verify_failed')
->set('updated', $update->createFunction('now()'))
->set('count', $update->createFunction('`count` + 1'))
->where($update->expr()->eq('file_id', $update->createNamedParameter($file->getId(), IQueryBuilder::PARAM_INT)));
if ($update->executeStatement() > 0) {
// Updated existing entry.
return;
}

$query = $this->db->getQueryBuilder();
$query->insert('c24_verify_failed')
->values(
[
'file_id' => $query->createNamedParameter($file->getId(), IQueryBuilder::PARAM_INT),
'created' => $query->createFunction('now()'),
'updated' => $query->createFunction('now()'),
'count' => $query->createNamedParameter(1, IQueryBuilder::PARAM_INT),
]
);

try {
$query->executeStatement();
} catch (UniqueConstraintViolationException $e) {
// Another user added the entry concurrently.
return;
}
}

public function deleteAllFailed(): void {
$query = $this->db->getQueryBuilder();
$query->delete('c24_verify_failed');
$query->executeStatement();
}

}
22 changes: 13 additions & 9 deletions src/views/FilesSidebarTab.vue
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,21 @@
{{ t('certificate24', 'Check manually') }}
</NcButton>
</div>
<div v-else-if="isSidebarSupportedForFile && (notSigned || encryptedFile)" class="emptycontent">
<div v-else-if="isSidebarSupportedForFile && (notSigned || errorValidating)" class="emptycontent">
<div class="icon icon-certificate24" />
<h2>{{ t('certificate24', 'Signatures') }}</h2>
<p v-if="notSigned">
{{ t('certificate24', 'The file is not signed.') }}
</p>
<p v-else-if="encryptedFile">
<p v-else-if="errorValidating === 'error_encrypted_file'">
{{ t('certificate24', 'The file is encrypted and can not be checked.') }}
</p>
<p v-else-if="errorValidating === 'error_parsing_file'">
{{ t('certificate24', 'The file could not be parsed and can not be checked.') }}
</p>
<p v-else>
{{ t('certificate24', 'Error fetching signature details.') }}
</p>
<NcButton @click="forceCheck">
{{ t('certificate24', 'Force recheck') }}
</NcButton>
Expand Down Expand Up @@ -79,7 +85,7 @@ export default {
sidebarState: OCA.Files.Sidebar.state,
signaturesPending: false,
notSigned: false,
encryptedFile: false,
errorValidating: null,
isSidebarSupportedForFile: undefined,
prevSignaturesPending: false,
prevNotSigned: false,
Expand Down Expand Up @@ -164,6 +170,7 @@ export default {
// When it is not possible to know whether the sidebar is
// supported for a file or not only from the data in the
// FileInfo it is necessary to query the server.
this.errorValidating = null
try {
this.isSidebarSupportedForFile = (await getFileSignatures({ fileId: fileInfo.id }, { force })) || false
} catch (error) {
Expand Down Expand Up @@ -191,6 +198,7 @@ export default {
// When it is not possible to know whether the sidebar is
// supported for a file or not only from the data in the
// FileInfo it is necessary to query the server.
this.errorValidating = null
try {
this.isSidebarSupportedForFile = (await getFileSignatures({ fileId: fileInfo.id }, { force })) || false
} catch (error) {
Expand All @@ -207,12 +215,8 @@ export default {
handleGetSignaturesError(error) {
switch (error.response?.status) {
case 400:
switch (error.response.data.ocs?.data?.code) {
case 'error_encrypted_file':
this.isSidebarSupportedForFile = true
this.encryptedFile = true
break
}
this.isSidebarSupportedForFile = true
this.errorValidating = error.response.data.ocs?.data?.code || null
break
case 404:
switch (error.response.data.ocs?.data?.status) {
Expand Down
13 changes: 9 additions & 4 deletions src/views/SignaturesView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
<div class="icon icon-loading" />
</div>

<div v-else-if="signatures.status === 'not_signed' || signatures.code === 'error_encrypted_file'">
<div v-else-if="signatures.status === 'not_signed' || signatures.code">
<div class="icon icon-certificate24" />
<h2>{{ t('certificate24', 'Signatures') }}</h2>
<p v-if="signatures.status === 'not_signed'">
Expand All @@ -33,6 +33,12 @@
<p v-else-if="signatures.code === 'error_encrypted_file'">
{{ t('certificate24', 'The file is encrypted and can not be checked.') }}
</p>
<p v-else-if="signatures.code === 'error_parsing_file'">
{{ t('certificate24', 'The file could not be parsed and can not be checked.') }}
</p>
<p v-else>
{{ t('certificate24', 'Error fetching signature details.') }}
</p>
</div>

<template v-else>
Expand Down Expand Up @@ -153,7 +159,7 @@ export default {
computed: {
isEmptyContent() {
return this.signatures === 'undefined' || this.signatures?.status === 'not_signed'
return this.signatures === 'undefined' || this.signatures?.status === 'not_signed' || this.signatures?.code
},
},
Expand Down Expand Up @@ -183,8 +189,7 @@ export default {
} catch (error) {
switch (error.response?.status) {
case 400:
switch (error.response.data.ocs?.data?.code) {
case 'error_encrypted_file':
if (error.response.data.ocs?.data?.code) {
this.signatures = error.response.data.ocs?.data
return
}
Expand Down
72 changes: 72 additions & 0 deletions tests/php/VerifyTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<?php

/**
* @copyright Copyright (c) 2023, struktur AG.
*
* @author Joachim Bauch <bauch@struktur.de>
*
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* 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, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
namespace OCA\Certificate24\Tests\php;

use OCA\Certificate24\Requests;
use OCA\Certificate24\Verify;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\Files\File;
use OCP\Files\IMimeTypeLoader;
use OCP\IDBConnection;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\LoggerInterface;
use Test\TestCase;

/**
* @group DB
*/
class VerifyTest extends TestCase {
/** @var MockObject|ITimeFactory $timeFactory */
protected ITimeFactory $timeFactory;
protected Verify $verify;

public function setUp(): void {
parent::setUp();

$this->timeFactory = $this->createMock(ITimeFactory::class);
$db = \OC::$server->query(IDBConnection::class);
$mimeTypeLoader = \OC::$server->query(IMimeTypeLoader::class);
$logger = $this->createMock(LoggerInterface::class);
$requests = $this->createMock(Requests::class);

$this->verify = new Verify($logger, $db, $mimeTypeLoader, $requests);
$this->verify->deleteAllFileSignatures();
$this->verify->deleteAllFailed();
}

public function testIncrementFailed() {
/** @var MockObject|File $file */
$file = $this->createMock(File::class);
$file
->method('getId')
->willReturn(1);

$this->assertEquals(0, $this->verify->getFailedCount($file));
$this->verify->storeFailed($file);
$this->assertEquals(1, $this->verify->getFailedCount($file));
$this->verify->storeFailed($file);
$this->assertEquals(2, $this->verify->getFailedCount($file));
$this->verify->deleteFailed($file);
$this->assertEquals(0, $this->verify->getFailedCount($file));
}
}
Loading

0 comments on commit 6cadc24

Please sign in to comment.