Compare commits

..

1 Commits

Author SHA1 Message Date
Constantin Graf
8d34513bde Prevent time entries with end before start
Partial updates of time entries only validated end against start if both
were part of the payload. Start and end are now also validated against the
persisted value if only one of them is sent (fixes #1189).

The importers now fail if the end of a time entry is before the start and
the database consistency check reports such time entries.
2026-10-09 13:23:51 +02:00
15 changed files with 301 additions and 92 deletions

View File

@@ -59,6 +59,14 @@ class SelfHostDatabaseConsistency extends Command
->get();
$this->logProblems($problems, 'Time entries have a client but no project', $hadAProblem);
// End of time entries can not be before the start
$problems = DB::table('time_entries')
->select(['id'])
->whereNotNull('end')
->whereColumn('end', '<', 'start')
->get();
$this->logProblems($problems, 'Time entries have an end that is before the start', $hadAProblem);
// Every user needs to be a member of at least one organization
$problems = DB::table('users')
->select(['users.id as id'])

View File

@@ -51,7 +51,6 @@ use Illuminate\Support\Carbon;
use Illuminate\Support\Collection;
use Illuminate\Support\Facades\Auth;
use Illuminate\Support\Facades\Blade;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Storage;
@@ -614,24 +613,7 @@ class TimeEntryController extends Controller
$this->checkPermission($organization, 'time-entries:create:all');
}
// Lock the creation of running time entries per user, so that concurrent requests can not create more than one running time entry
$lock = $request->input('end') === null ? Cache::lock('time-entries:running:'.$member->user_id, 10) : null;
$lock?->block(5);
try {
return $this->storeTimeEntry($organization, $member, $request);
} finally {
$lock?->release();
}
}
/**
* @throws TimeEntryStillRunningApiException
*/
private function storeTimeEntry(Organization $organization, Member $member, TimeEntryStoreRequest $request): JsonResource
{
// A user can only have one running time entry, across all organizations
if ($request->input('end') === null && TimeEntry::query()->where('user_id', $member->user_id)->whereNull('end')->exists()) {
if ($request->input('end') === null && TimeEntry::query()->whereBelongsTo($member, 'member')->where('end', null)->exists()) {
throw new TimeEntryStillRunningApiException;
}

View File

@@ -13,9 +13,11 @@ use App\Models\Tag;
use App\Models\Task;
use App\Models\TimeEntry;
use App\Service\PermissionStore;
use Carbon\Exceptions\InvalidFormatException;
use Closure;
use Illuminate\Contracts\Validation\ValidationRule;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Support\Carbon;
use Illuminate\Support\Facades\Auth;
use Illuminate\Validation\ConditionalRules;
use Illuminate\Validation\Rule;
@@ -90,12 +92,32 @@ class TimeEntryUpdateRequest extends BaseFormRequest
// Start of time entry (Format: "Y-m-d\TH:i:s\Z", UTC timezone, Example: "2000-02-22T14:58:59Z")
'start' => [
'date_format:Y-m-d\TH:i:s\Z',
function (string $attribute, mixed $value, Closure $fail) use ($timeEntry): void {
// If the payload does not contain an end, the start needs to be validated against the persisted end
if ($this->has('end') || $timeEntry?->end === null) {
return;
}
$start = $this->parseDate($value);
if ($start !== null && $start->gt($timeEntry->end)) {
$fail('The start field must be a date before or equal to end.');
}
},
],
// End of time entry (Format: "Y-m-d\TH:i:s\Z", UTC timezone, Example: "2000-02-22T14:58:59Z")
'end' => [
'nullable',
'date_format:Y-m-d\TH:i:s\Z',
'after_or_equal:start',
function (string $attribute, mixed $value, Closure $fail) use ($timeEntry): void {
// If the payload does not contain a start, the end needs to be validated against the persisted start
if ($this->has('start') || $timeEntry === null) {
return;
}
$end = $this->parseDate($value);
if ($end !== null && $end->lt($timeEntry->start)) {
$fail('The end field must be a date after or equal to start.');
}
},
],
// Whether time entry is billable
'billable' => [
@@ -137,4 +159,16 @@ class TimeEntryUpdateRequest extends BaseFormRequest
],
];
}
private function parseDate(mixed $value): ?Carbon
{
if (! is_string($value)) {
return null;
}
try {
return Carbon::createFromFormat('Y-m-d\TH:i:s\Z', $value, 'UTC');
} catch (InvalidFormatException) {
return null;
}
}
}

