Merge pull request #18910 from grokability/audit-visibility-fix

Fixed #18896 - Audit visibility fix
This commit is contained in:
snipe
2026-05-04 12:14:05 +01:00
committed by GitHub
6 changed files with 696 additions and 21 deletions
+83 -18
View File
@@ -9,9 +9,11 @@ use App\Presenters\ActionlogPresenter;
use App\Presenters\Presentable;
use Carbon\Carbon;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\MorphTo;
use Illuminate\Database\Eloquent\Relations\Relation;
use Illuminate\Database\Eloquent\SoftDeletes;
use Illuminate\Support\Facades\Schema;
use Illuminate\Support\Str;
/**
@@ -53,6 +55,13 @@ class Actionlog extends SnipeModel
use Searchable;
/**
* Cache whether a model table has a company_id column.
*
* @var array<string, bool>
*/
protected static array $companyColumnCache = [];
/**
* The attributes that should be included when searching the model.
*
@@ -116,25 +125,81 @@ class Actionlog extends SnipeModel
public static function boot()
{
parent::boot();
static::creating(
function (self $actionlog) {
// If the admin is a superadmin, let's see if the target instead has a company.
if (auth()->user() && auth()->user()->isSuperUser()) {
if ($actionlog->target) {
$actionlog->company_id = $actionlog->target->company_id;
} elseif ($actionlog->item) {
$actionlog->company_id = $actionlog->item->company_id;
}
} elseif (auth()->user() && auth()->user()->company) {
$actionlog->company_id = auth()->user()->company_id;
}
if ($actionlog->action_date == '') {
$actionlog->action_date = Carbon::now();
}
static::creating(function (self $actionlog): void {
// Only resolve company_id if it was never explicitly set by the caller.
// Using array_key_exists on getRawOriginal() / getAttributes() lets us
// distinguish "was set to null intentionally" from "was never set at all".
if (! array_key_exists('company_id', $actionlog->getAttributes())) {
$actionlog->company_id = static::resolveCompanyIdFromAttributes(
$actionlog->target_type,
$actionlog->target_id,
$actionlog->item_type,
$actionlog->item_id,
);
}
);
if ($actionlog->action_date == '') {
$actionlog->action_date = Carbon::now();
}
});
}
/**
* Resolve the company_id for a new action log by querying the item model
* directly, bypassing all global scopes to avoid FMCS filtering issues.
*
* We intentionally prefer the item (asset, license, etc.) over the target
* (user, location) because FMCS visibility is based on who *owns* the item,
* not who it was checked out to. If the item has no company_id we fall back
* to the target so that logs on unowned items still get a company stamp where
* possible.
*
* This has to include an exception for the asset models table, since they are
* not company-constrained (on purpose.)
*/
protected static function resolveCompanyIdFromAttributes(
?string $targetType,
?int $targetId,
?string $itemType,
?int $itemId,
): ?int {
// Prefer the item (the thing being acted upon) for FMCS ownership.
$companyId = static::resolveCompanyIdFromModelClass($itemType, $itemId);
if ($companyId !== null) {
return $companyId;
}
// Fall back to target only when the item has no company_id.
return static::resolveCompanyIdFromModelClass($targetType, $targetId);
}
/**
* Resolve company_id from a model class and ID, but only if that model's
* table has a company_id column.
*/
protected static function resolveCompanyIdFromModelClass(?string $modelClass, ?int $id): ?int
{
if (! $modelClass || ! $id || ! class_exists($modelClass) || ! is_subclass_of($modelClass, Model::class)) {
return null;
}
/** @var Model $instance */
$instance = app($modelClass);
$table = $instance->getTable();
$hasCompanyColumn = static::$companyColumnCache[$table]
??= Schema::hasColumn($table, 'company_id');
if (! $hasCompanyColumn) {
return null;
}
return $modelClass::withoutGlobalScopes()
->whereKey($id)
->value('company_id');
}
/**
+21
View File
@@ -177,6 +177,7 @@ trait Loggable
$log->note = $note;
$log->action_date = $action_date;
$log->quantity = $quantity;
$log->company_id = $this->resolveLoggableCompanyId();
$changed = [];
$array_to_flip = array_keys($fields_array);
@@ -221,6 +222,22 @@ trait Loggable
return $log;
}
/**
* Resolve the company_id that should be stamped on an action log entry.
*
* LicenseSeat does not carry a company_id directly — it belongs to a License,
* so we fetch the parent license's company_id in that case. All other models
* that use the Loggable trait have a company_id column directly.
*/
private function resolveLoggableCompanyId(): ?int
{
if (static::class === LicenseSeat::class) {
return $this->license?->company_id;
}
return $this->company_id ?? null;
}
/**
* @author Daniel Meltzer <dmeltzer.devel@gmail.com>
*
@@ -267,6 +284,7 @@ trait Loggable
$log->location_id = null;
$log->note = $note;
$log->action_date = $action_date;
$log->company_id = $this->resolveLoggableCompanyId();
if (! $action_date) {
$log->action_date = date('Y-m-d H:i:s');
@@ -383,6 +401,8 @@ trait Loggable
$log->created_by = auth()->id();
$log->filename = $filename;
$log->action_date = date('Y-m-d H:i:s');
// Explicitly stamp company_id from the item being audited so FMCS scoping works correctly.
$log->company_id = $this->resolveLoggableCompanyId();
$log->logaction('audit');
$params = [
@@ -468,6 +488,7 @@ trait Loggable
$log->action_date = date('Y-m-d H:i:s');
$log->note = $note;
$log->created_by = $created_by;
$log->company_id = $this->resolveLoggableCompanyId();
$log->logaction('create');
$log->save();
@@ -0,0 +1,74 @@
<?php
use Illuminate\Database\Migrations\Migration;
use Illuminate\Support\Facades\DB;
/**
* Backfill action_logs.company_id only for legacy asset audits where the
* value is currently NULL.
*
* Audits are only recorded on assets, so this migration intentionally scopes
* to action_type='audit' and item_type=App\Models\Asset.
*
* Rows whose asset genuinely has no company (assets.company_id IS NULL) are
* left as NULL.
*/
return new class extends Migration
{
private const ASSET_CLASS = 'App\\Models\\Asset';
private const AUDIT_ACTION = 'audit';
public function up(): void
{
$this->updateAssetAuditLogs(DB::getDriverName());
}
public function down(): void
{
// This backfill is intentionally non-reversible — we cannot know which
// rows were NULL before the migration ran vs which were backfilled.
}
/**
* Stamp company_id for legacy audit rows tied to assets.
*/
private function updateAssetAuditLogs(string $driver): void
{
if ($driver === 'mysql' || $driver === 'mariadb') {
// MySQL/MariaDB supports UPDATE ... JOIN directly
DB::statement('
UPDATE action_logs al
INNER JOIN assets src
ON src.id = al.item_id
AND src.company_id IS NOT NULL
SET al.company_id = src.company_id
WHERE al.action_type = ?
AND al.item_type = ?
AND al.company_id IS NULL
AND al.deleted_at IS NULL
', [self::AUDIT_ACTION, self::ASSET_CLASS]);
} else {
// SQLite / PostgreSQL: use a correlated subquery update
DB::statement('
UPDATE action_logs
SET company_id = (
SELECT src.company_id
FROM assets src
WHERE src.id = action_logs.item_id
AND src.company_id IS NOT NULL
LIMIT 1
)
WHERE action_type = ?
AND item_type = ?
AND company_id IS NULL
AND deleted_at IS NULL
AND EXISTS (
SELECT 1 FROM assets src2
WHERE src2.id = action_logs.item_id
AND src2.company_id IS NOT NULL
)
', [self::AUDIT_ACTION, self::ASSET_CLASS]);
}
}
};
@@ -0,0 +1,147 @@
<?php
namespace Tests\Feature\ActionLogs;
use App\Models\Asset;
use App\Models\Company;
use Illuminate\Support\Facades\DB;
use Tests\TestCase;
/**
* Verifies the backfill migration logic that stamps action_logs.company_id
* for legacy asset audit rows where the company is currently NULL.
*
* Rather than running the migration class directly (which would conflict with
* LazilyRefreshDatabase), we replicate the exact UPDATE SQL used by the
* migration and assert on the resulting rows.
*/
class ActionlogCompanyIdBackfillTest extends TestCase
{
private const ASSET_CLASS = 'App\\Models\\Asset';
private const AUDIT_ACTION = 'audit';
/**
* Insert an action_log row bypassing Eloquent so that company_id stays NULL,
* simulating a historical record written before FMCS stamping was added.
*/
private function insertLegacyLog(array $attributes): int
{
return DB::table('action_logs')->insertGetId(array_merge([
'action_type' => self::AUDIT_ACTION,
'item_type' => self::ASSET_CLASS,
'item_id' => null,
'company_id' => null,
'created_at' => now(),
'updated_at' => now(),
], $attributes));
}
/**
* Run the same UPDATE logic the migration uses.
*/
private function runBackfill(): void
{
$driver = DB::getDriverName();
if ($driver === 'mysql' || $driver === 'mariadb') {
DB::statement('
UPDATE action_logs al
INNER JOIN assets src ON src.id = al.item_id AND src.company_id IS NOT NULL
SET al.company_id = src.company_id
WHERE al.action_type = ?
AND al.item_type = ?
AND al.company_id IS NULL
AND al.deleted_at IS NULL
', [self::AUDIT_ACTION, self::ASSET_CLASS]);
} else {
DB::statement('
UPDATE action_logs
SET company_id = (
SELECT src.company_id FROM assets src
WHERE src.id = action_logs.item_id AND src.company_id IS NOT NULL
LIMIT 1
)
WHERE action_type = ?
AND item_type = ?
AND company_id IS NULL
AND deleted_at IS NULL
AND EXISTS (
SELECT 1 FROM assets src2
WHERE src2.id = action_logs.item_id AND src2.company_id IS NOT NULL
)
', [self::AUDIT_ACTION, self::ASSET_CLASS]);
}
}
// ──────────────────────────────────────────────────────────────────────────
public function test_backfill_populates_company_id_for_asset_audit(): void
{
$company = Company::factory()->create();
$asset = Asset::factory()->create(['company_id' => $company->id]);
$logId = $this->insertLegacyLog(['item_type' => self::ASSET_CLASS, 'item_id' => $asset->id]);
$this->runBackfill();
$this->assertDatabaseHas('action_logs', [
'id' => $logId,
'company_id' => $company->id,
]);
}
public function test_backfill_does_not_overwrite_existing_company_id(): void
{
$company = Company::factory()->create();
$otherCompany = Company::factory()->create();
$asset = Asset::factory()->create(['company_id' => $otherCompany->id]);
// Row already has a company_id — the backfill must leave it alone
$logId = $this->insertLegacyLog([
'item_type' => self::ASSET_CLASS,
'item_id' => $asset->id,
'company_id' => $company->id,
]);
$this->runBackfill();
$this->assertDatabaseHas('action_logs', [
'id' => $logId,
'company_id' => $company->id, // unchanged
]);
}
public function test_backfill_leaves_null_when_item_has_no_company(): void
{
$asset = Asset::factory()->create(['company_id' => null]);
$logId = $this->insertLegacyLog(['item_type' => self::ASSET_CLASS, 'item_id' => $asset->id]);
$this->runBackfill();
$this->assertDatabaseHas('action_logs', [
'id' => $logId,
'company_id' => null, // item has no company, so log stays null
]);
}
public function test_backfill_ignores_non_audit_action_logs(): void
{
$company = Company::factory()->create();
$asset = Asset::factory()->create(['company_id' => $company->id]);
$logId = $this->insertLegacyLog([
'action_type' => 'checkout',
'item_type' => self::ASSET_CLASS,
'item_id' => $asset->id,
]);
$this->runBackfill();
$this->assertDatabaseHas('action_logs', [
'id' => $logId,
'company_id' => null,
]);
}
}
@@ -0,0 +1,364 @@
<?php
namespace Tests\Feature\ActionLogs;
use App\Models\Accessory;
use App\Models\Asset;
use App\Models\AssetModel;
use App\Models\Company;
use App\Models\Component;
use App\Models\Consumable;
use App\Models\License;
use App\Models\LicenseSeat;
use App\Models\Location;
use App\Models\Statuslabel;
use App\Models\User;
use Tests\TestCase;
/**
* Confirms that action_logs.company_id is correctly stamped for every
* logged event so that FMCS scoping works correctly.
*
* Each test creates an item belonging to a specific company and triggers the
* relevant action, then asserts that the resulting action log row carries the
* same company_id as the item.
*/
class ActionlogCompanyIdTest extends TestCase
{
// -------------------------------------------------------------------------
// Asset events
// -------------------------------------------------------------------------
public function test_asset_audit_log_stores_the_assets_company_id(): void
{
$company = Company::factory()->create();
$asset = Asset::factory()->create(['company_id' => $company->id]);
$admin = User::factory()->superuser()->create();
$this->actingAsForApi($admin)
->postJson(route('api.asset.audit', $asset), ['note' => 'audit test'])
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => Asset::class,
'item_id' => $asset->id,
'action_type' => 'audit',
'company_id' => $company->id,
]);
}
public function test_asset_checkout_to_user_log_stores_the_assets_company_id(): void
{
$company = Company::factory()->create();
$asset = Asset::factory()->create(['company_id' => $company->id]);
$user = User::factory()->create();
$admin = User::factory()->superuser()->create();
$this->actingAsForApi($admin)
->postJson(route('api.asset.checkout', $asset), [
'checkout_to_type' => 'user',
'assigned_user' => $user->id,
'status_id' => $asset->status_id,
])
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => Asset::class,
'item_id' => $asset->id,
'action_type' => 'checkout',
'company_id' => $company->id,
]);
}
public function test_asset_checkout_to_location_log_stores_the_assets_company_id(): void
{
$company = Company::factory()->create();
$asset = Asset::factory()->create(['company_id' => $company->id]);
$location = Location::factory()->create();
$admin = User::factory()->superuser()->create();
$this->actingAsForApi($admin)
->postJson(route('api.asset.checkout', $asset), [
'checkout_to_type' => 'location',
'assigned_location' => $location->id,
'status_id' => $asset->status_id,
])
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => Asset::class,
'item_id' => $asset->id,
'action_type' => 'checkout',
'company_id' => $company->id,
]);
}
public function test_asset_checkin_log_stores_the_assets_company_id(): void
{
$company = Company::factory()->create();
$asset = Asset::factory()->assignedToUser()->create(['company_id' => $company->id]);
$admin = User::factory()->superuser()->create();
$this->actingAsForApi($admin)
->postJson(route('api.asset.checkin', $asset))
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => Asset::class,
'item_id' => $asset->id,
'action_type' => 'checkin from',
'company_id' => $company->id,
]);
}
public function test_asset_create_log_stores_the_assets_company_id(): void
{
$company = Company::factory()->create();
$admin = User::factory()->superuser()->create();
$model = AssetModel::factory()->create();
$status = Statuslabel::factory()->readyToDeploy()->create();
$tag = 'COMPANY-ID-TEST-'.uniqid();
$this->actingAsForApi($admin)
->postJson(route('api.assets.store'), [
'asset_tag' => $tag,
'model_id' => $model->id,
'status_id' => $status->id,
'company_id' => $company->id,
])
->assertStatusMessageIs('success');
$asset = Asset::where('asset_tag', $tag)->firstOrFail();
$this->assertDatabaseHas('action_logs', [
'item_type' => Asset::class,
'item_id' => $asset->id,
'action_type' => 'create',
'company_id' => $company->id,
]);
}
// -------------------------------------------------------------------------
// Accessory events
// -------------------------------------------------------------------------
public function test_accessory_checkout_log_stores_the_accessorys_company_id(): void
{
$company = Company::factory()->create();
$accessory = Accessory::factory()->create(['company_id' => $company->id]);
$user = User::factory()->create();
$admin = User::factory()->superuser()->create();
$this->actingAsForApi($admin)
->postJson(route('api.accessories.checkout', $accessory), [
'assigned_user' => $user->id,
'checkout_to_type' => 'user',
])
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => Accessory::class,
'item_id' => $accessory->id,
'action_type' => 'checkout',
'company_id' => $company->id,
]);
}
public function test_accessory_checkin_log_stores_the_accessorys_company_id(): void
{
$company = Company::factory()->create();
$accessory = Accessory::factory()->checkedOutToUser()->create(['company_id' => $company->id]);
$admin = User::factory()->superuser()->create();
$checkoutRecord = $accessory->checkouts->first();
$this->actingAsForApi($admin)
->postJson(route('api.accessories.checkin', $checkoutRecord))
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => Accessory::class,
'item_id' => $accessory->id,
'action_type' => 'checkin from',
'company_id' => $company->id,
]);
}
// -------------------------------------------------------------------------
// Consumable events
// -------------------------------------------------------------------------
public function test_consumable_checkout_log_stores_the_consumables_company_id(): void
{
$company = Company::factory()->create();
$consumable = Consumable::factory()->create(['company_id' => $company->id]);
$user = User::factory()->create();
$admin = User::factory()->superuser()->create();
$this->actingAsForApi($admin)
->postJson(route('api.consumables.checkout', $consumable), [
'assigned_to' => $user->id,
])
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => Consumable::class,
'item_id' => $consumable->id,
'action_type' => 'checkout',
'company_id' => $company->id,
]);
}
// -------------------------------------------------------------------------
// Component events
// -------------------------------------------------------------------------
public function test_component_checkout_log_stores_the_components_company_id(): void
{
$company = Company::factory()->create();
$component = Component::factory()->create(['company_id' => $company->id]);
$asset = Asset::factory()->create();
$admin = User::factory()->superuser()->create();
$this->actingAsForApi($admin)
->postJson(route('api.components.checkout', $component->id), [
'assigned_to' => $asset->id,
'assigned_qty' => 1,
])
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => Component::class,
'item_id' => $component->id,
'action_type' => 'checkout',
'company_id' => $company->id,
]);
}
public function test_component_checkin_log_stores_the_components_company_id(): void
{
$company = Company::factory()->create();
$component = Component::factory()->create(['company_id' => $company->id]);
$asset = Asset::factory()->create();
$admin = User::factory()->superuser()->create();
// Check out first
$this->actingAsForApi($admin)
->postJson(route('api.components.checkout', $component->id), [
'assigned_to' => $asset->id,
'assigned_qty' => 1,
])
->assertStatusMessageIs('success');
$pivotId = $component->assets()->first()->pivot->id;
$this->actingAsForApi($admin)
->postJson(route('api.components.checkin', $pivotId), [
'checkin_qty' => 1,
])
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => Component::class,
'item_id' => $component->id,
'action_type' => 'checkin from',
'company_id' => $company->id,
]);
}
// -------------------------------------------------------------------------
// License events
// -------------------------------------------------------------------------
public function test_license_checkout_log_stores_the_licenses_company_id(): void
{
$company = Company::factory()->create();
$license = License::factory()->create(['company_id' => $company->id]);
$seat = $license->freeSeats()->first();
$user = User::factory()->create();
$admin = User::factory()->superuser()->create();
$this->actingAsForApi($admin)
->patchJson(route('api.licenses.seats.update', [$license->id, $seat->id]), [
'assigned_to' => $user->id,
])
->assertStatusMessageIs('success');
// The log is stored against the License (item_type), not the LicenseSeat
$this->assertDatabaseHas('action_logs', [
'item_type' => License::class,
'item_id' => $license->id,
'action_type' => 'checkout',
'company_id' => $company->id,
]);
}
public function test_license_checkin_log_stores_the_licenses_company_id(): void
{
$company = Company::factory()->create();
$license = License::factory()->create(['company_id' => $company->id]);
$seat = $license->freeSeats()->first();
$user = User::factory()->create();
$admin = User::factory()->superuser()->create();
// Check out first
$seat->assigned_to = $user->id;
$seat->save();
$this->actingAsForApi($admin)
->patchJson(route('api.licenses.seats.update', [$license->id, $seat->id]), [
'assigned_to' => null,
])
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => License::class,
'item_id' => $license->id,
'action_type' => 'checkin from',
'company_id' => $company->id,
]);
}
// -------------------------------------------------------------------------
// Null company_id — items without a company should log null, not an error
// -------------------------------------------------------------------------
public function test_asset_audit_log_company_id_is_null_when_asset_has_no_company(): void
{
$asset = Asset::factory()->create(['company_id' => null]);
$admin = User::factory()->superuser()->create();
$this->actingAsForApi($admin)
->postJson(route('api.asset.audit', $asset), ['note' => 'no company'])
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => Asset::class,
'item_id' => $asset->id,
'action_type' => 'audit',
'company_id' => null,
]);
}
public function test_asset_checkout_log_company_id_is_null_when_asset_has_no_company(): void
{
$asset = Asset::factory()->create(['company_id' => null]);
$user = User::factory()->create();
$admin = User::factory()->superuser()->create();
$this->actingAsForApi($admin)
->postJson(route('api.asset.checkout', $asset), [
'checkout_to_type' => 'user',
'assigned_user' => $user->id,
'status_id' => $asset->status_id,
])
->assertStatusMessageIs('success');
$this->assertDatabaseHas('action_logs', [
'item_type' => Asset::class,
'item_id' => $asset->id,
'action_type' => 'checkout',
'company_id' => null,
]);
}
}
@@ -90,8 +90,10 @@ class ActivityReportTest extends TestCase
// I don't love this, since it doesn't test that we're actually storing the company ID appropriately
// but it's better than what we had
$response = $this->actingAsForApi($userInCompanyA)
->getJson(route('api.activity.index'))
$this->actingAsForApi($userInCompanyA)
->getJson(route('api.activity.index', [
'action_type' => 'update',
]))
->assertOk()
->assertJsonStructure([
'rows',
@@ -100,7 +102,9 @@ class ActivityReportTest extends TestCase
$this->actingAsForApi($userInCompanyB)
->getJson(
route('api.activity.index'))
route('api.activity.index', [
'action_type' => 'update',
]))
->assertOk()
->assertJsonStructure([
'rows',