feat(setup): "Claim this frame" checkbox for previously-bound MACs
CI / test (push) Has been cancelled

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) <noreply@anthropic.com>
This commit is contained in:
2026-05-08 14:45:52 -04:00
parent a9ad014bd1
commit ece0defe3f
6 changed files with 353 additions and 19 deletions
+46 -3
View File
@@ -8,6 +8,7 @@ use App\Entity\Device;
use App\Entity\User; use App\Entity\User;
use App\Enum\Orientation; use App\Enum\Orientation;
use App\Form\RegistrationFormType; use App\Form\RegistrationFormType;
use App\Service\DeviceClaimRequiredException;
use App\Service\DeviceService; use App\Service\DeviceService;
use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\EntityManagerInterface;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
@@ -30,27 +31,42 @@ class SetupController extends AbstractController
Security $security, Security $security,
DeviceService $deviceService, DeviceService $deviceService,
): Response { ): 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()) { if ($this->getUser()) {
/** @var User $user */ /** @var User $user */
$user = $this->getUser(); $user = $this->getUser();
if (!$deviceService->isClaimedByAnotherUser($mac, $user)) {
$device = $deviceService->linkToUser($mac, $user); $device = $deviceService->linkToUser($mac, $user);
if (empty($device->getName())) { if (empty($device->getName())) {
return $this->redirectToRoute('setup_configure', ['mac' => $mac]); return $this->redirectToRoute('setup_configure', ['mac' => $mac]);
} }
return $this->redirectToRoute('spa'); return $this->redirectToRoute('spa');
} }
}
$regForm = $this->createForm(RegistrationFormType::class, new User(), [ $regForm = $this->createForm(RegistrationFormType::class, new User(), [
'action' => $this->generateUrl('setup_register', ['mac' => $mac]), 'action' => $this->generateUrl('setup_register', ['mac' => $mac]),
]); ]);
$loginError = $request->getSession()->get('_setup_login_error'); $loginError = $request->getSession()->get('_setup_login_error');
$request->getSession()->remove('_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', [ return $this->render('setup/index.html.twig', [
'mac' => $mac, 'mac' => $mac,
'reg_form' => $regForm, 'reg_form' => $regForm,
'login_error' => $loginError, 'login_error' => $loginError,
'already_claimed' => $alreadyClaimed,
'claim_error' => $claimError,
]); ]);
} }
@@ -75,15 +91,33 @@ class SetupController extends AbstractController
$em->flush(); $em->flush();
$security->login($user, 'form_login', 'main'); $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]); return $this->redirectToRoute('setup_configure', ['mac' => $mac]);
} }
$existing = $em->getRepository(Device::class)->findOneBy(['mac' => strtoupper($mac)]);
return $this->render('setup/index.html.twig', [ return $this->render('setup/index.html.twig', [
'mac' => $mac, 'mac' => $mac,
'reg_form' => $form, 'reg_form' => $form,
'login_error' => null, '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]); $user = $em->getRepository(User::class)->findOneBy(['email' => $email]);
if ($user && $hasher->isPasswordValid($user, $password)) { if ($user && $hasher->isPasswordValid($user, $password)) {
$security->login($user, 'form_login', 'main'); $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]); return $this->redirectToRoute('setup_configure', ['mac' => $mac]);
} }
@@ -0,0 +1,20 @@
<?php
declare(strict_types=1);
namespace App\Service;
/**
* Thrown by DeviceService::linkToUser when the MAC is already bound to a
* different user and the caller hasn't passed allowClaim=true. The setup
* controller catches this and re-renders with the "Claim this frame"
* checkbox visible — a friend who reset the device for a new owner has to
* tick the box to acknowledge the prior owner's data will be wiped.
*/
final class DeviceClaimRequiredException extends \RuntimeException
{
public function __construct()
{
parent::__construct('Device is claimed by a different user; consent required.');
}
}
+37 -4
View File
@@ -19,10 +19,32 @@ class DeviceService
) {} ) {}
/** /**
* Atomically link a MAC address to a user. * True when the MAC is already claimed by a *different* user. Used by the
* If the device was previously owned by a different user, image history is purged. * setup flow to gate the "Claim this frame" checkbox so a friend can't
* silently take ownership without confirmation.
*/ */
public function linkToUser(string $mac, User $newOwner): Device public function isClaimedByAnotherUser(string $mac, User $candidate): bool
{
$device = $this->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); $mac = strtoupper($mac);
$device = $this->repo->findOneBy(['mac' => $mac]); $device = $this->repo->findOneBy(['mac' => $mac]);
@@ -31,9 +53,20 @@ class DeviceService
$device = new Device(); $device = new Device();
$device->setMac($mac); $device->setMac($mac);
} elseif ($device->getUser() !== null && $device->getUser()->getId() !== $newOwner->getId()) { } elseif ($device->getUser() !== null && $device->getUser()->getId() !== $newOwner->getId()) {
if (!$allowClaim) {
throw new DeviceClaimRequiredException();
}
// Ownership transfer: purge prior image history for this device. // Ownership transfer: purge prior image history for this device.
// Full purge logic added in Epic 3 when Image/Approval entities exist.
$this->purgeDeviceHistory($device); $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); $device->setUser($newOwner);
+32
View File
@@ -26,6 +26,14 @@
.btn { display: flex; align-items: center; justify-content: center; width: 100%; min-height: 44px; .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; margin-top: 1.25rem; background: #c97c3a; color: #fff; border: none; border-radius: 9999px;
font-size: 1rem; font-weight: 700; cursor: pointer; } 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; }
</style> </style>
</head> </head>
<body> <body>
@@ -33,6 +41,18 @@
<h1>Set up your frame</h1> <h1>Set up your frame</h1>
<p class="subtitle">Create an account or sign in to link this frame.</p> <p class="subtitle">Create an account or sign in to link this frame.</p>
{% if already_claimed %}
<p class="claim-banner" role="status">
<strong>This frame is already linked to another account.</strong>
If youre taking it over, tick the box below — the previous
owners photos and history for this frame will be permanently
removed.
</p>
{% if claim_error %}
<p class="field-error" role="alert" style="margin-bottom:1rem">{{ claim_error }}</p>
{% endif %}
{% endif %}
<div class="tabs"> <div class="tabs">
<a href="#register" class="tab {% if not login_error %}active{% endif %}" data-tab="register">Create account</a> <a href="#register" class="tab {% if not login_error %}active{% endif %}" data-tab="register">Create account</a>
<a href="#login" class="tab {% if login_error %}active{% endif %}" data-tab="login">Sign in</a> <a href="#login" class="tab {% if login_error %}active{% endif %}" data-tab="login">Sign in</a>
@@ -73,6 +93,12 @@
<p class="field-error" role="alert">{{ error.message }}</p> <p class="field-error" role="alert">{{ error.message }}</p>
{% endfor %} {% endfor %}
</div> </div>
{% if already_claimed %}
<label class="claim-check">
<input type="checkbox" name="claim_device" value="1" required>
<span>Claim this frame as my own (deletes the previous owners photos and history)</span>
</label>
{% endif %}
<button type="submit" class="btn">Create account &amp; link frame</button> <button type="submit" class="btn">Create account &amp; link frame</button>
{{ form_end(reg_form) }} {{ form_end(reg_form) }}
</div> </div>
@@ -91,6 +117,12 @@
<label for="login-pass">Password</label> <label for="login-pass">Password</label>
<input type="password" id="login-pass" name="_password" autocomplete="current-password"> <input type="password" id="login-pass" name="_password" autocomplete="current-password">
</div> </div>
{% if already_claimed %}
<label class="claim-check">
<input type="checkbox" name="claim_device" value="1" required>
<span>Claim this frame as my own (deletes the previous owners photos and history)</span>
</label>
{% endif %}
<button type="submit" class="btn">Sign in &amp; link frame</button> <button type="submit" class="btn">Sign in &amp; link frame</button>
</form> </form>
</div> </div>
@@ -5,6 +5,8 @@ declare(strict_types=1);
namespace App\Tests\Functional\Controller; namespace App\Tests\Functional\Controller;
use App\Entity\Device; use App\Entity\Device;
use App\Entity\DeviceImageHistory;
use App\Entity\Image;
use App\Enum\DeviceModel; use App\Enum\DeviceModel;
use App\Enum\Orientation; use App\Enum\Orientation;
use App\Tests\Functional\AppWebTestCase; use App\Tests\Functional\AppWebTestCase;
@@ -173,4 +175,151 @@ class SetupControllerTest extends AppWebTestCase
// Symfony 7 returns 422 for submitted-but-invalid forms // Symfony 7 returns 422 for submitted-but-invalid forms
$this->assertResponseStatusCodeSame(422); $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');
}
} }
+59 -2
View File
@@ -61,8 +61,9 @@ class DeviceServiceTest extends AppKernelTestCase
$this->em()->persist($history); $this->em()->persist($history);
$this->em()->flush(); $this->em()->flush();
// Transfer to user2 // Transfer to user2 — must explicitly opt in via allowClaim now,
$this->service->linkToUser('aa:bb:cc:dd:ee:03', $user2); // otherwise the service throws DeviceClaimRequiredException.
$this->service->linkToUser('aa:bb:cc:dd:ee:03', $user2, allowClaim: true);
// History should be gone // History should be gone
$count = $this->em()->createQueryBuilder() $count = $this->em()->createQueryBuilder()
@@ -79,4 +80,60 @@ class DeviceServiceTest extends AppKernelTestCase
$this->em()->refresh($image); $this->em()->refresh($image);
$this->assertFalse($image->isApprovedForDevice($device)); $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');
}
} }