View File

@@ -190,6 +190,9 @@ class ClockifyTimeEntriesImporter extends DefaultImporter
if ($end === null) {
throw new ImportException('End date ("'.$endDateStr.'") or time ("'.$endTimeStr.'") are invalid');
}
if ($end->lt($start)) {
throw new ImportException('End ("'.$endStr.'") is before start ("'.$startStr.'")');
}
$timeEntry->end = $end->utc();
$timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations(

View File

@@ -155,6 +155,9 @@ class GenericTimeEntriesImporter extends DefaultImporter
if ($end === null) {
throw new ImportException('Value of end ("'.$record['end'].'") is invalid');
}
if ($end->lt($start)) {
throw new ImportException('Value of end ("'.$record['end'].'") is before start ("'.$record['start'].'")');
}
$timeEntry->end = $end->utc();
$timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations(
$timeEntry,

View File

@@ -136,6 +136,9 @@ class HarvestTimeEntriesImporter extends DefaultImporter
throw new ImportException('Hours ("'.$record['Hours'].'") is invalid');
}
$hours = (float) $hoursField;
if ($hours < 0) {
throw new ImportException('Hours ("'.$record['Hours'].'") is negative');
}
$timeEntry->start = $date->copy()->startOfDay()->utc();
$timeEntry->end = $date->copy()->startOfDay()->addHours($hours)->utc();
$timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations(

View File

@@ -280,6 +280,9 @@ class SolidtimeImporter extends DefaultImporter
if ($end === null) {
throw new ImportException('End date ("'.$timeEntryRow['end'].'") is invalid');
}
if ($end->lt($start)) {
throw new ImportException('End date ("'.$timeEntryRow['end'].'") is before start date ("'.$timeEntryRow['start'].'")');
}
$timeEntry->end = $end->utc();
} else {
$timeEntry->end = null;

View File

@@ -139,6 +139,9 @@ class TogglTimeEntriesImporter extends DefaultImporter
if ($end === null) {
throw new ImportException('End date ("'.$record['End date'].'") or time ("'.$record['End time'].'") are invalid');
}
if ($end->lt($start)) {
throw new ImportException('End ("'.$record['End date'].' '.$record['End time'].'") is before start ("'.$record['Start date'].' '.$record['Start time'].'")');
}
$timeEntry->end = $end->utc();
$timeEntry->billable_rate = $this->billableRateService->getBillableRateForTimeEntryWithGivenRelations(
$timeEntry,

View File

@@ -13,6 +13,7 @@ use App\Models\Task;
use App\Models\TimeEntry;
use App\Models\User;
use Illuminate\Console\Command;
use Illuminate\Support\Carbon;
use Illuminate\Support\Facades\Artisan;
use PHPUnit\Framework\Attributes\CoversClass;
use Tests\TestCaseWithDatabase;
@@ -158,4 +159,26 @@ class SelfHostDatabaseConsistencyCommandTest extends TestCaseWithDatabase
$output = Artisan::output();
$this->assertSame("Consistency problem: Users have a current organization that they are not a member of\n - ".$user1->user->getKey()."\n", $output);
}
public function test_checks_that_end_of_time_entries_is_not_before_start(): void
{
// Arrange
$user = $this->createUserWithRole(Role::Owner);
$timeEntry = TimeEntry::factory()->forMember($user->member)->create([
'start' => Carbon::parse('2026-08-01T03:00:00Z'),
'end' => Carbon::parse('2026-08-01T02:59:55Z'),
]);
TimeEntry::factory()->forMember($user->member)->create([
'start' => Carbon::parse('2026-08-01T04:00:00Z'),
'end' => Carbon::parse('2026-08-01T04:00:00Z'),
]);
// Act
$exitCode = $this->withoutMockingConsoleOutput()->artisan('self-host:database-consistency');
// Assert
$this->assertSame(Command::FAILURE, $exitCode);
$output = Artisan::output();
$this->assertSame("Consistency problem: Time entries have an end that is before the start\n - ".$timeEntry->getKey()."\n", $output);
}
}

View File

@@ -24,9 +24,7 @@ use App\Models\Task;
use App\Models\TimeEntry;
use App\Models\User;
use App\Service\TimeEntryFilter;
use Illuminate\Contracts\Cache\LockTimeoutException;
use Illuminate\Support\Carbon;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\Config;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Log;
@@ -2446,77 +2444,6 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract
$response->assertJsonPath('error', true);
}
public function test_store_endpoint_fails_if_user_already_has_active_time_entry_in_another_organization(): void
{
// Arrange
$data = $this->createUserWithPermission([
'time-entries:create:own',
]);
$otherOrganization = Organization::factory()->create();
$otherMember = Member::factory()->forOrganization($otherOrganization)->forUser($data->user)->create();
TimeEntry::factory()->forOrganization($otherOrganization)->forMember($otherMember)->active()->create();
Passport::actingAs($data->user);
// Act
$response = $this->postJson(route('api.v1.time-entries.store', [$data->organization->getKey()]), [
'billable' => true,
'start' => Carbon::now()->toIso8601ZuluString(),
'end' => null,
'member_id' => $data->member->getKey(),
]);
// Assert
$response->assertStatus(400);
$response->assertJsonPath('key', 'time_entry_still_running');
$this->assertSame(0, TimeEntry::query()->whereBelongsTo($data->organization, 'organization')->count());
}
public function test_store_endpoint_releases_running_time_entry_lock_after_request(): void
{
// Arrange
$data = $this->createUserWithPermission([
'time-entries:create:own',
]);
Passport::actingAs($data->user);
// Act
$response = $this->postJson(route('api.v1.time-entries.store', [$data->organization->getKey()]), [
'billable' => true,
'start' => Carbon::now()->toIso8601ZuluString(),
'end' => null,
'member_id' => $data->member->getKey(),
]);
// Assert
$response->assertStatus(201);
$this->assertTrue(Cache::lock('time-entries:running:'.$data->user->getKey(), 10)->get());
}
public function test_store_endpoint_waits_for_running_time_entry_lock_and_fails_if_it_is_not_released(): void
{
// Arrange
$data = $this->createUserWithPermission([
'time-entries:create:own',
]);
Cache::lock('time-entries:running:'.$data->user->getKey(), 10)->get();
$this->withoutExceptionHandling();
Passport::actingAs($data->user);
// Act
try {
$this->postJson(route('api.v1.time-entries.store', [$data->organization->getKey()]), [
'billable' => true,
'start' => Carbon::now()->toIso8601ZuluString(),
'end' => null,
'member_id' => $data->member->getKey(),
]);
$this->fail('Expected LockTimeoutException');
} catch (LockTimeoutException) {
// Assert
$this->assertSame(0, TimeEntry::query()->count());
}
}
public function test_store_endpoint_validation_fails_if_task_id_does_not_belong_to_project_id(): void
{
// Arrange
@@ -3034,6 +2961,101 @@ class TimeEntryEndpointTest extends ApiEndpointTestAbstract
]);
}
public function test_update_endpoint_validation_fails_if_only_end_is_sent_and_it_is_before_the_persisted_start(): void
{
// Arrange
$data = $this->createUserWithPermission([
'time-entries:update:own',
]);
$timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->create([
'start' => Carbon::parse('2026-08-01T03:00:00Z'),
'end' => Carbon::parse('2026-08-01T03:01:00Z'),
]);
Passport::actingAs($data->user);
// Act
$response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [
'end' => '2026-08-01T02:59:55Z',
]);
// Assert
$response->assertStatus(422);
$response->assertJsonValidationErrors([
'end' => 'The end field must be a date after or equal to start.',
]);
$timeEntry->refresh();
$this->assertSame('2026-08-01T03:01:00Z', $timeEntry->end->toIso8601ZuluString());
}
public function test_update_endpoint_validation_fails_if_only_start_is_sent_and_it_is_after_the_persisted_end(): void
{
// Arrange
$data = $this->createUserWithPermission([
'time-entries:update:own',
]);
$timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->create([
'start' => Carbon::parse('2026-08-01T03:00:00Z'),
'end' => Carbon::parse('2026-08-01T03:01:00Z'),
]);
Passport::actingAs($data->user);
// Act
$response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [
'start' => '2026-08-01T03:01:05Z',
]);
// Assert
$response->assertStatus(422);
$response->assertJsonValidationErrors([
'start' => 'The start field must be a date before or equal to end.',
]);
$timeEntry->refresh();
$this->assertSame('2026-08-01T03:00:00Z', $timeEntry->start->toIso8601ZuluString());
}
public function test_update_endpoint_allows_updating_only_start_of_running_time_entry(): void
{
// Arrange
$data = $this->createUserWithPermission([
'time-entries:update:own',
]);
$timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->active()->create();
Passport::actingAs($data->user);
// Act
$response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [
'start' => '2026-08-01T03:00:00Z',
]);
// Assert
$response->assertStatus(200);
$timeEntry->refresh();
$this->assertSame('2026-08-01T03:00:00Z', $timeEntry->start->toIso8601ZuluString());
}
public function test_update_endpoint_allows_updating_only_end_if_it_is_after_the_persisted_start(): void
{
// Arrange
$data = $this->createUserWithPermission([
'time-entries:update:own',
]);
$timeEntry = TimeEntry::factory()->forOrganization($data->organization)->forMember($data->member)->create([
'start' => Carbon::parse('2026-08-01T03:00:00Z'),
'end' => Carbon::parse('2026-08-01T03:01:00Z'),
]);
Passport::actingAs($data->user);
// Act
$response = $this->putJson(route('api.v1.time-entries.update', [$data->organization->getKey(), $timeEntry->getKey()]), [
'end' => '2026-08-01T03:00:00Z',
]);
// Assert
$response->assertStatus(200);
$timeEntry->refresh();
$this->assertSame('2026-08-01T03:00:00Z', $timeEntry->end->toIso8601ZuluString());
}
public function test_update_endpoint_validation_fails_if_project_id_is_missing_but_request_has_task_id(): void
{
// Arrange

View File

@@ -231,4 +231,29 @@ class ClockifyTimeEntriesImporterTest extends ImporterTestAbstract
$this->assertSame(0, Tag::query()->count());
$this->assertSame(0, Client::query()->count());
}
public function test_import_fails_if_end_is_before_start(): void
{
// Arrange
$organization = Organization::factory()->create();
$timezone = 'Europe/Vienna';
$importer = new ClockifyTimeEntriesImporter;
$importer->init($organization);
$data = <<<'CSV'
"Project","Client","Description","Task","User","Group","Email","Tags","Billable","Start Date","Start Time","End Date","End Time","Duration (h)","Duration (decimal)","Billable Rate (USD)","Billable Amount (USD)"
"Project","Client","Working hard","","Peter Tester","","peter.test@email.test","","Yes","03/04/2024","10:30:00 AM","03/04/2024","10:00:00 AM","00:30:00","0.50","0.00","0.00"
CSV;
// Act
try {
$importer->importData($data, $timezone);
} catch (ImportException $e) {
// Assert
$this->assertSame('End ("03/04/2024 10:00:00 AM") is before start ("03/04/2024 10:30:00 AM")', $e->getMessage());
$this->assertSame(0, TimeEntry::query()->count());
return;
}
$this->fail();
}
}

