mirror of
https://github.com/InvoiceShelf/InvoiceShelf.git
synced 2026-04-19 03:04:05 +00:00
Fix CustomerPolicy missing hasCompany() check (IDOR) (#604)
* Fix CustomerPolicy missing hasCompany() check (cross-company IDOR) Add $user->hasCompany($customer->company_id) check to view, update, delete, restore, and forceDelete methods in CustomerPolicy, matching the pattern used by all other policies (InvoicePolicy, PaymentPolicy, EstimatePolicy, etc.). Without this check, a user in Company A with view-customer ability could access customers belonging to Company B by providing the target customer's ID. Add cross-company authorization tests to verify the fix. Closes #565 * Scope bulk delete to current company to prevent cross-company deletion Filter customer IDs through whereCompany() before passing to deleteCustomers(), ensuring users cannot delete customers belonging to other companies via the bulk delete endpoint.
This commit is contained in:
committed by
GitHub
parent
25986b7bd5
commit
defbfc6406
@@ -92,7 +92,11 @@ class CustomersController extends Controller
|
||||
{
|
||||
$this->authorize('delete multiple customers');
|
||||
|
||||
Customer::deleteCustomers($request->ids);
|
||||
$ids = Customer::whereCompany()
|
||||
->whereIn('id', $request->ids)
|
||||
->pluck('id');
|
||||
|
||||
Customer::deleteCustomers($ids);
|
||||
|
||||
return response()->json([
|
||||
'success' => true,
|
||||
|
||||
@@ -32,7 +32,7 @@ class CustomerPolicy
|
||||
*/
|
||||
public function view(User $user, Customer $customer): bool
|
||||
{
|
||||
if (BouncerFacade::can('view-customer', $customer)) {
|
||||
if (BouncerFacade::can('view-customer', $customer) && $user->hasCompany($customer->company_id)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -60,7 +60,7 @@ class CustomerPolicy
|
||||
*/
|
||||
public function update(User $user, Customer $customer): bool
|
||||
{
|
||||
if (BouncerFacade::can('edit-customer', $customer)) {
|
||||
if (BouncerFacade::can('edit-customer', $customer) && $user->hasCompany($customer->company_id)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -74,7 +74,7 @@ class CustomerPolicy
|
||||
*/
|
||||
public function delete(User $user, Customer $customer): bool
|
||||
{
|
||||
if (BouncerFacade::can('delete-customer', $customer)) {
|
||||
if (BouncerFacade::can('delete-customer', $customer) && $user->hasCompany($customer->company_id)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -88,7 +88,7 @@ class CustomerPolicy
|
||||
*/
|
||||
public function restore(User $user, Customer $customer): bool
|
||||
{
|
||||
if (BouncerFacade::can('delete-customer', $customer)) {
|
||||
if (BouncerFacade::can('delete-customer', $customer) && $user->hasCompany($customer->company_id)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -102,7 +102,7 @@ class CustomerPolicy
|
||||
*/
|
||||
public function forceDelete(User $user, Customer $customer): bool
|
||||
{
|
||||
if (BouncerFacade::can('delete-customer', $customer)) {
|
||||
if (BouncerFacade::can('delete-customer', $customer) && $user->hasCompany($customer->company_id)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
use App\Http\Controllers\V1\Admin\Customer\CustomersController;
|
||||
use App\Http\Requests\CustomerRequest;
|
||||
use App\Models\Company;
|
||||
use App\Models\Customer;
|
||||
use App\Models\Invoice;
|
||||
use App\Models\User;
|
||||
@@ -157,3 +158,40 @@ test('delete multiple customer', function () {
|
||||
'success' => true,
|
||||
]);
|
||||
});
|
||||
|
||||
test('cannot view customer from another company', function () {
|
||||
$otherCompany = Company::factory()->create();
|
||||
$otherCustomer = Customer::factory()->create([
|
||||
'company_id' => $otherCompany->id,
|
||||
]);
|
||||
|
||||
getJson("api/v1/customers/{$otherCustomer->id}")
|
||||
->assertForbidden();
|
||||
});
|
||||
|
||||
test('cannot update customer from another company', function () {
|
||||
$otherCompany = Company::factory()->create();
|
||||
$otherCustomer = Customer::factory()->create([
|
||||
'company_id' => $otherCompany->id,
|
||||
]);
|
||||
|
||||
putJson("api/v1/customers/{$otherCustomer->id}", [
|
||||
'name' => 'Hacked Name',
|
||||
'email' => 'hacked@example.com',
|
||||
])->assertForbidden();
|
||||
});
|
||||
|
||||
test('cannot bulk delete customer from another company', function () {
|
||||
$otherCompany = Company::factory()->create();
|
||||
$otherCustomer = Customer::factory()->create([
|
||||
'company_id' => $otherCompany->id,
|
||||
]);
|
||||
|
||||
postJson('api/v1/customers/delete', [
|
||||
'ids' => [$otherCustomer->id],
|
||||
])->assertOk();
|
||||
|
||||
$this->assertDatabaseHas('customers', [
|
||||
'id' => $otherCustomer->id,
|
||||
]);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user