fix: review feedback

romanetar · romanetar · commit 4cda54af42af · 2026-04-08T18:38:45.000+02:00
Signed-off-by: romanetar &lt;roman_ag@hotmail.com&gt;
diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitBadgeScanApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitBadgeScanApiController.php
@@ -126,6 +126,10 @@ protected function addChild(Summit $summit, array $payload): IEntity
         if (is_null($current_member))
             throw new HTTP403ForbiddenException();
 
+        if (!$current_member->hasSponsorMembershipsFor($summit)) {
+            throw new HTTP403ForbiddenException('Only sponsor users can add badge scans.');
+        }
+
         return $this->service->addBadgeScan($summit, $current_member, $payload);
     }
 
diff --git a/app/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.php b/app/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.php
@@ -60,11 +60,19 @@ public function handle(SponsorServicesMQJob $job): void
             Log::debug("UpdateSponsorMemberGroupsMQJob::handle payload {$json}");
 
             $data = $payload['data'];
+            if (!isset($data['user_external_id'], $data['group_slug'], $data['sponsor_id'], $data['summit_id'])) {
+                throw new ValidationException('Invalid payload: user_external_id, group_slug, sponsor_id and summit_id are required.');
+            }
+
             $user_external_id = intval($data['user_external_id']);
             $group_slug = $data['group_slug'];
             $sponsor_id = intval($data['sponsor_id']);
             $summit_id = intval($data['summit_id']);
 
+            if ($user_external_id <= 0 || $sponsor_id <= 0 || $summit_id <= 0 || trim((string)$group_slug) === '') {
+                throw new ValidationException('Invalid payload: identifiers must be positive and group_slug must be non-empty.');
+            }
+
             if ($event_type === EventTypes::AUTH_USER_ADDED_TO_GROUP) {
                 $this->service->addSponsorUserToGroup($user_external_id, $group_slug, $sponsor_id, $summit_id);
             } else if ($event_type === EventTypes::AUTH_USER_REMOVED_FROM_GROUP) {
@@ -85,4 +93,4 @@ public function failed(array $data, Throwable $exception): void
     {
         Log::error("UpdateSponsorMemberGroupsMQJob::failed {$exception->getMessage()}");
     }
-}
+}
diff --git a/app/Models/Foundation/Main/Member.php b/app/Models/Foundation/Main/Member.php
@@ -19,6 +19,7 @@
 use App\Models\Foundation\Main\IGroup;
 use App\Models\Foundation\Main\Strategies\MemberSummitStrategyFactory;
 use App\Models\Foundation\Summit\Events\RSVP\RSVPInvitation;
+use Doctrine\DBAL\Exception;
 use Doctrine\ORM\Query\ResultSetMappingBuilder;
 use Illuminate\Support\Facades\Config;
 use Doctrine\DBAL\ParameterType;
@@ -1860,21 +1861,20 @@ public function getActiveSummitsSponsorMemberships()
             return [];
         }
 
