Merge branch 'Fix-CWE-362' into dev

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);
     }
 
     /**

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();
         }

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<string, string>
+     */
+    public function messages(): array
+    {
+        return [
+            'name.not_in' => __('errors.reserved_name_please_choose_something_else'),
+        ];
+    }
 }

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));
     }
 }

app/Events/GroupDeleting.php

@@ -1,28 +0,0 @@
-<?php
-
-namespace App\Events;
-
-use App\Models\Group;
-use Illuminate\Broadcasting\InteractsWithSockets;
-use Illuminate\Foundation\Events\Dispatchable;
-use Illuminate\Queue\SerializesModels;
-
-class GroupDeleting
-{
-    use Dispatchable, InteractsWithSockets, SerializesModels;
-
-    /**
-     * @var \App\Models\Group
-     */
-    public $group;
-
-    /**
-     * Create a new event instance.
-     *
-     * @return void
-     */
-    public function __construct(Group $group)
-    {
-        $this->group = $group;
-    }
-}

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(

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);
+        }
     }
 
     /**

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));

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 => [

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');
         }

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')
     }

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',
 ];

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()
     {

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);
     }

tests/Unit/Events/GroupDeletingTest.php

@@ -1,25 +0,0 @@
-<?php
-
-namespace Tests\Unit\Events;
-
-use App\Events\GroupDeleting;
-use App\Models\Group;
-use PHPUnit\Framework\Attributes\CoversClass;
-use PHPUnit\Framework\Attributes\Test;
-use Tests\TestCase;
-
-/**
- * GroupDeletingTest test class
- */
-#[CoversClass(GroupDeleting::class)]
-class GroupDeletingTest extends TestCase
-{
-    #[Test]
-    public function test_event_constructor()
-    {
-        $group = Group::factory()->make();
-        $event = new GroupDeleting($group);
-
-        $this->assertSame($group, $event->group);
-    }
-}

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,
             ]
         );

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
         );
     }