Merge pull request #15112 from marcusmoore/livewire-importer-improvements

Improved handling attempted access of deleted files in importer
This commit is contained in:
snipe 2024-08-20 09:57:03 +01:00 committed by GitHub
commit f8c72fb0ac
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 157 additions and 124 deletions

View file

@ -3,30 +3,25 @@
namespace App\Livewire;
use App\Models\CustomField;
use Livewire\Component;
use App\Models\Import;
use Illuminate\Support\Facades\Storage;
use Illuminate\Foundation\Auth\Access\AuthorizesRequests;
use Livewire\Attributes\Computed;
use Livewire\Component;
class Importer extends Component
{
use AuthorizesRequests;
public $files;
public $progress; //upload progress - '-1' means don't show
public $progress = -1; //upload progress - '-1' means don't show
public $progress_message;
public $progress_bar_class;
public $progress_bar_class = 'progress-bar-warning';
public $message; //status/error message?
public $message_type; //success/error?
//originally from ImporterFile
public $import_errors; //
public ?Import $activeFile = null;
public $activeFileId;
public $headerRow = [];
public $typeOfImport;
public $importTypes;
public $columnOptions;
public $statusType;
@ -35,7 +30,6 @@ class Importer extends Component
public $send_welcome;
public $run_backup;
public $field_map; // we need a separate variable for the field-mapping, because the keys in the normal array are too complicated for Livewire to understand
public $file_id; // TODO: I can't figure out *why* we need this, but it really seems like we do. I can't seem to pull the id from the activeFile for some reason?
// Make these variables public - we set the properties in the constructor so we can localize them (versus the old static arrays)
public $accessories_fields;
@ -51,10 +45,8 @@ class Importer extends Component
'files.*.file_path' => 'required|string',
'files.*.created_at' => 'required|string',
'files.*.filesize' => 'required|integer',
'activeFile' => 'Import',
'activeFile.import_type' => 'string',
'activeFile.field_map' => 'array',
'activeFile.header_row' => 'array',
'headerRow' => 'array',
'typeOfImport' => 'string',
'field_map' => 'array'
];
@ -68,15 +60,13 @@ class Importer extends Component
{
$tmp = array();
if ($this->activeFile) {
$tmp = array_combine($this->activeFile->header_row, $this->field_map);
$tmp = array_combine($this->headerRow, $this->field_map);
$tmp = array_filter($tmp);
}
return json_encode($tmp);
}
private function getColumns($type)
{
switch ($type) {
@ -115,76 +105,66 @@ class Importer extends Component
return $results;
}
public function updating($name, $new_import_type)
public function updatingTypeOfImport($type)
{
if ($name == "activeFile.import_type") {
// go through each header, find a matching field to try and map it to.
foreach ($this->activeFile->header_row as $i => $header) {
// do we have something mapped already?
if (array_key_exists($i, $this->field_map)) {
// yes, we do. Is it valid for this type of import?
// (e.g. the import type might have been changed...?)
if (array_key_exists($this->field_map[$i], $this->columnOptions[$new_import_type])) {
//yes, this key *is* valid. Continue on to the next field.
continue;
} else {
//no, this key is *INVALID* for this import type. Better set it to null
// and we'll hope that the $aliases_fields or something else picks it up.
$this->field_map[$i] = null; // fingers crossed! But it's not likely, tbh.
} // TODO - strictly speaking, this isn't necessary here I don't think.
// go through each header, find a matching field to try and map it to.
foreach ($this->headerRow as $i => $header) {
// do we have something mapped already?
if (array_key_exists($i, $this->field_map)) {
// yes, we do. Is it valid for this type of import?
// (e.g. the import type might have been changed...?)
if (array_key_exists($this->field_map[$i], $this->columnOptions[$type])) {
//yes, this key *is* valid. Continue on to the next field.
continue;
} else {
//no, this key is *INVALID* for this import type. Better set it to null
// and we'll hope that the $aliases_fields or something else picks it up.
$this->field_map[$i] = null; // fingers crossed! But it's not likely, tbh.
} // TODO - strictly speaking, this isn't necessary here I don't think.
}
// first, check for exact matches
foreach ($this->columnOptions[$type] as $v => $text) {
if (strcasecmp($text, $header) === 0) { // case-INSENSITIVe on purpose!
$this->field_map[$i] = $v;
continue 2; //don't bother with the alias check, go to the next header
}
// first, check for exact matches
foreach ($this->columnOptions[$new_import_type] as $value => $text) {
if (strcasecmp($text, $header) === 0) { // case-INSENSITIVe on purpose!
$this->field_map[$i] = $value;
continue 2; //don't bother with the alias check, go to the next header
}
}
// if you got here, we didn't find a match. Try the $aliases_fields
foreach ($this->aliases_fields as $key => $alias_values) {
foreach ($alias_values as $alias_value) {
if (strcasecmp($alias_value, $header) === 0) { // aLsO CaSe-INSENSitiVE!
// Make *absolutely* sure that this key actually _exists_ in this import type -
// you can trigger this by importing accessories with a 'Warranty' column (which don't exist
// in "Accessories"!)
if (array_key_exists($key, $this->columnOptions[$new_import_type])) {
$this->field_map[$i] = $key;
continue 3; // bust out of both of these loops; as well as the surrounding one - e.g. move on to the next header
}
}
// if you got here, we didn't find a match. Try the $aliases_fields
foreach ($this->aliases_fields as $key => $alias_values) {
foreach ($alias_values as $alias_value) {
if (strcasecmp($alias_value, $header) === 0) { // aLsO CaSe-INSENSitiVE!
// Make *absolutely* sure that this key actually _exists_ in this import type -
// you can trigger this by importing accessories with a 'Warranty' column (which don't exist
// in "Accessories"!)
if (array_key_exists($key, $this->columnOptions[$type])) {
$this->field_map[$i] = $key;
continue 3; // bust out of both of these loops; as well as the surrounding one - e.g. move on to the next header
}
}
}
// and if you got here, we got nothing. Let's recommend 'null'
$this->field_map[$i] = null; // Booooo :(
}
// and if you got here, we got nothing. Let's recommend 'null'
$this->field_map[$i] = null; // Booooo :(
}
}
public function boot() { // FIXME - delete or undelete.
///////$this->activeFile = null; // I do *not* understand why I have to do this, but, well, whatever.
}
public function mount()
{
$this->authorize('import');
$this->progress = -1; // '-1' means 'don't show the progressbar'
$this->progress_bar_class = 'progress-bar-warning';
$this->importTypes = [
'asset' => trans('general.assets'),
'accessory' => trans('general.accessories'),
'asset' => trans('general.assets'),
'accessory' => trans('general.accessories'),
'consumable' => trans('general.consumables'),
'component' => trans('general.components'),
'license' => trans('general.licenses'),
'user' => trans('general.users'),
'location' => trans('general.locations'),
'component' => trans('general.components'),
'license' => trans('general.licenses'),
'user' => trans('general.users'),
'location' => trans('general.locations'),
];
/**
* These are the item-type specific columns
*/
$this->accessories_fields = [
$this->accessories_fields = [
'company' => trans('general.company'),
'location' => trans('general.location'),
'quantity' => trans('general.qty'),
@ -307,7 +287,7 @@ class Importer extends Component
'manufacturer' => trans('general.manufacturer'),
];
$this->users_fields = [
$this->users_fields = [
'id' => trans('general.id'),
'company' => trans('general.company'),
'location' => trans('general.location'),
@ -332,12 +312,12 @@ class Importer extends Component
'website' => trans('general.website'),
'avatar' => trans('general.image'),
'gravatar' => trans('general.importer.gravatar'),
'start_date' => trans('general.start_date'),
'end_date' => trans('general.end_date'),
'employee_num' => trans('general.employee_number'),
'start_date' => trans('general.start_date'),
'end_date' => trans('general.end_date'),
'employee_num' => trans('general.employee_number'),
];
$this->locations_fields = [
$this->locations_fields = [
'name' => trans('general.item_name_var', ['item' => trans('general.location')]),
'address' => trans('general.address'),
'address2' => trans('general.importer.address2'),
@ -510,19 +490,16 @@ class Importer extends Component
];
$this->columnOptions[''] = $this->getColumns(''); //blank mode? I don't know what this is supposed to mean
foreach($this->importTypes AS $type => $name) {
foreach ($this->importTypes as $type => $name) {
$this->columnOptions[$type] = $this->getColumns($type);
}
if ($this->activeFile) {
$this->field_map = $this->activeFile->field_map ? array_values($this->activeFile->field_map) : [];
}
}
public function selectFile($id)
{
$this->clearMessage();
$this->activeFile = Import::find($id);
$this->activeFileId = $id;
if (!$this->activeFile) {
$this->message = trans('admin/hardware/message.import.file_missing');
@ -531,15 +508,17 @@ class Importer extends Component
return;
}
$this->headerRow = $this->activeFile->header_row;
$this->typeOfImport = $this->activeFile->import_type;
$this->field_map = null;
foreach($this->activeFile->header_row as $element) {
if(isset($this->activeFile->field_map[$element])) {
foreach ($this->headerRow as $element) {
if (isset($this->activeFile->field_map[$element])) {
$this->field_map[] = $this->activeFile->field_map[$element];
} else {
$this->field_map[] = null; // re-inject the 'nulls' if a file was imported with some 'Do Not Import' settings
}
}
$this->file_id = $id;
$this->import_errors = null;
$this->statusText = null;
@ -547,21 +526,33 @@ class Importer extends Component
public function destroy($id)
{
// TODO: why don't we just do File::find($id)? This seems dumb.
foreach($this->files as $file) {
if ($id == $file->id) {
if (Storage::delete('private_uploads/imports/'.$file->file_path)) {
$file->delete();
$this->authorize('import');
$this->message = trans('admin/hardware/message.import.file_delete_success');
$this->message_type = 'success';
return;
} else {
$this->message = trans('admin/hardware/message.import.file_delete_error');
$this->message_type = 'danger';
}
}
$import = Import::find($id);
// Check that the import wasn't deleted after while page was already loaded...
// @todo: next up...handle the file being missing for other interactions...
// for example having an import open in two tabs, deleting it, and then changing
// the import type in the other tab. The error message below wouldn't display in that case.
if (!$import) {
$this->message = trans('admin/hardware/message.import.file_already_deleted');
$this->message_type = 'danger';
return;
}
if (Storage::delete('private_uploads/imports/' . $import->file_path)) {
$import->delete();
$this->message = trans('admin/hardware/message.import.file_delete_success');
$this->message_type = 'success';
unset($this->files);
return;
}
$this->message = trans('admin/hardware/message.import.file_delete_error');
$this->message_type = 'danger';
}
public function clearMessage()
@ -570,11 +561,22 @@ class Importer extends Component
$this->message_type = null;
}
#[Computed]
public function files()
{
return Import::orderBy('id', 'desc')->get();
}
#[Computed]
public function activeFile()
{
return Import::find($this->activeFileId);
}
public function render()
{
$this->files = Import::orderBy('id','desc')->get(); //HACK - slows down renders.
return view('livewire.importer')
->extends('layouts.default')
->section('content');
->extends('layouts.default')
->section('content');
}
}

View file

@ -296,6 +296,11 @@ class UserFactory extends Factory
return $this->appendPermission(['reports.view' => '1']);
}
public function canImport()
{
return $this->appendPermission(['import' => '1']);
}
private function appendPermission(array $permission)
{
return $this->state(function ($currentState) use ($permission) {

View file

@ -58,6 +58,7 @@ return [
'file_delete_success' => 'Your file has been been successfully deleted',
'file_delete_error' => 'The file was unable to be deleted',
'file_missing' => 'The file selected is missing',
'file_already_deleted' => 'The file selected was already deleted',
'header_row_has_malformed_characters' => 'One or more attributes in the header row contain malformed UTF-8 characters',
'content_row_has_malformed_characters' => 'One or more attributes in the first row of content contain malformed UTF-8 characters',
],

View file

@ -119,9 +119,9 @@
</th>
</tr>
@foreach($files as $currentFile)
@foreach($this->files as $currentFile)
<tr style="{{ ($activeFile && ($currentFile->id == $activeFile->id)) ? 'font-weight: bold' : '' }}" class="{{ ($activeFile && ($currentFile->id == $activeFile->id)) ? 'warning' : '' }}">
<tr style="{{ ($this->activeFile && ($currentFile->id == $this->activeFile->id)) ? 'font-weight: bold' : '' }}" class="{{ ($this->activeFile && ($currentFile->id == $this->activeFile->id)) ? 'warning' : '' }}">
<td class="col-md-6">{{ $currentFile->file_path }}</td>
<td class="col-md-3">{{ Helper::getFormattedDateObject($currentFile->created_at, 'datetime', false) }}</td>
<td class="col-md-1">{{ Helper::formatFilesizeUnits($currentFile->filesize) }}</td>
@ -130,25 +130,25 @@
<i class="fa-solid fa-list-check" aria-hidden="true"></i>
<span class="sr-only">{{ trans('general.import') }}</span>
</button>
<a href="#" wire:click.prevent="$set('activeFile',null)">
<a href="#" wire:click.prevent="$set('activeFileId',null)">
<button class="btn btn-sm btn-danger" wire:click="destroy({{ $currentFile->id }})">
<i class="fas fa-trash icon-white" aria-hidden="true"></i><span class="sr-only"></span></button>
</a>
</td>
</tr>
@if( $currentFile && $activeFile && ($currentFile->id == $activeFile->id))
@if( $currentFile && $this->activeFile && ($currentFile->id == $this->activeFile->id))
<tr class="warning">
<td colspan="4">
<div class="form-group">
<label for="activeFile.import_type" class="col-md-3 col-xs-12">
<label for="typeOfImport" class="col-md-3 col-xs-12">
{{ trans('general.import_type') }}
</label>
<div class="col-md-9 col-xs-12" wire:ignore>
{{ Form::select('activeFile.import_type', $importTypes, $activeFile->import_type, [
{{ Form::select('typeOfImport', $importTypes, $typeOfImport, [
'id' => 'import_type',
'class' => 'livewire-select2',
'style' => 'min-width: 350px',
@ -157,7 +157,7 @@
'data-minimum-results-for-search' => '-1', // Remove this if the list gets long enough that we need to search
'data-livewire-component' => $this->getId()
]) }}
@if ($activeFile->import_type === 'asset' && $snipeSettings->auto_increment_assets == 0)
@if ($typeOfImport === 'asset' && $snipeSettings->auto_increment_assets == 0)
<p class="help-block">
{{ trans('general.auto_incrementing_asset_tags_disabled_so_tags_required') }}
</p>
@ -170,7 +170,7 @@
<input type="checkbox" name="update" data-livewire-component="{{ $this->getId() }}" wire:model.live="update">
{{ trans('general.update_existing_values') }}
</label>
@if ($activeFile->import_type === 'asset' && $snipeSettings->auto_increment_assets == 1 && $update)
@if ($typeOfImport === 'asset' && $snipeSettings->auto_increment_assets == 1 && $update)
<p class="help-block">
{{ trans('general.auto_incrementing_asset_tags_enabled_so_now_assets_will_be_created') }}
</p>
@ -196,13 +196,13 @@
@endif
@if ($activeFile->import_type)
@if ($typeOfImport)
<div class="form-group col-md-12">
<hr style="border-top: 1px solid lightgray">
<h3>
<i class="{{ Helper::iconTypeByItem($activeFile->import_type) }}">
<i class="{{ Helper::iconTypeByItem($typeOfImport) }}">
</i>
{{ trans('general.map_fields', ['item_type' => ucwords($activeFile->import_type)]) }}
{{ trans('general.map_fields', ['item_type' => ucwords($typeOfImport)]) }}
</h3>
<hr style="border-top: 1px solid lightgray">
</div>
@ -218,16 +218,16 @@
</div>
</div><!-- /div row -->
@if($activeFile->header_row)
@if(! empty($headerRow))
@foreach($activeFile->header_row as $index => $header)
@foreach($headerRow as $index => $header)
<div class="form-group col-md-12" wire:key="header-row-{{ $index }}">
<label for="field_map.{{ $index }}" class="col-md-3 control-label text-right">{{ $header }}</label>
<div class="col-md-4" wire:ignore>
{{ Form::select('field_map.'.$index, $columnOptions[$activeFile->import_type], @$field_map[$index],
{{ Form::select('field_map.'.$index, $columnOptions[$typeOfImport], @$field_map[$index],
[
'class' => 'mappings livewire-select2',
'placeholder' => trans('general.importer.do_not_import'),
@ -238,9 +238,9 @@
])
}}
</div>
@if (($activeFile->first_row) && (array_key_exists($index, $activeFile->first_row)))
@if (($this->activeFile->first_row) && (array_key_exists($index, $this->activeFile->first_row)))
<div class="col-md-5">
<p class="form-control-static">{{ str_limit($activeFile->first_row[$index], 50, '...') }}</p>
<p class="form-control-static">{{ str_limit($this->activeFile->first_row[$index], 50, '...') }}</p>
</div>
@else
@php
@ -256,7 +256,7 @@
<div class="form-group col-md-12">
<div class="col-md-3 text-left">
<a href="#" wire:click.prevent="$set('activeFile',null)">{{ trans('general.cancel') }}</a>
<a href="#" wire:click.prevent="$set('activeFileId',null)">{{ trans('general.cancel') }}</a>
</div>
<div class="col-md-9">
<button type="submit" class="btn btn-primary col-md-5" id="import">{{ trans('admin/hardware/message.import.import_button') }}</button>
@ -272,10 +272,10 @@
@else
<div class="form-group col-md-10">
<div class="col-md-3 text-left">
<a href="#" wire:click.prevent="$set('activeFile',null)">{{ trans('general.cancel') }}</a>
<a href="#" wire:click.prevent="$set('activeFileId',null)">{{ trans('general.cancel') }}</a>
</div>
</div>
@endif {{-- end of if ... activeFile->import_type --}}
@endif {{-- end of if ... $typeOfImport --}}
</div><!-- /div v-show --> </td>
</tr>
@ -333,7 +333,7 @@
// we have to hook up to the `<tr id='importer-file'>` at the root of this display,
// because the #import button isn't visible until you click an import_type
$('#upload-table').on('click', '#import', function () {
if (!$wire.$get('activeFile.import_type')) {
if (!$wire.$get('typeOfImport')) {
$wire.$set('statusType', 'error');
$wire.$set('statusText', "An import type is required... "); //TODO: translate?
return;
@ -345,15 +345,15 @@
// console.warn("Here is the mappings:")
// console.dir(mappings)
// console.warn("Uh, active file id is, I guess: "+$wire.$get('activeFile.id'))
var this_file = $wire.$get('file_id'); // okay, I actually don't know what I'm doing here.
var file_id = $wire.$get('activeFileId');
$.post({
{{-- I want to do something like: route('api.imports.importFile', $activeFile->id) }} --}}
url: "api/v1/imports/process/"+this_file, // maybe? Good a guess as any..FIXME. HARDCODED DUMB FILE
url: "api/v1/imports/process/"+file_id, // maybe? Good a guess as any..FIXME. HARDCODED DUMB FILE
contentType: 'application/json',
data: JSON.stringify({
'import-update': !!$wire.$get('update'),
'send-welcome': !!$wire.$get('send_welcome'),
'import-type': $wire.$get('activeFile.import_type'),
'import-type': $wire.$get('typeOfImport'),
'run-backup': !!$wire.$get('run_backup'),
'column-mappings': mappings
}),
@ -393,7 +393,7 @@
}
}
$wire.$set('activeFile', null); //$wire.$set('hideDetails')
$wire.$set('activeFileId', null); //$wire.$set('hideDetails')
});
})
return false;

View file

@ -0,0 +1,25 @@
<?php
namespace Tests\Feature\Livewire;
use App\Livewire\Importer;
use App\Models\User;
use Livewire\Livewire;
use Tests\TestCase;
class ImporterTest extends TestCase
{
public function testRendersSuccessfully()
{
Livewire::actingAs(User::factory()->canImport()->create())
->test(Importer::class)
->assertStatus(200);
}
public function testRequiresPermission()
{
Livewire::actingAs(User::factory()->create())
->test(Importer::class)
->assertStatus(403);
}
}