-        // Step 2 — load each Sponsor by PK. find() uses a different code path that avoids
-        // the ORM 3 assertion failure triggered by the OneToOne inverse associations on Sponsor
-        // (lead_report_setting, sponsorservices_statistics) when using DQL/native query hydration.
-        $sponsors = [];
-        foreach ($ids as $id) {
-            $sponsor = $this->getEM()->find(\models\summit\Sponsor::class, $id);
-            if ($sponsor !== null) {
-                $sponsors[] = $sponsor;
-            }
-        }
+        // Step 2 — load all sponsors in a single IN query. findBy() uses PK-based hydration
+        // which avoids the ORM 3 assertion failure triggered by the OneToOne inverse associations
+        // on Sponsor (lead_report_setting, sponsorservices_statistics) that DQL/native-query
+        // hydration hits. The result set is then re-sorted to match the SQL ORDER BY.
+        $position = array_flip($ids);
+        $sponsors = EntityManager::getRepository(Sponsor::class)->findBy(['id' => $ids]);
+        usort($sponsors, fn($a, $b) => $position[$a->getId()] <=> $position[$b->getId()]);
         return $sponsors;
     }
 
     /**
+     * @param Summit $summit
      * @return array
+     * @throws Exception
      */
     public function getSponsorMembershipIds(Summit $summit): array
     {
@@ -1902,8 +1902,9 @@ public function getSponsorMembershipIds(Summit $summit): array
     public function hasSponsorMembershipsFor(Summit $summit, Sponsor $sponsor = null): bool
     {
         try {
-           $canHaveSponsorMemberships = $this->isSponsorUser() || $this->isExternalSponsorUser();
-           if(!$canHaveSponsorMemberships) return false;
+            $canHaveSponsorMemberships = $this->isSponsorUser() || $this->isExternalSponsorUser();
+            if(!$canHaveSponsorMemberships) return false;
+
         $sql = <<<SQL
 SELECT COUNT(Sponsor_Users.SponsorID)
 FROM Sponsor_Users
@@ -3466,9 +3467,23 @@ public function getIndividualMemberJoinDate(): ?\DateTime
      * Appends $group_slug to the Permissions JSON array on the Sponsor_Users row
      * for this member and the given sponsor. Idempotent: the slug is only added
      * when it is not already present.
+     *
+     * An exclusive row lock (SELECT … FOR UPDATE) is acquired first so that
+     * concurrent jobs for the same (member, sponsor, slug) serialize here and
+     * the second job always reads the post-first-job value, preventing duplicates.
+     *
+     * Returns the number of rows matched by the WHERE clause (0 when the
+     * Sponsor_Users row does not yet exist, 1 when it does).
      */
-    public function addSponsorPermission(int $sponsor_id, string $group_slug): void
+    public function addSponsorPermission(int $sponsor_id, string $group_slug): int
     {
+        // Lock the row before the read-modify-write so concurrent transactions
+        // serialize and the IF(JSON_CONTAINS) in the UPDATE sees the committed state.
+        $this->prepareRawSQL(
+            'SELECT Permissions FROM Sponsor_Users WHERE SponsorID = :sponsor_id AND MemberID = :member_id FOR UPDATE',
+            ['sponsor_id' => $sponsor_id, 'member_id' => $this->getId()]
+        )->executeQuery();
+
         $sql = <<<SQL
 UPDATE Sponsor_Users
 SET Permissions = IF(
@@ -3478,45 +3493,65 @@ public function addSponsorPermission(int $sponsor_id, string $group_slug): void
 )
 WHERE SponsorID = :sponsor_id AND MemberID = :member_id
 SQL;
-        $stmt = $this->prepareRawSQL($sql, [
+        return $this->prepareRawSQL($sql, [
             'group_slug' => $group_slug,
             'sponsor_id' => $sponsor_id,
             'member_id'  => $this->getId(),
-        ]);
-        $stmt->executeStatement();
+        ])->executeStatement();
     }
 
     /**
      * Removes $group_slug from the Permissions JSON array on the Sponsor_Users row
      * for this member and the given sponsor, then returns how many other Sponsor_Users
      * rows for this member still carry that slug. The caller uses the count to decide
      * whether to also revoke the global group membership.
+     *
+     * An exclusive row lock is acquired first so the remove UPDATE and the
+     * remaining-count SELECT are not interleaved with concurrent operations.
+     * All occurrences of the slug are removed (via JSON_ARRAYAGG filter) to
+     * prevent stale entries if a prior race introduced duplicates.
      */
     public function removeSponsorPermission(int $sponsor_id, string $group_slug): int
     {
+        // Serialize concurrent removals for the same row.
+        $this->prepareRawSQL(
+            'SELECT Permissions FROM Sponsor_Users WHERE SponsorID = :sponsor_id AND MemberID = :member_id FOR UPDATE',
+            ['sponsor_id' => $sponsor_id, 'member_id' => $this->getId()]
+        )->executeQuery();
+
+        // Remove ALL occurrences (not just the first) so duplicate slugs
+        // introduced by any prior race cannot leave stale entries behind.
         $removeSQL = <<<SQL
 UPDATE Sponsor_Users
-SET Permissions = JSON_REMOVE(Permissions, JSON_UNQUOTE(JSON_SEARCH(Permissions, 'one', :group_slug)))
+SET Permissions = COALESCE(
+    (
+        SELECT JSON_ARRAYAGG(element)
+        FROM JSON_TABLE(
+            COALESCE(Permissions, '[]'), '$[*]'
+            COLUMNS(element VARCHAR(255) PATH '$')
+        ) AS jt
+        WHERE element != :group_slug
+    ),
+    '[]'
+)
 WHERE SponsorID = :sponsor_id AND MemberID = :member_id
 AND JSON_CONTAINS(COALESCE(Permissions, '[]'), JSON_QUOTE(:group_slug))
 SQL;
-        $stmt = $this->prepareRawSQL($removeSQL, [
+        $this->prepareRawSQL($removeSQL, [
             'group_slug' => $group_slug,
             'sponsor_id' => $sponsor_id,
             'member_id'  => $this->getId(),
-        ]);
-        $stmt->executeStatement();
+        ])->executeStatement();
 
         $countSQL = <<<SQL
 SELECT COUNT(*) FROM Sponsor_Users
 WHERE MemberID = :member_id
 AND JSON_CONTAINS(COALESCE(Permissions, '[]'), JSON_QUOTE(:group_slug))
 SQL;
-        $stmt = $this->prepareRawSQL($countSQL, [
+        return intval($this->prepareRawSQL($countSQL, [
             'member_id'  => $this->getId(),
             'group_slug' => $group_slug,
-        ]);
-        return intval($stmt->executeQuery()->fetchOne());
+        ])->executeQuery()->fetchOne());
     }
 
     public function addSponsorMembership(Sponsor $sponsor):void
diff --git a/app/Services/Model/Imp/SponsorUserInfoGrantService.php b/app/Services/Model/Imp/SponsorUserInfoGrantService.php
@@ -193,6 +193,9 @@ public function addBadgeScan(Summit $summit, Member $current_member, array $data
                     throw new ValidationException("Current member does not belong to the selected summit sponsor.");
             }
 
+            if(!$current_member->hasSponsorMembershipsFor($summit, $sponsor))
+                throw new ValidationException("Current member does not have badge scan permissions for the selected sponsor.");
+
             $scan = new SponsorBadgeScan();
             $scan->setScanDate($scan_date);
             $scan->setQRCode($qr_code);
diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php
@@ -154,7 +154,23 @@ public function addSponsorUserToGroup(int $user_id, string $group_slug, int $spo
             }
 
             // Add permission entry to the Sponsor_Users JSON column for this sponsor-member pair.
-            $member->addSponsorPermission($sponsor_id, $group_slug);
+            // If the row does not exist yet (MQ ordering race: group event arrived before membership
+            // event), create it eagerly so the permission is never silently dropped.
+            if ($member->addSponsorPermission($sponsor_id, $group_slug) === 0) {
+                Log::warning(
+                    "SponsorUserSyncService::addSponsorUserToGroup no Sponsor_Users row found for " .
+                    "member {$member->getId()} / sponsor {$sponsor_id} — creating it eagerly");
+
+                $summit = $this->summit_repository->getById($summit_id);
+                if (!$summit instanceof Summit) {
+                    throw new EntityNotFoundException("Summit {$summit_id} not found");
+                }
+
+                $this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId());
+
+                // Retry now that the row exists.
+                $member->addSponsorPermission($sponsor_id, $group_slug);
+            }
 
             // Add to global group only if not already a member.
             if (!$member->belongsToGroup($group_slug)) {
diff --git a/app/Services/Model/Imp/SummitSponsorService.php b/app/Services/Model/Imp/SummitSponsorService.php
@@ -394,7 +394,9 @@ public function addSponsorUser(Summit $summit, int $sponsor_id, int $member_id):
                 // due a member could be on 2 diff places at same time ...
                 // (StartA <= EndB)  and  (EndA >= StartB)
 
-                if ($current_summit_begin_date <= $former_summit_end_date && $current_summit_end_date >= $former_summit_begin_date) {
+                if ($summit->getId() != $former_summit->getId() &&
+                    $current_summit_begin_date <= $former_summit_end_date &&
+                    $current_summit_end_date >= $former_summit_begin_date) {
                     throw new ValidationException
                     (
                         sprintf
diff --git a/database/migrations/model/Version20260408102410.php b/database/migrations/model/Version20260408102410.php
@@ -0,0 +1,60 @@
+<?php namespace Database\Migrations\Model;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * Class Version20260408120000
+ * @package Database\Migrations\Model
+ *
+ * Backfills the Permissions JSON column on Sponsor_Users with the group codes
+ * (Group.Code) of every group the member belongs to, derived from Group_Members.
+ * Only rows where Permissions IS NULL are updated (i.e. rows that pre-date the
+ * per-sponsor permission tracking feature added in Version20260402153110).
+ */
+final class Version20260408102410 extends AbstractMigration
+{
+    public function getDescription(): string
+    {
+        return 'Backfill Sponsor_Users.Permissions JSON from existing Group_Members rows.';
+    }
+
+    public function up(Schema $schema): void
+    {
+        // For each Sponsor_Users row whose Permissions have not been set yet,
+        // aggregate all Group.Code values for the member and store them as a
+        // JSON array. JSON_ARRAYAGG returns NULL when there are no matching
+        // groups, which is a valid (empty-permissions) state.
+        $this->addSql(<<<SQL
+UPDATE Sponsor_Users su
+SET su.Permissions = (
+    SELECT JSON_ARRAYAGG(g.Code)
+    FROM Group_Members gm
+    INNER JOIN `Group` g ON g.ID = gm.GroupID
+    WHERE gm.MemberID = su.MemberID
+)
+WHERE su.Permissions IS NULL
+SQL
+        );
+    }
+
+    public function down(Schema $schema): void
+    {
+        // Revert the backfill by clearing all Permissions values. This is the
+        // only safe inverse: we cannot distinguish backfilled values from those
+        // written by the application after the migration ran.
+        $this->addSql("UPDATE Sponsor_Users SET Permissions = NULL");
+    }
+}
diff --git a/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php b/tests/oauth2/OAuth2SummitBadgeScanApiControllerTest.php
@@ -49,6 +49,18 @@ protected function setUp():void
         $this->sponsor_group->setCode(IGroup::Sponsors);
         $this->sponsor_group->setTitle(IGroup::Sponsors);
         self::$em->persist($this->sponsor_group);
+
+        // Pre-wire self::$member as a permissioned sponsor user for sponsors[0] so that
+        // tests which exercise the happy path do not need per-test boilerplate.
+        self::$member->add2Group($this->sponsor_group);
+        $sponsor0 = self::$sponsors[0];
+        $sponsor0->addUser(self::$member);
+        self::$em->persist(self::$member);
+        self::$em->persist($sponsor0);
+        self::$em->flush();
+
+        // Write IGroup::Sponsors into Sponsor_Users.Permissions so hasSponsorMembershipsFor passes.
+        self::$member->addSponsorPermission($sponsor0->getId(), IGroup::Sponsors);
     }
 
     protected function tearDown():void
@@ -60,6 +72,7 @@ protected function tearDown():void
     public function testAddEncryptedBadgeScan(){
         // set test data
         self::$member->clearGroups();
+        self::$member->add2Group($this->sponsor_group);
         self::$member->add2Group($this->external_sponsor_group);
         self::$em->persist(self::$member);
         self::$em->flush();
@@ -69,6 +82,8 @@ public function testAddEncryptedBadgeScan(){
         self::$em->persist($sponsor);
         self::$em->flush();
 
+        self::$member->addSponsorPermission($sponsor->getId(), IGroup::SponsorExternalUsers);
+
         $attendee =  self::$summit->getAttendees()[0];
 
         self::$summit->setQRCodesEncKey('35NVOF4I5T6AAM28IJPKB8KRUW98KPDO');
@@ -259,6 +274,52 @@ public function testAddBadgeScanMissingQrCodeAndEmailFails(){
         $this->assertResponseStatus(412);
     }
 
+    public function testAddBadgeScanFailsWhenSponsorHasNoPermissionSlug()
+    {
+        // member2 is in the global sponsors group so hasSponsorMembershipsFor doesn't
+        // short-circuit, but the Sponsor_Users row has no permission slug
+        // (simulates the MQ group event never arriving).
+        self::$member2->add2Group($this->sponsor_group);
+        $sponsor = self::$sponsors[0];
+        $sponsor->addUser(self::$member2);
+        self::$em->persist(self::$member2);
+        self::$em->persist($sponsor);
+        self::$em->flush();
+        // Sponsor_Users row was created with Permissions = NULL — deliberately no addSponsorPermission call.
+
+        // Impersonate member2 for this request.
+        self::$service->setUserId(self::$member2->getUserExternalId());
+        self::$service->setUserExternalId(self::$member2->getUserExternalId());
+        self::$service->setUserEmail(self::$member2->getEmail());
+        self::$service->setUserFirstName(self::$member2->getFirstName());
+        self::$service->setUserLastName(self::$member2->getLastName());
+
+        $params = [
+            'id' => self::$summit->getId(),
+        ];
+
+        $attendee = self::$summit->getAttendeeByMemberId(self::$defaultMember->getId());
+        $badge = $attendee->getFirstTicket()->getBadge();
+
+        $data = [
+            'qr_code'   => $badge->generateQRCode(),
+            'scan_date' => 1572019200,
+        ];
+
+        $this->action(
+            "POST",
+            "OAuth2SummitBadgeScanApiController@add",
+            $params,
+            [],
+            [],
+            [],
+            $this->getAuthHeaders(),
+            json_encode($data)
+        );
+
+        $this->assertResponseStatus(412);
+    }
+
     public function testAddBadgeScanByUnknownAttendeeEmailFails(){
         self::$member->clearGroups();
         self::$member->add2Group($this->sponsor_group);

Original file line number	Diff line number	Diff line change
`@@ -126,6 +126,10 @@ protected function addChild(Summit $summit, array $payload): IEntity`
`126`	`126`	`if (is_null($current_member))`
`127`	`127`	`throw new HTTP403ForbiddenException();`
`128`	`128`
	`129`	`+ if (!$current_member->hasSponsorMembershipsFor($summit)) {`
	`130`	`+ throw new HTTP403ForbiddenException('Only sponsor users can add badge scans.');`
	`131`	`+ }`
	`132`	`+`
`129`	`133`	`return $this->service->addBadgeScan($summit, $current_member, $payload);`
`130`	`134`	`}`
`131`	`135`
Original file line number	Diff line number	Diff line change
`@@ -193,6 +193,9 @@ public function addBadgeScan(Summit $summit, Member $current_member, array $data`
`193`	`193`	`throw new ValidationException("Current member does not belong to the selected summit sponsor.");`
`194`	`194`	`}`
`195`	`195`
	`196`	`+ if(!$current_member->hasSponsorMembershipsFor($summit, $sponsor))`
	`197`	`+ throw new ValidationException("Current member does not have badge scan permissions for the selected sponsor.");`
	`198`	`+`
`196`	`199`	`$scan = new SponsorBadgeScan();`
`197`	`200`	`$scan->setScanDate($scan_date);`
`198`	`201`	`$scan->setQRCode($qr_code);`