From ece0defe3f04b78ac07521e629652a769836eac3 Mon Sep 17 00:00:00 2001 From: Matt Edholm Date: Fri, 8 May 2026 14:45:52 -0400 Subject: [PATCH] feat(setup): "Claim this frame" checkbox for previously-bound MACs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use case: old owner sells the device to a friend. Friend holds the BOOT button to wipe NVS, joins the device's AP, sets new WiFi. The old owner's account is still bound to the MAC server-side, so without explicit consent the friend would silently take over (or, worse, the old owner's photos would keep displaying until claim). Flow now: - GET /setup/{mac} detects MAC bound to anyone and renders a "Claim this frame as my own" checkbox + a banner explaining what the takeover wipes. Both register and login panels carry the checkbox; submitting either form without it bounces back through the index with a session-flashed error. - DeviceService::linkToUser now requires allowClaim=true to transfer ownership. Without it, throws DeviceClaimRequiredException that the controller catches and turns into the bounce-with-error. - On a successful claim, the takeover wipes: * old image-device approvals * device_image_history rows for the device * name, wakeTimes, currentImage*, lockedImage, nextPollExpectedAt so the new owner starts from a fresh slate, not inheriting the seller's "Living Room / 4:30 AM" preset. - Already-logged-in user visiting /setup/{mac} for someone else's device falls through to the form (instead of silently transferring on page load) so the checkbox is the only path. Test matrix: - SetupControllerTest: 5 new functional cases — checkbox renders for bound MACs, register/login without checkbox bounce + retain old ownership, register WITH checkbox transfers + purges, logged-in other-user falls through to form. - DeviceServiceTest: 3 new unit cases — throw without consent, isClaimedByAnotherUser true/false matrix, takeover resets device state. Coverage: 99.70% lines / 98.19% methods backend, 333 frontend tests green via ddev tests. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Controller/SetupController.php | 69 ++++++-- src/Service/DeviceClaimRequiredException.php | 20 +++ src/Service/DeviceService.php | 41 ++++- templates/setup/index.html.twig | 32 ++++ .../Controller/SetupControllerTest.php | 149 ++++++++++++++++++ tests/Unit/Service/DeviceServiceTest.php | 61 ++++++- 6 files changed, 353 insertions(+), 19 deletions(-) create mode 100644 src/Service/DeviceClaimRequiredException.php diff --git a/src/Controller/SetupController.php b/src/Controller/SetupController.php index f55008e..77b89a6 100644 --- a/src/Controller/SetupController.php +++ b/src/Controller/SetupController.php @@ -8,6 +8,7 @@ use App\Entity\Device; use App\Entity\User; use App\Enum\Orientation; use App\Form\RegistrationFormType; +use App\Service\DeviceClaimRequiredException; use App\Service\DeviceService; use Doctrine\ORM\EntityManagerInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; @@ -30,15 +31,19 @@ class SetupController extends AbstractController Security $security, DeviceService $deviceService, ): Response { - // If already authenticated, link device and proceed to configure + // If already authenticated, try to link silently. If the MAC is owned + // by someone else, fall through to the form so the user has to tick + // the "Claim this frame" checkbox before transferring ownership. if ($this->getUser()) { /** @var User $user */ $user = $this->getUser(); - $device = $deviceService->linkToUser($mac, $user); - if (empty($device->getName())) { - return $this->redirectToRoute('setup_configure', ['mac' => $mac]); + if (!$deviceService->isClaimedByAnotherUser($mac, $user)) { + $device = $deviceService->linkToUser($mac, $user); + if (empty($device->getName())) { + return $this->redirectToRoute('setup_configure', ['mac' => $mac]); + } + return $this->redirectToRoute('spa'); } - return $this->redirectToRoute('spa'); } $regForm = $this->createForm(RegistrationFormType::class, new User(), [ @@ -46,11 +51,22 @@ class SetupController extends AbstractController ]); $loginError = $request->getSession()->get('_setup_login_error'); $request->getSession()->remove('_setup_login_error'); + $claimError = $request->getSession()->get('_setup_claim_error'); + $request->getSession()->remove('_setup_claim_error'); + + // The setup page only knows about the MAC at this point — the + // candidate user is whoever's logging in / registering — so we can't + // call isClaimedByAnotherUser() yet. We just check "is the MAC bound + // to anyone at all?" which is enough to surface the checkbox. + $existing = $em->getRepository(Device::class)->findOneBy(['mac' => strtoupper($mac)]); + $alreadyClaimed = $existing !== null && $existing->getUser() !== null; return $this->render('setup/index.html.twig', [ - 'mac' => $mac, - 'reg_form' => $regForm, - 'login_error' => $loginError, + 'mac' => $mac, + 'reg_form' => $regForm, + 'login_error' => $loginError, + 'already_claimed' => $alreadyClaimed, + 'claim_error' => $claimError, ]); } @@ -75,15 +91,33 @@ class SetupController extends AbstractController $em->flush(); $security->login($user, 'form_login', 'main'); - $deviceService->linkToUser($mac, $user); + + $allowClaim = $request->request->getBoolean('claim_device'); + try { + $deviceService->linkToUser($mac, $user, $allowClaim); + } catch (DeviceClaimRequiredException) { + // New account just created and claim wasn't acknowledged. + // Bounce back through the setup page; the checkbox will be + // visible because the device is now bound (to no one — wait, + // actually still bound to the prior owner since we threw + // before persisting). + $request->getSession()->set( + '_setup_claim_error', + 'This frame is already linked to another account. Tick "Claim this frame" to take it over and erase the previous owner\'s photos.', + ); + return $this->redirectToRoute('setup_index', ['mac' => $mac]); + } return $this->redirectToRoute('setup_configure', ['mac' => $mac]); } + $existing = $em->getRepository(Device::class)->findOneBy(['mac' => strtoupper($mac)]); return $this->render('setup/index.html.twig', [ - 'mac' => $mac, - 'reg_form' => $form, - 'login_error' => null, + 'mac' => $mac, + 'reg_form' => $form, + 'login_error' => null, + 'already_claimed' => $existing !== null && $existing->getUser() !== null, + 'claim_error' => null, ]); } @@ -102,7 +136,16 @@ class SetupController extends AbstractController $user = $em->getRepository(User::class)->findOneBy(['email' => $email]); if ($user && $hasher->isPasswordValid($user, $password)) { $security->login($user, 'form_login', 'main'); - $deviceService->linkToUser($mac, $user); + $allowClaim = $request->request->getBoolean('claim_device'); + try { + $deviceService->linkToUser($mac, $user, $allowClaim); + } catch (DeviceClaimRequiredException) { + $request->getSession()->set( + '_setup_claim_error', + 'This frame is already linked to another account. Tick "Claim this frame" to take it over and erase the previous owner\'s photos.', + ); + return $this->redirectToRoute('setup_index', ['mac' => $mac]); + } return $this->redirectToRoute('setup_configure', ['mac' => $mac]); } diff --git a/src/Service/DeviceClaimRequiredException.php b/src/Service/DeviceClaimRequiredException.php new file mode 100644 index 0000000..367c1f5 --- /dev/null +++ b/src/Service/DeviceClaimRequiredException.php @@ -0,0 +1,20 @@ +repo->findOneBy(['mac' => strtoupper($mac)]); + return $device !== null + && $device->getUser() !== null + && $device->getUser()->getId() !== $candidate->getId(); + } + + /** + * Atomically link a MAC address to a user. + * + * If the device is already owned by a *different* user, this is an + * ownership transfer — and we require explicit consent via $allowClaim, + * since the transfer permanently purges the prior owner's history and + * image-device approvals for this device. The setup page surfaces this + * as a "Claim this frame" checkbox. + * + * @throws DeviceClaimRequiredException when transfer is needed but + * $allowClaim is false. Callers should re-render the setup + * page with the checkbox visible and a message. + */ + public function linkToUser(string $mac, User $newOwner, bool $allowClaim = false): Device { $mac = strtoupper($mac); $device = $this->repo->findOneBy(['mac' => $mac]); @@ -31,9 +53,20 @@ class DeviceService $device = new Device(); $device->setMac($mac); } elseif ($device->getUser() !== null && $device->getUser()->getId() !== $newOwner->getId()) { + if (!$allowClaim) { + throw new DeviceClaimRequiredException(); + } // Ownership transfer: purge prior image history for this device. - // Full purge logic added in Epic 3 when Image/Approval entities exist. $this->purgeDeviceHistory($device); + // Reset device-specific state so the new owner doesn't inherit + // schedule, locked image, current image, or pending poll info. + $device->setLockedImage(null); + $device->setCurrentImage(null); + $device->setCurrentImageOrientation(null); + $device->setCurrentRenderedAt(null); + $device->setNextPollExpectedAt(null); + $device->setName(''); + $device->setWakeTimes([]); } $device->setUser($newOwner); diff --git a/templates/setup/index.html.twig b/templates/setup/index.html.twig index 612e020..83e1711 100644 --- a/templates/setup/index.html.twig +++ b/templates/setup/index.html.twig @@ -26,6 +26,14 @@ .btn { display: flex; align-items: center; justify-content: center; width: 100%; min-height: 44px; margin-top: 1.25rem; background: #c97c3a; color: #fff; border: none; border-radius: 9999px; font-size: 1rem; font-weight: 700; cursor: pointer; } + .claim-banner { background: #fff5e8; border: 1px solid #f0c987; border-radius: 10px; + padding: .75rem .875rem; margin-bottom: 1.25rem; font-size: .875rem; line-height: 1.4; + color: #5c3f1c; } + .claim-banner strong { display: block; margin-bottom: .25rem; } + .claim-check { display: flex; align-items: flex-start; gap: .625rem; margin-top: 1rem; + font-size: .875rem; line-height: 1.35; cursor: pointer; } + .claim-check input[type="checkbox"] { width: 18px; height: 18px; flex: 0 0 auto; + margin-top: 2px; accent-color: #c97c3a; cursor: pointer; } @@ -33,6 +41,18 @@

Set up your frame

Create an account or sign in to link this frame.

+ {% if already_claimed %} +

+ This frame is already linked to another account. + If you’re taking it over, tick the box below — the previous + owner’s photos and history for this frame will be permanently + removed. +

+ {% if claim_error %} + + {% endif %} + {% endif %} +
@@ -73,6 +93,12 @@ {% endfor %}
+ {% if already_claimed %} + + {% endif %} {{ form_end(reg_form) }} @@ -91,6 +117,12 @@ + {% if already_claimed %} + + {% endif %} diff --git a/tests/Functional/Controller/SetupControllerTest.php b/tests/Functional/Controller/SetupControllerTest.php index 6d31ed8..883dec1 100644 --- a/tests/Functional/Controller/SetupControllerTest.php +++ b/tests/Functional/Controller/SetupControllerTest.php @@ -5,6 +5,8 @@ declare(strict_types=1); namespace App\Tests\Functional\Controller; use App\Entity\Device; +use App\Entity\DeviceImageHistory; +use App\Entity\Image; use App\Enum\DeviceModel; use App\Enum\Orientation; use App\Tests\Functional\AppWebTestCase; @@ -173,4 +175,151 @@ class SetupControllerTest extends AppWebTestCase // Symfony 7 returns 422 for submitted-but-invalid forms $this->assertResponseStatusCodeSame(422); } + + // ── Sell-to-friend / claim-device flow ─────────────────────────────── + + private function makeBoundDevice(string $mac, string $ownerEmail): array + { + $owner = $this->createUser($ownerEmail); + $device = new Device(); + $device->setMac($mac); + $device->setName('Old Owner Frame'); + $device->setUser($owner); + $device->setModel(DeviceModel::V1); + $device->setOrientation(Orientation::Landscape); + $this->em()->persist($device); + $this->em()->flush(); + return [$owner, $device]; + } + + // S-CLAIM-01: GET /setup/{mac} for an already-bound MAC shows the claim + // checkbox so the new owner has to acknowledge what they're erasing. + public function test_setup_index_shows_claim_checkbox_when_mac_already_bound(): void + { + [, $device] = $this->makeBoundDevice(self::MAC, 'old-owner@example.com'); + + $crawler = $this->client->request('GET', '/setup/' . self::MAC); + $this->assertResponseIsSuccessful(); + $this->assertSelectorTextContains('.claim-banner', 'already linked to another account'); + // Checkbox appears in BOTH the register and login panels. + $this->assertCount(2, $crawler->filter('input[name="claim_device"][type="checkbox"]')); + } + + // S-CLAIM-02: POST /register without claim_device on a bound MAC bounces + // back through the setup index with an error in session. + public function test_register_without_claim_checkbox_bounces_with_error(): void + { + $this->makeBoundDevice(self::MAC, 'old-claim02@example.com'); + + $this->client->request('POST', '/setup/' . self::MAC . '/register', [ + 'registration_form' => [ + 'email' => 'new-claim02@example.com', + 'plainPassword' => [ + 'first' => 'secretpass1', + 'second' => 'secretpass1', + ], + ], + ]); + $this->assertResponseRedirects('/setup/' . self::MAC); + + // Follow the redirect and assert the error surfaces. + $this->client->followRedirect(); + $this->assertSelectorTextContains('.field-error', 'already linked'); + + // Device ownership should NOT have transferred. + $this->em()->clear(); + $reloaded = $this->em()->getRepository(Device::class)->findOneBy(['mac' => self::MAC]); + $this->assertNotNull($reloaded->getUser()); + $this->assertSame('old-claim02@example.com', $reloaded->getUser()->getEmail()); + } + + // S-CLAIM-03: POST /register WITH claim_device=1 transfers ownership and + // purges old image history + image-device approvals. + public function test_register_with_claim_checkbox_transfers_ownership_and_purges_history(): void + { + [$oldOwner, $device] = $this->makeBoundDevice(self::MAC, 'old-claim03@example.com'); + + // Old owner has an image approved for this device + a history row. + $image = (new Image())->setUser($oldOwner)->setOriginalFilename('p.jpg')->setStoragePath('p'); + $image->approveForDevice($device); + $this->em()->persist($image); + $history = new DeviceImageHistory($device, $image); + $this->em()->persist($history); + $device->setName('Living Room')->setWakeTimes([6 * 60]); + $this->em()->flush(); + + $deviceId = $device->getId(); + + $this->client->request('POST', '/setup/' . self::MAC . '/register', [ + 'registration_form' => [ + 'email' => 'new-claim03@example.com', + 'plainPassword' => [ + 'first' => 'secretpass1', + 'second' => 'secretpass1', + ], + ], + 'claim_device' => '1', + ]); + $this->assertResponseRedirects('/setup/' . self::MAC . '/configure'); + + $this->em()->clear(); + $reloaded = $this->em()->find(Device::class, $deviceId); + $this->assertSame('new-claim03@example.com', $reloaded->getUser()->getEmail(), 'ownership transferred'); + $this->assertSame('', $reloaded->getName(), 'name reset on takeover'); + $this->assertSame([], $reloaded->getWakeTimes(), 'wakeTimes reset on takeover'); + + // Old history is gone. + $count = (int) $this->em()->createQueryBuilder() + ->select('COUNT(h.id)') + ->from(DeviceImageHistory::class, 'h') + ->where('h.device = :d') + ->setParameter('d', $reloaded) + ->getQuery() + ->getSingleScalarResult(); + $this->assertSame(0, $count, 'history purged'); + + // Old image's approval for this device is gone too. + $reloadedImage = $this->em()->find(Image::class, $image->getId()); + $approvedIds = array_map( + fn(Device $d) => $d->getId(), + $reloadedImage->getApprovedDevices()->toArray(), + ); + $this->assertNotContains($deviceId, $approvedIds, 'image-device approval revoked'); + } + + // S-CLAIM-04: POST /login without claim_device on a bound MAC bounces + // back with the error (login still happens — the user is now logged in, + // but the device transfer didn't go through). + public function test_login_without_claim_checkbox_bounces_with_error(): void + { + $this->makeBoundDevice(self::MAC, 'old-claim04@example.com'); + $newOwner = $this->createUser('new-claim04@example.com'); + + $this->client->request('POST', '/setup/' . self::MAC . '/login', [ + '_username' => 'new-claim04@example.com', + '_password' => 'password', + ]); + $this->assertResponseRedirects('/setup/' . self::MAC); + $this->client->followRedirect(); + $this->assertSelectorTextContains('.field-error', 'already linked'); + + $this->em()->clear(); + $reloaded = $this->em()->getRepository(Device::class)->findOneBy(['mac' => self::MAC]); + $this->assertSame('old-claim04@example.com', $reloaded->getUser()->getEmail(), 'no transfer without checkbox'); + } + + // S-CLAIM-05: GET /setup/{mac} for an already-logged-in user who is NOT + // the current owner falls through to the form (showing the checkbox) + // rather than silently transferring on visit. This is the "sold to a + // friend whose phone is already logged in" scenario. + public function test_index_falls_through_to_form_when_logged_in_user_is_not_current_owner(): void + { + $this->makeBoundDevice(self::MAC, 'old-claim05@example.com'); + $other = $this->createUser('other-claim05@example.com'); + $this->loginAs($other); + + $this->client->request('GET', '/setup/' . self::MAC); + $this->assertResponseIsSuccessful(); + $this->assertSelectorTextContains('.claim-banner', 'already linked'); + } } diff --git a/tests/Unit/Service/DeviceServiceTest.php b/tests/Unit/Service/DeviceServiceTest.php index 1a7d0f1..56c59d4 100644 --- a/tests/Unit/Service/DeviceServiceTest.php +++ b/tests/Unit/Service/DeviceServiceTest.php @@ -61,8 +61,9 @@ class DeviceServiceTest extends AppKernelTestCase $this->em()->persist($history); $this->em()->flush(); - // Transfer to user2 - $this->service->linkToUser('aa:bb:cc:dd:ee:03', $user2); + // Transfer to user2 — must explicitly opt in via allowClaim now, + // otherwise the service throws DeviceClaimRequiredException. + $this->service->linkToUser('aa:bb:cc:dd:ee:03', $user2, allowClaim: true); // History should be gone $count = $this->em()->createQueryBuilder() @@ -79,4 +80,60 @@ class DeviceServiceTest extends AppKernelTestCase $this->em()->refresh($image); $this->assertFalse($image->isApprovedForDevice($device)); } + + public function test_link_throws_DeviceClaimRequiredException_without_consent(): void + { + $user1 = $this->createUser('claimreq1@example.com'); + $user2 = $this->createUser('claimreq2@example.com'); + + $device = new Device(); + $device->setMac('AA:BB:CC:DD:EE:04'); + $device->setName('Old'); + $device->setUser($user1); + $this->em()->persist($device); + $this->em()->flush(); + + $this->expectException(\App\Service\DeviceClaimRequiredException::class); + $this->service->linkToUser('AA:BB:CC:DD:EE:04', $user2); + } + + public function test_isClaimedByAnotherUser_returns_true_only_when_owned_by_someone_else(): void + { + $owner = $this->createUser('claim-owner@example.com'); + $other = $this->createUser('claim-other@example.com'); + + $device = new Device(); + $device->setMac('AA:BB:CC:DD:EE:05'); + $device->setName('Frame'); + $device->setUser($owner); + $this->em()->persist($device); + $this->em()->flush(); + + $this->assertFalse($this->service->isClaimedByAnotherUser('AA:BB:CC:DD:EE:05', $owner)); + $this->assertTrue($this->service->isClaimedByAnotherUser('AA:BB:CC:DD:EE:05', $other)); + // Unknown MAC: not claimed by anyone, so not "claimed by another." + $this->assertFalse($this->service->isClaimedByAnotherUser('FF:FF:FF:FF:FF:FF', $other)); + } + + public function test_link_resets_device_specific_state_on_takeover(): void + { + $oldOwner = $this->createUser('takeover-old@example.com'); + $newOwner = $this->createUser('takeover-new@example.com'); + + $device = new Device(); + $device->setMac('AA:BB:CC:DD:EE:06'); + $device->setName('Living Room'); + $device->setUser($oldOwner); + $device->setWakeTimes([6 * 60, 22 * 60]); + $this->em()->persist($device); + $this->em()->flush(); + + $this->service->linkToUser('AA:BB:CC:DD:EE:06', $newOwner, allowClaim: true); + + $this->em()->refresh($device); + $this->assertSame('', $device->getName(), 'name reset'); + $this->assertSame([], $device->getWakeTimes(), 'wakeTimes reset'); + $this->assertNull($device->getCurrentImage(), 'currentImage reset'); + $this->assertNull($device->getNextPollExpectedAt(), 'next-poll reset'); + } }