View File

@@ -5,6 +5,7 @@ declare(strict_types=1);
namespace Tests\Unit\Service\Import\Importers;
use App\Models\Organization;
use App\Models\TimeEntry;
use App\Service\Import\Importers\DefaultImporter;
use App\Service\Import\Importers\GenericTimeEntriesImporter;
use App\Service\Import\Importers\ImportException;
@@ -89,4 +90,27 @@ class GenericTimeEntriesImporterTest extends ImporterTestAbstract
}
$this->fail();
}
public function test_import_fails_if_end_is_before_start(): void
{
// Arrange
$organization = Organization::factory()->create();
$timezone = 'Europe/Vienna';
$importer = new GenericTimeEntriesImporter;
$importer->init($organization);
$data = "description,billable,client,project,tags,start,end,task,user_name,user_email\n".
'"Working hard","true","Big Company","Project for Big Company","","2024-03-04T10:23:00Z","2024-03-04T09:23:00Z","","Peter Tester","peter.test@email.test"';
// Act
try {
$importer->importData($data, $timezone);
} catch (ImportException $e) {
// Assert
$this->assertSame('Value of end ("2024-03-04T09:23:00Z") is before start ("2024-03-04T10:23:00Z")', $e->getMessage());
$this->assertSame(0, TimeEntry::query()->count());
return;
}
$this->fail();
}
}

