Merge pull request #15516 from snipe/fixes/fmcs_edit_user

Fixed check for outside assets on user update validation
snipe · Sep 17, 2024 · 6c996b7 · 6c996b7
2 parents fe5fc6e + 9e957ba
commit 6c996b7
Show file tree

Hide file tree

Showing 5 changed files with 105 additions and 8 deletions.
diff --git a/app/Rules/UserCannotSwitchCompaniesIfItemsAssigned.php b/app/Rules/UserCannotSwitchCompaniesIfItemsAssigned.php
@@ -16,8 +16,14 @@ class UserCannotSwitchCompaniesIfItemsAssigned implements ValidationRule
     public function validate(string $attribute, mixed $value, Closure $fail): void
     {
         $user = User::find(request()->route('user')->id);
-        if (($value) && ($user->allAssignedCount() > 0) && (Setting::getSettings()->full_multiple_companies_support)) {
-            $fail(trans('admin/users/message.error.multi_company_items_assigned'));
+
+        if (($value) && ($user->allAssignedCount() > 0) && (Setting::getSettings()->full_multiple_companies_support=='1')) {
+
+            // Check for assets with a different company_id than the selected company_id
+            $user_assets = $user->assets()->where('assets.company_id', '!=', $value)->count();
+            if ($user_assets > 0) {
+                $fail(trans('admin/users/message.error.multi_company_items_assigned'));
+            }
         }
     }
 }
diff --git a/resources/lang/en-US/admin/users/message.php b/resources/lang/en-US/admin/users/message.php
@@ -53,7 +53,7 @@
         'ldap_could_not_search' => 'Could not search the LDAP server. Please check your LDAP server configuration in the LDAP config file. <br>Error from LDAP Server:',
         'ldap_could_not_get_entries' => 'Could not get entries from the LDAP server. Please check your LDAP server configuration in the LDAP config file. <br>Error from LDAP Server:',
         'password_ldap' => 'The password for this account is managed by LDAP/Active Directory. Please contact your IT department to change your password. ',
-        'multi_company_items_assigned' => 'This user has items assigned, please check them in before moving companies.'
+        'multi_company_items_assigned' => 'This user has items assigned that belong to a different company. Please check them in or edit their company.'
     ),
 
     'deletefile' => array(

diff --git a/resources/views/users/view.blade.php b/resources/views/users/view.blade.php
@@ -330,7 +330,13 @@
                         {{ trans('general.company') }}
                       </div>
                       <div class="col-md-9">
-                        {{ $user->company->name }}
+                          @can('view', 'App\Models\Company')
+                            <a href="{{ route('companies.show', $user->company->id) }}">
+                                {{ $user->company->name }}
+                            </a>
+                              @else
+                              {{ $user->company->name }}
+                            @endcan
                       </div>
 
                     </div>

diff --git a/tests/Feature/Users/Api/UpdateUserTest.php b/tests/Feature/Users/Api/UpdateUserTest.php
@@ -422,7 +422,7 @@ public function testMultipleGroupsUpdateBySuperUser()
         $this->assertTrue($user->refresh()->groups->contains($groupB));
     }
 
-    public function testMultiCompanyUserCannotBeMovedIfHasAsset()
+    public function testMultiCompanyUserCannotBeMovedIfHasAssetInDifferentCompany()
     {
         $this->settings->enableMultipleFullCompanySupport();
 
@@ -434,7 +434,9 @@ public function testMultiCompanyUserCannotBeMovedIfHasAsset()
         ]);
         $superUser = User::factory()->superuser()->create();
 
-        $asset = Asset::factory()->create();
+        $asset = Asset::factory()->create([
+            'company_id' => $companyA->id,
+        ]);
 
         // no assets assigned, therefore success
         $this->actingAsForApi($superUser)->patchJson(route('api.users.update', $user), [
@@ -465,4 +467,49 @@ public function testMultiCompanyUserCannotBeMovedIfHasAsset()
         ])->assertStatusMessageIs('error');
     }
 
