diff --git a/app/Filament/Dashboard/Resources/ReportResource/Pages/EditReport.php b/app/Filament/Dashboard/Resources/ReportResource/Pages/EditReport.php index dee2d439..2226fce0 100644 --- a/app/Filament/Dashboard/Resources/ReportResource/Pages/EditReport.php +++ b/app/Filament/Dashboard/Resources/ReportResource/Pages/EditReport.php @@ -26,30 +26,18 @@ protected function mutateFormDataBeforeFill(array $data): array 'references' => $molecule_data['references'] ?? [], ]); } elseif ($this->record['report_category'] === ReportCategory::UPDATE->value) { - // initiate the flags to show only the fields that need to be shown - overall changes are always from the initial suggestions - if (array_key_exists('geo_location_changes', $this->record['suggested_changes']['overall_changes'])) { - $data['show_geo_location_existing'] = $this->record['suggested_changes']['overall_changes']['geo_location_changes']['delete'] ? true : false; - $data['show_geo_location_new'] = $this->record['suggested_changes']['overall_changes']['geo_location_changes']['add'] ? true : false; - } - if (array_key_exists('synonym_changes', $this->record['suggested_changes']['overall_changes'])) { - $data['show_synonym_existing'] = $this->record['suggested_changes']['overall_changes']['synonym_changes']['delete'] ? true : false; - $data['show_synonym_new'] = $this->record['suggested_changes']['overall_changes']['synonym_changes']['add'] ? true : false; - } - if (array_key_exists('name_change', $this->record['suggested_changes']['overall_changes'])) { - $data['show_name_change'] = $this->record['suggested_changes']['overall_changes']['name_change'] ? true : false; - } - if (array_key_exists('cas_changes', $this->record['suggested_changes']['overall_changes'])) { - $data['show_cas_existing'] = $this->record['suggested_changes']['overall_changes']['cas_changes']['delete'] ? true : false; - $data['show_cas_new'] = $this->record['suggested_changes']['overall_changes']['cas_changes']['add'] ? true : false; - } - if (array_key_exists('organism_changes', $this->record['suggested_changes']['overall_changes'])) { - $data['show_organism_existing'] = $this->record['suggested_changes']['overall_changes']['organism_changes']['delete'] ? true : false; - $data['show_organism_new'] = $this->record['suggested_changes']['overall_changes']['organism_changes']['add'] ? true : false; - } - if (array_key_exists('citation_changes', $this->record['suggested_changes']['overall_changes'])) { - $data['show_citation_existing'] = $this->record['suggested_changes']['overall_changes']['citation_changes']['delete'] ? true : false; - $data['show_citation_new'] = $this->record['suggested_changes']['overall_changes']['citation_changes']['add'] ? true : false; - } + // Curators may edit all change fields, not only those the reporter requested (#738) + $data['show_geo_location_existing'] = true; + $data['show_geo_location_new'] = true; + $data['show_synonym_existing'] = true; + $data['show_synonym_new'] = true; + $data['show_name_change'] = true; + $data['show_cas_existing'] = true; + $data['show_cas_new'] = true; + $data['show_organism_existing'] = true; + $data['show_organism_new'] = true; + $data['show_citation_existing'] = true; + $data['show_citation_new'] = true; $curators_copy_changes = $this->record['suggested_changes']['curator']; $data['existing_geo_locations'] = $curators_copy_changes['existing_geo_locations']; diff --git a/app/Models/Report.php b/app/Models/Report.php index 90e8aa15..60be97a5 100644 --- a/app/Models/Report.php +++ b/app/Models/Report.php @@ -2,6 +2,7 @@ namespace App\Models; +use App\Enums\ReportStatus; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; @@ -104,4 +105,43 @@ public function transformAudit(array $data): array { return changeAudit($data); } + + /** + * Whether the given user may edit this report as an assigned curator + * (preserves the two-curator / four-eyes workflow). + */ + public function assignedCuratorCanEdit(User $user): bool + { + if ($this->status === ReportStatus::SUBMITTED->value) { + if (! $this->curators()->wherePivot('curator_number', 1)->exists()) { + return true; + } + + /** @var User|null $curator1 */ + $curator1 = $this->curators()->wherePivot('curator_number', 1)->first(); + + return $curator1?->id === $user->id; + } + + if ($this->status === ReportStatus::PENDING_APPROVAL->value + || $this->status === ReportStatus::PENDING_REJECTION->value) { + /** @var User|null $firstCurator */ + $firstCurator = $this->curators()->wherePivot('curator_number', 1)->first(); + + if ($firstCurator?->id === $user->id) { + return false; + } + + if (! $this->curators()->wherePivot('curator_number', 2)->exists()) { + return true; + } + + /** @var User|null $curator2 */ + $curator2 = $this->curators()->wherePivot('curator_number', 2)->first(); + + return $curator2?->id === $user->id; + } + + return false; + } } diff --git a/app/Policies/ReportPolicy.php b/app/Policies/ReportPolicy.php index 7a20a165..51f2e48d 100644 --- a/app/Policies/ReportPolicy.php +++ b/app/Policies/ReportPolicy.php @@ -2,7 +2,6 @@ namespace App\Policies; -use App\Enums\ReportStatus; use App\Models\Report; use App\Models\User; use Illuminate\Auth\Access\HandlesAuthorization; @@ -43,47 +42,9 @@ public function create(User $user): bool */ public function update(User $user, Report $report): bool { - // Allow users with update_report permission in these cases: - if ($user->can('update_report')) { - // Case 1: Report is submitted - if ($report->status == ReportStatus::SUBMITTED->value) { - // Allow if no curator 1 is assigned yet - if (! $report->curators()->wherePivot('curator_number', 1)->exists()) { - return true; - } - - // Or if user is already assigned as curator 1 - /** @var User|null $curator1 */ - $curator1 = $report->curators()->wherePivot('curator_number', 1)->first(); - if ($curator1?->id == $user->id) { - return true; - } - } - - // Case 2: Report is pending approval/rejection - if ($report->status == ReportStatus::PENDING_APPROVAL->value || $report->status == ReportStatus::PENDING_REJECTION->value) { - // Get the first curator ID - /** @var User|null $firstCurator */ - $firstCurator = $report->curators()->wherePivot('curator_number', 1)->first(); - $firstCuratorId = $firstCurator?->id; - - // Don't allow if user was the first curator (enforce four-eyes principle) - if ($firstCuratorId == $user->id) { - return false; - } - - // Allow if no curator 2 is assigned yet - if (! $report->curators()->wherePivot('curator_number', 2)->exists()) { - return true; - } - - // Or if user is already assigned as curator 2 - /** @var User|null $curator2 */ - $curator2 = $report->curators()->wherePivot('curator_number', 2)->first(); - if ($curator2?->id == $user->id) { - return true; - } - } + if (($user->can('update_report') || $user->hasRole('curator')) + && $report->assignedCuratorCanEdit($user)) { + return true; } // Allow report creator to update only if status is null (report is being created) diff --git a/database/seeders/ShieldSeeder.php b/database/seeders/ShieldSeeder.php index e2e4bcde..21b3e050 100644 --- a/database/seeders/ShieldSeeder.php +++ b/database/seeders/ShieldSeeder.php @@ -12,7 +12,7 @@ public function run(): void { app()[PermissionRegistrar::class]->forgetCachedPermissions(); - $rolesWithPermissions = '[{"name":"super_admin","guard_name":"web","permissions":["view_role","view_any_role","create_role","update_role","delete_role","delete_any_role"]},{"name":"admin","guard_name":"web","permissions":["view_role","view_any_role"]},{"name":"curator","guard_name":"web","permissions":[]}]'; + $rolesWithPermissions = '[{"name":"super_admin","guard_name":"web","permissions":["view_role","view_any_role","create_role","update_role","delete_role","delete_any_role"]},{"name":"admin","guard_name":"web","permissions":["view_role","view_any_role"]},{"name":"curator","guard_name":"web","permissions":["view_any_report","view_report","update_report"]}]'; $directPermissions = '[]'; static::makeRolesWithPermissions($rolesWithPermissions); diff --git a/tests/Unit/ReportPolicyTest.php b/tests/Unit/ReportPolicyTest.php new file mode 100644 index 00000000..dfa000e4 --- /dev/null +++ b/tests/Unit/ReportPolicyTest.php @@ -0,0 +1,130 @@ +policy = new ReportPolicy; + Role::create(['name' => 'curator', 'guard_name' => 'web']); + + DB::statement('ALTER TABLE reports DROP CONSTRAINT IF EXISTS reports_status_check'); + DB::statement('ALTER TABLE reports DROP CONSTRAINT IF EXISTS check_status'); + DB::statement( + "ALTER TABLE reports ADD CONSTRAINT check_status CHECK (status IN ('" + .implode("','", ReportStatus::values()) + ."'))" + ); + } + + public function test_assigned_curator_one_can_update_submitted_report_without_update_report_permission(): void + { + $reporter = User::factory()->create(); + $curator = User::factory()->create(); + $curator->assignRole('curator'); + + $report = $this->createReport($reporter, ReportStatus::SUBMITTED); + $this->assignCurator($report, $curator, 1); + + $this->assertTrue($this->policy->update($curator, $report)); + } + + public function test_curator_one_cannot_update_pending_approval_report_four_eyes(): void + { + $reporter = User::factory()->create(); + $curatorOne = User::factory()->create(); + $curatorOne->assignRole('curator'); + + $report = $this->createReport($reporter, ReportStatus::PENDING_APPROVAL); + $this->assignCurator($report, $curatorOne, 1); + + $this->assertFalse($this->policy->update($curatorOne, $report)); + } + + public function test_assigned_curator_two_can_update_pending_approval_report(): void + { + $reporter = User::factory()->create(); + $curatorOne = User::factory()->create(); + $curatorTwo = User::factory()->create(); + $curatorTwo->assignRole('curator'); + + $report = $this->createReport($reporter, ReportStatus::PENDING_APPROVAL); + $this->assignCurator($report, $curatorOne, 1); + $this->assignCurator($report, $curatorTwo, 2); + + $this->assertTrue($this->policy->update($curatorTwo, $report)); + } + + public function test_curator_cannot_update_approved_or_rejected_report(): void + { + $reporter = User::factory()->create(); + $curator = User::factory()->create(); + $curator->assignRole('curator'); + + foreach ([ReportStatus::APPROVED, ReportStatus::REJECTED] as $status) { + $report = $this->createReport($reporter, $status); + $this->assignCurator($report, $curator, 1); + + $this->assertFalse( + $this->policy->update($curator, $report), + "Curator should not update {$status->value} reports" + ); + } + } + + public function test_reporter_cannot_update_submitted_report(): void + { + $reporter = User::factory()->create(); + $report = $this->createReport($reporter, ReportStatus::SUBMITTED); + + $this->assertFalse($this->policy->update($reporter, $report)); + } + + public function test_unassigned_curator_can_update_submitted_report_with_no_curator_one(): void + { + $reporter = User::factory()->create(); + $curator = User::factory()->create(); + $curator->assignRole('curator'); + + $report = $this->createReport($reporter, ReportStatus::SUBMITTED); + + $this->assertTrue($this->policy->update($curator, $report)); + } + + private function createReport(User $reporter, ReportStatus $status): Report + { + return Report::create([ + 'title' => 'Test report', + 'user_id' => $reporter->id, + 'status' => $status->value, + 'report_category' => ReportCategory::UPDATE->value, + 'suggested_changes' => ['overall_changes' => [], 'curator' => []], + ]); + } + + private function assignCurator(Report $report, User $curator, int $curatorNumber): void + { + $report->curators()->attach($curator->id, [ + 'curator_number' => $curatorNumber, + 'status' => null, + 'comment' => null, + ]); + } +}