From 78e337021ea87eee239293346510796dcf7c1bd0 Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Thu, 13 Feb 2025 17:23:54 +0100 Subject: [PATCH 01/11] Quick fix --- app/Exceptions/ConflictException.php | 12 ++++++++ app/Exceptions/Handler.php | 9 ++++++ app/Services/GroupService.php | 30 ++++++++++++++----- .../components/DestinationGroupSelector.vue | 2 +- 4 files changed, 44 insertions(+), 9 deletions(-) create mode 100644 app/Exceptions/ConflictException.php diff --git a/app/Exceptions/ConflictException.php b/app/Exceptions/ConflictException.php new file mode 100644 index 00000000..1d92dabb --- /dev/null +++ b/app/Exceptions/ConflictException.php @@ -0,0 +1,12 @@ +renderable(function (ConflictException $exception, $request) { + return response()->json([ + 'message' => 'conflict', + 'reason' => [$exception->getMessage()], + ], 409); + }); + + } } diff --git a/app/Services/GroupService.php b/app/Services/GroupService.php index a7898b4d..62c12fd1 100644 --- a/app/Services/GroupService.php +++ b/app/Services/GroupService.php @@ -2,11 +2,13 @@ namespace App\Services; +use App\Exceptions\ConflictException; use App\Models\Group; use App\Models\TwoFAccount; use App\Models\User; use Illuminate\Auth\Access\AuthorizationException; use Illuminate\Database\Eloquent\Collection; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; class GroupService @@ -19,6 +21,7 @@ class GroupService * @param mixed $targetGroup The group the accounts should be assigned to * * @throws \Illuminate\Auth\Access\AuthorizationException + * @throws App\Exceptions\ConflictException */ public static function assign($ids, User $user, mixed $targetGroup = null) : void { @@ -53,17 +56,28 @@ class GroupService } if ($group) { - $ids = is_array($ids) ? $ids : [$ids]; - $twofaccounts = TwoFAccount::find($ids); + $ids = is_array($ids) ? $ids : [$ids]; - if ($user->cannot('updateEach', [(new TwoFAccount), $twofaccounts])) { - throw new AuthorizationException; - } + DB::transaction(function () use ($group, $ids, $user) { + // Check if group still exists within transaction + $group = Group::lockForUpdate()->find($group->id); + $twofaccounts = TwoFAccount::sharedLock()->find($ids); - $group->twofaccounts()->saveMany($twofaccounts); - $group->loadCount('twofaccounts'); + if ($user->cannot('updateEach', [(new TwoFAccount), $twofaccounts])) { + throw new AuthorizationException; + } + + if (! $group) { + throw new ConflictException(__('errors.resource_not_found')); + } + + // Proceed with assignment + $group->twofaccounts()->saveMany($twofaccounts); + $group->loadCount('twofaccounts'); + + Log::info(sprintf('Twofaccounts #%s assigned to group %s (ID #%s)', implode(',', $ids), var_export($group->name, true), $group->id)); + }); - Log::info(sprintf('Twofaccounts #%s assigned to group %s (ID #%s)', implode(',', $ids), var_export($group->name, true), $group->id)); } else { Log::info('Cannot find a group to assign the TwoFAccounts to'); } diff --git a/resources/js/components/DestinationGroupSelector.vue b/resources/js/components/DestinationGroupSelector.vue index d94bc026..661689fa 100644 --- a/resources/js/components/DestinationGroupSelector.vue +++ b/resources/js/components/DestinationGroupSelector.vue @@ -21,7 +21,7 @@ if (destinationGroupId.value === 0) { await twofaccountService.withdraw(props.selectedAccountsIds) } - else await groupService.assign(props.selectedAccountsIds, destinationGroupId.value) + else groupService.assign(props.selectedAccountsIds, destinationGroupId.value, { returnError: true }) emit('accounts-moved') } From 37afe2057f91084512de80ca0952903d2613578b Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Wed, 19 Feb 2025 11:10:24 +0100 Subject: [PATCH 02/11] Remove quick fix --- app/Exceptions/ConflictException.php | 12 ------------ app/Exceptions/Handler.php | 9 --------- app/Services/GroupService.php | 6 ------ 3 files changed, 27 deletions(-) delete mode 100644 app/Exceptions/ConflictException.php diff --git a/app/Exceptions/ConflictException.php b/app/Exceptions/ConflictException.php deleted file mode 100644 index 1d92dabb..00000000 --- a/app/Exceptions/ConflictException.php +++ /dev/null @@ -1,12 +0,0 @@ -renderable(function (ConflictException $exception, $request) { - return response()->json([ - 'message' => 'conflict', - 'reason' => [$exception->getMessage()], - ], 409); - }); - - } } diff --git a/app/Services/GroupService.php b/app/Services/GroupService.php index 62c12fd1..6de1444c 100644 --- a/app/Services/GroupService.php +++ b/app/Services/GroupService.php @@ -2,7 +2,6 @@ namespace App\Services; -use App\Exceptions\ConflictException; use App\Models\Group; use App\Models\TwoFAccount; use App\Models\User; @@ -21,7 +20,6 @@ class GroupService * @param mixed $targetGroup The group the accounts should be assigned to * * @throws \Illuminate\Auth\Access\AuthorizationException - * @throws App\Exceptions\ConflictException */ public static function assign($ids, User $user, mixed $targetGroup = null) : void { @@ -66,10 +64,6 @@ class GroupService if ($user->cannot('updateEach', [(new TwoFAccount), $twofaccounts])) { throw new AuthorizationException; } - - if (! $group) { - throw new ConflictException(__('errors.resource_not_found')); - } // Proceed with assignment $group->twofaccounts()->saveMany($twofaccounts); From 180f8e3448246b7b767c3e2f0038eb67d1bda4f7 Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Wed, 19 Feb 2025 11:14:17 +0100 Subject: [PATCH 03/11] Enforce route binding to avoid 404 when requesting the 'All' group --- app/Models/Group.php | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/app/Models/Group.php b/app/Models/Group.php index 67b06aa5..3b997b3c 100644 --- a/app/Models/Group.php +++ b/app/Models/Group.php @@ -102,6 +102,31 @@ class Group extends Model Log::info(sprintf('Group %s (id #%d) deleted ', var_export($model->name, true), $model->id)); }); } + + /** + * Retrieve the model for a bound value. + * + * @param mixed $value + * @param string|null $field + * @return \Illuminate\Database\Eloquent\Model|null + */ + public function resolveRouteBinding($value, $field = null) + { + // The All group is a virtual group with id==0. + // It never exists in database so we enforce the route binding + // resolution logic to return an instance instead of not found. + if ($value === '0') { + $group = new self([ + 'name' => __('commons.all'), + ]); + $group->id = 0; + + return $group; + } + else { + return parent::resolveRouteBinding($value, $field); + } + } /** * Get the TwoFAccounts of the group. From ceb9d6947811fc09b89fe292a439b60b7470784e Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Wed, 19 Feb 2025 11:16:13 +0100 Subject: [PATCH 04/11] Reject group creation with reserved 'All' name --- app/Api/v1/Requests/GroupStoreRequest.php | 13 +++++++++++++ resources/lang/en/errors.php | 1 + 2 files changed, 14 insertions(+) diff --git a/app/Api/v1/Requests/GroupStoreRequest.php b/app/Api/v1/Requests/GroupStoreRequest.php index 9a22a92f..ffb87716 100644 --- a/app/Api/v1/Requests/GroupStoreRequest.php +++ b/app/Api/v1/Requests/GroupStoreRequest.php @@ -30,8 +30,21 @@ class GroupStoreRequest extends FormRequest 'required', 'regex:/^[A-zÀ-ú0-9\s\-_]+$/', 'max:32', + Rule::notIn([__('commons.all')]), Rule::unique('groups')->where(fn ($query) => $query->where('user_id', $this->user()->id)), ], ]; } + + /** + * Get the error messages for the defined validation rules. + * + * @return array + */ + public function messages(): array + { + return [ + 'name.not_in' => __('errors.reserved_name_please_choose_something_else'), + ]; + } } diff --git a/resources/lang/en/errors.php b/resources/lang/en/errors.php index 29c6ffc6..f87217ed 100644 --- a/resources/lang/en/errors.php +++ b/resources/lang/en/errors.php @@ -74,4 +74,5 @@ return [ 'qrcode_has_invalid_checksum' => 'QR code has invalid checksum', 'no_readable_qrcode' => 'No readable QR code', 'failed_icon_store_database_toggling' => 'Migration of icons failed. The setting has been restored to its previous value.', + 'reserved_name_please_choose_something_else' => 'Reserved name, please choose something else', ]; \ No newline at end of file From b82a7eb604ddfe994fadce7db3a9e4a201c54a83 Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Wed, 19 Feb 2025 11:23:42 +0100 Subject: [PATCH 05/11] Trigger twofaccounts withdrawal after group deletion, not during --- app/Events/GroupDeleted.php | 3 +++ app/Listeners/DissociateTwofaccountFromGroup.php | 4 ++-- app/Models/Group.php | 3 --- app/Providers/EventServiceProvider.php | 5 +---- 4 files changed, 6 insertions(+), 9 deletions(-) diff --git a/app/Events/GroupDeleted.php b/app/Events/GroupDeleted.php index 6f5acc13..d7b50c58 100644 --- a/app/Events/GroupDeleted.php +++ b/app/Events/GroupDeleted.php @@ -6,6 +6,7 @@ use App\Models\Group; use Illuminate\Broadcasting\InteractsWithSockets; use Illuminate\Foundation\Events\Dispatchable; use Illuminate\Queue\SerializesModels; +use Illuminate\Support\Facades\Log; class GroupDeleted { @@ -24,5 +25,7 @@ class GroupDeleted public function __construct(Group $group) { $this->group = $group; + + Log::info(sprintf('Group %s (id #%d) deleted ', var_export($group->name, true), $group->id)); } } diff --git a/app/Listeners/DissociateTwofaccountFromGroup.php b/app/Listeners/DissociateTwofaccountFromGroup.php index d3c9dd74..9fe461c7 100644 --- a/app/Listeners/DissociateTwofaccountFromGroup.php +++ b/app/Listeners/DissociateTwofaccountFromGroup.php @@ -2,7 +2,7 @@ namespace App\Listeners; -use App\Events\GroupDeleting; +use App\Events\GroupDeleted; use App\Models\TwoFAccount; use Illuminate\Support\Facades\Log; @@ -23,7 +23,7 @@ class DissociateTwofaccountFromGroup * * @return void */ - public function handle(GroupDeleting $event) + public function handle(GroupDeleted $event) { TwoFAccount::where('group_id', $event->group->id) ->update( diff --git a/app/Models/Group.php b/app/Models/Group.php index 3b997b3c..1dc0a212 100644 --- a/app/Models/Group.php +++ b/app/Models/Group.php @@ -98,9 +98,6 @@ class Group extends Model static::updated(function (object $model) { Log::info(sprintf('Group %s (id #%d) updated by user ID #%s', var_export($model->name, true), $model->id, $model->user_id)); }); - static::deleted(function (object $model) { - Log::info(sprintf('Group %s (id #%d) deleted ', var_export($model->name, true), $model->id)); - }); } /** diff --git a/app/Providers/EventServiceProvider.php b/app/Providers/EventServiceProvider.php index c198d743..5f5114e3 100644 --- a/app/Providers/EventServiceProvider.php +++ b/app/Providers/EventServiceProvider.php @@ -3,7 +3,6 @@ namespace App\Providers; use App\Events\GroupDeleted; -use App\Events\GroupDeleting; use App\Events\ScanForNewReleaseCalled; use App\Events\StoreIconsInDatabaseSettingChanged; use App\Events\TwoFAccountDeleted; @@ -44,10 +43,8 @@ class EventServiceProvider extends ServiceProvider TwoFAccountDeleted::class => [ CleanIconStorage::class, ], - GroupDeleting::class => [ - DissociateTwofaccountFromGroup::class, - ], GroupDeleted::class => [ + DissociateTwofaccountFromGroup::class, ResetUsersPreference::class, ], ScanForNewReleaseCalled::class => [ From 166b39beeaa5c6b1e07a4c520fa3d6c705a524ed Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Wed, 19 Feb 2025 15:46:31 +0100 Subject: [PATCH 06/11] Handle missing group during group assignment --- app/Api/v1/Controllers/GroupController.php | 7 ++++++- app/Api/v1/Controllers/TwoFAccountController.php | 15 +++++++++++++-- app/Services/GroupService.php | 10 +++++++--- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/app/Api/v1/Controllers/GroupController.php b/app/Api/v1/Controllers/GroupController.php index 3dc24f07..4131c6d2 100644 --- a/app/Api/v1/Controllers/GroupController.php +++ b/app/Api/v1/Controllers/GroupController.php @@ -10,6 +10,7 @@ use App\Facades\Groups; use App\Http\Controllers\Controller; use App\Models\Group; use App\Models\User; +use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Http\Request; class GroupController extends Controller @@ -93,7 +94,11 @@ class GroupController extends Controller $validated = $request->validated(); - Groups::assign($validated['ids'], $request->user(), $group); + try { + Groups::assign($validated['ids'], $request->user(), $group); + } catch (ModelNotFoundException $exc) { + abort(404); + } return new GroupResource($group); } diff --git a/app/Api/v1/Controllers/TwoFAccountController.php b/app/Api/v1/Controllers/TwoFAccountController.php index 806195f7..70997761 100644 --- a/app/Api/v1/Controllers/TwoFAccountController.php +++ b/app/Api/v1/Controllers/TwoFAccountController.php @@ -21,6 +21,7 @@ use App\Helpers\Helpers; use App\Http\Controllers\Controller; use App\Models\TwoFAccount; use App\Models\User; +use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Http\Request; use Illuminate\Support\Arr; @@ -89,7 +90,12 @@ class TwoFAccountController extends Controller $request->user()->twofaccounts()->save($twofaccount); // Possible group association - Groups::assign($twofaccount->id, $request->user(), Arr::get($validated, 'group_id', null)); + try { + Groups::assign($twofaccount->id, $request->user(), Arr::get($validated, 'group_id', null)); + } catch (\Throwable $th) { + // The group association might fail but we don't want the twofaccount + // creation to be reverted so we do nothing here. + } return (new TwoFAccountReadResource($twofaccount->refresh())) ->response() @@ -116,7 +122,12 @@ class TwoFAccountController extends Controller if ((int) $groupId === 0) { TwoFAccounts::withdraw($twofaccount->id); } else { - Groups::assign($twofaccount->id, $request->user(), $groupId); + try { + Groups::assign($twofaccount->id, $request->user(), $groupId); + } catch (ModelNotFoundException $exc) { + // The destination group no longer exists, the twofaccount is withdrawn + TwoFAccounts::withdraw($twofaccount->id); + } } $twofaccount->refresh(); } diff --git a/app/Services/GroupService.php b/app/Services/GroupService.php index 6de1444c..c319da4e 100644 --- a/app/Services/GroupService.php +++ b/app/Services/GroupService.php @@ -7,6 +7,7 @@ use App\Models\TwoFAccount; use App\Models\User; use Illuminate\Auth\Access\AuthorizationException; use Illuminate\Database\Eloquent\Collection; +use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; @@ -20,6 +21,7 @@ class GroupService * @param mixed $targetGroup The group the accounts should be assigned to * * @throws \Illuminate\Auth\Access\AuthorizationException + * @throws \Illuminate\Database\Eloquent\ModelNotFoundException */ public static function assign($ids, User $user, mixed $targetGroup = null) : void { @@ -57,21 +59,23 @@ class GroupService $ids = is_array($ids) ? $ids : [$ids]; DB::transaction(function () use ($group, $ids, $user) { - // Check if group still exists within transaction $group = Group::lockForUpdate()->find($group->id); $twofaccounts = TwoFAccount::sharedLock()->find($ids); + + if (! $group) { + throw new ModelNotFoundException('group no longer exists'); + } if ($user->cannot('updateEach', [(new TwoFAccount), $twofaccounts])) { throw new AuthorizationException; } - // Proceed with assignment $group->twofaccounts()->saveMany($twofaccounts); - $group->loadCount('twofaccounts'); Log::info(sprintf('Twofaccounts #%s assigned to group %s (ID #%s)', implode(',', $ids), var_export($group->name, true), $group->id)); }); + $group->loadCount('twofaccounts'); } else { Log::info('Cannot find a group to assign the TwoFAccounts to'); } From 98033bcc567b4305a22549829d147e3a543c3085 Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Wed, 19 Feb 2025 15:51:33 +0100 Subject: [PATCH 07/11] Allow viewing of the All group, with matching twofaccount count --- app/Api/v1/Controllers/GroupController.php | 24 +++++++++++++++++++--- app/Policies/GroupPolicy.php | 2 +- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/app/Api/v1/Controllers/GroupController.php b/app/Api/v1/Controllers/GroupController.php index 4131c6d2..b42b002a 100644 --- a/app/Api/v1/Controllers/GroupController.php +++ b/app/Api/v1/Controllers/GroupController.php @@ -60,10 +60,18 @@ class GroupController extends Controller * * @return \App\Api\v1\Resources\GroupResource */ - public function show(Group $group) + public function show(Request $request, Group $group) { $this->authorize('view', $group); + // group with id==0 is the 'All' virtual group. + // Eloquent specifically returns a non-persisted Group instance + // with just the name property. The twofaccounts_count has to be + // set here. + if ($group->id === 0) { + $group->twofaccounts_count = $request->user()->twofaccounts->count(); + } + return new GroupResource($group); } @@ -108,11 +116,21 @@ class GroupController extends Controller * * @return \App\Api\v1\Resources\TwoFAccountCollection */ - public function accounts(Group $group) + public function accounts(Request $request, Group $group) { $this->authorize('view', $group); - return new TwoFAccountCollection($group->twofaccounts); + // group with id==0 is the 'All' virtual group that lists + // all the user's twofaccounts. From the db pov the accounts + // are not assigned to any group record. + if ($group->id === 0) { + $twofaccounts = $request->user()->twofaccounts; + } + else { + $twofaccounts = $group->twofaccounts; + } + + return new TwoFAccountCollection($twofaccounts); } /** diff --git a/app/Policies/GroupPolicy.php b/app/Policies/GroupPolicy.php index 74b979ea..66f8674f 100644 --- a/app/Policies/GroupPolicy.php +++ b/app/Policies/GroupPolicy.php @@ -28,7 +28,7 @@ class GroupPolicy */ public function view(User $user, Group $group) { - $can = $this->isOwnerOf($user, $group); + $can = $this->isOwnerOf($user, $group) || $group->id === 0; if (! $can) { Log::notice(sprintf('User ID #%s cannot view group %s (ID #%s)', $user->id, var_export($group->name, true), $group->id)); From f4ebbf401d96f58564baa27ed281548d0013a1a9 Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Wed, 19 Feb 2025 16:24:26 +0100 Subject: [PATCH 08/11] Remove useless GroupDeleting event --- app/Events/GroupDeleting.php | 28 ---------------------------- app/Models/Group.php | 2 -- 2 files changed, 30 deletions(-) delete mode 100644 app/Events/GroupDeleting.php diff --git a/app/Events/GroupDeleting.php b/app/Events/GroupDeleting.php deleted file mode 100644 index 2add46f8..00000000 --- a/app/Events/GroupDeleting.php +++ /dev/null @@ -1,28 +0,0 @@ -group = $group; - } -} diff --git a/app/Models/Group.php b/app/Models/Group.php index 1dc0a212..14c4635b 100644 --- a/app/Models/Group.php +++ b/app/Models/Group.php @@ -3,7 +3,6 @@ namespace App\Models; use App\Events\GroupDeleted; -use App\Events\GroupDeleting; use Database\Factories\GroupFactory; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; @@ -79,7 +78,6 @@ class Group extends Model * @var array */ protected $dispatchesEvents = [ - 'deleting' => GroupDeleting::class, 'deleted' => GroupDeleted::class, ]; From de3603fcfa4b806a07b0db463100308b3f823369 Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Wed, 19 Feb 2025 16:24:40 +0100 Subject: [PATCH 09/11] Update tests --- .../v1/Controllers/GroupControllerTest.php | 54 +++++++++++++++++++ .../v1/Controllers/GroupControllerTest.php | 6 ++- tests/Unit/Events/GroupDeletingTest.php | 25 --------- tests/Unit/GroupModelTest.php | 2 - .../DissociateTwofaccountFromGroupTest.php | 6 +-- 5 files changed, 61 insertions(+), 32 deletions(-) delete mode 100644 tests/Unit/Events/GroupDeletingTest.php diff --git a/tests/Api/v1/Controllers/GroupControllerTest.php b/tests/Api/v1/Controllers/GroupControllerTest.php index 969b5e43..4ed75e14 100644 --- a/tests/Api/v1/Controllers/GroupControllerTest.php +++ b/tests/Api/v1/Controllers/GroupControllerTest.php @@ -145,6 +145,26 @@ class GroupControllerTest extends FeatureTestCase ]); } + #[Test] + public function test_store_with_existing_group_name_returns_validation_error() + { + $this->actingAs($this->user, 'api-guard') + ->json('POST', '/api/v1/groups', [ + 'name' => $this->userGroupA->name, + ]) + ->assertStatus(422); + } + + #[Test] + public function test_store_with_all_group_name_returns_validation_error() + { + $this->actingAs($this->user, 'api-guard') + ->json('POST', '/api/v1/groups', [ + 'name' => __('commons.all'), + ]) + ->assertStatus(422); + } + #[Test] public function test_store_invalid_data_returns_validation_error() { @@ -193,6 +213,20 @@ class GroupControllerTest extends FeatureTestCase ]); } + #[Test] + public function test_show_missing_group_with_id_0_returns_the_virtual_all_group_resource() + { + $userTwofaccounts = $this->user->twofaccounts; + + $response = $this->actingAs($this->user, 'api-guard') + ->json('GET', '/api/v1/groups/0') + ->assertOk() + ->assertJsonFragment([ + 'name' => __('commons.all'), + 'twofaccounts_count' => $userTwofaccounts->count(), + ]); + } + #[Test] public function test_update_returns_updated_group_resource() { @@ -392,6 +426,15 @@ class GroupControllerTest extends FeatureTestCase ]); } + #[Test] + public function test_accounts_of_the_all_group_returns_user_twofaccounts_collection() + { + $response = $this->actingAs($this->user, 'api-guard') + ->json('GET', '/api/v1/groups/0/twofaccounts') + ->assertOk() + ->assertJsonCount(2); + } + /** * test Group deletion via API */ @@ -430,6 +473,17 @@ class GroupControllerTest extends FeatureTestCase ]); } + #[Test] + public function test_destroy_the_all_group_is_forbidden() + { + $response = $this->actingAs($this->anotherUser, 'api-guard') + ->json('DELETE', '/api/v1/groups/0') + ->assertForbidden() + ->assertJsonStructure([ + 'message', + ]); + } + #[Test] public function test_destroy_group_resets_user_preferences() { diff --git a/tests/Unit/Api/v1/Controllers/GroupControllerTest.php b/tests/Unit/Api/v1/Controllers/GroupControllerTest.php index d6c3c3bf..a2988897 100644 --- a/tests/Unit/Api/v1/Controllers/GroupControllerTest.php +++ b/tests/Unit/Api/v1/Controllers/GroupControllerTest.php @@ -88,10 +88,11 @@ class GroupControllerTest extends TestCase #[Test] public function test_show_returns_api_resource() { + $request = Mockery::mock(GroupStoreRequest::class); $controller = Mockery::mock(GroupController::class)->makePartial(); $group = Group::factory()->make(); - $response = $controller->show($group); + $response = $controller->show($request, $group); $this->assertInstanceOf(GroupResource::class, $response); } @@ -138,10 +139,11 @@ class GroupControllerTest extends TestCase #[Test] public function test_accounts_returns_api_resources() { + $request = Mockery::mock(GroupStoreRequest::class); $controller = Mockery::mock(GroupController::class)->makePartial(); $group = Group::factory()->make(); - $response = $controller->accounts($group); + $response = $controller->accounts($request, $group); $this->assertContainsOnlyInstancesOf(TwoFAccountReadResource::class, $response->collection); } diff --git a/tests/Unit/Events/GroupDeletingTest.php b/tests/Unit/Events/GroupDeletingTest.php deleted file mode 100644 index fe907de2..00000000 --- a/tests/Unit/Events/GroupDeletingTest.php +++ /dev/null @@ -1,25 +0,0 @@ -make(); - $event = new GroupDeleting($group); - - $this->assertSame($group, $event->group); - } -} diff --git a/tests/Unit/GroupModelTest.php b/tests/Unit/GroupModelTest.php index 6c651dc8..05716e9a 100644 --- a/tests/Unit/GroupModelTest.php +++ b/tests/Unit/GroupModelTest.php @@ -3,7 +3,6 @@ namespace Tests\Unit; use App\Events\GroupDeleted; -use App\Events\GroupDeleting; use App\Models\Group; use App\Models\TwoFAccount; use Illuminate\Database\Eloquent\Relations\BelongsTo; @@ -32,7 +31,6 @@ class GroupModelTest extends ModelTestCase 'user_id' => 'integer', ], [ - 'deleting' => GroupDeleting::class, 'deleted' => GroupDeleted::class, ] ); diff --git a/tests/Unit/Listeners/DissociateTwofaccountFromGroupTest.php b/tests/Unit/Listeners/DissociateTwofaccountFromGroupTest.php index 829494fa..177140fb 100644 --- a/tests/Unit/Listeners/DissociateTwofaccountFromGroupTest.php +++ b/tests/Unit/Listeners/DissociateTwofaccountFromGroupTest.php @@ -2,7 +2,7 @@ namespace Tests\Unit\Listeners; -use App\Events\GroupDeleting; +use App\Events\GroupDeleted; use App\Listeners\DissociateTwofaccountFromGroup; use Illuminate\Support\Facades\Event; use PHPUnit\Framework\Attributes\CoversClass; @@ -16,12 +16,12 @@ use Tests\TestCase; class DissociateTwofaccountFromGroupTest extends TestCase { #[Test] - public function test_DissociateTwofaccountFromGroup_listen_to_groupDeleting_event() + public function test_DissociateTwofaccountFromGroup_listen_to_groupDeleted_event() { Event::fake(); Event::assertListening( - GroupDeleting::class, + GroupDeleted::class, DissociateTwofaccountFromGroup::class ); } From 19f3a71c0347d0a445d091f8060299d77a9fd1e3 Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Mon, 24 Feb 2025 13:48:03 +0100 Subject: [PATCH 10/11] Move group->loadCount from the Assign void method to the caller --- app/Api/v1/Controllers/GroupController.php | 1 + app/Services/GroupService.php | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/Api/v1/Controllers/GroupController.php b/app/Api/v1/Controllers/GroupController.php index b42b002a..53bc544b 100644 --- a/app/Api/v1/Controllers/GroupController.php +++ b/app/Api/v1/Controllers/GroupController.php @@ -104,6 +104,7 @@ class GroupController extends Controller try { Groups::assign($validated['ids'], $request->user(), $group); + $group->loadCount('twofaccounts'); } catch (ModelNotFoundException $exc) { abort(404); } diff --git a/app/Services/GroupService.php b/app/Services/GroupService.php index c319da4e..882fb3d7 100644 --- a/app/Services/GroupService.php +++ b/app/Services/GroupService.php @@ -74,8 +74,6 @@ class GroupService Log::info(sprintf('Twofaccounts #%s assigned to group %s (ID #%s)', implode(',', $ids), var_export($group->name, true), $group->id)); }); - - $group->loadCount('twofaccounts'); } else { Log::info('Cannot find a group to assign the TwoFAccounts to'); } From c8b5bd32a6d829f0c33f4ad314aa1ac1e967bc1a Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Mon, 24 Feb 2025 15:13:13 +0100 Subject: [PATCH 11/11] Set multiple retries with lighter lock & Send 409 in case of deadlock --- app/Api/v1/Controllers/GroupController.php | 2 ++ app/Services/GroupService.php | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/Api/v1/Controllers/GroupController.php b/app/Api/v1/Controllers/GroupController.php index 53bc544b..2abdf7f9 100644 --- a/app/Api/v1/Controllers/GroupController.php +++ b/app/Api/v1/Controllers/GroupController.php @@ -107,6 +107,8 @@ class GroupController extends Controller $group->loadCount('twofaccounts'); } catch (ModelNotFoundException $exc) { abort(404); + } catch (\Throwable $th) { + abort(409, 'Conflict'); } return new GroupResource($group); diff --git a/app/Services/GroupService.php b/app/Services/GroupService.php index 882fb3d7..79d8f59c 100644 --- a/app/Services/GroupService.php +++ b/app/Services/GroupService.php @@ -59,7 +59,7 @@ class GroupService $ids = is_array($ids) ? $ids : [$ids]; DB::transaction(function () use ($group, $ids, $user) { - $group = Group::lockForUpdate()->find($group->id); + $group = Group::sharedLock()->find($group->id); $twofaccounts = TwoFAccount::sharedLock()->find($ids); if (! $group) { @@ -73,7 +73,7 @@ class GroupService $group->twofaccounts()->saveMany($twofaccounts); Log::info(sprintf('Twofaccounts #%s assigned to group %s (ID #%s)', implode(',', $ids), var_export($group->name, true), $group->id)); - }); + }, 5); } else { Log::info('Cannot find a group to assign the TwoFAccounts to'); }