+    public function testMultiCompanyUserCanBeUpdatedIfHasAssetInSameCompany()
+    {
+        $this->settings->enableMultipleFullCompanySupport();
+
+        $companyA = Company::factory()->create();
+        $companyB = Company::factory()->create();
+
+        $user = User::factory()->create([
+            'company_id' => $companyA->id,
+        ]);
+        $superUser = User::factory()->superuser()->create();
+
+        $asset = Asset::factory()->create([
+            'company_id' => $companyA->id,
+        ]);
+
+        // no assets assigned from other company, therefore success
+        $this->actingAsForApi($superUser)->patchJson(route('api.users.update', $user), [
+            'username'   => 'test',
+            'company_id' => $companyB->id,
+        ])->assertStatusMessageIs('success');
+
+        // same test but PUT
+        $this->actingAsForApi($superUser)->putJson(route('api.users.update', $user), [
+            'username'   => 'test',
+            'first_name' => 'Test',
+            'company_id' => $companyB->id,
+        ])->assertStatusMessageIs('success');
+
+        $asset->checkOut($user, $superUser);
+
+        // asset assigned from other company, therefore error
+        $this->actingAsForApi($superUser)->patchJson(route('api.users.update', $user), [
+            'username'   => 'test',
+            'company_id' => $companyB->id,
+        ])->assertStatusMessageIs('error');
+
+        // same test but PUT
+        $this->actingAsForApi($superUser)->putJson(route('api.users.update', $user), [
+            'username'   => 'test',
+            'first_name' => 'Test',
+            'company_id' => $companyB->id,
+        ])->assertStatusMessageIs('error');
+    }
+
 }
diff --git a/tests/Feature/Users/Ui/UpdateUserTest.php b/tests/Feature/Users/Ui/UpdateUserTest.php
@@ -82,7 +82,7 @@ public function testUsersUpdatingThemselvesDoNotDeactivateTheirAccount()
         $this->assertEquals(1, $admin->refresh()->activated);
     }
 
-    public function testMultiCompanyUserCannotBeMovedIfHasAsset()
+    public function testMultiCompanyUserCannotBeMovedIfHasAssetInDifferentCompany()
     {
         $this->settings->enableMultipleFullCompanySupport();
 
@@ -94,7 +94,9 @@ public function testMultiCompanyUserCannotBeMovedIfHasAsset()
         ]);
         $superUser = User::factory()->superuser()->create();
 
-        $asset = Asset::factory()->create();
+        $asset = Asset::factory()->create([
+            'company_id' => $companyA->id,
+        ]);
 
         // no assets assigned, therefore success
         $this->actingAs($superUser)->put(route('users.update', $user), [
@@ -116,4 +118,40 @@ public function testMultiCompanyUserCannotBeMovedIfHasAsset()
 
         $this->followRedirects($response)->assertSee('error');
     }
+
+    public function testMultiCompanyUserCanBeUpdatedIfHasAssetInSameCompany()
+    {
+        $this->settings->enableMultipleFullCompanySupport();
+
+        $companyA = Company::factory()->create();
+
+        $user = User::factory()->create([
+            'company_id' => $companyA->id,
+        ]);
+        $superUser = User::factory()->superuser()->create();
+
+        $asset = Asset::factory()->create([
+            'company_id' => $companyA->id,
+        ]);
+
+        // no assets assigned, therefore success
+        $this->actingAs($superUser)->put(route('users.update', $user), [
+            'first_name'      => 'test',
+            'username'        => 'test',
+            'company_id'      => $companyA->id,
+            'redirect_option' => 'index'
+        ])->assertRedirect(route('users.index'));
+
+        $asset->checkOut($user, $superUser);
+
+        // asset assigned, therefore error
+        $response = $this->actingAs($superUser)->patchJson(route('users.update', $user), [
+            'first_name'      => 'test',
+            'username'        => 'test',
+            'company_id'      => $companyA->id,
+            'redirect_option' => 'index'
+        ]);
+
+        $this->followRedirects($response)->assertSee('success');
+    }
 }