From 3f2821d75a4d2451f397d98d4095547d520f660e Mon Sep 17 00:00:00 2001 From: Alexis Saettler Date: Mon, 3 May 2021 20:52:37 +0200 Subject: [PATCH] fix: fix import vcard stability (#5160) --- app/Exceptions/AccountLimitException.php | 12 ------ app/Exceptions/Handler.php | 1 - .../Api/Contact/ApiDocumentController.php | 11 +++++ .../Contacts/DocumentsController.php | 10 +++++ app/Http/Controllers/SettingsController.php | 26 ++++++------ app/Models/Account/ImportJob.php | 41 +++++++++++++------ .../Contact/Contact/CreateContact.php | 3 +- .../Contact/Document/UploadDocument.php | 7 +--- 8 files changed, 64 insertions(+), 47 deletions(-) delete mode 100644 app/Exceptions/AccountLimitException.php diff --git a/app/Exceptions/AccountLimitException.php b/app/Exceptions/AccountLimitException.php deleted file mode 100644 index cd6f0f2e90a..00000000000 --- a/app/Exceptions/AccountLimitException.php +++ /dev/null @@ -1,12 +0,0 @@ -middleware('limitations')->only('store'); + parent::__construct(); + } + /** * Get the list of documents. * diff --git a/app/Http/Controllers/Contacts/DocumentsController.php b/app/Http/Controllers/Contacts/DocumentsController.php index 01850d92a6f..396c8cdc3e1 100644 --- a/app/Http/Controllers/Contacts/DocumentsController.php +++ b/app/Http/Controllers/Contacts/DocumentsController.php @@ -16,6 +16,16 @@ class DocumentsController extends Controller { use JsonRespondController; + /** + * Instantiate a new controller instance. + * + * @return void + */ + public function __construct() + { + $this->middleware('limitations')->only('store'); + } + /** * Display the list of documents. * diff --git a/app/Http/Controllers/SettingsController.php b/app/Http/Controllers/SettingsController.php index 2ddbd95588e..718c0b3ed12 100644 --- a/app/Http/Controllers/SettingsController.php +++ b/app/Http/Controllers/SettingsController.php @@ -22,15 +22,24 @@ use LaravelWebauthn\Models\WebauthnKey; use App\Http\Requests\InvitationRequest; use App\Services\Contact\Tag\DestroyTag; -use App\Exceptions\AccountLimitException; use App\Services\Account\Settings\ResetAccount; use App\Services\Account\Settings\DestroyAccount; use PragmaRX\Google2FALaravel\Facade as Google2FA; use App\Http\Resources\Contact\ContactShort as ContactResource; use App\Http\Resources\Settings\WebauthnKey\WebauthnKey as WebauthnKeyResource; -class SettingsController +class SettingsController extends Controller { + /** + * Instantiate a new controller instance. + * + * @return void + */ + public function __construct() + { + $this->middleware('limitations')->only(['inviteUser', 'storeImport']); + } + /** * Display a listing of the resource. * @@ -233,16 +242,9 @@ public function upload() public function storeImport(ImportsRequest $request) { - $account = auth()->user()->account; - if (AccountHelper::hasReachedContactLimit($account) - && AccountHelper::hasLimitations($account) - && ! $account->legacy_free_plan_unlimited_contacts) { - throw new AccountLimitException(); - } - $filename = $request->file('vcard')->store('imports', config('filesystems.default')); - $importJob = $account->importjobs()->create([ + $importJob = auth()->user()->account->importjobs()->create([ 'user_id' => auth()->user()->id, 'type' => 'vcard', 'filename' => $filename, @@ -308,10 +310,6 @@ public function addUser() */ public function inviteUser(InvitationRequest $request) { - if (AccountHelper::hasLimitations(auth()->user()->account)) { - throw new AccountLimitException(); - } - // Make sure the confirmation to invite has not been bypassed if (! $request->input('confirmation')) { return redirect()->back()->withErrors(trans('settings.users_error_please_confirm'))->withInput(); diff --git a/app/Models/Account/ImportJob.php b/app/Models/Account/ImportJob.php index d408653de0b..0c440892626 100644 --- a/app/Models/Account/ImportJob.php +++ b/app/Models/Account/ImportJob.php @@ -5,6 +5,7 @@ use App\Models\User\User; use Sabre\VObject\Reader; use Illuminate\Support\Arr; +use App\Helpers\AccountHelper; use Sabre\VObject\Component\VCard; use App\Services\VCard\ImportVCard; use Illuminate\Database\Eloquent\Model; @@ -49,7 +50,7 @@ class ImportJob extends Model * * @var VCardReader */ - public $entries; + public $entries = null; /** * The attributes that aren't mass assignable. @@ -104,15 +105,17 @@ public function process($behaviour = ImportVCard::BEHAVIOUR_ADD) { $this->initJob(); - $this->getPhysicalFile(); + if (! $this->failed && $this->getPhysicalFile()) { + $this->getEntries(); - $this->getEntries(); - - $this->processEntries($behaviour); + $this->processEntries($behaviour); + } $this->deletePhysicalFile(); - $this->endJob(); + if (! $this->failed) { + $this->endJob(); + } } /** @@ -122,6 +125,10 @@ public function process($behaviour = ImportVCard::BEHAVIOUR_ADD) */ private function initJob(): void { + if (AccountHelper::hasLimitations($this->account)) { + $this->fail(trans('auth.not_authorized')); + } + $this->started_at = now(); $this->contacts_imported = 0; $this->contacts_skipped = 0; @@ -149,36 +156,44 @@ private function endJob(): void private function fail(string $reason): void { $this->failed = true; - $this->failed_reason = $reason; + if (! $this->failed_reason) { + $this->failed_reason = $reason; + } $this->endJob(); } /** * Get the physical file (the vCard file). * - * @return self + * @return bool */ - private function getPhysicalFile() + private function getPhysicalFile(): bool { try { $this->physicalFile = Storage::disk(config('filesystems.default'))->readStream($this->filename); } catch (FileNotFoundException $exception) { $this->fail(trans('settings.import_vcard_file_not_found')); + + return false; } - return $this; + return true; } /** * Delete the physical file from the disk. * - * @return void + * @return bool */ - private function deletePhysicalFile(): void + private function deletePhysicalFile(): bool { if (! Storage::disk(config('filesystems.default'))->delete($this->filename)) { $this->fail(trans('settings.import_vcard_file_not_found')); + + return false; } + + return true; } /** @@ -202,7 +217,7 @@ private function processEntries($behaviour = ImportVCard::BEHAVIOUR_ADD) while (true) { try { /** @var VCard|null */ - $entry = $this->entries->getNext(); + $entry = $this->entries !== null ? $this->entries->getNext() : null; if (! $entry) { // file end break; diff --git a/app/Services/Contact/Contact/CreateContact.php b/app/Services/Contact/Contact/CreateContact.php index 4e0a622e693..2d5bf1a1200 100644 --- a/app/Services/Contact/Contact/CreateContact.php +++ b/app/Services/Contact/Contact/CreateContact.php @@ -11,7 +11,6 @@ use App\Models\Account\Account; use App\Models\Contact\Contact; use App\Jobs\AuditLog\LogAccountAudit; -use App\Exceptions\AccountLimitException; use App\Jobs\Avatars\GenerateDefaultAvatar; use App\Jobs\Avatars\GetAvatarsFromInternet; @@ -64,7 +63,7 @@ public function execute(array $data): Contact if (AccountHelper::hasReachedContactLimit($account) && AccountHelper::hasLimitations($account) && ! $account->legacy_free_plan_unlimited_contacts) { - throw new AccountLimitException(); + abort(402); } // filter out the data that shall not be updated here diff --git a/app/Services/Contact/Document/UploadDocument.php b/app/Services/Contact/Document/UploadDocument.php index b5b02e3b0bf..52b1cecc810 100644 --- a/app/Services/Contact/Document/UploadDocument.php +++ b/app/Services/Contact/Document/UploadDocument.php @@ -7,7 +7,6 @@ use App\Models\Account\Account; use App\Models\Contact\Contact; use App\Models\Contact\Document; -use App\Exceptions\AccountLimitException; class UploadDocument extends BaseService { @@ -36,10 +35,8 @@ public function execute(array $data): Document $this->validate($data); $account = Account::find($data['account_id']); - if (AccountHelper::hasReachedContactLimit($account) - && AccountHelper::hasLimitations($account) - && ! $account->legacy_free_plan_unlimited_contacts) { - throw new AccountLimitException(); + if (AccountHelper::hasLimitations($account)) { + abort(402); } Contact::where('account_id', $data['account_id'])