Don’t strip comany association if company_id is passed to the user (old integrations)

This commit is contained in:
snipe
2026-06-03 12:30:08 +01:00
parent 371d44b2a7
commit 00d4d6c7a8
4 changed files with 97 additions and 21 deletions
+10 -6
View File
@@ -626,12 +626,16 @@ class UsersController extends Controller
$user->groups()->sync($request->input('groups'));
}
// Sync company memberships when company_ids[] or company_id is provided
if ($request->has('company_ids') || $request->filled('company_id')) {
$companyIds = array_filter(
(array) ($request->input('company_ids') ?? ($request->filled('company_id') ? [$request->input('company_id')] : []))
);
$user->syncCompaniesWithLogging(Company::getIdsForCurrentUser(array_map('intval', $companyIds)));
// company_ids (new format) = full replacement sync.
// Legacy company_id = add without removing other associations.
if ($request->has('company_ids')) {
$companyIds = array_filter(array_map('intval', (array) $request->input('company_ids')));
$user->syncCompaniesWithLogging(Company::getIdsForCurrentUser($companyIds));
} elseif ($request->filled('company_id')) {
$filtered = Company::getIdsForCurrentUser([(int) $request->input('company_id')]);
if (! empty($filtered)) {
$user->companies()->syncWithoutDetaching($filtered);
}
}
return response()->json(Helper::formatStandardApiResponse('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update')));
+11 -15
View File
@@ -626,30 +626,26 @@ class User extends SnipeModel implements AuthenticatableContract, AuthorizableCo
}
/**
* Returns whether an FMCS company check should block this user from receiving
* Returns whether an FMCS company check should allow this user to receive
* an asset that belongs to the given company.
*
* - If the user has no company associations at all: returns true (no restriction).
* - If the user has associations: returns true only when $companyId is among them.
*
* Checks both the primary company_id column and the many-to-many pivot table so
* that users assigned to multiple companies can receive assets from any of them.
*/
public function canReceiveFromCompany(int $companyId): bool
{
// Primary company matches
if (! is_null($this->company_id) && (int) $this->company_id === $companyId) {
// Query the pivot directly to avoid the Company model's FMCS global scope,
// which would restrict results to the current actor's visible companies.
$userCompanyIds = DB::table('company_user')
->where('user_id', $this->id)
->pluck('company_id');
if ($userCompanyIds->contains($companyId)) {
return true;
}
// Pivot company matches
if ($this->companies()->where('companies.id', $companyId)->exists()) {
return true;
}
// User has no company associations — don't enforce (mirrors legacy behaviour
// where a null company_id on the user skipped the FMCS check entirely).
if (is_null($this->company_id) && ! $this->companies()->exists()) {
// User has no company associations — don't enforce.
if ($userCompanyIds->isEmpty()) {
return true;
}
@@ -663,7 +659,7 @@ class User extends SnipeModel implements AuthenticatableContract, AuthorizableCo
*/
public function allCompanies(): Collection
{
return $this->companies->push($this->company)->filter()->unique('id')->values();
return $this->companies->unique('id')->values();
}
/**
@@ -119,6 +119,30 @@ class AssetCheckoutTest extends TestCase
Event::assertNotDispatched(CheckoutableCheckedOut::class);
}
public function test_can_checkout_to_user_with_multiple_companies_via_pivot_when_fmcs_enabled()
{
$this->settings->enableMultipleFullCompanySupport();
$companyA = Company::factory()->create();
$companyB = Company::factory()->create();
// User's primary company is A but is also assigned to B via pivot
$user = User::factory()->for($companyA)->create();
$user->companies()->sync([$companyA->id, $companyB->id]);
// Asset belongs to company B
$asset = Asset::factory()->for($companyB)->create();
$this->actingAs(User::factory()->superuser()->create())
->post(route('hardware.checkout.store', $asset), [
'checkout_to_type' => 'user',
'assigned_user' => $user->id,
])
->assertRedirect();
Event::assertDispatched(CheckoutableCheckedOut::class);
}
public function test_page_renders()
{
$this->actingAs(User::factory()->superuser()->create())
@@ -214,6 +214,58 @@ class UserCompanyMembershipTest extends TestCase
$this->assertTrue($user->companies->contains($company));
}
public function test_legacy_company_id_on_update_adds_without_removing_other_associations()
{
// An older integration that hasn't been updated still sends company_id (scalar).
// If the user already belongs to multiple companies via the pivot, the legacy
// company_id should be added (if not already present) without stripping others.
[$companyA, $companyB, $companyC] = Company::factory()->count(3)->create();
$user = User::factory()->create();
$user->companies()->sync([$companyA->id, $companyB->id]);
$actor = User::factory()->superuser()->create();
$this->actingAsForApi($actor)
->patchJson(route('api.users.update', $user), [
'first_name' => $user->first_name,
'last_name' => $user->last_name,
'username' => $user->username,
'company_id' => $companyC->id,
])
->assertOk()
->assertStatusMessageIs('success');
$user->refresh();
$this->assertCount(3, $user->companies, 'All three companies should be present after legacy company_id update');
$this->assertTrue($user->companies->contains($companyA), 'companyA should not have been stripped');
$this->assertTrue($user->companies->contains($companyB), 'companyB should not have been stripped');
$this->assertTrue($user->companies->contains($companyC), 'companyC should have been added');
}
public function test_legacy_company_id_on_update_is_idempotent_when_already_a_member()
{
[$companyA, $companyB] = Company::factory()->count(2)->create();
$user = User::factory()->create();
$user->companies()->sync([$companyA->id, $companyB->id]);
$actor = User::factory()->superuser()->create();
$this->actingAsForApi($actor)
->patchJson(route('api.users.update', $user), [
'first_name' => $user->first_name,
'last_name' => $user->last_name,
'username' => $user->username,
'company_id' => $companyA->id,
])
->assertOk()
->assertStatusMessageIs('success');
$user->refresh();
$this->assertCount(2, $user->companies, 'Company count should not change when company_id already in pivot');
}
public function test_post_with_invalid_company_ids_returns_error()
{
$actor = User::factory()->superuser()->create();