View File

@@ -105,4 +105,27 @@ class HarvestTimeEntriesImporterTest extends ImporterTestAbstract
$this->assertSame(2, $report->projectsCreated);
$this->assertSame(1, $report->clientsCreated);
}
public function test_import_fails_if_hours_are_negative(): void
{
// Arrange
$organization = Organization::factory()->create();
$timezone = 'Europe/Vienna';
$importer = new HarvestTimeEntriesImporter;
$importer->init($organization);
$data = "Date,Client,Project,Project Code,Task,Notes,Hours,Billable?,Invoiced?,Approved?,First Name,Last Name,Roles,Employee?,Billable Rate,Billable Amount,Cost Rate,Cost Amount,Currency,External Reference URL\n".
'2024-03-04,,Project without Client,,,"","-2,0",No,No,No,Peter,Tester,,Yes,"100,0","2.000,0","0,0","0,0",Euro - EUR,';
// Act
try {
$importer->importData($data, $timezone);
} catch (ImportException $e) {
// Assert
$this->assertSame('Hours ("-2,0") is negative', $e->getMessage());
$this->assertSame(0, TimeEntry::query()->count());
return;
}
$this->fail();
}
}

View File

@@ -15,7 +15,9 @@ use App\Service\Import\Importers\SolidtimeImporter;
use Exception;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Queue;
use Illuminate\Support\Str;
use PHPUnit\Framework\Attributes\CoversClass;
use ZipArchive;
#[CoversClass(SolidtimeImporter::class)]
#[CoversClass(ImportException::class)]
@@ -177,4 +179,32 @@ class SolidtimeImporterTest extends ImporterTestAbstract
Queue::assertPushed(RecalculateSpentTimeForProject::class, 1);
Queue::assertPushed(RecalculateSpentTimeForTask::class, 1);
}
public function test_import_fails_if_end_of_time_entry_is_before_start(): void
{
// Arrange
$organization = Organization::factory()->create();
$timezone = 'Europe/Vienna';
$zipPath = $this->createTestZip('solidtime_import_test_1');
$zip = new ZipArchive;
$zip->open($zipPath);
$timeEntries = $zip->getFromName('time_entries.csv');
$timeEntries = Str::replaceFirst(',2024-03-04T09:23:52Z,2024-03-04T09:23:52Z,', ',2024-03-04T09:23:52Z,2024-03-04T08:23:52Z,', $timeEntries);
$zip->addFromString('time_entries.csv', $timeEntries);
$zip->close();
$importer = new SolidtimeImporter;
$importer->init($organization);
$data = file_get_contents($zipPath);
// Act
try {
$importer->importData($data, $timezone);
} catch (ImportException $e) {
// Assert
$this->assertSame('End date ("2024-03-04T08:23:52Z") is before start date ("2024-03-04T09:23:52Z")', $e->getMessage());
return;
}
$this->fail();
}
}

View File

@@ -120,4 +120,27 @@ class TogglTimeEntriesImporterTest extends ImporterTestAbstract
Queue::assertPushed(RecalculateSpentTimeForProject::class, 2);
Queue::assertPushed(RecalculateSpentTimeForTask::class, 1);
}
public function test_import_fails_if_end_is_before_start(): void
{
// Arrange
$organization = Organization::factory()->create();
$timezone = 'Europe/Vienna';
$importer = new TogglTimeEntriesImporter;
$importer->init($organization);
$data = "User,Email,Client,Project,Task,Description,Billable,Start date,Start time,End date,End time,Duration,Tags,Amount (EUR)\n".
'Peter Tester,peter.test@email.test,,Project without Client,,"",No,2024-03-04,10:23:52,2024-03-04,09:23:52,-01:00:00,"",';
// Act
try {
$importer->importData($data, $timezone);
} catch (ImportException $e) {
// Assert
$this->assertSame('End ("2024-03-04 09:23:52") is before start ("2024-03-04 10:23:52")', $e->getMessage());
$this->assertSame(0, TimeEntry::query()->count());
return;
}
$this->fail();
}
}