diff --git a/src/Controller/Mail/MailController.php b/src/Controller/Mail/MailController.php index 7ff2199f..58c7f95d 100644 --- a/src/Controller/Mail/MailController.php +++ b/src/Controller/Mail/MailController.php @@ -9,6 +9,7 @@ use App\Entity\Label\Label; use App\Entity\Mail\Message; use App\Entity\Mail\MessageThread; use App\Repository\Label\LabelRepository; +use App\Repository\Mail\AccountRepository; use App\Repository\Mail\MailboxRepository; use App\Repository\Mail\MessageRepository; use App\Repository\Mail\MessageThreadRepository; @@ -27,6 +28,7 @@ final class MailController extends AbstractController private readonly MessageRepository $messageRepository, private readonly MessageThreadRepository $threadRepository, private readonly LabelRepository $labelRepository, + private readonly AccountRepository $accountRepository, ) { } @@ -94,14 +96,70 @@ final class MailController extends AbstractController private function renderLabel(Label $label, Request $request): Response { + $account = $this->requestedAccount($request); $page = max(1, (int) $request->query->get('page', 1)); - $threads = $this->threadRepository->findForLabel($label, $page); - $total = $this->threadRepository->countForLabel($label); + $threads = $this->threadRepository->findForLabel($label, $account, $page); + $total = $this->threadRepository->countForLabel($label, $account); $this->threadRepository->preloadLabels($threads); return $this->render('mail/label.html.twig', [ 'label' => $label, + 'account' => $account, + 'threads' => $threads, + 'page' => $page, + 'total' => $total, + 'per_page' => 50, + ]); + } + + /** + * The `?account=` a label view may be narrowed by. + * + * A query parameter rather than a second route: it is a filter on the same + * view, and the label id alone still has to resolve — the sidebar's own + * label list means "across every account" and links without it. + * + * Ownership is checked here rather than trusted, because this comes + * straight off the query string: without it, appending someone else's + * account id would scope a label you own to threads you do not. + */ + private function requestedAccount(Request $request): ?Account + { + $id = $request->query->get('account'); + + if (null === $id || '' === $id) { + return null; + } + + $account = $this->accountRepository->find($id); + + if (null === $account || $account->getUsr() !== $this->getUser()) { + throw $this->createAccessDeniedException(); + } + + return $account; + } + + /** + * Everything in one account — what clicking the account itself in the + * sidebar now does. Its labels sit underneath for narrowing further. + */ + #[Route('/account/{account}', name: 'account')] + public function accountView(Account $account, Request $request): Response + { + if ($account->getUsr() !== $this->getUser()) { + throw $this->createAccessDeniedException(); + } + + $page = max(1, (int) $request->query->get('page', 1)); + $threads = $this->threadRepository->findForAccount($account, $page); + $total = $this->threadRepository->countForAccount($account); + + $this->threadRepository->preloadLabels($threads); + + return $this->render('mail/account.html.twig', [ + 'account' => $account, 'threads' => $threads, 'page' => $page, 'total' => $total, diff --git a/src/Repository/Mail/MessageThreadRepository.php b/src/Repository/Mail/MessageThreadRepository.php index 4508db02..a4a27826 100644 --- a/src/Repository/Mail/MessageThreadRepository.php +++ b/src/Repository/Mail/MessageThreadRepository.php @@ -10,6 +10,7 @@ use App\Entity\Label\Label; use App\Entity\Mail\MessageThread; use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository; use Doctrine\DBAL\ParameterType; +use Doctrine\ORM\QueryBuilder; use Doctrine\Persistence\ManagerRegistry; use Symfony\Component\Security\Core\User\UserInterface; @@ -160,14 +161,53 @@ class MessageThreadRepository extends ServiceEntityRepository ->getQuery() ->getSingleScalarResult(); } - public function findForLabel(Label $label, int $page = 1, int $perPage = 50): array + /** + * Threads carrying a label, optionally narrowed to one account. + * + * Labels are user-scoped and can be bound to several accounts at once, so + * a label on its own spans every account that has it. That is right for the + * sidebar's own label list — one "Receipts" across the whole mailbox — and + * wrong under an account, where the same entry is read as "this account's + * Receipts" and returned everybody's. + */ + public function findForLabel(Label $label, ?Account $account = null, int $page = 1, int $perPage = 50): array + { + $offset = ($page - 1) * $perPage; + + $qb = $this->createQueryBuilder('t') + ->join('t.labels', 'l') + ->where('l = :label') + ->setParameter('label', $label) + ->orderBy('t.lastMessageAt', 'DESC') + ->setFirstResult($offset) + ->setMaxResults($perPage); + + $this->narrowToAccount($qb, $account); + + return $qb->getQuery()->getResult(); + } + + public function countForLabel(Label $label, ?Account $account = null): int + { + $qb = $this->createQueryBuilder('t') + ->select('COUNT(t.id)') + ->join('t.labels', 'l') + ->where('l = :label') + ->setParameter('label', $label); + + $this->narrowToAccount($qb, $account); + + return (int) $qb->getQuery()->getSingleScalarResult(); + } + + /** Everything in one account, whatever it is labelled. */ + public function findForAccount(Account $account, int $page = 1, int $perPage = 50): array { $offset = ($page - 1) * $perPage; return $this->createQueryBuilder('t') - ->join('t.labels', 'l') - ->where('l = :label') - ->setParameter('label', $label) + ->where('t.account = :account') + ->setParameter('account', $account) ->orderBy('t.lastMessageAt', 'DESC') ->setFirstResult($offset) ->setMaxResults($perPage) @@ -175,16 +215,25 @@ class MessageThreadRepository extends ServiceEntityRepository ->getResult(); } - public function countForLabel(Label $label): int + public function countForAccount(Account $account): int { return (int) $this->createQueryBuilder('t') ->select('COUNT(t.id)') - ->join('t.labels', 'l') - ->where('l = :label') - ->setParameter('label', $label) + ->where('t.account = :account') + ->setParameter('account', $account) ->getQuery() ->getSingleScalarResult(); } + + /** No-op when no account was asked for, so callers need no branch. */ + private function narrowToAccount(QueryBuilder $qb, ?Account $account): void + { + if (null === $account) { + return; + } + + $qb->andWhere('t.account = :account')->setParameter('account', $account); + } public function findForStarred(UserInterface $user, int $page = 1, int $perPage = 50): array { $offset = ($page - 1) * $perPage; diff --git a/templates/_partials/_account_list.html.twig b/templates/_partials/_account_list.html.twig index c7054c4f..6df16dd8 100644 --- a/templates/_partials/_account_list.html.twig +++ b/templates/_partials/_account_list.html.twig @@ -5,18 +5,27 @@ data-settings--account-expand-folders-url-value="{{ path('app_mail_account_folders', { account: account.id }) }}" data-settings--account-expand-active-value="false"> - {# The whole row toggles the label list — per-account "folders" are - labels now, so there is no INBOX landing link anymore. #} -
-
+ {# Two targets, not one. The name opens the account — everything in it, + whatever it is labelled — and the chevron opens its label list. The + whole row used to toggle, which left no way to ask for the account + itself, and expanding was the only thing clicking an account could + do. #} +
+ {{ account.displayAddress }} -
+
+{% endblock %} + +{% block message_list %} + {% if threads is empty %} +
  • + {{ 'label.empty'|trans }} +
    • + {% else %} + {% for thread in threads %} + {% include '_partials/_thread_row.html.twig' with { thread: thread } %} + {% endfor %} + {% endif %} +{% endblock %} diff --git a/templates/mail/label.html.twig b/templates/mail/label.html.twig index 8ee1dec3..16185df2 100644 --- a/templates/mail/label.html.twig +++ b/templates/mail/label.html.twig @@ -9,8 +9,18 @@ mail/label.html.twig — thread list for a custom label. {% endblock %} {% block inbox_tabs %} -
    +

    {{ label.fullName }}

    + {# Narrowed to one account. Said out loud, and with a way back out: + the same label under a different account looks identical otherwise, + and an empty one is then indistinguishable from an empty label. #} + {% if account is defined and account %} + {{ account.displayAddress }} +     + {{ 'label.all_accounts'|trans }} + + {% endif %}
    {% endblock %} diff --git a/tests/e2e/account-scope.spec.ts b/tests/e2e/account-scope.spec.ts new file mode 100644 index 00000000..c601eba3 --- /dev/null +++ b/tests/e2e/account-scope.spec.ts @@ -0,0 +1,129 @@ +import { test, expect } from "./support/test"; +import { seed } from "./support/config"; + +/** + * Per-account navigation in the sidebar. + * + * Labels are user-scoped and bind to several accounts at once, so a label on + * its own means "across the whole mailbox". Under an account it is read as + * "this account's" — and it used to link to the unscoped view, listing every + * account's mail under a heading that said otherwise. + */ +test.beforeEach(() => { + seed("seed-mail"); +}); + +test.describe("account scoping", () => { + const accountLink = (page: import("@playwright/test").Page) => + page.locator('#sidebar a[href^="/mail/account/"]').first(); + + test("clicking an account shows that account rather than expanding it", async ({ page }) => { + await page.goto("/mail/inbox"); + + const link = accountLink(page); + await expect(link).toBeVisible(); + + const href = await link.getAttribute("href"); + await link.click(); + + await page.waitForURL(new RegExp(href!.replace(/[/]/g, "\\/"))); + await expect(page.locator("h2").first()).toHaveText("E2E Mailbox"); + }); + + /** + * The chevron kept the old job. Clicking the row used to be the only thing + * an account could do, which left no way to ask for the account itself. + */ + test("the chevron still expands the account's labels", async ({ page }) => { + await page.goto("/mail/inbox"); + + await page.locator('#sidebar button[aria-label="Toggle account folders"]').first().click(); + + const labels = page.locator('#sidebar turbo-frame[id^="account-folders-"] a'); + await expect(labels.first()).toBeVisible(); + }); + + test("labels under an account are scoped to it", async ({ page }) => { + await page.goto("/mail/inbox"); + await page.locator('#sidebar button[aria-label="Toggle account folders"]').first().click(); + + const scoped = page.locator('#sidebar turbo-frame a[href*="/mail/label/"]'); + await expect(scoped.first()).toBeVisible(); + + // Every one of them, not just the first: a single unscoped entry is the + // bug all over again for whichever label it happens to be. + const hrefs = await scoped.evaluateAll((links) => + links.map((link) => link.getAttribute("href") ?? ""), + ); + + expect(hrefs.length).toBeGreaterThan(0); + expect(hrefs.every((href) => href.includes("account="))).toBe(true); + }); + + test("an account-scoped label view says so, and offers the way back out", async ({ page }) => { + await page.goto("/mail/inbox"); + await page.locator('#sidebar button[aria-label="Toggle account folders"]').first().click(); + + const scoped = page.locator('#sidebar turbo-frame a[href*="/mail/label/"]').first(); + await expect(scoped).toBeVisible(); + await scoped.click(); + + await page.waitForURL(/\/mail\/label\/\d+\?account=/); + + const header = page.locator("h2").first().locator(".."); + await expect(header).toContainText("E2E Mailbox"); + + // Back to the label across every account. + const allAccounts = header.getByRole("link"); + await expect(allAccounts).toBeVisible(); + await allAccounts.click(); + + await page.waitForURL(/\/mail\/label\/\d+$/); + }); + + /** + * `?account=` arrives off the query string, so it is checked rather than + * trusted — otherwise appending someone else's account id would scope a + * label you own to threads you do not. + */ + test("refuses an account the user does not own", async ({ page }) => { + await page.goto("/mail/inbox"); + await page.locator('#sidebar button[aria-label="Toggle account folders"]').first().click(); + + const scoped = page.locator('#sidebar turbo-frame a[href*="/mail/label/"]').first(); + await expect(scoped).toBeVisible(); + + const href = (await scoped.getAttribute("href"))!; + const foreign = href.replace(/account=\d+/, "account=999999"); + + const response = await page.request.get(foreign); + expect(response.status()).toBe(403); + }); +}); + +test.describe("archive", () => { + /** + * The Archive route existed and nothing linked to it: the sidebar entry was + * gated on the user having switched the Archive label visible in settings, + * and it is created hidden. + */ + test("is reachable from the sidebar without touching label settings", async ({ page }) => { + await page.goto("/mail/inbox"); + + const more = page + .locator("#sidebar details > summary") + .filter({ hasText: /^\s*(More|Mehr)\s*$/ }); + await expect(more).toBeVisible(); + + // Collapsed by default — it is not somewhere you go often. + await expect(page.locator('#sidebar a[href="/mail/archive"]')).toBeHidden(); + + await more.click(); + + const archive = page.locator('#sidebar a[href="/mail/archive"]'); + await expect(archive).toBeVisible(); + await archive.click(); + + await page.waitForURL(/\/mail\/archive/); + }); +}); diff --git a/translations/messages.de.yaml b/translations/messages.de.yaml index d099dd85..545c956f 100644 --- a/translations/messages.de.yaml +++ b/translations/messages.de.yaml @@ -381,6 +381,7 @@ integration: too_big: 'Zu groß zum Anhängen' label: + all_accounts: Alle Konten color: amber: Bernstein blue: Blau @@ -1127,6 +1128,7 @@ sidebar: archive: Archiv drafts: Entwürfe inbox: Posteingang + more: Mehr sent: Gesendet snoozed: Zurückgestellt starred: Markiert diff --git a/translations/messages.en.yaml b/translations/messages.en.yaml index 476b3fc9..30a18158 100644 --- a/translations/messages.en.yaml +++ b/translations/messages.en.yaml @@ -379,6 +379,7 @@ integration: too_big: 'Too large to attach' label: + all_accounts: All accounts color: amber: Amber blue: Blue @@ -1108,6 +1109,7 @@ sidebar: archive: Archive drafts: Drafts inbox: Inbox + more: More sent: Sent snoozed: Snoozed starred: Starred