From 914ffce014e132b1b5f0fc4d8aafe9bd591df408 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 9 Oct 2026 15:47:56 +0200 Subject: [PATCH 1/8] feat(sponsors): admin-only force delete of sponsor extra questions and answer options Follow-up to #624, which refuses every delete because devices may hold answers the server has not seen. Without an escape hatch the question ends up deleted straight in the database, with no checks, no record and no undo. An admin who has checked the devices can confirm a delete is safe, so give admins a guarded and audited way to do it. Sponsors still can never delete. DELETE .../extra-questions/{id} and .../values/{value_id}: - without force=true: refused (412) for everyone, as in #624 - force=true from a non-admin: 403. Admin is isAdmin(), or isSummitAdmin() allowed on the summit - force=true from an admin: a non-empty reason is required (412) - collected answers: refused with their count unless delete_answers=true - a force deleted question frees its slot under the 5 questions cap Deleting a question destroys its collected answers first (the answer table references it without cascade); the scans are kept. Deleting an option deletes an answer only when it was its only selection, otherwise the option id is taken out of the answer's list. The ids are matched one by one: the answer value is a comma separated list, so a substring match would take 15 for 115. GET .../extra-questions/{id}/usage and .../values/{value_id}/usage (admin only) return the answers a force delete would affect and the sponsor's scan activity per rep. Registered by a config migration plus the seeder, the validator rejects routes missing from api_endpoints. Every force delete leaves an audit entry with who, summit, sponsor, the question or option, the reason and the answers deleted or modified. The onFlush listener cannot carry the reason nor the counts, so it is emitted explicitly. --- ...ponsorExtraQuestionForceDeleteAuditLog.php | 119 +++++ .../OAuth2SummitSponsorApiController.php | 232 ++++++++- .../ISponsorExtraQuestionTypeRepository.php | 9 +- .../ISponsorUserInfoGrantRepository.php | 10 +- ...rineSponsorExtraQuestionTypeRepository.php | 19 +- ...DoctrineSponsorUserInfoGrantRepository.php | 18 + app/Services/Model/ISummitSponsorService.php | 40 +- .../Model/Imp/SummitSponsorService.php | 301 ++++++++++- .../config/Version20261009130000.php | 89 ++++ database/seeders/ApiEndpointsSeeder.php | 30 ++ routes/api_v1.php | 2 + tests/oauth2/OAuth2SummitSponsorApiTest.php | 113 +++++ ...SponsorExtraQuestionForceDeleteApiTest.php | 470 ++++++++++++++++++ 13 files changed, 1424 insertions(+), 28 deletions(-) create mode 100644 app/Audit/SponsorExtraQuestionForceDeleteAuditLog.php create mode 100644 database/migrations/config/Version20261009130000.php create mode 100644 tests/oauth2/OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php diff --git a/app/Audit/SponsorExtraQuestionForceDeleteAuditLog.php b/app/Audit/SponsorExtraQuestionForceDeleteAuditLog.php new file mode 100644 index 0000000000..7b89fd4279 --- /dev/null +++ b/app/Audit/SponsorExtraQuestionForceDeleteAuditLog.php @@ -0,0 +1,119 @@ +getId(), + $summit->getId(), + $by->getEmail(), + $by->getId(), + $reason, + $answers_deleted, + $answers_modified + ); + + $request = request(); + + $data = [ + 'audit.action' => IAuditStrategy::ACTION_DELETE, + 'audit.event_type' => IAuditStrategy::EVENT_ENTITY_DELETION, + 'audit.entity' => $is_question ? 'SummitSponsorExtraQuestionType' : 'ExtraQuestionTypeValue', + 'audit.entity_id' => (string)$target_id, + 'audit.entity_class' => $is_question ? SummitSponsorExtraQuestionType::class : ExtraQuestionTypeValue::class, + 'audit.timestamp' => now()->toISOString(), + 'audit.summit_id' => (string)$summit->getId(), + 'audit.sponsor_id' => (string)$sponsor->getId(), + 'audit.force' => 'true', + 'audit.reason' => $reason, + 'audit.target_label' => $target_label, + 'audit.answers_deleted' => $answers_deleted, + 'audit.answers_modified' => $answers_modified, + 'audit.description' => $description, + 'auth.user.id' => $by->getId(), + 'auth.user.email' => $by->getEmail(), + 'auth.user.first_name' => $by->getFirstName(), + 'auth.user.last_name' => $by->getLastName(), + 'http.route' => $request?->path(), + 'http.method' => $request?->method(), + 'client.ip' => $request?->ip(), + 'user_agent' => $request?->userAgent(), + 'elasticsearch.index' => config('opentelemetry.logs.elasticsearch_index', 'logs-audit'), + ]; + + // always leave a trace in the application log, the OTLP pipeline may be off + Log::warning(self::LogMessage, $data); + + if (config('opentelemetry.enabled', false)) { + JobDispatcher::withDbFallback(job: new EmitAuditLogJob(self::LogMessage, $data)); + } + + return $data; + } +} diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php index b121c8e78d..fe12c5f757 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php @@ -27,6 +27,7 @@ use models\summit\ISummitRepository; use models\summit\Sponsor; use models\summit\SponsorMaterial; +use models\main\Member; use models\summit\Summit; use models\utils\IEntity; use ModelSerializers\SerializerRegistry; @@ -3684,8 +3685,8 @@ public function updateExtraQuestion($summit_id, $sponsor_id, $extra_question_id) #[OA\Delete( path: "/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}", - summary: 'Delete Sponsor Extra Question (always refused)', - description: "Always refused with 412: devices may hold answers for this question that the server has not seen yet. Edit the wording or turn off Mandatory instead. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators . ", " . IGroup::Sponsors . ", " . IGroup::SponsorExternalUsers, + summary: 'Delete Sponsor Extra Question (admin force only)', + description: "Refused with 412 unless force=true is sent by an admin (Super Admins, Administrators or Summit Administrators allowed on the summit): devices may hold answers for this question that the server has not seen yet, so edit the wording or turn off Mandatory instead. A force delete needs a reason, is audited, and when the server holds answers it is refused with their count unless delete_answers=true. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators . ", " . IGroup::Sponsors . ", " . IGroup::SponsorExternalUsers, operationId: 'deleteSponsorExtraQuestion', tags: ['Sponsors'], x: [ @@ -3727,13 +3728,37 @@ public function updateExtraQuestion($summit_id, $sponsor_id, $extra_question_id) schema: new OA\Schema(type: 'integer'), description: 'The extra question id' ), + new OA\Parameter( + name: 'force', + in: 'query', + required: false, + schema: new OA\Schema(type: 'boolean'), + description: 'Admin only force delete. Without it the delete is always refused (412); a non-admin sending it gets 403' + ), + new OA\Parameter( + name: 'delete_answers', + in: 'query', + required: false, + schema: new OA\Schema(type: 'boolean'), + description: 'Confirms that collected answers are destroyed. Required when the server holds answers, otherwise 412 with their count' + ), ], + requestBody: new OA\RequestBody( + required: false, + content: new OA\JsonContent( + properties: [ + new OA\Property(property: 'reason', type: 'string', description: 'Required with force=true, goes to the audit entry'), + ] + ) + ), responses: [ + new OA\Response(response: Response::HTTP_NO_CONTENT, description: 'Question force deleted'), new OA\Response( response: Response::HTTP_PRECONDITION_FAILED, - description: "Validation Error - always returned: questions can't be deleted once created, because devices may already be using them" + description: "Validation Error - questions can't be deleted once created (no force), a reason is missing, or collected answers would be destroyed and delete_answers is not true" ), new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), + new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden - force=true is for admins only"), new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Not Found"), new OA\Response(response: Response::HTTP_INTERNAL_SERVER_ERROR, description: "Server Error"), ] @@ -3759,7 +3784,10 @@ public function deleteExtraQuestion($summit_id, $sponsor_id, $extra_question_id) if(!$current_member->isAuthzFor($summit, $sponsor)) throw new HTTP403ForbiddenException("You are not allowed to perform this action"); - $this->service->deleteSponsorExtraQuestion($summit, intval($sponsor_id), intval($extra_question_id)); + $force_by = $this->getForceDeleteAdmin($current_member, $summit); + [$reason, $delete_answers] = $this->getForceDeleteParams(); + + $this->service->deleteSponsorExtraQuestion($summit, intval($sponsor_id), intval($extra_question_id), $force_by, $reason, $delete_answers); return $this->deleted(); @@ -3979,8 +4007,8 @@ function ($value_id, $payload, $summit, $sponsor_id, $extra_question_id) { use ParametrizedDeleteEntity; #[OA\Delete( path: "/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}/values/{value_id}", - description: "Always refused with 412: devices may hold answers for this option that the server has not seen yet. Edit the wording instead. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators . ", " . IGroup::Sponsors . ", " . IGroup::SponsorExternalUsers, - summary: 'Delete Extra Question Value (always refused)', + description: "Refused with 412 unless force=true is sent by an admin (Super Admins, Administrators or Summit Administrators allowed on the summit): devices may hold answers for this option that the server has not seen yet, so edit the wording instead. A force delete needs a reason and is audited. Collected answers that selected the option are deleted when it was their only selection, otherwise they only lose its id; when the server holds any, it is refused with their count unless delete_answers=true. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators . ", " . IGroup::Sponsors . ", " . IGroup::SponsorExternalUsers, + summary: 'Delete Extra Question Value (admin force only)', operationId: 'deleteSponsorExtraQuestionValue', tags: ['Sponsors'], x: [ @@ -4029,13 +4057,37 @@ function ($value_id, $payload, $summit, $sponsor_id, $extra_question_id) { schema: new OA\Schema(type: 'integer'), description: 'The question value id' ), + new OA\Parameter( + name: 'force', + in: 'query', + required: false, + schema: new OA\Schema(type: 'boolean'), + description: 'Admin only force delete. Without it the delete is always refused (412); a non-admin sending it gets 403' + ), + new OA\Parameter( + name: 'delete_answers', + in: 'query', + required: false, + schema: new OA\Schema(type: 'boolean'), + description: 'Confirms that collected answers are changed. Required when the server holds answers that selected the option, otherwise 412 with their count' + ), ], + requestBody: new OA\RequestBody( + required: false, + content: new OA\JsonContent( + properties: [ + new OA\Property(property: 'reason', type: 'string', description: 'Required with force=true, goes to the audit entry'), + ] + ) + ), responses: [ + new OA\Response(response: Response::HTTP_NO_CONTENT, description: 'Option force deleted'), new OA\Response( response: Response::HTTP_PRECONDITION_FAILED, - description: "Validation Error - always returned: answer options can't be deleted once created, because devices may already be using them" + description: "Validation Error - answer options can't be deleted once created (no force), a reason is missing, or collected answers would be changed and delete_answers is not true" ), new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), + new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden - force=true is for admins only"), new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Not Found"), new OA\Response(response: Response::HTTP_INTERNAL_SERVER_ERROR, description: "Server Error") ] @@ -4063,14 +4115,176 @@ public function deleteExtraQuestionValue($summit_id, $sponsor_id, $extra_questio if(!$current_member->isAuthzFor($summit, $sponsor)) throw new HTTP403ForbiddenException("You are not allowed to perform this action"); - return $this->_delete($value_id, function ($value_id, $summit, $sponsor_id, $extra_question_id) { - $this->service->deleteExtraQuestionValue($summit, intval($sponsor_id), intval($extra_question_id), intval($value_id)); + $force_by = $this->getForceDeleteAdmin($current_member, $summit); + [$reason, $delete_answers] = $this->getForceDeleteParams(); + + return $this->_delete($value_id, function ($value_id, $summit, $sponsor_id, $extra_question_id) use ($force_by, $reason, $delete_answers) { + $this->service->deleteExtraQuestionValue($summit, intval($sponsor_id), intval($extra_question_id), intval($value_id), $force_by, $reason, $delete_answers); } , ...$args); }); } + /** + * Force delete is for admins only: Super Admins, Administrators or Summit Administrators allowed on the summit. + * @param Member|null $member + * @param Summit $summit + * @return bool + */ + private function isAdminFor(?Member $member, Summit $summit): bool + { + if (is_null($member)) return false; + return $member->isAdmin() || ($member->isSummitAdmin() && $member->isSummitAllowed($summit)); + } + + /** + * The admin asking for a force delete (?force=true), null when it is a plain delete + * (the service refuses it for everyone). The flag never helps a sponsor. + * @param Member|null $member + * @param Summit $summit + * @return Member|null + * @throws HTTP403ForbiddenException + */ + private function getForceDeleteAdmin(?Member $member, Summit $summit): ?Member + { + $force = filter_var(request()->input('force', false), FILTER_VALIDATE_BOOLEAN); + if (!$force) return null; + if (!$this->isAdminFor($member, $summit)) + throw new HTTP403ForbiddenException("Only an administrator can force delete."); + return $member; + } + + /** + * reason and delete_answers can come in the body or the query string. + * @return array [?string $reason, bool $delete_answers] + */ + private function getForceDeleteParams(): array + { + $request = request(); + $reason = $request->input('reason'); + return [ + is_string($reason) ? $reason : null, + filter_var($request->input('delete_answers', false), FILTER_VALIDATE_BOOLEAN), + ]; + } + + #[OA\Get( + path: "/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}/usage", + description: "What a force delete of the question affects: collected answers on the server and the sponsor's badge scan activity (reps and their last scan). It does not prove devices have nothing pending. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators, + summary: 'Get Sponsor Extra Question usage', + operationId: 'getSponsorExtraQuestionUsage', + tags: ['Sponsors'], + x: [ + 'required-groups' => [ + IGroup::SuperAdmins, + IGroup::Administrators, + IGroup::SummitAdministrators, + ] + ], + security: [ + [ + 'summit_sponsor_oauth2' => [ + SummitScopes::ReadSummitData, + SummitScopes::ReadAllSummitData, + SummitScopes::ReadSponsorExtraQuestions, + ] + ] + ], + parameters: [ + new OA\Parameter(name: 'id', in: 'path', required: true, schema: new OA\Schema(type: 'integer'), description: 'The summit id'), + new OA\Parameter(name: 'sponsor_id', in: 'path', required: true, schema: new OA\Schema(type: 'integer'), description: 'The sponsor id'), + new OA\Parameter(name: 'extra_question_id', in: 'path', required: true, schema: new OA\Schema(type: 'integer'), description: 'The extra question id'), + ], + responses: [ + new OA\Response(response: Response::HTTP_OK, description: 'answers_count, reps_count and reps (member_id, first_name, last_name, email, scans_count, last_scan_date)'), + new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), + new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden"), + new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Not Found"), + new OA\Response(response: Response::HTTP_INTERNAL_SERVER_ERROR, description: "Server Error"), + ] + )] + /** + * @param $summit_id + * @param $sponsor_id + * @param $extra_question_id + * @return mixed + */ + public function getExtraQuestionUsage($summit_id, $sponsor_id, $extra_question_id) + { + return $this->processRequest(function () use ($summit_id, $sponsor_id, $extra_question_id) { + $summit = SummitFinderStrategyFactory::build($this->getSummitRepository(), $this->resource_server_context)->find($summit_id); + if (is_null($summit)) return $this->error404(); + + $sponsor = $summit->getSummitSponsorById(intval($sponsor_id)); + if (is_null($sponsor)) return $this->error404(); + + if (!$this->isAdminFor($this->resource_server_context->getCurrentUser(), $summit)) + throw new HTTP403ForbiddenException("You are not allowed to perform this action"); + + return $this->ok($this->service->getSponsorExtraQuestionUsage($summit, intval($sponsor_id), intval($extra_question_id))); + }); + } + + #[OA\Get( + path: "/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}/values/{value_id}/usage", + description: "What a force delete of the option affects: collected answers that selected it (answers_to_delete when it was their only selection, answers_to_modify when they only lose its id) and the sponsor's badge scan activity. It does not prove devices have nothing pending. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators, + summary: 'Get Extra Question Value usage', + operationId: 'getSponsorExtraQuestionValueUsage', + tags: ['Sponsors'], + x: [ + 'required-groups' => [ + IGroup::SuperAdmins, + IGroup::Administrators, + IGroup::SummitAdministrators, + ] + ], + security: [ + [ + 'summit_sponsor_oauth2' => [ + SummitScopes::ReadSummitData, + SummitScopes::ReadAllSummitData, + SummitScopes::ReadSponsorExtraQuestions, + ] + ] + ], + parameters: [ + new OA\Parameter(name: 'id', in: 'path', required: true, schema: new OA\Schema(type: 'integer'), description: 'The summit id'), + new OA\Parameter(name: 'sponsor_id', in: 'path', required: true, schema: new OA\Schema(type: 'integer'), description: 'The sponsor id'), + new OA\Parameter(name: 'extra_question_id', in: 'path', required: true, schema: new OA\Schema(type: 'integer'), description: 'The extra question id'), + new OA\Parameter(name: 'value_id', in: 'path', required: true, schema: new OA\Schema(type: 'integer'), description: 'The question value id'), + ], + responses: [ + new OA\Response(response: Response::HTTP_OK, description: 'answers_count, answers_to_delete, answers_to_modify, reps_count and reps (member_id, first_name, last_name, email, scans_count, last_scan_date)'), + new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), + new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden"), + new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Not Found"), + new OA\Response(response: Response::HTTP_INTERNAL_SERVER_ERROR, description: "Server Error"), + ] + )] + /** + * @param $summit_id + * @param $sponsor_id + * @param $extra_question_id + * @param $value_id + * @return mixed + */ + public function getExtraQuestionValueUsage($summit_id, $sponsor_id, $extra_question_id, $value_id) + { + return $this->processRequest(function () use ($summit_id, $sponsor_id, $extra_question_id, $value_id) { + $summit = SummitFinderStrategyFactory::build($this->getSummitRepository(), $this->resource_server_context)->find($summit_id); + if (is_null($summit)) return $this->error404(); + + $sponsor = $summit->getSummitSponsorById(intval($sponsor_id)); + if (is_null($sponsor)) return $this->error404(); + + if (!$this->isAdminFor($this->resource_server_context->getCurrentUser(), $summit)) + throw new HTTP403ForbiddenException("You are not allowed to perform this action"); + + return $this->ok($this->service->getExtraQuestionValueUsage($summit, intval($sponsor_id), intval($extra_question_id), intval($value_id))); + }); + } + #[OA\Get( path: "/api/v1/summits/{id}/sponsors/{sponsor_id}/lead-report-settings/metadata", description: "required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators . ", " . IGroup::Sponsors . ", " . IGroup::SponsorExternalUsers, diff --git a/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php b/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php index 82356a1439..b662af8f93 100644 --- a/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php +++ b/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php @@ -12,12 +12,19 @@ * limitations under the License. **/ +use App\Models\Foundation\ExtraQuestions\ExtraQuestionType; use App\Models\Foundation\ExtraQuestions\IExtraQuestionTypeRepository; +use models\summit\SponsorBadgeScanExtraQuestionAnswer; /** * Interface ISponsorExtraQuestionTypeRepository * @package App\Models\Foundation\Summit\Repositories */ interface ISponsorExtraQuestionTypeRepository extends IExtraQuestionTypeRepository { - + /** + * Every badge scan answer collected so far for a question. + * @param ExtraQuestionType $question + * @return SponsorBadgeScanExtraQuestionAnswer[] + */ + public function getBadgeScanAnswersByQuestion(ExtraQuestionType $question): array; } \ No newline at end of file diff --git a/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php b/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php index 0788b1261a..6cdfb04bda 100644 --- a/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php +++ b/app/Models/Foundation/Summit/Repositories/ISponsorUserInfoGrantRepository.php @@ -29,4 +29,12 @@ interface ISponsorUserInfoGrantRepository extends IBaseRepository * @return SponsorBadgeScan|null */ public function findExistingBadgeScan(Sponsor $sponsor, SummitAttendeeBadge $badge, Member $member, \DateTime $scan_date): ?SponsorBadgeScan; -} \ No newline at end of file + + /** + * Badge scan activity of a sponsor, one row per rep (member) that scanned for it, + * most recently active first. + * @param Sponsor $sponsor + * @return array + */ + public function getScanActivityBySponsor(Sponsor $sponsor): array; +} diff --git a/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php b/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php index 1bca6bab19..2268ce4002 100644 --- a/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php +++ b/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php @@ -12,11 +12,13 @@ * limitations under the License. **/ +use App\Models\Foundation\ExtraQuestions\ExtraQuestionType; use App\Models\Foundation\ExtraQuestions\ExtraQuestionTypeConstants; use App\Models\Foundation\Summit\ExtraQuestions\SummitSponsorExtraQuestionType; use App\Models\Foundation\Summit\Repositories\ISponsorExtraQuestionTypeRepository; use App\Repositories\Main\DoctrineExtraQuestionTypeRepository; use models\summit\Sponsor; +use models\summit\SponsorBadgeScanExtraQuestionAnswer; use utils\DoctrineLeftJoinFilterMapping; use utils\Filter; use utils\Order; @@ -102,4 +104,19 @@ function ($query) { return $query; }); } -} \ No newline at end of file + + /** + * @inheritDoc + */ + public function getBadgeScanAnswersByQuestion(ExtraQuestionType $question): array + { + return $this->getEntityManager() + ->createQueryBuilder() + ->select("a") + ->from(SponsorBadgeScanExtraQuestionAnswer::class, "a") + ->where("a.question = :question") + ->setParameter("question", $question) + ->getQuery() + ->getResult(); + } +} diff --git a/app/Repositories/Summit/DoctrineSponsorUserInfoGrantRepository.php b/app/Repositories/Summit/DoctrineSponsorUserInfoGrantRepository.php index 92608c4340..c874f0fa77 100644 --- a/app/Repositories/Summit/DoctrineSponsorUserInfoGrantRepository.php +++ b/app/Repositories/Summit/DoctrineSponsorUserInfoGrantRepository.php @@ -158,4 +158,22 @@ public function findExistingBadgeScan(Sponsor $sponsor, SummitAttendeeBadge $bad return $query->getQuery()->getOneOrNullResult(); } + + /** + * @inheritDoc + */ + public function getScanActivityBySponsor(Sponsor $sponsor): array + { + return $this->getEntityManager() + ->createQueryBuilder() + ->select("m.id AS member_id, m.first_name AS first_name, m.last_name AS last_name, m.email AS email, COUNT(e.id) AS scans_count, MAX(e.scan_date) AS last_scan_date") + ->from(SponsorBadgeScan::class, "e") + ->join("e.user", "m") + ->where("e.sponsor = :sponsor") + ->setParameter("sponsor", $sponsor) + ->groupBy("m.id") + ->orderBy("last_scan_date", "DESC") + ->getQuery() + ->getArrayResult(); + } } diff --git a/app/Services/Model/ISummitSponsorService.php b/app/Services/Model/ISummitSponsorService.php index c64d0b16a8..5ec554202d 100644 --- a/app/Services/Model/ISummitSponsorService.php +++ b/app/Services/Model/ISummitSponsorService.php @@ -20,6 +20,7 @@ use models\exceptions\EntityNotFoundException; use models\exceptions\ValidationException; use models\main\File; +use models\main\Member; use models\summit\Sponsor; use models\summit\SponsorAd; use models\summit\SponsorMaterial; @@ -271,14 +272,30 @@ public function addSponsorExtraQuestion(Summit $summit, int $sponsor_id, array $ public function updateSponsorExtraQuestion(Summit $summit, int $sponsor_id, int $extra_question_id, array $payload):SummitSponsorExtraQuestionType; /** + * Refused for everyone unless $force_by is given. $force_by is an admin the caller + * already authorized: a device may hold answers the server has not seen, and only an + * admin who checked the devices can tell a delete is safe. * @param Summit $summit * @param int $sponsor_id * @param int $extra_question_id + * @param Member|null $force_by admin forcing the delete, null refuses it + * @param string|null $reason required when forcing, goes to the audit entry + * @param bool $delete_answers confirms the collected answers of the question are destroyed * @return void * @throws EntityNotFoundException * @throws ValidationException */ - public function deleteSponsorExtraQuestion(Summit $summit, int $sponsor_id, int $extra_question_id):void; + public function deleteSponsorExtraQuestion(Summit $summit, int $sponsor_id, int $extra_question_id, ?Member $force_by = null, ?string $reason = null, bool $delete_answers = false):void; + + /** + * What a force delete of the question would affect. + * @param Summit $summit + * @param int $sponsor_id + * @param int $extra_question_id + * @return array + * @throws EntityNotFoundException + */ + public function getSponsorExtraQuestionUsage(Summit $summit, int $sponsor_id, int $extra_question_id):array; /** * @param Summit $summit @@ -302,13 +319,32 @@ public function addExtraQuestionValue(Summit $summit, int $sponsor_id ,int $ques public function updateExtraQuestionValue(Summit $summit, int $sponsor_id, int $question_id, int $value_id, array $payload):ExtraQuestionTypeValue; /** + * Refused for everyone unless $force_by is given, see deleteSponsorExtraQuestion. + * Collected answers that selected the option are deleted when it was their only + * selection, otherwise they only lose its id. * @param Summit $summit * @param int $sponsor_id * @param int $question_id * @param int $value_id + * @param Member|null $force_by admin forcing the delete, null refuses it + * @param string|null $reason required when forcing, goes to the audit entry + * @param bool $delete_answers confirms the collected answers that selected the option are changed * @return void + * @throws EntityNotFoundException + * @throws ValidationException + */ + public function deleteExtraQuestionValue(Summit $summit, int $sponsor_id, int $question_id, int $value_id, ?Member $force_by = null, ?string $reason = null, bool $delete_answers = false):void; + + /** + * What a force delete of the option would affect. + * @param Summit $summit + * @param int $sponsor_id + * @param int $question_id + * @param int $value_id + * @return array + * @throws EntityNotFoundException */ - public function deleteExtraQuestionValue(Summit $summit, int $sponsor_id, int $question_id, int $value_id):void; + public function getExtraQuestionValueUsage(Summit $summit, int $sponsor_id, int $question_id, int $value_id):array; /** * @param Summit $summit diff --git a/app/Services/Model/Imp/SummitSponsorService.php b/app/Services/Model/Imp/SummitSponsorService.php index b464a0279d..79b2783d01 100644 --- a/app/Services/Model/Imp/SummitSponsorService.php +++ b/app/Services/Model/Imp/SummitSponsorService.php @@ -12,12 +12,15 @@ * limitations under the License. **/ +use App\Audit\SponsorExtraQuestionForceDeleteAuditLog; use App\Events\SponsorServices\SponsorDomainEvents; use App\Events\SponsorServices\SummitSponsorCreatedEventDTO; use App\Events\SponsorServices\SummitSponsorshipCreatedEventDTO; use App\Events\SponsorServices\DeletedEventDTO; use App\Http\Utils\IFileUploader; use App\Jobs\SponsorServices\PublishSponsorServiceDomainEventsJob; +use App\Models\Foundation\ExtraQuestions\ExtraQuestionType; +use App\Models\Foundation\ExtraQuestions\ExtraQuestionTypeConstants; use App\Models\Foundation\ExtraQuestions\ExtraQuestionTypeValue; use App\Models\Foundation\Main\IFileConstants; use App\Models\Foundation\Summit\ExtraQuestions\SummitSponsorExtraQuestionType; @@ -43,8 +46,10 @@ use models\main\ICompanyRepository; use models\main\IMemberRepository; use models\main\Member; +use models\summit\ISponsorUserInfoGrantRepository; use models\summit\Sponsor; use models\summit\SponsorAd; +use models\summit\SponsorBadgeScanExtraQuestionAnswer; use models\summit\SponsorMaterial; use models\summit\SponsorSocialNetwork; use models\summit\Summit; @@ -80,11 +85,17 @@ final class SummitSponsorService */ private $file_uploader; + /** + * @var ISponsorUserInfoGrantRepository + */ + private $user_info_grant_repository; + /** * @param IMemberRepository $member_repository * @param ICompanyRepository $company_repository * @param ISponsorExtraQuestionTypeRepository $repository * @param ISummitSponsorshipRepository $sponsorship_repository + * @param ISponsorUserInfoGrantRepository $user_info_grant_repository * @param IFileUploader $file_uploader * @param ITransactionService $tx_service */ @@ -94,11 +105,13 @@ public function __construct ICompanyRepository $company_repository, ISponsorExtraQuestionTypeRepository $repository, ISummitSponsorshipRepository $sponsorship_repository, + ISponsorUserInfoGrantRepository $user_info_grant_repository, IFileUploader $file_uploader, ITransactionService $tx_service ) { parent::__construct($tx_service); + $this->user_info_grant_repository = $user_info_grant_repository; $this->member_repository = $member_repository; $this->company_repository = $company_repository; $this->sponsorship_repository = $sponsorship_repository; @@ -1082,14 +1095,181 @@ public function updateSponsorExtraQuestion(Summit $summit, int $sponsor_id, int /** * @inheritDoc */ - public function deleteSponsorExtraQuestion(Summit $summit, int $sponsor_id, int $extra_question_id): void + public function deleteSponsorExtraQuestion + ( + Summit $summit, + int $sponsor_id, + int $extra_question_id, + ?Member $force_by = null, + ?string $reason = null, + bool $delete_answers = false + ): void { - // Always refused: a device may hold answers for this question that the server has not seen yet. - throw new ValidationException( - "Questions can't be deleted once created, because devices may already be using them. You can edit the wording, turn off Mandatory, or contact support." + if (is_null($force_by)) { + // Refused: a device may hold answers for this question that the server has not seen yet. + // Only an admin who checked the devices can force it (see $force_by). + throw new ValidationException( + "Questions can't be deleted once created, because devices may already be using them. You can edit the wording, turn off Mandatory, or contact support." + ); + } + + $reason = $this->requireForceDeleteReason($reason); + + [$sponsor, $label, $answers_deleted] = $this->tx_service->transaction(function () use ($summit, $sponsor_id, $extra_question_id, $delete_answers) { + $summit_sponsor = $summit->getSummitSponsorById($sponsor_id); + if (is_null($summit_sponsor)) + throw new EntityNotFoundException("Sponsor not found."); + + $extra_question = $summit_sponsor->getExtraQuestionById($extra_question_id); + + if (!$extra_question instanceof SummitSponsorExtraQuestionType) + throw new EntityNotFoundException("Sponsor extra question not found."); + + $answers = $this->repository->getBadgeScanAnswersByQuestion($extra_question); + $answers_count = count($answers); + + if ($answers_count > 0 && !$delete_answers) { + throw new ValidationException( + sprintf( + "%s collected answers will be permanently deleted. Send delete_answers=true to confirm.", + $answers_count + ) + ); + } + + // the answers reference the question (FK without cascade), so they go first + foreach ($answers as $answer) { + $this->removeBadgeScanAnswer($answer); + } + + $label = $extra_question->getLabel(); + $summit_sponsor->removeExtraQuestion($extra_question); + + return [$summit_sponsor, $label, $answers_count]; + }); + + SponsorExtraQuestionForceDeleteAuditLog::record( + $force_by, + $summit, + $sponsor, + SponsorExtraQuestionForceDeleteAuditLog::TargetQuestion, + $extra_question_id, + $label, + $reason, + $answers_deleted ); } + /** + * @inheritDoc + */ + public function getSponsorExtraQuestionUsage(Summit $summit, int $sponsor_id, int $extra_question_id): array + { + return $this->tx_service->transaction(function () use ($summit, $sponsor_id, $extra_question_id) { + $summit_sponsor = $summit->getSummitSponsorById($sponsor_id); + if (is_null($summit_sponsor)) + throw new EntityNotFoundException("Sponsor not found."); + + $extra_question = $summit_sponsor->getExtraQuestionById($extra_question_id); + + if (!$extra_question instanceof SummitSponsorExtraQuestionType) + throw new EntityNotFoundException("Sponsor extra question not found."); + + return array_merge( + [ + 'answers_count' => count($this->repository->getBadgeScanAnswersByQuestion($extra_question)), + ], + $this->getScanActivity($summit_sponsor) + ); + }); + } + + /** + * Removes a collected answer from its badge scan (the scan owns it, orphan removal deletes it). + * @param SponsorBadgeScanExtraQuestionAnswer $answer + */ + private function removeBadgeScanAnswer(SponsorBadgeScanExtraQuestionAnswer $answer): void + { + $scan = $answer->getBadgeScan(); + if (is_null($scan)) { + $this->repository->delete($answer); + return; + } + $scan->removeExtraQuestionAnswer($answer); + $scan->updateLastEdited(); + } + + /** + * @param string|null $reason + * @return string + * @throws ValidationException + */ + private function requireForceDeleteReason(?string $reason): string + { + $reason = trim($reason ?? ''); + if ($reason === '') + throw new ValidationException("A reason is required to force delete."); + return $reason; + } + + /** + * Badge scan activity of the sponsor, to see what is at stake before a force delete. + * It does not prove devices have nothing pending, only the devices can show that. + * @param Sponsor $sponsor + * @return array + */ + private function getScanActivity(Sponsor $sponsor): array + { + $reps = []; + foreach ($this->user_info_grant_repository->getScanActivityBySponsor($sponsor) as $row) { + $last_scan_date = $row['last_scan_date']; + $reps[] = [ + 'member_id' => intval($row['member_id']), + 'first_name' => $row['first_name'], + 'last_name' => $row['last_name'], + 'email' => $row['email'], + 'scans_count' => intval($row['scans_count']), + 'last_scan_date' => $last_scan_date instanceof \DateTimeInterface + ? $last_scan_date->getTimestamp() + : (new \DateTime($last_scan_date))->getTimestamp(), + ]; + } + return [ + 'reps_count' => count($reps), + 'reps' => $reps, + ]; + } + + /** + * Collected answers that use an option. The answer value holds the ids of the + * selected options separated by commas, so it is matched id by id: a plain + * substring match would take 15 for 115 and a legacy free text like "15 apples" for 15. + * Answers whose only selected option is this one are to be deleted, the others + * only lose the id from their list. + * @param ExtraQuestionType $question + * @param int $value_id + * @return array [SponsorBadgeScanExtraQuestionAnswer[] $to_delete, array $to_modify] + */ + private function splitAnswersUsingOption(ExtraQuestionType $question, int $value_id): array + { + $to_delete = []; + $to_modify = []; + foreach ($this->repository->getBadgeScanAnswersByQuestion($question) as $answer) { + $selected = array_map('trim', explode(ExtraQuestionTypeConstants::AnswerCharDelimiter, $answer->getValue())); + $remaining = array_values(array_filter( + $selected, + fn(string $id) => $id !== '' && !(ctype_digit($id) && intval($id) === $value_id) + )); + $uses_option = count($remaining) !== count(array_filter($selected, fn(string $id) => $id !== '')); + if (!$uses_option) continue; + if (count($remaining) === 0) + $to_delete[] = $answer; + else + $to_modify[] = [$answer, implode(ExtraQuestionTypeConstants::AnswerCharDelimiter, $remaining)]; + } + return [$to_delete, $to_modify]; + } + /** * @param Summit $summit * @param int $sponsor_id @@ -1143,21 +1323,114 @@ public function updateExtraQuestionValue(Summit $summit, int $sponsor_id, int $q } /** - * @param Summit $summit - * @param int $sponsor_id - * @param int $question_id - * @param int $value_id - * @return void - * @throws \Exception + * @inheritDoc */ - public function deleteExtraQuestionValue(Summit $summit, int $sponsor_id, int $question_id, int $value_id): void + public function deleteExtraQuestionValue + ( + Summit $summit, + int $sponsor_id, + int $question_id, + int $value_id, + ?Member $force_by = null, + ?string $reason = null, + bool $delete_answers = false + ): void { - // Always refused: a device may hold answers for this option that the server has not seen yet. - throw new ValidationException( - "Answer options can't be deleted once created, because devices may already be using them. You can edit the wording, or contact support." + if (is_null($force_by)) { + // Refused: a device may hold answers for this option that the server has not seen yet. + // Only an admin who checked the devices can force it (see $force_by). + throw new ValidationException( + "Answer options can't be deleted once created, because devices may already be using them. You can edit the wording, or contact support." + ); + } + + $reason = $this->requireForceDeleteReason($reason); + + [$sponsor, $label, $answers_deleted, $answers_modified] = $this->tx_service->transaction(function () use ($summit, $sponsor_id, $question_id, $value_id, $delete_answers) { + $summit_sponsor = $summit->getSummitSponsorById($sponsor_id); + if (is_null($summit_sponsor)) + throw new EntityNotFoundException("Sponsor not found."); + + $extra_question = $summit_sponsor->getExtraQuestionById($question_id); + + if (!$extra_question instanceof SummitSponsorExtraQuestionType) + throw new EntityNotFoundException("Sponsor extra question not found."); + + $value = $extra_question->getValueById($value_id); + if (is_null($value)) + throw new EntityNotFoundException("value not found"); + + [$to_delete, $to_modify] = $this->splitAnswersUsingOption($extra_question, $value_id); + + if ((count($to_delete) + count($to_modify)) > 0 && !$delete_answers) { + throw new ValidationException( + sprintf( + "%s collected answers use this option: %s will be permanently deleted and %s will only lose this option. Send delete_answers=true to confirm.", + count($to_delete) + count($to_modify), + count($to_delete), + count($to_modify) + ) + ); + } + + foreach ($to_delete as $answer) { + $this->removeBadgeScanAnswer($answer); + } + + foreach ($to_modify as [$answer, $remaining]) { + $answer->setValue($remaining); + } + + $label = $value->getLabel(); + $extra_question->removeValue($value); + + return [$summit_sponsor, $label, count($to_delete), count($to_modify)]; + }); + + SponsorExtraQuestionForceDeleteAuditLog::record( + $force_by, + $summit, + $sponsor, + SponsorExtraQuestionForceDeleteAuditLog::TargetOption, + $value_id, + $label, + $reason, + $answers_deleted, + $answers_modified ); } + /** + * @inheritDoc + */ + public function getExtraQuestionValueUsage(Summit $summit, int $sponsor_id, int $question_id, int $value_id): array + { + return $this->tx_service->transaction(function () use ($summit, $sponsor_id, $question_id, $value_id) { + $summit_sponsor = $summit->getSummitSponsorById($sponsor_id); + if (is_null($summit_sponsor)) + throw new EntityNotFoundException("Sponsor not found."); + + $extra_question = $summit_sponsor->getExtraQuestionById($question_id); + + if (!$extra_question instanceof SummitSponsorExtraQuestionType) + throw new EntityNotFoundException("Sponsor extra question not found."); + + if (is_null($extra_question->getValueById($value_id))) + throw new EntityNotFoundException("value not found"); + + [$to_delete, $to_modify] = $this->splitAnswersUsingOption($extra_question, $value_id); + + return array_merge( + [ + 'answers_count' => count($to_delete) + count($to_modify), + 'answers_to_delete' => count($to_delete), + 'answers_to_modify' => count($to_modify), + ], + $this->getScanActivity($summit_sponsor) + ); + }); + } + /** * @param Summit $summit * @param int $sponsor_id diff --git a/database/migrations/config/Version20261009130000.php b/database/migrations/config/Version20261009130000.php new file mode 100644 index 0000000000..ed899f27c9 --- /dev/null +++ b/database/migrations/config/Version20261009130000.php @@ -0,0 +1,89 @@ + 'get-sponsor-extra-question-usage', + 'route' => '/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}/usage', + 'http_method' => 'GET', + 'scopes' => [ + SummitScopes::ReadSummitData, + SummitScopes::ReadAllSummitData, + SummitScopes::ReadSponsorExtraQuestions, + ], + 'authz_groups' => [ + IGroup::SuperAdmins, + IGroup::Administrators, + IGroup::SummitAdministrators, + ], + ], + [ + 'name' => 'get-sponsor-extra-question-value-usage', + 'route' => '/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}/values/{value_id}/usage', + 'http_method' => 'GET', + 'scopes' => [ + SummitScopes::ReadSummitData, + SummitScopes::ReadAllSummitData, + SummitScopes::ReadSponsorExtraQuestions, + ], + 'authz_groups' => [ + IGroup::SuperAdmins, + IGroup::Administrators, + IGroup::SummitAdministrators, + ], + ], + ]; + + public function getDescription(): string + { + return 'Register the sponsor extra question and answer option usage endpoints.'; + } + + public function up(Schema $schema): void + { + $this->registerEndpoints(self::API_NAME, self::ENDPOINTS); + } + + public function down(Schema $schema): void + { + $this->unregisterEndpoints(self::API_NAME, array_column(self::ENDPOINTS, 'name')); + } +} diff --git a/database/seeders/ApiEndpointsSeeder.php b/database/seeders/ApiEndpointsSeeder.php index f7af63e2ed..f0ca8306ee 100644 --- a/database/seeders/ApiEndpointsSeeder.php +++ b/database/seeders/ApiEndpointsSeeder.php @@ -2647,6 +2647,36 @@ private function seedSummitEndpoints() IGroup::SponsorExternalUsers, ] ], + [ + 'name' => 'get-sponsor-extra-question-usage', + 'route' => '/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}/usage', + 'http_method' => 'GET', + 'scopes' => [ + SummitScopes::ReadSummitData, + SummitScopes::ReadAllSummitData, + SummitScopes::ReadSponsorExtraQuestions, + ], + 'authz_groups' => [ + IGroup::SuperAdmins, + IGroup::Administrators, + IGroup::SummitAdministrators, + ] + ], + [ + 'name' => 'get-sponsor-extra-question-value-usage', + 'route' => '/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}/values/{value_id}/usage', + 'http_method' => 'GET', + 'scopes' => [ + SummitScopes::ReadSummitData, + SummitScopes::ReadAllSummitData, + SummitScopes::ReadSponsorExtraQuestions, + ], + 'authz_groups' => [ + IGroup::SuperAdmins, + IGroup::Administrators, + IGroup::SummitAdministrators, + ] + ], // lead report settings [ 'name' => 'get-sponsor-report-settings-metadata', diff --git a/routes/api_v1.php b/routes/api_v1.php index 537d27590c..bf0df1cd1d 100644 --- a/routes/api_v1.php +++ b/routes/api_v1.php @@ -1350,12 +1350,14 @@ Route::get('', ['middleware' => 'auth.user', 'uses' => 'OAuth2SummitSponsorApiController@getExtraQuestion']); Route::put('', ['middleware' => 'auth.user', 'uses' => 'OAuth2SummitSponsorApiController@updateExtraQuestion']); Route::delete('', ['middleware' => 'auth.user', 'uses' => 'OAuth2SummitSponsorApiController@deleteExtraQuestion']); + Route::get('usage', ['middleware' => 'auth.user', 'uses' => 'OAuth2SummitSponsorApiController@getExtraQuestionUsage']); // multi values Route::group(['prefix' => 'values'], function () { Route::post('', ['middleware' => 'auth.user', 'uses' => 'OAuth2SummitSponsorApiController@addExtraQuestionValue']); Route::group(['prefix' => '{value_id}'], function () { Route::put('', ['middleware' => 'auth.user', 'uses' => 'OAuth2SummitSponsorApiController@updateExtraQuestionValue']); Route::delete('', ['middleware' => 'auth.user', 'uses' => 'OAuth2SummitSponsorApiController@deleteExtraQuestionValue']); + Route::get('usage', ['middleware' => 'auth.user', 'uses' => 'OAuth2SummitSponsorApiController@getExtraQuestionValueUsage']); }); }); }); diff --git a/tests/oauth2/OAuth2SummitSponsorApiTest.php b/tests/oauth2/OAuth2SummitSponsorApiTest.php index 6fbf2aa0e2..c7b447b594 100644 --- a/tests/oauth2/OAuth2SummitSponsorApiTest.php +++ b/tests/oauth2/OAuth2SummitSponsorApiTest.php @@ -17,6 +17,7 @@ use Mockery; use models\summit\SummitLeadReportSetting; use App\Models\Foundation\Main\IGroup; +use App\Models\ResourceServer\IAccessTokenService; /** * Class OAuth2SummitSponsorApiTest */ @@ -1621,6 +1622,118 @@ public function testDeleteExtraQuestionValue(){ $this->assertStringContainsString("Answer options can't be deleted once created", $response->getContent()); } + // ---- Force delete is for admins only ---- + + /** + * Makes the user a member of sponsors[0] so the sponsor authz passes. It has to run + * before the first request: the entity manager is closed after each one. + */ + private function makeMemberSponsorUser(): void + { + $sponsor = self::$sponsors[0]; + $sponsor->addUser(self::$member); + self::$em->persist(self::$member); + self::$em->persist($sponsor); + self::$em->flush(); + self::$member->addSponsorPermission($sponsor->getId(), IGroup::Sponsors); + } + + /** + * The token stub grants the user the administrators group, and the first request syncs + * the token groups into the member's DB groups, so swap it for one that only has the + * sponsors group before the first request. + */ + private function useSponsorOnlyToken(): void + { + $stub = new AccessTokenServiceStub([['slug' => IGroup::Sponsors]]); + $stub->setUserId(self::$member->getUserExternalId()); + $stub->setUserExternalId(self::$member->getUserExternalId()); + $stub->setUserEmail(self::$member->getEmail()); + $stub->setUserFirstName(self::$member->getFirstName()); + $stub->setUserLastName(self::$member->getLastName()); + self::$service = $stub; + $this->app->singleton(IAccessTokenService::class, function () use ($stub) { return $stub; }); + } + + public function testForceDeleteSponsorExtraQuestionBySponsorIsForbidden(){ + $this->makeMemberSponsorUser(); + $this->useSponsorOnlyToken(); + $q = $this->testAddSponsorExtraQuestions(); + + $params = [ + 'id' => self::$summit->getId(), + 'sponsor_id' => self::$sponsors[0]->getId(), + 'extra_question_id' => $q->id, + ]; + + // the sponsor is authorized on the sponsor: a plain delete is refused... + $this->action("DELETE", "OAuth2SummitSponsorApiController@deleteExtraQuestion", $params, [], [], [], $this->getAuthHeaders()); + $this->assertResponseStatus(412); + + // ...and the flag never helps a sponsor + $this->action( + "DELETE", + "OAuth2SummitSponsorApiController@deleteExtraQuestion", + array_merge($params, ['force' => 'true']), + [], + [], + [], + $this->getAuthHeaders(), + json_encode(['reason' => 'sponsor insists']) + ); + $this->assertResponseStatus(403); + } + + public function testForceDeleteExtraQuestionValueBySponsorIsForbidden(){ + $this->makeMemberSponsorUser(); + $this->useSponsorOnlyToken(); + $result = $this->testAddExtraQuestionValue(); + + $params = [ + 'id' => self::$summit->getId(), + 'sponsor_id' => self::$sponsors[0]->getId(), + 'extra_question_id' => $result->question_id, + 'value_id' => $result->value->id, + ]; + + $this->action("DELETE", "OAuth2SummitSponsorApiController@deleteExtraQuestionValue", $params, [], [], [], $this->getAuthHeaders()); + $this->assertResponseStatus(412); + + $this->action( + "DELETE", + "OAuth2SummitSponsorApiController@deleteExtraQuestionValue", + array_merge($params, ['force' => 'true']), + [], + [], + [], + $this->getAuthHeaders(), + json_encode(['reason' => 'sponsor insists']) + ); + $this->assertResponseStatus(403); + } + + public function testGetExtraQuestionUsageBySponsorIsForbidden(){ + $this->makeMemberSponsorUser(); + $this->useSponsorOnlyToken(); + $q = $this->testAddSponsorExtraQuestions(); + + $this->action( + "GET", + "OAuth2SummitSponsorApiController@getExtraQuestionUsage", + [ + 'id' => self::$summit->getId(), + 'sponsor_id' => self::$sponsors[0]->getId(), + 'extra_question_id' => $q->id, + ], + [], + [], + [], + $this->getAuthHeaders() + ); + + $this->assertResponseStatus(403); + } + // ---- Public API ---- public function testGetAllSponsorsBySummitPublic(){ diff --git a/tests/oauth2/OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php b/tests/oauth2/OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php new file mode 100644 index 0000000000..aae99c7806 --- /dev/null +++ b/tests/oauth2/OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php @@ -0,0 +1,470 @@ +shouldIgnoreMissing(); + + $fileUploaderMock->shouldReceive('build')->andReturn(new \models\main\File()); + + $app->instance(\App\Http\Utils\IFileUploader::class, $fileUploaderMock); + + return $app; + } + + protected function setUp(): void + { + parent::setUp(); + self::$defaultMember = self::$member; + self::$defaultMember2 = self::$member2; + self::insertSummitTestData(); + } + + protected function tearDown(): void + { + Mockery::close(); + self::clearSummitTestData(); + parent::tearDown(); + } + + // ---- helpers ---- + + /** + * Sponsors are seeded with the maximum number of extra questions: makes room directly + * on the model, then adds a question of the given type with the given option labels. + * @param string $type + * @param string[] $option_labels + * @return array [question id, option ids] + */ + private function addQuestion(string $type, array $option_labels = []): array + { + $sponsor = self::$sponsors[0]; + $sponsor->removeExtraQuestion($sponsor->getExtraQuestions()->last()); + self::$em->persist($sponsor); + self::$em->flush(); + + $question = new SummitSponsorExtraQuestionType(); + $question->setType($type); + $question->setLabel('Force delete question'); + $question->setName('FORCE_DELETE_' . str_random(5)); + $values = []; + foreach ($option_labels as $label) { + $value = new ExtraQuestionTypeValue(); + $value->setLabel($label); + $value->setValue($label); + $question->addValue($value); + $values[] = $value; + } + $sponsor->addExtraQuestion($question); + self::$em->persist($sponsor); + self::$em->flush(); + + return [$question->getId(), array_map(fn($v) => $v->getId(), $values)]; + } + + /** + * A scan of the sponsor that carries the given answer for the question. + * @param int $question_id + * @param string $answer + * @param int $idx keeps the scans distinct + * @return int scan id + */ + private function addScanWithAnswer(int $question_id, string $answer, int $idx): int + { + $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId()); + $service = App::make(ISponsorUserInfoGrantService::class); + $scan = $service->addBadgeScan(self::$summit, self::$member, [ + 'qr_code' => $attendee->getFirstTicket()->getBadge()->generateQRCode(), + 'scan_date' => 1572019200 + $idx, + 'sponsor_id' => self::$sponsors[0]->getId(), + 'extra_questions' => [ + ['question_id' => $question_id, 'answer' => $answer], + ], + ]); + return $scan->getId(); + } + + private function answersOf(int $question_id): array + { + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $em->clear(); + $answers = $em->getRepository(SponsorBadgeScanExtraQuestionAnswer::class) + ->createQueryBuilder('a') + ->where('IDENTITY(a.question) = :q') + ->setParameter('q', $question_id) + ->getQuery() + ->getResult(); + return $answers; + } + + private function answerValueOfScan(int $scan_id): ?string + { + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $em->clear(); + $scan = $em->getRepository(SponsorBadgeScan::class)->find($scan_id); + $answer = $scan->getExtraQuestionAnswers()->first(); + return $answer === false ? null : $answer->getValue(); + } + + private function sponsorQuestionIds(): array + { + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $em->clear(); + return $em->getRepository(SummitSponsorExtraQuestionType::class) + ->createQueryBuilder('q') + ->select('q.id') + ->where('IDENTITY(q.sponsor) = :s') + ->setParameter('s', self::$sponsors[0]->getId()) + ->getQuery() + ->getSingleColumnResult(); + } + + private function deleteQuestion(int $question_id, array $query = [], ?array $body = null) + { + return $this->action( + "DELETE", + "OAuth2SummitSponsorApiController@deleteExtraQuestion", + array_merge([ + 'id' => self::$summit->getId(), + 'sponsor_id' => self::$sponsors[0]->getId(), + 'extra_question_id' => $question_id, + ], $query), + [], + [], + [], + $this->getAuthHeaders(), + is_null($body) ? null : json_encode($body) + ); + } + + private function deleteValue(int $question_id, int $value_id, array $query = [], ?array $body = null) + { + return $this->action( + "DELETE", + "OAuth2SummitSponsorApiController@deleteExtraQuestionValue", + array_merge([ + 'id' => self::$summit->getId(), + 'sponsor_id' => self::$sponsors[0]->getId(), + 'extra_question_id' => $question_id, + 'value_id' => $value_id, + ], $query), + [], + [], + [], + $this->getAuthHeaders(), + is_null($body) ? null : json_encode($body) + ); + } + + /** + * The force delete audit entry that was emitted, if any. + * @return EmitAuditLogJob|null + */ + private function emittedAudit(): ?EmitAuditLogJob + { + $jobs = Bus::dispatched(EmitAuditLogJob::class); + return $jobs->isEmpty() ? null : $jobs->first(); + } + + protected function enableAuditCapture(): void + { + config(['opentelemetry.enabled' => true]); + Bus::fake([EmitAuditLogJob::class]); + } + + // ---- questions ---- + + public function testAdminDeleteQuestionWithoutForceIsRefused() + { + [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); + + $response = $this->deleteQuestion($question_id); + + $this->assertResponseStatus(412); + $this->assertStringContainsString("Questions can't be deleted once created", $response->getContent()); + $this->assertContains($question_id, $this->sponsorQuestionIds()); + } + + public function testAdminForceDeleteQuestionWithoutReasonIsRefused() + { + [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); + + $response = $this->deleteQuestion($question_id, ['force' => 'true']); + $this->assertResponseStatus(412); + $this->assertStringContainsString("A reason is required", $response->getContent()); + + $this->deleteQuestion($question_id, ['force' => 'true'], ['reason' => ' ']); + $this->assertResponseStatus(412); + + $this->assertContains($question_id, $this->sponsorQuestionIds()); + } + + public function testAdminForceDeleteUnknownQuestionIsNotFound() + { + $this->deleteQuestion(999999999, ['force' => 'true'], ['reason' => 'cleanup']); + $this->assertResponseStatus(404); + } + + public function testAdminForceDeleteQuestionWithoutAnswersFreesTheSlotAndIsAudited() + { + $this->enableAuditCapture(); + [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); + // the sponsor is at the cap + $this->assertCount(5, $this->sponsorQuestionIds()); + + $this->deleteQuestion($question_id, ['force' => 'true'], ['reason' => 'created by mistake, 0 pending uploads']); + + $this->assertResponseStatus(204); + $this->assertCount(4, $this->sponsorQuestionIds()); + $this->assertNotContains($question_id, $this->sponsorQuestionIds()); + + $audit = $this->emittedAudit(); + $this->assertNotNull($audit); + $this->assertEquals('created by mistake, 0 pending uploads', $audit->auditData['audit.reason']); + $this->assertEquals((string)$question_id, $audit->auditData['audit.entity_id']); + $this->assertEquals(0, $audit->auditData['audit.answers_deleted']); + $this->assertEquals((string)self::$member->getId(), (string)$audit->auditData['auth.user.id']); + + // the slot is free right away + $this->action( + "POST", + "OAuth2SummitSponsorApiController@addExtraQuestion", + [ + 'id' => self::$summit->getId(), + 'sponsor_id' => self::$sponsors[0]->getId(), + ], + [], + [], + [], + $this->getAuthHeaders(), + json_encode([ + 'name' => 'REPLACEMENT_' . str_random(5), + 'type' => ExtraQuestionTypeConstants::TextQuestionType, + 'label' => 'Replacement question', + 'mandatory' => false, + ]) + ); + $this->assertResponseStatus(201); + $this->assertCount(5, $this->sponsorQuestionIds()); + } + + public function testAdminForceDeleteQuestionWithAnswersIsRefusedWithTheirCount() + { + [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); + $this->addScanWithAnswer($question_id, 'one', 1); + $this->addScanWithAnswer($question_id, 'two', 2); + $this->addScanWithAnswer($question_id, 'three', 3); + + $response = $this->deleteQuestion($question_id, ['force' => 'true'], ['reason' => 'sponsor insists']); + + $this->assertResponseStatus(412); + $this->assertStringContainsString("3 collected answers will be permanently deleted", $response->getContent()); + $this->assertContains($question_id, $this->sponsorQuestionIds()); + $this->assertCount(3, $this->answersOf($question_id)); + } + + public function testAdminForceDeleteQuestionWithAnswersAndConfirmationDestroysThemAndIsAudited() + { + $this->enableAuditCapture(); + [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); + $scan_ids = [ + $this->addScanWithAnswer($question_id, 'one', 1), + $this->addScanWithAnswer($question_id, 'two', 2), + $this->addScanWithAnswer($question_id, 'three', 3), + ]; + + $this->deleteQuestion($question_id, ['force' => 'true', 'delete_answers' => 'true'], ['reason' => 'sponsor insists']); + + $this->assertResponseStatus(204); + $this->assertNotContains($question_id, $this->sponsorQuestionIds()); + $this->assertCount(0, $this->answersOf($question_id)); + // the scans themselves are kept + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + foreach ($scan_ids as $scan_id) { + $this->assertNotNull($em->getRepository(SponsorBadgeScan::class)->find($scan_id)); + } + + $audit = $this->emittedAudit(); + $this->assertNotNull($audit); + $this->assertEquals(3, $audit->auditData['audit.answers_deleted']); + $this->assertEquals('sponsor insists', $audit->auditData['audit.reason']); + } + + // ---- options ---- + + public function testAdminDeleteOptionWithoutForceIsRefused() + { + [$question_id, [$a]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B']); + + $response = $this->deleteValue($question_id, $a); + $this->assertResponseStatus(412); + $this->assertStringContainsString("Answer options can't be deleted once created", $response->getContent()); + } + + public function testAdminForceDeleteOptionWithoutReasonIsRefused() + { + [$question_id, [$a]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B']); + + $this->deleteValue($question_id, $a, ['force' => 'true']); + $this->assertResponseStatus(412); + } + + public function testAdminForceDeleteUnusedOptionDeletesIt() + { + $this->enableAuditCapture(); + [$question_id, [$a, $b]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B']); + // only B is used + $this->addScanWithAnswer($question_id, (string)$b, 1); + + $this->deleteValue($question_id, $a, ['force' => 'true'], ['reason' => 'option created by mistake']); + + $this->assertResponseStatus(204); + $audit = $this->emittedAudit(); + $this->assertNotNull($audit); + $this->assertEquals(0, $audit->auditData['audit.answers_deleted']); + $this->assertEquals(0, $audit->auditData['audit.answers_modified']); + // B is untouched + $this->assertCount(1, $this->answersOf($question_id)); + } + + public function testAdminForceDeleteUsedOptionIsRefusedWithTheCountOfDeletedAndModifiedAnswers() + { + [$question_id, [$a, $b, $c]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B', 'C']); + $this->addScanWithAnswer($question_id, (string)$a, 1); // only A: deleted + $this->addScanWithAnswer($question_id, "$a,$b", 2); // A and B: loses A + $this->addScanWithAnswer($question_id, "$b,$c", 3); // no A: untouched + + $response = $this->deleteValue($question_id, $a, ['force' => 'true'], ['reason' => 'sponsor insists']); + + $this->assertResponseStatus(412); + $this->assertStringContainsString("2 collected answers use this option: 1 will be permanently deleted and 1 will only lose this option", $response->getContent()); + $this->assertCount(3, $this->answersOf($question_id)); + } + + public function testAdminForceDeleteUsedOptionWithConfirmationDeletesOnlyAnswersWithNoOtherOptionAndStripsTheRest() + { + $this->enableAuditCapture(); + [$question_id, [$a, $b, $c]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B', 'C']); + $only_a = $this->addScanWithAnswer($question_id, (string)$a, 1); + $a_and_b = $this->addScanWithAnswer($question_id, "$a,$b", 2); + $b_and_c = $this->addScanWithAnswer($question_id, "$b,$c", 3); + $only_c = $this->addScanWithAnswer($question_id, (string)$c, 4); + + $this->deleteValue($question_id, $a, ['force' => 'true', 'delete_answers' => 'true'], ['reason' => 'sponsor insists']); + + $this->assertResponseStatus(204); + $this->assertNull($this->answerValueOfScan($only_a), 'the answer that only selected the option is deleted'); + $this->assertEquals((string)$b, $this->answerValueOfScan($a_and_b), 'the option id is taken out of the list'); + $this->assertEquals("$b,$c", $this->answerValueOfScan($b_and_c)); + $this->assertEquals((string)$c, $this->answerValueOfScan($only_c)); + + $audit = $this->emittedAudit(); + $this->assertNotNull($audit); + $this->assertEquals(1, $audit->auditData['audit.answers_deleted']); + $this->assertEquals(1, $audit->auditData['audit.answers_modified']); + $this->assertEquals('sponsor insists', $audit->auditData['audit.reason']); + + // the option is gone + $this->deleteValue($question_id, $a, ['force' => 'true'], ['reason' => 'again']); + $this->assertResponseStatus(404); + } + + // ---- usage ---- + + public function testAdminGetsQuestionUsage() + { + [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); + $this->addScanWithAnswer($question_id, 'one', 1); + $this->addScanWithAnswer($question_id, 'two', 2); + + $response = $this->action( + "GET", + "OAuth2SummitSponsorApiController@getExtraQuestionUsage", + [ + 'id' => self::$summit->getId(), + 'sponsor_id' => self::$sponsors[0]->getId(), + 'extra_question_id' => $question_id, + ], + [], + [], + [], + $this->getAuthHeaders() + ); + + $this->assertResponseStatus(200); + $usage = json_decode($response->getContent()); + $this->assertEquals(2, $usage->answers_count); + $this->assertEquals(1, $usage->reps_count); + $this->assertEquals(self::$member->getId(), $usage->reps[0]->member_id); + $this->assertEquals(2, $usage->reps[0]->scans_count); + $this->assertGreaterThan(0, $usage->reps[0]->last_scan_date); + } + + public function testAdminGetsOptionUsage() + { + [$question_id, [$a, $b]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B']); + $this->addScanWithAnswer($question_id, (string)$a, 1); + $this->addScanWithAnswer($question_id, "$a,$b", 2); + $this->addScanWithAnswer($question_id, (string)$b, 3); + + $response = $this->action( + "GET", + "OAuth2SummitSponsorApiController@getExtraQuestionValueUsage", + [ + 'id' => self::$summit->getId(), + 'sponsor_id' => self::$sponsors[0]->getId(), + 'extra_question_id' => $question_id, + 'value_id' => $a, + ], + [], + [], + [], + $this->getAuthHeaders() + ); + + $this->assertResponseStatus(200); + $usage = json_decode($response->getContent()); + $this->assertEquals(2, $usage->answers_count); + $this->assertEquals(1, $usage->answers_to_delete); + $this->assertEquals(1, $usage->answers_to_modify); + $this->assertEquals(1, $usage->reps_count); + } +} From 7f680b95f0e52016909a01b4a2bf6ef601bf1e0e Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 9 Oct 2026 17:21:35 +0200 Subject: [PATCH 2/8] fix(sponsors): persist force delete audit as a SummitAuditLog within the delete transaction The force delete audit only reached the OTLP pipeline (when enabled) and a warning that the default error log level drops, so a delete could commit with no durable record. It is now always stored as a SummitAuditLog, in the same transaction as the delete, and emitted to the log/OTLP after commit. --- ...ponsorExtraQuestionForceDeleteAuditLog.php | 32 ++++++++-- .../Model/Imp/SummitSponsorService.php | 61 +++++++++++-------- ...SponsorExtraQuestionForceDeleteApiTest.php | 43 +++++++++++++ 3 files changed, 107 insertions(+), 29 deletions(-) diff --git a/app/Audit/SponsorExtraQuestionForceDeleteAuditLog.php b/app/Audit/SponsorExtraQuestionForceDeleteAuditLog.php index 7b89fd4279..5047c76d1e 100644 --- a/app/Audit/SponsorExtraQuestionForceDeleteAuditLog.php +++ b/app/Audit/SponsorExtraQuestionForceDeleteAuditLog.php @@ -16,9 +16,11 @@ use App\Jobs\EmitAuditLogJob; use App\Jobs\Utils\JobDispatcher; use App\Models\Foundation\ExtraQuestions\ExtraQuestionTypeValue; +use App\Models\Foundation\Main\Repositories\IAuditLogRepository; use App\Models\Foundation\Summit\ExtraQuestions\SummitSponsorExtraQuestionType; use Illuminate\Support\Facades\Log; use models\main\Member; +use models\main\SummitAuditLog; use models\summit\Sponsor; use models\summit\Summit; @@ -28,6 +30,10 @@ * The onFlush listener already audits the entities removed by the delete, but it * cannot carry the reason the admin gave nor how many collected answers were * destroyed or modified, so this entry records them explicitly. + * + * The entry is always persisted as a SummitAuditLog (queryable through + * /api/v1/audit-logs) within the same transaction as the delete, so a force delete + * never commits without its audit record, whether or not the OTLP pipeline is on. * @package App\Audit */ final class SponsorExtraQuestionForceDeleteAuditLog @@ -38,6 +44,8 @@ final class SponsorExtraQuestionForceDeleteAuditLog public const LogMessage = 'audit.sponsor_extra_question.force_deleted'; /** + * Persists the audit entry, call it within the transaction that performs the delete. + * @param IAuditLogRepository $repository * @param Member $by * @param Summit $summit * @param Sponsor $sponsor @@ -47,10 +55,11 @@ final class SponsorExtraQuestionForceDeleteAuditLog * @param string $reason * @param int $answers_deleted answers removed from the server * @param int $answers_modified answers that only lost the deleted option - * @return array the audit data that was emitted + * @return array the audit data, to be handed to emit() once the transaction commits */ public static function record ( + IAuditLogRepository $repository, Member $by, Summit $summit, Sponsor $sponsor, @@ -107,13 +116,28 @@ public static function record 'elasticsearch.index' => config('opentelemetry.logs.elasticsearch_index', 'logs-audit'), ]; - // always leave a trace in the application log, the OTLP pipeline may be off + // Metadata is a varchar(255), the full description (reason included) goes into Action + $repository->add(new SummitAuditLog( + $by, + $description, + $summit, + sprintf('sponsor:%s %s:%s', $sponsor->getId(), $target_type, $target_id) + )); + + return $data; + } + + /** + * Ships the audit data to the application log and, when enabled, to the OTLP pipeline. + * Call it once the delete transaction has committed. + * @param array $data as returned by record() + */ + public static function emit(array $data): void + { Log::warning(self::LogMessage, $data); if (config('opentelemetry.enabled', false)) { JobDispatcher::withDbFallback(job: new EmitAuditLogJob(self::LogMessage, $data)); } - - return $data; } } diff --git a/app/Services/Model/Imp/SummitSponsorService.php b/app/Services/Model/Imp/SummitSponsorService.php index 79b2783d01..3d003914bc 100644 --- a/app/Services/Model/Imp/SummitSponsorService.php +++ b/app/Services/Model/Imp/SummitSponsorService.php @@ -23,6 +23,7 @@ use App\Models\Foundation\ExtraQuestions\ExtraQuestionTypeConstants; use App\Models\Foundation\ExtraQuestions\ExtraQuestionTypeValue; use App\Models\Foundation\Main\IFileConstants; +use App\Models\Foundation\Main\Repositories\IAuditLogRepository; use App\Models\Foundation\Summit\ExtraQuestions\SummitSponsorExtraQuestionType; use App\Models\Foundation\Summit\Factories\LeadReportSettingsFactory; use App\Models\Foundation\Summit\Factories\SponsorAdFactory; @@ -90,6 +91,11 @@ final class SummitSponsorService */ private $user_info_grant_repository; + /** + * @var IAuditLogRepository + */ + private $audit_log_repository; + /** * @param IMemberRepository $member_repository * @param ICompanyRepository $company_repository @@ -97,6 +103,7 @@ final class SummitSponsorService * @param ISummitSponsorshipRepository $sponsorship_repository * @param ISponsorUserInfoGrantRepository $user_info_grant_repository * @param IFileUploader $file_uploader + * @param IAuditLogRepository $audit_log_repository * @param ITransactionService $tx_service */ public function __construct @@ -107,10 +114,12 @@ public function __construct ISummitSponsorshipRepository $sponsorship_repository, ISponsorUserInfoGrantRepository $user_info_grant_repository, IFileUploader $file_uploader, + IAuditLogRepository $audit_log_repository, ITransactionService $tx_service ) { parent::__construct($tx_service); + $this->audit_log_repository = $audit_log_repository; $this->user_info_grant_repository = $user_info_grant_repository; $this->member_repository = $member_repository; $this->company_repository = $company_repository; @@ -1115,7 +1124,7 @@ public function deleteSponsorExtraQuestion $reason = $this->requireForceDeleteReason($reason); - [$sponsor, $label, $answers_deleted] = $this->tx_service->transaction(function () use ($summit, $sponsor_id, $extra_question_id, $delete_answers) { + $audit_data = $this->tx_service->transaction(function () use ($summit, $sponsor_id, $extra_question_id, $force_by, $reason, $delete_answers) { $summit_sponsor = $summit->getSummitSponsorById($sponsor_id); if (is_null($summit_sponsor)) throw new EntityNotFoundException("Sponsor not found."); @@ -1145,19 +1154,20 @@ public function deleteSponsorExtraQuestion $label = $extra_question->getLabel(); $summit_sponsor->removeExtraQuestion($extra_question); - return [$summit_sponsor, $label, $answers_count]; + return SponsorExtraQuestionForceDeleteAuditLog::record( + $this->audit_log_repository, + $force_by, + $summit, + $summit_sponsor, + SponsorExtraQuestionForceDeleteAuditLog::TargetQuestion, + $extra_question_id, + $label, + $reason, + $answers_count + ); }); - SponsorExtraQuestionForceDeleteAuditLog::record( - $force_by, - $summit, - $sponsor, - SponsorExtraQuestionForceDeleteAuditLog::TargetQuestion, - $extra_question_id, - $label, - $reason, - $answers_deleted - ); + SponsorExtraQuestionForceDeleteAuditLog::emit($audit_data); } /** @@ -1346,7 +1356,7 @@ public function deleteExtraQuestionValue $reason = $this->requireForceDeleteReason($reason); - [$sponsor, $label, $answers_deleted, $answers_modified] = $this->tx_service->transaction(function () use ($summit, $sponsor_id, $question_id, $value_id, $delete_answers) { + $audit_data = $this->tx_service->transaction(function () use ($summit, $sponsor_id, $question_id, $value_id, $force_by, $reason, $delete_answers) { $summit_sponsor = $summit->getSummitSponsorById($sponsor_id); if (is_null($summit_sponsor)) throw new EntityNotFoundException("Sponsor not found."); @@ -1384,20 +1394,21 @@ public function deleteExtraQuestionValue $label = $value->getLabel(); $extra_question->removeValue($value); - return [$summit_sponsor, $label, count($to_delete), count($to_modify)]; + return SponsorExtraQuestionForceDeleteAuditLog::record( + $this->audit_log_repository, + $force_by, + $summit, + $summit_sponsor, + SponsorExtraQuestionForceDeleteAuditLog::TargetOption, + $value_id, + $label, + $reason, + count($to_delete), + count($to_modify) + ); }); - SponsorExtraQuestionForceDeleteAuditLog::record( - $force_by, - $summit, - $sponsor, - SponsorExtraQuestionForceDeleteAuditLog::TargetOption, - $value_id, - $label, - $reason, - $answers_deleted, - $answers_modified - ); + SponsorExtraQuestionForceDeleteAuditLog::emit($audit_data); } /** diff --git a/tests/oauth2/OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php b/tests/oauth2/OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php index aae99c7806..8127d4a7b6 100644 --- a/tests/oauth2/OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php +++ b/tests/oauth2/OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php @@ -21,6 +21,7 @@ use Illuminate\Support\Facades\Bus; use LaravelDoctrine\ORM\Facades\Registry; use Mockery; +use models\main\SummitAuditLog; use models\summit\SponsorBadgeScan; use models\summit\SponsorBadgeScanExtraQuestionAnswer; use models\utils\SilverstripeBaseModel; @@ -204,6 +205,22 @@ private function emittedAudit(): ?EmitAuditLogJob return $jobs->isEmpty() ? null : $jobs->first(); } + /** + * The force delete audit records persisted for the given target. + * @param string $target_type question | option + * @param int $target_id + * @return SummitAuditLog[] + */ + private function persistedAudits(string $target_type, int $target_id): array + { + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $em->clear(); + $logs = $em->getRepository(SummitAuditLog::class)->findBy(['summit' => self::$summit->getId()]); + return array_values(array_filter($logs, function (SummitAuditLog $log) use ($target_type, $target_id) { + return str_ends_with((string)$log->getMetadata(), sprintf(' %s:%s', $target_type, $target_id)); + })); + } + protected function enableAuditCapture(): void { config(['opentelemetry.enabled' => true]); @@ -263,6 +280,11 @@ public function testAdminForceDeleteQuestionWithoutAnswersFreesTheSlotAndIsAudit $this->assertEquals(0, $audit->auditData['audit.answers_deleted']); $this->assertEquals((string)self::$member->getId(), (string)$audit->auditData['auth.user.id']); + $logs = $this->persistedAudits('question', $question_id); + $this->assertCount(1, $logs); + $this->assertEquals(self::$member->getId(), $logs[0]->getUser()->getId()); + $this->assertStringContainsString('Reason: created by mistake, 0 pending uploads', $logs[0]->getAction()); + // the slot is free right away $this->action( "POST", @@ -299,6 +321,8 @@ public function testAdminForceDeleteQuestionWithAnswersIsRefusedWithTheirCount() $this->assertStringContainsString("3 collected answers will be permanently deleted", $response->getContent()); $this->assertContains($question_id, $this->sponsorQuestionIds()); $this->assertCount(3, $this->answersOf($question_id)); + // nothing was deleted, so nothing is audited + $this->assertCount(0, $this->persistedAudits('question', $question_id)); } public function testAdminForceDeleteQuestionWithAnswersAndConfirmationDestroysThemAndIsAudited() @@ -328,6 +352,21 @@ public function testAdminForceDeleteQuestionWithAnswersAndConfirmationDestroysTh $this->assertEquals('sponsor insists', $audit->auditData['audit.reason']); } + public function testAdminForceDeleteQuestionIsPersistedInTheAuditLogWhenOpenTelemetryIsOff() + { + config(['opentelemetry.enabled' => false]); + Bus::fake([EmitAuditLogJob::class]); + [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); + + $this->deleteQuestion($question_id, ['force' => 'true'], ['reason' => 'created by mistake']); + + $this->assertResponseStatus(204); + $this->assertNull($this->emittedAudit()); + $logs = $this->persistedAudits('question', $question_id); + $this->assertCount(1, $logs); + $this->assertStringContainsString('Reason: created by mistake', $logs[0]->getAction()); + } + // ---- options ---- public function testAdminDeleteOptionWithoutForceIsRefused() @@ -402,6 +441,10 @@ public function testAdminForceDeleteUsedOptionWithConfirmationDeletesOnlyAnswers $this->assertEquals(1, $audit->auditData['audit.answers_modified']); $this->assertEquals('sponsor insists', $audit->auditData['audit.reason']); + $logs = $this->persistedAudits('option', $a); + $this->assertCount(1, $logs); + $this->assertStringContainsString('Collected answers deleted: 1, modified: 1', $logs[0]->getAction()); + // the option is gone $this->deleteValue($question_id, $a, ['force' => 'true'], ['reason' => 'again']); $this->assertResponseStatus(404); From ff9542b478a5708bcee9da8f8702a0d050e99c36 Mon Sep 17 00:00:00 2001 From: romanetar Date: Sun, 11 Oct 2026 15:14:54 +0200 Subject: [PATCH 3/8] refactor(sponsors): drop the force flag, deleting extra questions and options is admin only Review feedback on #625: force=true added no protection beyond the reason and delete_answers rules, it is the only flag of its kind in summit-api, and it made every plain DELETE fail for everybody. DELETE of a sponsor extra question or answer option is now admin only (Super Admins, Administrators, Summit Admins allowed on the summit): - a non-admin gets 403, sponsors are removed from the endpoint authz groups (seeder and the existing config migration, restored by down()) - reason is required (412 if missing or blank) and goes to the audit entry - delete_answers is required as true or false (412 if missing); false with answers on the server is refused with their count, true deletes them and audits the count The audit log class is renamed to SponsorExtraQuestionDeleteAuditLog and the audit.force field is gone. --- ...=> SponsorExtraQuestionDeleteAuditLog.php} | 11 +-- .../OAuth2SummitSponsorApiController.php | 94 +++++++------------ app/Services/Model/ISummitSponsorService.php | 24 ++--- .../Model/Imp/SummitSponsorService.php | 74 +++++++-------- .../config/Version20261009130000.php | 26 ++++- database/seeders/ApiEndpointsSeeder.php | 4 - tests/oauth2/OAuth2SummitSponsorApiTest.php | 45 ++++----- ...mmitSponsorExtraQuestionDeleteApiTest.php} | 70 +++++++------- 8 files changed, 166 insertions(+), 182 deletions(-) rename app/Audit/{SponsorExtraQuestionForceDeleteAuditLog.php => SponsorExtraQuestionDeleteAuditLog.php} (93%) rename tests/oauth2/{OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php => OAuth2SummitSponsorExtraQuestionDeleteApiTest.php} (85%) diff --git a/app/Audit/SponsorExtraQuestionForceDeleteAuditLog.php b/app/Audit/SponsorExtraQuestionDeleteAuditLog.php similarity index 93% rename from app/Audit/SponsorExtraQuestionForceDeleteAuditLog.php rename to app/Audit/SponsorExtraQuestionDeleteAuditLog.php index 5047c76d1e..7a2f46f598 100644 --- a/app/Audit/SponsorExtraQuestionForceDeleteAuditLog.php +++ b/app/Audit/SponsorExtraQuestionDeleteAuditLog.php @@ -25,23 +25,23 @@ use models\summit\Summit; /** - * Audit entry for an admin force delete of a sponsor extra question or answer option. + * Audit entry for an admin delete of a sponsor extra question or answer option. * * The onFlush listener already audits the entities removed by the delete, but it * cannot carry the reason the admin gave nor how many collected answers were * destroyed or modified, so this entry records them explicitly. * * The entry is always persisted as a SummitAuditLog (queryable through - * /api/v1/audit-logs) within the same transaction as the delete, so a force delete + * /api/v1/audit-logs) within the same transaction as the delete, so a delete * never commits without its audit record, whether or not the OTLP pipeline is on. * @package App\Audit */ -final class SponsorExtraQuestionForceDeleteAuditLog +final class SponsorExtraQuestionDeleteAuditLog { public const TargetQuestion = 'question'; public const TargetOption = 'option'; - public const LogMessage = 'audit.sponsor_extra_question.force_deleted'; + public const LogMessage = 'audit.sponsor_extra_question.deleted'; /** * Persists the audit entry, call it within the transaction that performs the delete. @@ -74,7 +74,7 @@ public static function record $is_question = $target_type === self::TargetQuestion; $description = sprintf ( - "Sponsor %s %s '%s' (ID: %s) of Sponsor %s (Summit %s) force deleted by user %s (%s). Reason: %s. Collected answers deleted: %s, modified: %s", + "Sponsor %s %s '%s' (ID: %s) of Sponsor %s (Summit %s) deleted by user %s (%s). Reason: %s. Collected answers deleted: %s, modified: %s", $is_question ? 'extra question' : 'extra question option', $target_type, $target_label, @@ -99,7 +99,6 @@ public static function record 'audit.timestamp' => now()->toISOString(), 'audit.summit_id' => (string)$summit->getId(), 'audit.sponsor_id' => (string)$sponsor->getId(), - 'audit.force' => 'true', 'audit.reason' => $reason, 'audit.target_label' => $target_label, 'audit.answers_deleted' => $answers_deleted, diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php index fe12c5f757..1caccaf3c3 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php @@ -3685,8 +3685,8 @@ public function updateExtraQuestion($summit_id, $sponsor_id, $extra_question_id) #[OA\Delete( path: "/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}", - summary: 'Delete Sponsor Extra Question (admin force only)', - description: "Refused with 412 unless force=true is sent by an admin (Super Admins, Administrators or Summit Administrators allowed on the summit): devices may hold answers for this question that the server has not seen yet, so edit the wording or turn off Mandatory instead. A force delete needs a reason, is audited, and when the server holds answers it is refused with their count unless delete_answers=true. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators . ", " . IGroup::Sponsors . ", " . IGroup::SponsorExternalUsers, + summary: 'Delete Sponsor Extra Question (admin only)', + description: "Admin only (Super Admins, Administrators or Summit Administrators allowed on the summit), anyone else gets 403. Devices may hold answers for this question that the server has not seen yet, so the admin must have checked them first. It needs a reason and delete_answers (true or false), and is audited; when the server holds answers it is refused with their count unless delete_answers=true. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators, operationId: 'deleteSponsorExtraQuestion', tags: ['Sponsors'], x: [ @@ -3728,37 +3728,30 @@ public function updateExtraQuestion($summit_id, $sponsor_id, $extra_question_id) schema: new OA\Schema(type: 'integer'), description: 'The extra question id' ), - new OA\Parameter( - name: 'force', - in: 'query', - required: false, - schema: new OA\Schema(type: 'boolean'), - description: 'Admin only force delete. Without it the delete is always refused (412); a non-admin sending it gets 403' - ), new OA\Parameter( name: 'delete_answers', in: 'query', - required: false, + required: true, schema: new OA\Schema(type: 'boolean'), - description: 'Confirms that collected answers are destroyed. Required when the server holds answers, otherwise 412 with their count' + description: 'Required (true or false). With false and answers on the server it is refused with 412 and their count; true destroys them' ), ], requestBody: new OA\RequestBody( required: false, content: new OA\JsonContent( properties: [ - new OA\Property(property: 'reason', type: 'string', description: 'Required with force=true, goes to the audit entry'), + new OA\Property(property: 'reason', type: 'string', description: 'Required, goes to the audit entry'), ] ) ), responses: [ - new OA\Response(response: Response::HTTP_NO_CONTENT, description: 'Question force deleted'), + new OA\Response(response: Response::HTTP_NO_CONTENT, description: 'Question deleted'), new OA\Response( response: Response::HTTP_PRECONDITION_FAILED, - description: "Validation Error - questions can't be deleted once created (no force), a reason is missing, or collected answers would be destroyed and delete_answers is not true" + description: "Validation Error - a reason or delete_answers is missing, or collected answers would be destroyed and delete_answers is false" ), new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), - new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden - force=true is for admins only"), + new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden - admins only"), new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Not Found"), new OA\Response(response: Response::HTTP_INTERNAL_SERVER_ERROR, description: "Server Error"), ] @@ -3784,10 +3777,12 @@ public function deleteExtraQuestion($summit_id, $sponsor_id, $extra_question_id) if(!$current_member->isAuthzFor($summit, $sponsor)) throw new HTTP403ForbiddenException("You are not allowed to perform this action"); - $force_by = $this->getForceDeleteAdmin($current_member, $summit); - [$reason, $delete_answers] = $this->getForceDeleteParams(); + if (!$this->isAdminFor($current_member, $summit)) + throw new HTTP403ForbiddenException("Only an administrator can delete."); + + [$reason, $delete_answers] = $this->getDeleteParams(); - $this->service->deleteSponsorExtraQuestion($summit, intval($sponsor_id), intval($extra_question_id), $force_by, $reason, $delete_answers); + $this->service->deleteSponsorExtraQuestion($summit, intval($sponsor_id), intval($extra_question_id), $current_member, $reason, $delete_answers); return $this->deleted(); @@ -4007,8 +4002,8 @@ function ($value_id, $payload, $summit, $sponsor_id, $extra_question_id) { use ParametrizedDeleteEntity; #[OA\Delete( path: "/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}/values/{value_id}", - description: "Refused with 412 unless force=true is sent by an admin (Super Admins, Administrators or Summit Administrators allowed on the summit): devices may hold answers for this option that the server has not seen yet, so edit the wording instead. A force delete needs a reason and is audited. Collected answers that selected the option are deleted when it was their only selection, otherwise they only lose its id; when the server holds any, it is refused with their count unless delete_answers=true. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators . ", " . IGroup::Sponsors . ", " . IGroup::SponsorExternalUsers, - summary: 'Delete Extra Question Value (admin force only)', + description: "Admin only (Super Admins, Administrators or Summit Administrators allowed on the summit), anyone else gets 403. Devices may hold answers for this option that the server has not seen yet, so the admin must have checked them first. It needs a reason and delete_answers (true or false), and is audited. Collected answers that selected the option are deleted when it was their only selection, otherwise they only lose its id; when the server holds any, it is refused with their count unless delete_answers=true. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators, + summary: 'Delete Extra Question Value (admin only)', operationId: 'deleteSponsorExtraQuestionValue', tags: ['Sponsors'], x: [ @@ -4057,37 +4052,30 @@ function ($value_id, $payload, $summit, $sponsor_id, $extra_question_id) { schema: new OA\Schema(type: 'integer'), description: 'The question value id' ), - new OA\Parameter( - name: 'force', - in: 'query', - required: false, - schema: new OA\Schema(type: 'boolean'), - description: 'Admin only force delete. Without it the delete is always refused (412); a non-admin sending it gets 403' - ), new OA\Parameter( name: 'delete_answers', in: 'query', - required: false, + required: true, schema: new OA\Schema(type: 'boolean'), - description: 'Confirms that collected answers are changed. Required when the server holds answers that selected the option, otherwise 412 with their count' + description: 'Required (true or false). With false and answers that selected the option on the server it is refused with 412 and their count; true changes them' ), ], requestBody: new OA\RequestBody( required: false, content: new OA\JsonContent( properties: [ - new OA\Property(property: 'reason', type: 'string', description: 'Required with force=true, goes to the audit entry'), + new OA\Property(property: 'reason', type: 'string', description: 'Required, goes to the audit entry'), ] ) ), responses: [ - new OA\Response(response: Response::HTTP_NO_CONTENT, description: 'Option force deleted'), + new OA\Response(response: Response::HTTP_NO_CONTENT, description: 'Option deleted'), new OA\Response( response: Response::HTTP_PRECONDITION_FAILED, - description: "Validation Error - answer options can't be deleted once created (no force), a reason is missing, or collected answers would be changed and delete_answers is not true" + description: "Validation Error - a reason or delete_answers is missing, or collected answers would be changed and delete_answers is false" ), new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), - new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden - force=true is for admins only"), + new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden - admins only"), new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Not Found"), new OA\Response(response: Response::HTTP_INTERNAL_SERVER_ERROR, description: "Server Error") ] @@ -4115,11 +4103,13 @@ public function deleteExtraQuestionValue($summit_id, $sponsor_id, $extra_questio if(!$current_member->isAuthzFor($summit, $sponsor)) throw new HTTP403ForbiddenException("You are not allowed to perform this action"); - $force_by = $this->getForceDeleteAdmin($current_member, $summit); - [$reason, $delete_answers] = $this->getForceDeleteParams(); + if (!$this->isAdminFor($current_member, $summit)) + throw new HTTP403ForbiddenException("Only an administrator can delete."); - return $this->_delete($value_id, function ($value_id, $summit, $sponsor_id, $extra_question_id) use ($force_by, $reason, $delete_answers) { - $this->service->deleteExtraQuestionValue($summit, intval($sponsor_id), intval($extra_question_id), intval($value_id), $force_by, $reason, $delete_answers); + [$reason, $delete_answers] = $this->getDeleteParams(); + + return $this->_delete($value_id, function ($value_id, $summit, $sponsor_id, $extra_question_id) use ($current_member, $reason, $delete_answers) { + $this->service->deleteExtraQuestionValue($summit, intval($sponsor_id), intval($extra_question_id), intval($value_id), $current_member, $reason, $delete_answers); } , ...$args); }); @@ -4127,7 +4117,7 @@ public function deleteExtraQuestionValue($summit_id, $sponsor_id, $extra_questio } /** - * Force delete is for admins only: Super Admins, Administrators or Summit Administrators allowed on the summit. + * Delete is for admins only: Super Admins, Administrators or Summit Administrators allowed on the summit. * @param Member|null $member * @param Summit $summit * @return bool @@ -4138,40 +4128,24 @@ private function isAdminFor(?Member $member, Summit $summit): bool return $member->isAdmin() || ($member->isSummitAdmin() && $member->isSummitAllowed($summit)); } - /** - * The admin asking for a force delete (?force=true), null when it is a plain delete - * (the service refuses it for everyone). The flag never helps a sponsor. - * @param Member|null $member - * @param Summit $summit - * @return Member|null - * @throws HTTP403ForbiddenException - */ - private function getForceDeleteAdmin(?Member $member, Summit $summit): ?Member - { - $force = filter_var(request()->input('force', false), FILTER_VALIDATE_BOOLEAN); - if (!$force) return null; - if (!$this->isAdminFor($member, $summit)) - throw new HTTP403ForbiddenException("Only an administrator can force delete."); - return $member; - } - /** * reason and delete_answers can come in the body or the query string. - * @return array [?string $reason, bool $delete_answers] + * delete_answers is null when missing or not a boolean, the service refuses it (412). + * @return array [?string $reason, ?bool $delete_answers] */ - private function getForceDeleteParams(): array + private function getDeleteParams(): array { $request = request(); $reason = $request->input('reason'); return [ is_string($reason) ? $reason : null, - filter_var($request->input('delete_answers', false), FILTER_VALIDATE_BOOLEAN), + filter_var($request->input('delete_answers'), FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE), ]; } #[OA\Get( path: "/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}/usage", - description: "What a force delete of the question affects: collected answers on the server and the sponsor's badge scan activity (reps and their last scan). It does not prove devices have nothing pending. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators, + description: "What deleting the question affects: collected answers on the server and the sponsor's badge scan activity (reps and their last scan). It does not prove devices have nothing pending. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators, summary: 'Get Sponsor Extra Question usage', operationId: 'getSponsorExtraQuestionUsage', tags: ['Sponsors'], @@ -4228,7 +4202,7 @@ public function getExtraQuestionUsage($summit_id, $sponsor_id, $extra_question_i #[OA\Get( path: "/api/v1/summits/{id}/sponsors/{sponsor_id}/extra-questions/{extra_question_id}/values/{value_id}/usage", - description: "What a force delete of the option affects: collected answers that selected it (answers_to_delete when it was their only selection, answers_to_modify when they only lose its id) and the sponsor's badge scan activity. It does not prove devices have nothing pending. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators, + description: "What deleting the option affects: collected answers that selected it (answers_to_delete when it was their only selection, answers_to_modify when they only lose its id) and the sponsor's badge scan activity. It does not prove devices have nothing pending. required-groups " . IGroup::SuperAdmins . ", " . IGroup::Administrators . ", " . IGroup::SummitAdministrators, summary: 'Get Extra Question Value usage', operationId: 'getSponsorExtraQuestionValueUsage', tags: ['Sponsors'], diff --git a/app/Services/Model/ISummitSponsorService.php b/app/Services/Model/ISummitSponsorService.php index 5ec554202d..5ee65b688b 100644 --- a/app/Services/Model/ISummitSponsorService.php +++ b/app/Services/Model/ISummitSponsorService.php @@ -272,23 +272,23 @@ public function addSponsorExtraQuestion(Summit $summit, int $sponsor_id, array $ public function updateSponsorExtraQuestion(Summit $summit, int $sponsor_id, int $extra_question_id, array $payload):SummitSponsorExtraQuestionType; /** - * Refused for everyone unless $force_by is given. $force_by is an admin the caller - * already authorized: a device may hold answers the server has not seen, and only an - * admin who checked the devices can tell a delete is safe. + * $deleted_by is an admin the caller already authorized: a device may hold answers the + * server has not seen, and only an admin who checked the devices can tell a delete is safe. + * Refused when the server holds answers and $delete_answers is false. * @param Summit $summit * @param int $sponsor_id * @param int $extra_question_id - * @param Member|null $force_by admin forcing the delete, null refuses it - * @param string|null $reason required when forcing, goes to the audit entry + * @param Member $deleted_by the admin deleting, goes to the audit entry + * @param string|null $reason required (ValidationException if blank), goes to the audit entry * @param bool $delete_answers confirms the collected answers of the question are destroyed * @return void * @throws EntityNotFoundException * @throws ValidationException */ - public function deleteSponsorExtraQuestion(Summit $summit, int $sponsor_id, int $extra_question_id, ?Member $force_by = null, ?string $reason = null, bool $delete_answers = false):void; + public function deleteSponsorExtraQuestion(Summit $summit, int $sponsor_id, int $extra_question_id, Member $deleted_by, ?string $reason, ?bool $delete_answers):void; /** - * What a force delete of the question would affect. + * What deleting the question would affect. * @param Summit $summit * @param int $sponsor_id * @param int $extra_question_id @@ -319,24 +319,24 @@ public function addExtraQuestionValue(Summit $summit, int $sponsor_id ,int $ques public function updateExtraQuestionValue(Summit $summit, int $sponsor_id, int $question_id, int $value_id, array $payload):ExtraQuestionTypeValue; /** - * Refused for everyone unless $force_by is given, see deleteSponsorExtraQuestion. + * Admin only, see deleteSponsorExtraQuestion. * Collected answers that selected the option are deleted when it was their only * selection, otherwise they only lose its id. * @param Summit $summit * @param int $sponsor_id * @param int $question_id * @param int $value_id - * @param Member|null $force_by admin forcing the delete, null refuses it - * @param string|null $reason required when forcing, goes to the audit entry + * @param Member $deleted_by the admin deleting, goes to the audit entry + * @param string|null $reason required (ValidationException if blank), goes to the audit entry * @param bool $delete_answers confirms the collected answers that selected the option are changed * @return void * @throws EntityNotFoundException * @throws ValidationException */ - public function deleteExtraQuestionValue(Summit $summit, int $sponsor_id, int $question_id, int $value_id, ?Member $force_by = null, ?string $reason = null, bool $delete_answers = false):void; + public function deleteExtraQuestionValue(Summit $summit, int $sponsor_id, int $question_id, int $value_id, Member $deleted_by, ?string $reason, ?bool $delete_answers):void; /** - * What a force delete of the option would affect. + * What deleting the option would affect. * @param Summit $summit * @param int $sponsor_id * @param int $question_id diff --git a/app/Services/Model/Imp/SummitSponsorService.php b/app/Services/Model/Imp/SummitSponsorService.php index 3d003914bc..bfb14de56d 100644 --- a/app/Services/Model/Imp/SummitSponsorService.php +++ b/app/Services/Model/Imp/SummitSponsorService.php @@ -12,7 +12,7 @@ * limitations under the License. **/ -use App\Audit\SponsorExtraQuestionForceDeleteAuditLog; +use App\Audit\SponsorExtraQuestionDeleteAuditLog; use App\Events\SponsorServices\SponsorDomainEvents; use App\Events\SponsorServices\SummitSponsorCreatedEventDTO; use App\Events\SponsorServices\SummitSponsorshipCreatedEventDTO; @@ -1109,22 +1109,15 @@ public function deleteSponsorExtraQuestion Summit $summit, int $sponsor_id, int $extra_question_id, - ?Member $force_by = null, - ?string $reason = null, - bool $delete_answers = false + Member $deleted_by, + ?string $reason, + ?bool $delete_answers ): void { - if (is_null($force_by)) { - // Refused: a device may hold answers for this question that the server has not seen yet. - // Only an admin who checked the devices can force it (see $force_by). - throw new ValidationException( - "Questions can't be deleted once created, because devices may already be using them. You can edit the wording, turn off Mandatory, or contact support." - ); - } - - $reason = $this->requireForceDeleteReason($reason); + $reason = $this->requireDeleteReason($reason); + $delete_answers = $this->requireDeleteAnswers($delete_answers); - $audit_data = $this->tx_service->transaction(function () use ($summit, $sponsor_id, $extra_question_id, $force_by, $reason, $delete_answers) { + $audit_data = $this->tx_service->transaction(function () use ($summit, $sponsor_id, $extra_question_id, $deleted_by, $reason, $delete_answers) { $summit_sponsor = $summit->getSummitSponsorById($sponsor_id); if (is_null($summit_sponsor)) throw new EntityNotFoundException("Sponsor not found."); @@ -1154,12 +1147,12 @@ public function deleteSponsorExtraQuestion $label = $extra_question->getLabel(); $summit_sponsor->removeExtraQuestion($extra_question); - return SponsorExtraQuestionForceDeleteAuditLog::record( + return SponsorExtraQuestionDeleteAuditLog::record( $this->audit_log_repository, - $force_by, + $deleted_by, $summit, $summit_sponsor, - SponsorExtraQuestionForceDeleteAuditLog::TargetQuestion, + SponsorExtraQuestionDeleteAuditLog::TargetQuestion, $extra_question_id, $label, $reason, @@ -1167,7 +1160,7 @@ public function deleteSponsorExtraQuestion ); }); - SponsorExtraQuestionForceDeleteAuditLog::emit($audit_data); + SponsorExtraQuestionDeleteAuditLog::emit($audit_data); } /** @@ -1214,16 +1207,28 @@ private function removeBadgeScanAnswer(SponsorBadgeScanExtraQuestionAnswer $answ * @return string * @throws ValidationException */ - private function requireForceDeleteReason(?string $reason): string + private function requireDeleteReason(?string $reason): string { $reason = trim($reason ?? ''); if ($reason === '') - throw new ValidationException("A reason is required to force delete."); + throw new ValidationException("A reason is required to delete."); return $reason; } /** - * Badge scan activity of the sponsor, to see what is at stake before a force delete. + * @param bool|null $delete_answers + * @return bool + * @throws ValidationException + */ + private function requireDeleteAnswers(?bool $delete_answers): bool + { + if (is_null($delete_answers)) + throw new ValidationException("delete_answers is required (true or false)."); + return $delete_answers; + } + + /** + * Badge scan activity of the sponsor, to see what is at stake before deleting. * It does not prove devices have nothing pending, only the devices can show that. * @param Sponsor $sponsor * @return array @@ -1341,22 +1346,15 @@ public function deleteExtraQuestionValue int $sponsor_id, int $question_id, int $value_id, - ?Member $force_by = null, - ?string $reason = null, - bool $delete_answers = false + Member $deleted_by, + ?string $reason, + ?bool $delete_answers ): void { - if (is_null($force_by)) { - // Refused: a device may hold answers for this option that the server has not seen yet. - // Only an admin who checked the devices can force it (see $force_by). - throw new ValidationException( - "Answer options can't be deleted once created, because devices may already be using them. You can edit the wording, or contact support." - ); - } - - $reason = $this->requireForceDeleteReason($reason); + $reason = $this->requireDeleteReason($reason); + $delete_answers = $this->requireDeleteAnswers($delete_answers); - $audit_data = $this->tx_service->transaction(function () use ($summit, $sponsor_id, $question_id, $value_id, $force_by, $reason, $delete_answers) { + $audit_data = $this->tx_service->transaction(function () use ($summit, $sponsor_id, $question_id, $value_id, $deleted_by, $reason, $delete_answers) { $summit_sponsor = $summit->getSummitSponsorById($sponsor_id); if (is_null($summit_sponsor)) throw new EntityNotFoundException("Sponsor not found."); @@ -1394,12 +1392,12 @@ public function deleteExtraQuestionValue $label = $value->getLabel(); $extra_question->removeValue($value); - return SponsorExtraQuestionForceDeleteAuditLog::record( + return SponsorExtraQuestionDeleteAuditLog::record( $this->audit_log_repository, - $force_by, + $deleted_by, $summit, $summit_sponsor, - SponsorExtraQuestionForceDeleteAuditLog::TargetOption, + SponsorExtraQuestionDeleteAuditLog::TargetOption, $value_id, $label, $reason, @@ -1408,7 +1406,7 @@ public function deleteExtraQuestionValue ); }); - SponsorExtraQuestionForceDeleteAuditLog::emit($audit_data); + SponsorExtraQuestionDeleteAuditLog::emit($audit_data); } /** diff --git a/database/migrations/config/Version20261009130000.php b/database/migrations/config/Version20261009130000.php index ed899f27c9..c18d3a0d2c 100644 --- a/database/migrations/config/Version20261009130000.php +++ b/database/migrations/config/Version20261009130000.php @@ -19,7 +19,7 @@ /** * Register the sponsor extra question and answer option usage endpoints, the pre-check an admin - * reads before force deleting. + * reads before deleting, and make the DELETE endpoints of both admin only (sponsor groups removed). * * ApiEndpointsSeeder covers fresh installs only -- the k8s deploy flow does not re-run * seeders -- and OAuth2BearerAccessTokenRequestValidator rejects any route missing from @@ -72,18 +72,40 @@ final class Version20261009130000 extends AbstractMigration ], ]; + private const DELETE_ENDPOINTS = [ + 'delete-sponsor-extra-question', + 'delete-sponsor-extra-question-value', + ]; + + private const SPONSOR_GROUPS = [ + IGroup::Sponsors, + IGroup::SponsorExternalUsers, + ]; + public function getDescription(): string { - return 'Register the sponsor extra question and answer option usage endpoints.'; + return 'Register the sponsor extra question and answer option usage endpoints, make their DELETE admin only.'; } public function up(Schema $schema): void { $this->registerEndpoints(self::API_NAME, self::ENDPOINTS); + + foreach (self::DELETE_ENDPOINTS as $endpoint) { + foreach (self::SPONSOR_GROUPS as $group) { + $this->addSql($this->deleteEndpointAuthzGroup(self::API_NAME, $endpoint, $group)); + } + } } public function down(Schema $schema): void { + foreach (self::DELETE_ENDPOINTS as $endpoint) { + foreach (self::SPONSOR_GROUPS as $group) { + $this->addSql($this->insertEndpointAuthzGroup(self::API_NAME, $endpoint, $group)); + } + } + $this->unregisterEndpoints(self::API_NAME, array_column(self::ENDPOINTS, 'name')); } } diff --git a/database/seeders/ApiEndpointsSeeder.php b/database/seeders/ApiEndpointsSeeder.php index f0ca8306ee..af083c4c3a 100644 --- a/database/seeders/ApiEndpointsSeeder.php +++ b/database/seeders/ApiEndpointsSeeder.php @@ -2578,8 +2578,6 @@ private function seedSummitEndpoints() IGroup::SuperAdmins, IGroup::Administrators, IGroup::SummitAdministrators, - IGroup::Sponsors, - IGroup::SponsorExternalUsers, ] ], [ @@ -2643,8 +2641,6 @@ private function seedSummitEndpoints() IGroup::SuperAdmins, IGroup::Administrators, IGroup::SummitAdministrators, - IGroup::Sponsors, - IGroup::SponsorExternalUsers, ] ], [ diff --git a/tests/oauth2/OAuth2SummitSponsorApiTest.php b/tests/oauth2/OAuth2SummitSponsorApiTest.php index c7b447b594..47384ca961 100644 --- a/tests/oauth2/OAuth2SummitSponsorApiTest.php +++ b/tests/oauth2/OAuth2SummitSponsorApiTest.php @@ -691,28 +691,29 @@ public function testUpdateSponsorExtraQuestionsBySponsor(){ return $question; } - public function testDeleteSponsorExtraQuestionsBySponsor(){ + public function testDeleteSponsorExtraQuestionsByAdmin(){ $q = $this->testAddSponsorExtraQuestions(); $params = [ 'id' => self::$summit->getId(), 'sponsor_id' => self::$sponsors[0]->getId(), - 'extra_question_id' => $q->id + 'extra_question_id' => $q->id, + 'delete_answers' => 'false', ]; - $response = $this->action( + $this->action( "DELETE", "OAuth2SummitSponsorApiController@deleteExtraQuestion", $params, [], [], [], - $this->getAuthHeaders() + $this->getAuthHeaders(), + json_encode(['reason' => 'created by mistake']) ); - // questions can never be deleted: devices may hold answers the server has not seen - $this->assertResponseStatus(412); - $this->assertStringContainsString("Questions can't be deleted once created", $response->getContent()); + // admin only, with a reason and an explicit delete_answers + $this->assertResponseStatus(204); } public function testAddLeadReportSettings(){ @@ -1605,24 +1606,25 @@ public function testDeleteExtraQuestionValue(){ 'sponsor_id' => self::$sponsors[0]->getId(), 'extra_question_id' => $result->question_id, 'value_id' => $result->value->id, + 'delete_answers' => 'false', ]; - $response = $this->action( + $this->action( "DELETE", "OAuth2SummitSponsorApiController@deleteExtraQuestionValue", $params, [], [], [], - $this->getAuthHeaders() + $this->getAuthHeaders(), + json_encode(['reason' => 'created by mistake']) ); - // answer options can never be deleted: devices may hold answers the server has not seen - $this->assertResponseStatus(412); - $this->assertStringContainsString("Answer options can't be deleted once created", $response->getContent()); + // admin only, with a reason and an explicit delete_answers + $this->assertResponseStatus(204); } - // ---- Force delete is for admins only ---- + // ---- Delete is for admins only ---- /** * Makes the user a member of sponsors[0] so the sponsor authz passes. It has to run @@ -1655,7 +1657,7 @@ private function useSponsorOnlyToken(): void $this->app->singleton(IAccessTokenService::class, function () use ($stub) { return $stub; }); } - public function testForceDeleteSponsorExtraQuestionBySponsorIsForbidden(){ + public function testDeleteSponsorExtraQuestionBySponsorIsForbidden(){ $this->makeMemberSponsorUser(); $this->useSponsorOnlyToken(); $q = $this->testAddSponsorExtraQuestions(); @@ -1666,15 +1668,11 @@ public function testForceDeleteSponsorExtraQuestionBySponsorIsForbidden(){ 'extra_question_id' => $q->id, ]; - // the sponsor is authorized on the sponsor: a plain delete is refused... - $this->action("DELETE", "OAuth2SummitSponsorApiController@deleteExtraQuestion", $params, [], [], [], $this->getAuthHeaders()); - $this->assertResponseStatus(412); - - // ...and the flag never helps a sponsor + // the sponsor is authorized on the sponsor, but delete is admin only $this->action( "DELETE", "OAuth2SummitSponsorApiController@deleteExtraQuestion", - array_merge($params, ['force' => 'true']), + array_merge($params, ['delete_answers' => 'true']), [], [], [], @@ -1684,7 +1682,7 @@ public function testForceDeleteSponsorExtraQuestionBySponsorIsForbidden(){ $this->assertResponseStatus(403); } - public function testForceDeleteExtraQuestionValueBySponsorIsForbidden(){ + public function testDeleteExtraQuestionValueBySponsorIsForbidden(){ $this->makeMemberSponsorUser(); $this->useSponsorOnlyToken(); $result = $this->testAddExtraQuestionValue(); @@ -1696,13 +1694,10 @@ public function testForceDeleteExtraQuestionValueBySponsorIsForbidden(){ 'value_id' => $result->value->id, ]; - $this->action("DELETE", "OAuth2SummitSponsorApiController@deleteExtraQuestionValue", $params, [], [], [], $this->getAuthHeaders()); - $this->assertResponseStatus(412); - $this->action( "DELETE", "OAuth2SummitSponsorApiController@deleteExtraQuestionValue", - array_merge($params, ['force' => 'true']), + array_merge($params, ['delete_answers' => 'true']), [], [], [], diff --git a/tests/oauth2/OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php b/tests/oauth2/OAuth2SummitSponsorExtraQuestionDeleteApiTest.php similarity index 85% rename from tests/oauth2/OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php rename to tests/oauth2/OAuth2SummitSponsorExtraQuestionDeleteApiTest.php index 8127d4a7b6..285ac6177d 100644 --- a/tests/oauth2/OAuth2SummitSponsorExtraQuestionForceDeleteApiTest.php +++ b/tests/oauth2/OAuth2SummitSponsorExtraQuestionDeleteApiTest.php @@ -27,13 +27,13 @@ use models\utils\SilverstripeBaseModel; /** - * Class OAuth2SummitSponsorExtraQuestionForceDeleteApiTest + * Class OAuth2SummitSponsorExtraQuestionDeleteApiTest * Sponsors can never delete an extra question or an answer option (devices may hold answers - * the server has not seen); an admin can force it, with a reason, an explicit confirmation + * the server has not seen); an admin can, with a reason, an explicit delete_answers * when collected answers are destroyed, and an audit entry. * The current user is an administrator; the sponsor user cases live in OAuth2SummitSponsorApiTest. */ -final class OAuth2SummitSponsorExtraQuestionForceDeleteApiTest extends ProtectedApiTestCase +final class OAuth2SummitSponsorExtraQuestionDeleteApiTest extends ProtectedApiTestCase { use InsertSummitTestData; @@ -84,8 +84,8 @@ private function addQuestion(string $type, array $option_labels = []): array $question = new SummitSponsorExtraQuestionType(); $question->setType($type); - $question->setLabel('Force delete question'); - $question->setName('FORCE_DELETE_' . str_random(5)); + $question->setLabel('Delete question'); + $question->setName('DELETE_' . str_random(5)); $values = []; foreach ($option_labels as $label) { $value = new ExtraQuestionTypeValue(); @@ -196,7 +196,7 @@ private function deleteValue(int $question_id, int $value_id, array $query = [], } /** - * The force delete audit entry that was emitted, if any. + * The delete audit entry that was emitted, if any. * @return EmitAuditLogJob|null */ private function emittedAudit(): ?EmitAuditLogJob @@ -206,7 +206,7 @@ private function emittedAudit(): ?EmitAuditLogJob } /** - * The force delete audit records persisted for the given target. + * The delete audit records persisted for the given target. * @param string $target_type question | option * @param int $target_id * @return SummitAuditLog[] @@ -229,45 +229,45 @@ protected function enableAuditCapture(): void // ---- questions ---- - public function testAdminDeleteQuestionWithoutForceIsRefused() + public function testAdminDeleteQuestionWithoutDeleteAnswersIsRefused() { [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); - $response = $this->deleteQuestion($question_id); + $response = $this->deleteQuestion($question_id, [], ['reason' => 'cleanup']); $this->assertResponseStatus(412); - $this->assertStringContainsString("Questions can't be deleted once created", $response->getContent()); + $this->assertStringContainsString("delete_answers is required", $response->getContent()); $this->assertContains($question_id, $this->sponsorQuestionIds()); } - public function testAdminForceDeleteQuestionWithoutReasonIsRefused() + public function testAdminDeleteQuestionWithoutReasonIsRefused() { [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); - $response = $this->deleteQuestion($question_id, ['force' => 'true']); + $response = $this->deleteQuestion($question_id, ['delete_answers' => 'false']); $this->assertResponseStatus(412); $this->assertStringContainsString("A reason is required", $response->getContent()); - $this->deleteQuestion($question_id, ['force' => 'true'], ['reason' => ' ']); + $this->deleteQuestion($question_id, ['delete_answers' => 'false'], ['reason' => ' ']); $this->assertResponseStatus(412); $this->assertContains($question_id, $this->sponsorQuestionIds()); } - public function testAdminForceDeleteUnknownQuestionIsNotFound() + public function testAdminDeleteUnknownQuestionIsNotFound() { - $this->deleteQuestion(999999999, ['force' => 'true'], ['reason' => 'cleanup']); + $this->deleteQuestion(999999999, ['delete_answers' => 'false'], ['reason' => 'cleanup']); $this->assertResponseStatus(404); } - public function testAdminForceDeleteQuestionWithoutAnswersFreesTheSlotAndIsAudited() + public function testAdminDeleteQuestionWithoutAnswersFreesTheSlotAndIsAudited() { $this->enableAuditCapture(); [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); // the sponsor is at the cap $this->assertCount(5, $this->sponsorQuestionIds()); - $this->deleteQuestion($question_id, ['force' => 'true'], ['reason' => 'created by mistake, 0 pending uploads']); + $this->deleteQuestion($question_id, ['delete_answers' => 'false'], ['reason' => 'created by mistake, 0 pending uploads']); $this->assertResponseStatus(204); $this->assertCount(4, $this->sponsorQuestionIds()); @@ -308,14 +308,14 @@ public function testAdminForceDeleteQuestionWithoutAnswersFreesTheSlotAndIsAudit $this->assertCount(5, $this->sponsorQuestionIds()); } - public function testAdminForceDeleteQuestionWithAnswersIsRefusedWithTheirCount() + public function testAdminDeleteQuestionWithAnswersIsRefusedWithTheirCount() { [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); $this->addScanWithAnswer($question_id, 'one', 1); $this->addScanWithAnswer($question_id, 'two', 2); $this->addScanWithAnswer($question_id, 'three', 3); - $response = $this->deleteQuestion($question_id, ['force' => 'true'], ['reason' => 'sponsor insists']); + $response = $this->deleteQuestion($question_id, ['delete_answers' => 'false'], ['reason' => 'sponsor insists']); $this->assertResponseStatus(412); $this->assertStringContainsString("3 collected answers will be permanently deleted", $response->getContent()); @@ -325,7 +325,7 @@ public function testAdminForceDeleteQuestionWithAnswersIsRefusedWithTheirCount() $this->assertCount(0, $this->persistedAudits('question', $question_id)); } - public function testAdminForceDeleteQuestionWithAnswersAndConfirmationDestroysThemAndIsAudited() + public function testAdminDeleteQuestionWithAnswersAndConfirmationDestroysThemAndIsAudited() { $this->enableAuditCapture(); [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); @@ -335,7 +335,7 @@ public function testAdminForceDeleteQuestionWithAnswersAndConfirmationDestroysTh $this->addScanWithAnswer($question_id, 'three', 3), ]; - $this->deleteQuestion($question_id, ['force' => 'true', 'delete_answers' => 'true'], ['reason' => 'sponsor insists']); + $this->deleteQuestion($question_id, ['delete_answers' => 'true'], ['reason' => 'sponsor insists']); $this->assertResponseStatus(204); $this->assertNotContains($question_id, $this->sponsorQuestionIds()); @@ -352,13 +352,13 @@ public function testAdminForceDeleteQuestionWithAnswersAndConfirmationDestroysTh $this->assertEquals('sponsor insists', $audit->auditData['audit.reason']); } - public function testAdminForceDeleteQuestionIsPersistedInTheAuditLogWhenOpenTelemetryIsOff() + public function testAdminDeleteQuestionIsPersistedInTheAuditLogWhenOpenTelemetryIsOff() { config(['opentelemetry.enabled' => false]); Bus::fake([EmitAuditLogJob::class]); [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); - $this->deleteQuestion($question_id, ['force' => 'true'], ['reason' => 'created by mistake']); + $this->deleteQuestion($question_id, ['delete_answers' => 'false'], ['reason' => 'created by mistake']); $this->assertResponseStatus(204); $this->assertNull($this->emittedAudit()); @@ -369,31 +369,31 @@ public function testAdminForceDeleteQuestionIsPersistedInTheAuditLogWhenOpenTele // ---- options ---- - public function testAdminDeleteOptionWithoutForceIsRefused() + public function testAdminDeleteOptionWithoutDeleteAnswersIsRefused() { [$question_id, [$a]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B']); - $response = $this->deleteValue($question_id, $a); + $response = $this->deleteValue($question_id, $a, [], ['reason' => 'cleanup']); $this->assertResponseStatus(412); - $this->assertStringContainsString("Answer options can't be deleted once created", $response->getContent()); + $this->assertStringContainsString("delete_answers is required", $response->getContent()); } - public function testAdminForceDeleteOptionWithoutReasonIsRefused() + public function testAdminDeleteOptionWithoutReasonIsRefused() { [$question_id, [$a]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B']); - $this->deleteValue($question_id, $a, ['force' => 'true']); + $this->deleteValue($question_id, $a, ['delete_answers' => 'false']); $this->assertResponseStatus(412); } - public function testAdminForceDeleteUnusedOptionDeletesIt() + public function testAdminDeleteUnusedOptionDeletesIt() { $this->enableAuditCapture(); [$question_id, [$a, $b]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B']); // only B is used $this->addScanWithAnswer($question_id, (string)$b, 1); - $this->deleteValue($question_id, $a, ['force' => 'true'], ['reason' => 'option created by mistake']); + $this->deleteValue($question_id, $a, ['delete_answers' => 'false'], ['reason' => 'option created by mistake']); $this->assertResponseStatus(204); $audit = $this->emittedAudit(); @@ -404,21 +404,21 @@ public function testAdminForceDeleteUnusedOptionDeletesIt() $this->assertCount(1, $this->answersOf($question_id)); } - public function testAdminForceDeleteUsedOptionIsRefusedWithTheCountOfDeletedAndModifiedAnswers() + public function testAdminDeleteUsedOptionIsRefusedWithTheCountOfDeletedAndModifiedAnswers() { [$question_id, [$a, $b, $c]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B', 'C']); $this->addScanWithAnswer($question_id, (string)$a, 1); // only A: deleted $this->addScanWithAnswer($question_id, "$a,$b", 2); // A and B: loses A $this->addScanWithAnswer($question_id, "$b,$c", 3); // no A: untouched - $response = $this->deleteValue($question_id, $a, ['force' => 'true'], ['reason' => 'sponsor insists']); + $response = $this->deleteValue($question_id, $a, ['delete_answers' => 'false'], ['reason' => 'sponsor insists']); $this->assertResponseStatus(412); $this->assertStringContainsString("2 collected answers use this option: 1 will be permanently deleted and 1 will only lose this option", $response->getContent()); $this->assertCount(3, $this->answersOf($question_id)); } - public function testAdminForceDeleteUsedOptionWithConfirmationDeletesOnlyAnswersWithNoOtherOptionAndStripsTheRest() + public function testAdminDeleteUsedOptionWithConfirmationDeletesOnlyAnswersWithNoOtherOptionAndStripsTheRest() { $this->enableAuditCapture(); [$question_id, [$a, $b, $c]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B', 'C']); @@ -427,7 +427,7 @@ public function testAdminForceDeleteUsedOptionWithConfirmationDeletesOnlyAnswers $b_and_c = $this->addScanWithAnswer($question_id, "$b,$c", 3); $only_c = $this->addScanWithAnswer($question_id, (string)$c, 4); - $this->deleteValue($question_id, $a, ['force' => 'true', 'delete_answers' => 'true'], ['reason' => 'sponsor insists']); + $this->deleteValue($question_id, $a, ['delete_answers' => 'true'], ['reason' => 'sponsor insists']); $this->assertResponseStatus(204); $this->assertNull($this->answerValueOfScan($only_a), 'the answer that only selected the option is deleted'); @@ -446,7 +446,7 @@ public function testAdminForceDeleteUsedOptionWithConfirmationDeletesOnlyAnswers $this->assertStringContainsString('Collected answers deleted: 1, modified: 1', $logs[0]->getAction()); // the option is gone - $this->deleteValue($question_id, $a, ['force' => 'true'], ['reason' => 'again']); + $this->deleteValue($question_id, $a, ['delete_answers' => 'false'], ['reason' => 'again']); $this->assertResponseStatus(404); } From a9196a0207f22fac826f1d6344944a9c34ddc030 Mon Sep 17 00:00:00 2001 From: romanetar Date: Sun, 11 Oct 2026 15:14:54 +0200 Subject: [PATCH 4/8] fix(sponsors): lock the question while deleting it so the audit count is exact Review feedback on #625. ExtraQuestionAnswer.QuestionID is ON DELETE CASCADE, so the delete never fails on the FK: an answer committed between reading the answers and the flush is cascade deleted silently and never counted in audit.answers_deleted. Badge scan ingestion holds no question scoped lock and the transaction runs at READ COMMITTED. Take a PESSIMISTIC_WRITE lock on the question row at the top of the transaction, for both question and option deletes. The ingestion INSERT already takes a shared lock on that row when InnoDB checks the FK, so both sides are serialized without touching addBadgeScanLocked. The usage endpoints are read only and take no lock. Also fix the comment that said the FK had no cascade: the explicit removal exists so the delete goes through the ORM (orphan removal, LastEdited, onFlush audit). --- .../ISponsorExtraQuestionTypeRepository.php | 10 +++++++++- .../DoctrineSponsorExtraQuestionTypeRepository.php | 9 +++++++++ app/Services/Model/Imp/SummitSponsorService.php | 10 +++++++++- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php b/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php index b662af8f93..7419141841 100644 --- a/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php +++ b/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php @@ -27,4 +27,12 @@ interface ISponsorExtraQuestionTypeRepository extends IExtraQuestionTypeReposito * @return SponsorBadgeScanExtraQuestionAnswer[] */ public function getBadgeScanAnswersByQuestion(ExtraQuestionType $question): array; -} \ No newline at end of file + + /** + * Takes a write lock on the question row, to be called at the top of a transaction. + * Badge scan ingestion takes a shared lock on that row when InnoDB checks the answer FK, so + * the answers read afterwards can't change until the transaction ends. + * @param ExtraQuestionType $question + */ + public function lockQuestion(ExtraQuestionType $question): void; +} diff --git a/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php b/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php index 2268ce4002..cf42009d07 100644 --- a/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php +++ b/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php @@ -19,6 +19,7 @@ use App\Repositories\Main\DoctrineExtraQuestionTypeRepository; use models\summit\Sponsor; use models\summit\SponsorBadgeScanExtraQuestionAnswer; +use Doctrine\DBAL\LockMode; use utils\DoctrineLeftJoinFilterMapping; use utils\Filter; use utils\Order; @@ -119,4 +120,12 @@ public function getBadgeScanAnswersByQuestion(ExtraQuestionType $question): arra ->getQuery() ->getResult(); } + + /** + * @inheritDoc + */ + public function lockQuestion(ExtraQuestionType $question): void + { + $this->getEntityManager()->lock($question, LockMode::PESSIMISTIC_WRITE); + } } diff --git a/app/Services/Model/Imp/SummitSponsorService.php b/app/Services/Model/Imp/SummitSponsorService.php index bfb14de56d..e3aba2febc 100644 --- a/app/Services/Model/Imp/SummitSponsorService.php +++ b/app/Services/Model/Imp/SummitSponsorService.php @@ -1127,6 +1127,10 @@ public function deleteSponsorExtraQuestion if (!$extra_question instanceof SummitSponsorExtraQuestionType) throw new EntityNotFoundException("Sponsor extra question not found."); + // serialize with badge scan ingestion: an answer committed after the read below would be + // cascade deleted (ExtraQuestionAnswer.QuestionID is ON DELETE CASCADE) without being counted + $this->repository->lockQuestion($extra_question); + $answers = $this->repository->getBadgeScanAnswersByQuestion($extra_question); $answers_count = count($answers); @@ -1139,7 +1143,8 @@ public function deleteSponsorExtraQuestion ); } - // the answers reference the question (FK without cascade), so they go first + // the FK would cascade the delete anyway, the explicit removal goes through the ORM so + // orphan removal, LastEdited and the onFlush audit run, and the count is exact (see lockQuestion) foreach ($answers as $answer) { $this->removeBadgeScanAnswer($answer); } @@ -1368,6 +1373,9 @@ public function deleteExtraQuestionValue if (is_null($value)) throw new EntityNotFoundException("value not found"); + // serialize with badge scan ingestion, so the answers split below are the ones that get changed + $this->repository->lockQuestion($extra_question); + [$to_delete, $to_modify] = $this->splitAnswersUsingOption($extra_question, $value_id); if ((count($to_delete) + count($to_modify)) > 0 && !$delete_answers) { From 2a4b7d3f4e06c33dc0ed6f53c914875979bd1870 Mon Sep 17 00:00:00 2001 From: romanetar Date: Sun, 11 Oct 2026 15:19:41 +0200 Subject: [PATCH 5/8] docs(sponsors): document reason and delete_answers in both the query string and the body Review feedback on #625. The controller reads reason and delete_answers with $request->input(), which merges the body and the query string, and the PR relies on the all-query form for clients and proxies that drop the body of a DELETE. The OpenAPI annotations only documented delete_answers in the query and reason in the body, so a generated client could not build that request, and the alternate path had no test. Document reason as an optional query parameter and delete_answers as a body property on both DELETE operations (required in one of the two places), and test the all-query form for a question and an option and the all-body form for a question. --- .../OAuth2SummitSponsorApiController.php | 28 +++++++++++---- ...ummitSponsorExtraQuestionDeleteApiTest.php | 36 +++++++++++++++++++ 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php index 1caccaf3c3..a64a7e7868 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php @@ -3731,16 +3731,24 @@ public function updateExtraQuestion($summit_id, $sponsor_id, $extra_question_id) new OA\Parameter( name: 'delete_answers', in: 'query', - required: true, + required: false, schema: new OA\Schema(type: 'boolean'), - description: 'Required (true or false). With false and answers on the server it is refused with 412 and their count; true destroys them' + description: 'Required, true or false, in the query string or in the body. With false and answers on the server it is refused with 412 and their count; true destroys them' + ), + new OA\Parameter( + name: 'reason', + in: 'query', + required: false, + schema: new OA\Schema(type: 'string'), + description: 'Alternative to the body property for clients that cannot send a DELETE body. Required in one of the two, goes to the audit entry' ), ], requestBody: new OA\RequestBody( required: false, content: new OA\JsonContent( properties: [ - new OA\Property(property: 'reason', type: 'string', description: 'Required, goes to the audit entry'), + new OA\Property(property: 'reason', type: 'string', description: 'Required (here or in the query string), goes to the audit entry'), + new OA\Property(property: 'delete_answers', type: 'boolean', description: 'Alternative to the query parameter, required (here or in the query string)'), ] ) ), @@ -4055,16 +4063,24 @@ function ($value_id, $payload, $summit, $sponsor_id, $extra_question_id) { new OA\Parameter( name: 'delete_answers', in: 'query', - required: true, + required: false, schema: new OA\Schema(type: 'boolean'), - description: 'Required (true or false). With false and answers that selected the option on the server it is refused with 412 and their count; true changes them' + description: 'Required, true or false, in the query string or in the body. With false and answers that selected the option on the server it is refused with 412 and their count; true changes them' + ), + new OA\Parameter( + name: 'reason', + in: 'query', + required: false, + schema: new OA\Schema(type: 'string'), + description: 'Alternative to the body property for clients that cannot send a DELETE body. Required in one of the two, goes to the audit entry' ), ], requestBody: new OA\RequestBody( required: false, content: new OA\JsonContent( properties: [ - new OA\Property(property: 'reason', type: 'string', description: 'Required, goes to the audit entry'), + new OA\Property(property: 'reason', type: 'string', description: 'Required (here or in the query string), goes to the audit entry'), + new OA\Property(property: 'delete_answers', type: 'boolean', description: 'Alternative to the query parameter, required (here or in the query string)'), ] ) ), diff --git a/tests/oauth2/OAuth2SummitSponsorExtraQuestionDeleteApiTest.php b/tests/oauth2/OAuth2SummitSponsorExtraQuestionDeleteApiTest.php index 285ac6177d..fb63247121 100644 --- a/tests/oauth2/OAuth2SummitSponsorExtraQuestionDeleteApiTest.php +++ b/tests/oauth2/OAuth2SummitSponsorExtraQuestionDeleteApiTest.php @@ -367,6 +367,31 @@ public function testAdminDeleteQuestionIsPersistedInTheAuditLogWhenOpenTelemetry $this->assertStringContainsString('Reason: created by mistake', $logs[0]->getAction()); } + public function testAdminDeleteQuestionWithReasonAndDeleteAnswersInTheQueryString() + { + [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); + $this->addScanWithAnswer($question_id, 'one', 1); + + // for clients that can't send a DELETE body + $this->deleteQuestion($question_id, ['delete_answers' => 'true', 'reason' => 'no body']); + + $this->assertResponseStatus(204); + $this->assertNotContains($question_id, $this->sponsorQuestionIds()); + $this->assertCount(0, $this->answersOf($question_id)); + } + + public function testAdminDeleteQuestionWithReasonAndDeleteAnswersInTheBody() + { + [$question_id] = $this->addQuestion(ExtraQuestionTypeConstants::TextQuestionType); + $this->addScanWithAnswer($question_id, 'one', 1); + + $this->deleteQuestion($question_id, [], ['delete_answers' => true, 'reason' => 'all in the body']); + + $this->assertResponseStatus(204); + $this->assertNotContains($question_id, $this->sponsorQuestionIds()); + $this->assertCount(0, $this->answersOf($question_id)); + } + // ---- options ---- public function testAdminDeleteOptionWithoutDeleteAnswersIsRefused() @@ -450,6 +475,17 @@ public function testAdminDeleteUsedOptionWithConfirmationDeletesOnlyAnswersWithN $this->assertResponseStatus(404); } + public function testAdminDeleteOptionWithReasonAndDeleteAnswersInTheQueryString() + { + [$question_id, [$a]] = $this->addQuestion(ExtraQuestionTypeConstants::CheckBoxListQuestionType, ['A', 'B']); + $this->addScanWithAnswer($question_id, (string)$a, 1); + + $this->deleteValue($question_id, $a, ['delete_answers' => 'true', 'reason' => 'no body']); + + $this->assertResponseStatus(204); + $this->assertCount(0, $this->answersOf($question_id)); + } + // ---- usage ---- public function testAdminGetsQuestionUsage() From 9bbbda118f4e051af2a2535708db82aa77e49f64 Mon Sep 17 00:00:00 2001 From: romanetar Date: Sun, 11 Oct 2026 15:36:37 +0200 Subject: [PATCH 6/8] refactor(sponsors): count the collected answers instead of loading them when only the count is needed Review feedback on #625. getBadgeScanAnswersByQuestion() hydrates every answer entity, but the usage endpoint of a question and the delete_answers pre-check only need how many there are. Add countBadgeScanAnswersByQuestion() (a count query) and use it for the question usage and the pre-check. The answers are loaded only when the delete goes ahead, and the loop over the entities stays, so orphan removal and the onFlush audit still run. The option usage and delete still load them: they need each answer value to split the ones deleted from the ones that only lose the option. --- .../ISponsorExtraQuestionTypeRepository.php | 7 +++++++ ...DoctrineSponsorExtraQuestionTypeRepository.php | 15 +++++++++++++++ app/Services/Model/Imp/SummitSponsorService.php | 7 ++++--- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php b/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php index 7419141841..913b1f60db 100644 --- a/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php +++ b/app/Models/Foundation/Summit/Repositories/ISponsorExtraQuestionTypeRepository.php @@ -28,6 +28,13 @@ interface ISponsorExtraQuestionTypeRepository extends IExtraQuestionTypeReposito */ public function getBadgeScanAnswersByQuestion(ExtraQuestionType $question): array; + /** + * How many badge scan answers were collected so far for a question, without hydrating them. + * @param ExtraQuestionType $question + * @return int + */ + public function countBadgeScanAnswersByQuestion(ExtraQuestionType $question): int; + /** * Takes a write lock on the question row, to be called at the top of a transaction. * Badge scan ingestion takes a shared lock on that row when InnoDB checks the answer FK, so diff --git a/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php b/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php index cf42009d07..ff5c7a6be0 100644 --- a/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php +++ b/app/Repositories/Summit/DoctrineSponsorExtraQuestionTypeRepository.php @@ -121,6 +121,21 @@ public function getBadgeScanAnswersByQuestion(ExtraQuestionType $question): arra ->getResult(); } + /** + * @inheritDoc + */ + public function countBadgeScanAnswersByQuestion(ExtraQuestionType $question): int + { + return (int)$this->getEntityManager() + ->createQueryBuilder() + ->select("count(a.id)") + ->from(SponsorBadgeScanExtraQuestionAnswer::class, "a") + ->where("a.question = :question") + ->setParameter("question", $question) + ->getQuery() + ->getSingleScalarResult(); + } + /** * @inheritDoc */ diff --git a/app/Services/Model/Imp/SummitSponsorService.php b/app/Services/Model/Imp/SummitSponsorService.php index e3aba2febc..d7ed335f56 100644 --- a/app/Services/Model/Imp/SummitSponsorService.php +++ b/app/Services/Model/Imp/SummitSponsorService.php @@ -1131,8 +1131,8 @@ public function deleteSponsorExtraQuestion // cascade deleted (ExtraQuestionAnswer.QuestionID is ON DELETE CASCADE) without being counted $this->repository->lockQuestion($extra_question); - $answers = $this->repository->getBadgeScanAnswersByQuestion($extra_question); - $answers_count = count($answers); + // a count is enough to refuse, the answers are only loaded when they are going to be removed + $answers_count = $this->repository->countBadgeScanAnswersByQuestion($extra_question); if ($answers_count > 0 && !$delete_answers) { throw new ValidationException( @@ -1145,6 +1145,7 @@ public function deleteSponsorExtraQuestion // the FK would cascade the delete anyway, the explicit removal goes through the ORM so // orphan removal, LastEdited and the onFlush audit run, and the count is exact (see lockQuestion) + $answers = $answers_count > 0 ? $this->repository->getBadgeScanAnswersByQuestion($extra_question) : []; foreach ($answers as $answer) { $this->removeBadgeScanAnswer($answer); } @@ -1185,7 +1186,7 @@ public function getSponsorExtraQuestionUsage(Summit $summit, int $sponsor_id, in return array_merge( [ - 'answers_count' => count($this->repository->getBadgeScanAnswersByQuestion($extra_question)), + 'answers_count' => $this->repository->countBadgeScanAnswersByQuestion($extra_question), ], $this->getScanActivity($summit_sponsor) ); From 70b250d681a7798d40c13c077d3641571410cec8 Mon Sep 17 00:00:00 2001 From: romanetar Date: Sun, 11 Oct 2026 16:01:37 +0200 Subject: [PATCH 7/8] fix(sponsors): refuse a delete that doesn't send delete_answers Review feedback on #625. filter_var(null, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE) returns false, not null, so a missing delete_answers became false and requireDeleteAnswers() never saw null: the documented 412 for a missing delete_answers could not happen, and an admin sending only a reason deleted a question or option that had no stored answers without confirming. Return null before calling filter_var when the value is missing or empty, so only a real boolean gets through and anything else is refused with 412. --- .../Protected/Summit/OAuth2SummitSponsorApiController.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php index a64a7e7868..b54359ab0a 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php @@ -4153,9 +4153,14 @@ private function getDeleteParams(): array { $request = request(); $reason = $request->input('reason'); + // filter_var turns a missing value into false, so it has to be checked first + $raw = $request->input('delete_answers'); + $delete_answers = (is_null($raw) || $raw === '') + ? null + : filter_var($raw, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE); return [ is_string($reason) ? $reason : null, - filter_var($request->input('delete_answers'), FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE), + $delete_answers, ]; } From fb43a2caac4588b1d0f374fd49aed5148eca7cc0 Mon Sep 17 00:00:00 2001 From: romanetar Date: Sun, 11 Oct 2026 16:34:27 +0200 Subject: [PATCH 8/8] docs(sponsors): drop the sponsor groups from the extra question and option DELETE annotations --- .../Protected/Summit/OAuth2SummitSponsorApiController.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php index b54359ab0a..d9bc5c8a26 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSponsorApiController.php @@ -3694,8 +3694,6 @@ public function updateExtraQuestion($summit_id, $sponsor_id, $extra_question_id) IGroup::SuperAdmins, IGroup::Administrators, IGroup::SummitAdministrators, - IGroup::Sponsors, - IGroup::SponsorExternalUsers, ] ], security: [ @@ -4019,8 +4017,6 @@ function ($value_id, $payload, $summit, $sponsor_id, $extra_question_id) { IGroup::SuperAdmins, IGroup::Administrators, IGroup::SummitAdministrators, - IGroup::Sponsors, - IGroup::SponsorExternalUsers, ] ], security: [