diff --git a/app/Api/v1/Controllers/GroupController.php b/app/Api/v1/Controllers/GroupController.php index 3dc24f07..2abdf7f9 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 @@ -59,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); } @@ -93,7 +102,14 @@ class GroupController extends Controller $validated = $request->validated(); - Groups::assign($validated['ids'], $request->user(), $group); + try { + Groups::assign($validated['ids'], $request->user(), $group); + $group->loadCount('twofaccounts'); + } catch (ModelNotFoundException $exc) { + abort(404); + } catch (\Throwable $th) { + abort(409, 'Conflict'); + } return new GroupResource($group); } @@ -103,11 +119,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/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/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/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/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/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 67b06aa5..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, ]; @@ -98,9 +96,31 @@ 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)); - }); + } + + /** + * 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); + } } /** 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)); 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 => [ diff --git a/app/Services/GroupService.php b/app/Services/GroupService.php index a7898b4d..79d8f59c 100644 --- a/app/Services/GroupService.php +++ b/app/Services/GroupService.php @@ -7,6 +7,8 @@ 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; 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 \Illuminate\Database\Eloquent\ModelNotFoundException */ public static function assign($ids, User $user, mixed $targetGroup = null) : void { @@ -53,17 +56,24 @@ 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) { + $group = Group::sharedLock()->find($group->id); + $twofaccounts = TwoFAccount::sharedLock()->find($ids); + + if (! $group) { + throw new ModelNotFoundException('group no longer exists'); + } - $group->twofaccounts()->saveMany($twofaccounts); - $group->loadCount('twofaccounts'); + if ($user->cannot('updateEach', [(new TwoFAccount), $twofaccounts])) { + throw new AuthorizationException; + } - Log::info(sprintf('Twofaccounts #%s assigned to group %s (ID #%s)', implode(',', $ids), var_export($group->name, true), $group->id)); + $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'); } 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') } diff --git a/resources/lang/en/errors.php b/resources/lang/en/errors.php index c3ffffb2..3aa6e35b 100644 --- a/resources/lang/en/errors.php +++ b/resources/lang/en/errors.php @@ -74,5 +74,6 @@ 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.', - 'failed_to_retrieve_app_settings' => 'Failed to retrieve application settings' + 'failed_to_retrieve_app_settings' => 'Failed to retrieve application settings', + 'reserved_name_please_choose_something_else' => 'Reserved name, please choose something else', ]; \ No newline at end of file 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 ); }