diff --git a/components/filters/filter-rule-modal.tsx b/components/filters/filter-rule-modal.tsx index 93672cf73..25d15ee10 100644 --- a/components/filters/filter-rule-modal.tsx +++ b/components/filters/filter-rule-modal.tsx @@ -19,6 +19,7 @@ import type { Mailbox } from "@/lib/jmap/types"; import { buildMailboxTree, flattenMailboxTree, type MailboxNode, generateUUID, cn } from "@/lib/utils"; import type { RuleSuggestion } from "@/lib/filters/quick-rules"; import { retroactiveSupport } from "@/lib/filters/retroactive"; +import { ruleForwards, ruleStops } from "@/lib/filters/forward-limit"; import { useSettingsStore } from "@/stores/settings-store"; import { useKeywordFormat } from "@/hooks/use-keyword-format"; @@ -36,8 +37,13 @@ interface FilterRuleModalProps { mailboxes: Mailbox[]; /** Server cap on redirects per message (Sieve `maxNumberRedirects`). */ maxRedirects?: number | null; - /** Forward actions in the other enabled rules, which share that cap. */ - otherForwards?: number; + /** + * Forwards a message can have collected when it reaches this rule, and + * the most it can still collect below it when the rule does not stop + * (see forwardsAround in lib/filters/forward-limit.ts). + */ + forwardsBefore?: number; + forwardsAfter?: number; onSave: (rule: FilterRule, options?: { applyToExisting: boolean }) => void; onClose: () => void; } @@ -107,7 +113,8 @@ export function FilterRuleModal({ offerApplyToExisting = false, mailboxes, maxRedirects, - otherForwards = 0, + forwardsBefore = 0, + forwardsAfter = 0, onSave, onClose, }: FilterRuleModalProps) { @@ -159,7 +166,11 @@ export function FilterRuleModal({ return ""; }, [mailboxPathMap]); - const forwardCount = actions.filter((a) => a.type === "forward").length; + // What counts is the most forwards one message can collect: those of the + // rules above that let it go on, this rule's, and those below unless this + // rule stops. + const forwardCount = ruleForwards({ actions }); + const otherForwards = forwardsBefore + (ruleStops({ actions, stopProcessing }) ? 0 : forwardsAfter); const forwardLimit = typeof maxRedirects === "number" && maxRedirects > 0 ? maxRedirects : null; const forwardOverLimit = forwardLimit !== null && forwardCount + otherForwards > forwardLimit; diff --git a/components/settings/__tests__/filter-settings-forward-limit.test.tsx b/components/settings/__tests__/filter-settings-forward-limit.test.tsx new file mode 100644 index 000000000..2cea5d85d --- /dev/null +++ b/components/settings/__tests__/filter-settings-forward-limit.test.tsx @@ -0,0 +1,153 @@ +import { fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import React from 'react'; +import type { FilterAction, FilterRule, SieveCapabilities } from '@/lib/jmap/sieve-types'; +import { useAuthStore } from '@/stores/auth-store'; +import { useEmailStore } from '@/stores/email-store'; +import { useFilterStore } from '@/stores/filter-store'; +import { FilterSettings } from '../filter-settings'; + +// The redirect limit on the settings page: the real page and rule dialog, +// with a server that allows one redirect per message, as Stalwart does. + +vi.mock('@/stores/toast-store', () => ({ toast: { error: vi.fn(), success: vi.fn(), info: vi.fn() } })); + +const CAPABILITIES: SieveCapabilities = { + implementation: 'Stalwart', + maxSizeScript: 102400, + sieveExtensions: ['fileinto', 'mailbox', 'imap4flags', 'copy'], + notificationMethods: [], + externalLists: [], + maxNumberRedirects: 1, +}; + +const forward: FilterAction = { type: 'forward', value: 'colleague@example.com' }; +const rule = (id: string, name: string, actions: FilterAction[], stopProcessing: boolean): FilterRule => ({ + id, + name, + enabled: true, + matchType: 'all', + conditions: [{ field: 'from', comparator: 'contains', value: 'reports@vendor.example' }], + actions, + stopProcessing, +}); +// Forwards to a colleague, deletes silently, and stops processing. +const vendor = (stops = true) => rule('vendor', 'Vendor reports', [forward, { type: 'discard' }], stops); +const everyoneElse = rule('rest', 'Everyone else', [{ type: 'forward', value: 'office@example.com' }], false); + +const initial = { + auth: useAuthStore.getState(), + email: useEmailStore.getState(), + filter: useFilterStore.getState(), +}; + +beforeEach(() => { + useAuthStore.setState({ client: { updateSieveScript: vi.fn(async () => {}), getSieveAccountId: () => 'primary' } as never }); + useEmailStore.setState({ fetchMailboxes: vi.fn(async () => {}) as never }); +}); + +afterEach(() => { + useAuthStore.setState(initial.auth, true); + useEmailStore.setState(initial.email, true); + useFilterStore.setState(initial.filter, true); +}); + +function openSettings(rules: FilterRule[]) { + useFilterStore.setState({ + rules, + isSupported: true, + isLoading: false, + isOpaque: false, + activeScriptId: 'script-1', + selectedAccountId: 'primary', + sieveCapabilities: CAPABILITIES, + selectAccount: vi.fn(async () => {}), + }); + render(); +} + +const forwardOption = (dialog: ReturnType) => + dialog.getAllByRole('combobox', { name: 'actions' })[0].querySelector('option[value="forward"]') as HTMLOptionElement; + +describe('the redirect limit on the settings page', () => { + it('offers forwarding in a new rule below one that forwards and stops', () => { + openSettings([vendor()]); + fireEvent.click(screen.getByText('add_rule')); + const dialog = within(screen.getByRole('dialog')); + expect(forwardOption(dialog).disabled).toBe(false); + expect(screen.queryByText('forward_limit')).toBeNull(); + }); + + it('keeps forwarding locked below a rule that forwards and lets messages go on', () => { + openSettings([vendor(false)]); + fireEvent.click(screen.getByText('add_rule')); + expect(forwardOption(within(screen.getByRole('dialog'))).disabled).toBe(true); + }); + + it('says so in the list when the order lets one message collect too many', () => { + openSettings([everyoneElse, vendor()]); + expect(screen.getByText('forward_limit')).toBeInTheDocument(); + }); + + it('says nothing in the list when the rule that stops comes first', () => { + openSettings([vendor(), everyoneElse]); + expect(screen.queryByText('forward_limit')).toBeNull(); + }); + + it('says so in the list once a rule is switched on that lets one message collect too many', async () => { + openSettings([{ ...everyoneElse, enabled: false }, vendor()]); + expect(screen.queryByText('forward_limit')).toBeNull(); + fireEvent.click(within(screen.getByRole('list', { name: 'rule_list' })).getAllByRole('switch')[0]); + // Saved, not put back. + await waitFor(() => expect(useFilterStore.getState().rawScript).toContain('redirect "office@example.com"')); + expect(screen.getByText('forward_limit')).toBeInTheDocument(); + }); + + it('says so in the list once a rule is moved where one message collects too many', async () => { + openSettings([vendor(), everyoneElse]); + expect(screen.queryByText('forward_limit')).toBeNull(); + const [vendorRow, restRow] = within(screen.getByRole('list', { name: 'rule_list' })).getAllByRole('listitem'); + const dataTransfer = { setData: vi.fn(), effectAllowed: '', dropEffect: '' }; + fireEvent.dragStart(restRow, { dataTransfer }); + fireEvent.dragOver(vendorRow, { dataTransfer }); + fireEvent.drop(vendorRow, { dataTransfer }); + await waitFor(() => expect(useFilterStore.getState().rawScript).toMatch(/# Rule: Everyone else[\s\S]*# Rule: Vendor reports/)); + expect(screen.getByText('forward_limit')).toBeInTheDocument(); + }); + + it('counts the forward of an edited rule once', () => { + openSettings([vendor(), everyoneElse]); + fireEvent.click(screen.getByText('Everyone else')); + const dialog = within(screen.getByRole('dialog')); + expect(dialog.queryByText('forward_limit')).toBeNull(); + }); + + it('does not count a forward behind a Stop action', () => { + const office: FilterAction = { type: 'forward', value: 'office@example.com' }; + openSettings([rule('cut', 'Cut short', [forward, { type: 'stop' }, office], false)]); + fireEvent.click(screen.getByText('Cut short')); + expect(within(screen.getByRole('dialog')).queryByText('forward_limit')).toBeNull(); + }); + + it('puts a new rule above the rules managed elsewhere', () => { + const archive: FilterAction = { type: 'forward', value: 'archive@example.org' }; + openSettings([{ ...rule('ext', 'Managed elsewhere', [archive], true), origin: 'external' }]); + fireEvent.click(screen.getByText('add_rule')); + const dialog = within(screen.getByRole('dialog')); + // A message the new rule matches still reaches the one below it ... + expect(forwardOption(dialog).disabled).toBe(true); + // ... unless the new rule stops. + fireEvent.click(dialog.getByLabelText('stop_processing')); + expect(forwardOption(dialog).disabled).toBe(false); + }); + + it('warns in the dialog once the rule above stops no more', () => { + openSettings([vendor(), everyoneElse]); + fireEvent.click(screen.getByText('Vendor reports')); + const dialog = within(screen.getByRole('dialog')); + expect(dialog.queryByText('forward_limit')).toBeNull(); + // Without its stop, a vendor report also reaches the rule below. + fireEvent.click(dialog.getByLabelText('stop_processing')); + expect(dialog.getByText('forward_limit')).toBeInTheDocument(); + }); +}); diff --git a/components/settings/filter-settings.tsx b/components/settings/filter-settings.tsx index 2e3c9ba05..8993be414 100644 --- a/components/settings/filter-settings.tsx +++ b/components/settings/filter-settings.tsx @@ -13,6 +13,7 @@ import { useSettingsStore } from "@/stores/settings-store"; import { toast } from "@/stores/toast-store"; import type { FilterRule } from "@/lib/jmap/sieve-types"; import type { Mailbox } from "@/lib/jmap/types"; +import { forwardsAround, worstCaseForwards } from "@/lib/filters/forward-limit"; import { useVacationStore } from "@/stores/vacation-store"; import { useManagedAccountStore } from "@/stores/managed-account-store"; import { @@ -248,6 +249,20 @@ export function FilterSettings() { const [dragOverIndex, setDragOverIndex] = useState(null); const draggedIndexRef = useRef(null); + // A message can collect only so many forwards on this server. The rules run + // in the order listed, and one that stops ends the script for its messages, + // so the order counts (see lib/filters/forward-limit.ts). + const redirectLimit = sieveCapabilities?.maxNumberRedirects; + const tooManyForwards = + typeof redirectLimit === "number" && redirectLimit > 0 && worstCaseForwards(rules) > redirectLimit; + // The edited rule where it is; a new one goes below Bulwark's own rules. + const editedIndex = editingRule ? rules.findIndex((r) => r.id === editingRule.id) : -1; + const forwardsOfEdited = forwardsAround( + rules, + editedIndex >= 0 ? editedIndex : rules.filter((r) => !isReadonlyRule(r)).length, + editedIndex >= 0, + ); + useEffect(() => { if (client && isSupported) { // Always pass a concrete account id (managed shared account, or the @@ -516,6 +531,12 @@ export function FilterSettings() { )} + {!isOpaque && tooManyForwards && ( +

+ {t("forward_limit", { count: redirectLimit })} +

+ )} + {!isOpaque && rules.length > 0 && (
{rules.map((rule, index) => { @@ -702,9 +723,8 @@ export function FilterSettings() { rule={editingRule} mailboxes={mailboxes} maxRedirects={sieveCapabilities?.maxNumberRedirects} - otherForwards={rules - .filter((r) => r.enabled && r.id !== editingRule?.id) - .reduce((n, r) => n + r.actions.filter((a) => a.type === "forward").length, 0)} + forwardsBefore={forwardsOfEdited.before} + forwardsAfter={forwardsOfEdited.after} onSave={handleSaveRule} onClose={() => { setShowRuleModal(false); diff --git a/lib/filters/__tests__/forward-limit.test.ts b/lib/filters/__tests__/forward-limit.test.ts new file mode 100644 index 000000000..ec8393ab5 --- /dev/null +++ b/lib/filters/__tests__/forward-limit.test.ts @@ -0,0 +1,162 @@ +import { describe, expect, it } from 'vitest'; +import type { FilterAction, FilterRule } from '@/lib/jmap/sieve-types'; +import { generateScript } from '@/lib/sieve/generator'; +import { forwardsAround, ruleForwards, ruleStops, worstCaseForwards } from '../forward-limit'; + +const forward: FilterAction = { type: 'forward', value: 'x@example.com' }; +const rule = (actions: FilterAction[], stopProcessing = false, enabled = true): FilterRule => ({ + id: `r-${Math.random()}`, + name: 'R', + enabled, + matchType: 'all', + conditions: [{ field: 'from', comparator: 'contains', value: 'a' }], + actions, + stopProcessing, +}); + +// "Forward to a colleague, delete silently, stop processing", and a rule for +// every other message that forwards too. +const vendor = rule([forward, { type: 'discard' }], true); +const everyoneElse = rule([forward]); + +describe('the forwards of one rule', () => { + it('counts those ahead of a Stop action', () => { + expect(ruleForwards(rule([forward, forward]))).toBe(2); + expect(ruleForwards(rule([forward, { type: 'stop' }, forward]))).toBe(1); + expect(ruleForwards(rule([{ type: 'mark_read' }]))).toBe(0); + }); + + it('knows a rule stops by its checkbox or a Stop action', () => { + expect(ruleStops(rule([forward], true))).toBe(true); + expect(ruleStops(rule([forward, { type: 'stop' }]))).toBe(true); + // Delete silently and Reject alone do not end the script. + expect(ruleStops(rule([forward, { type: 'discard' }]))).toBe(false); + expect(ruleStops(rule([{ type: 'reject', value: 'no' }]))).toBe(false); + }); +}); + +describe('the most forwards one message can collect', () => { + it('stays at one when a rule that forwards stops ahead of another that forwards', () => { + expect(worstCaseForwards([vendor, everyoneElse])).toBe(1); + }); + + it('is two once the rule for every message comes first', () => { + // A message for the colleague meets both: the second forward would be skipped. + expect(worstCaseForwards([everyoneElse, vendor])).toBe(2); + }); + + it('adds up the rules that let a message go on', () => { + expect(worstCaseForwards([everyoneElse, rule([forward]), rule([forward])])).toBe(3); + expect(worstCaseForwards([rule([forward], true), rule([forward, forward], true)])).toBe(2); + }); + + it('leaves out rules that are off', () => { + expect(worstCaseForwards([rule([forward], false, false), vendor])).toBe(1); + }); + + it('is none without forwards', () => { + expect(worstCaseForwards([])).toBe(0); + expect(worstCaseForwards([rule([{ type: 'mark_read' }], true)])).toBe(0); + }); +}); + +describe('the forwards around one rule', () => { + it('takes nothing from a rule above that stops, for a new rule below it', () => { + expect(forwardsAround([vendor], 1, false)).toEqual({ before: 0, after: 0 }); + }); + + it('takes the forwards of a rule above that lets messages go on', () => { + const going = rule([forward, { type: 'discard' }]); + expect(forwardsAround([going], 1, false)).toEqual({ before: 1, after: 0 }); + }); + + it('takes the worst case below an edited rule, without the rule itself', () => { + expect(forwardsAround([vendor, everyoneElse], 0, true)).toEqual({ before: 0, after: 1 }); + expect(forwardsAround([everyoneElse, vendor], 1, true)).toEqual({ before: 1, after: 0 }); + }); + + it('puts a new rule at the index, keeping the rule there below it', () => { + expect(forwardsAround([everyoneElse, vendor], 1, false)).toEqual({ before: 1, after: 1 }); + }); +}); + +describe('the count and the script', () => { + it('agree on the forwards a rule runs and whether it stops', () => { + const variants: FilterAction[][] = [ + [forward], + [forward, { type: 'discard' }], + [forward, { type: 'reject', value: 'no' }], + [forward, { type: 'move', value: 'Archive' }], + [{ type: 'mark_read' }, forward, forward], + [forward, { type: 'stop' }, forward], + [{ type: 'stop' }, forward], + ]; + for (const actions of variants) { + for (const stopProcessing of [false, true]) { + const r = rule(actions, stopProcessing); + const lines = generateScript([r]).split('# Rule:')[1].split('\n').map((line) => line.trim()); + const stop = lines.indexOf('stop;'); + const runs = stop === -1 ? lines : lines.slice(0, stop); + const label = `${actions.map((a) => a.type).join(', ')}${stopProcessing ? ', stop processing' : ''}`; + expect(runs.filter((line) => line.startsWith('redirect ')).length, label).toBe(ruleForwards(r)); + expect(stop !== -1, label).toBe(ruleStops(r)); + } + } + }); +}); + +describe('against every set of rules a message can match', () => { + // A message that matches the rules in `matched`: the forwards it collects, + // and the rules that run for it. + function walk(rules: FilterRule[], matched: Set): { forwards: number; ran: Set } { + let forwards = 0; + const ran = new Set(); + for (let i = 0; i < rules.length; i++) { + if (!rules[i].enabled || !matched.has(i)) continue; + ran.add(i); + forwards += ruleForwards(rules[i]); + if (ruleStops(rules[i])) break; + } + return { forwards, ran }; + } + // The most forwards one message can collect; with `runs`, one the rule + // there runs for. + function mostCollected(rules: FilterRule[], runs?: number): number { + let most = 0; + for (let mask = 0; mask < 1 << rules.length; mask++) { + const matched = new Set(rules.map((_, i) => i).filter((i) => mask & (1 << i))); + const { forwards, ran } = walk(rules, matched); + if (runs === undefined || ran.has(runs)) most = Math.max(most, forwards); + } + return most; + } + + // Fixed seed: the same rule lists on every run. + let seed = 7; + const random = () => (seed = (seed * 16807) % 2147483647) / 2147483647; + const pool: FilterAction[][] = [ + [], [forward], [forward, forward], [forward, { type: 'discard' }], + [forward, { type: 'stop' }], [{ type: 'stop' }, forward], [{ type: 'mark_read' }], + ]; + const lists = Array.from({ length: 300 }, () => + Array.from({ length: Math.floor(random() * 7) }, () => + rule(pool[Math.floor(random() * pool.length)], random() < 0.4, random() < 0.8))); + + it('finds the most forwards one message can collect', () => { + for (const rules of lists) expect(worstCaseForwards(rules)).toBe(mostCollected(rules)); + }); + + it('finds the most a message the rule runs for can collect, for rules in place and new ones', () => { + for (const rules of lists) { + for (let i = 0; i < rules.length; i++) { + const r = rules[i]; + if (!r.enabled) continue; + const { before, after } = forwardsAround(rules, i, true); + expect(before + ruleForwards(r) + (ruleStops(r) ? 0 : after)).toBe(mostCollected(rules, i)); + // The same rule about to be put there, rather than there already. + const others = rules.filter((_, j) => j !== i); + expect(forwardsAround(others, i, false)).toEqual({ before, after }); + } + } + }); +}); diff --git a/lib/filters/forward-limit.ts b/lib/filters/forward-limit.ts new file mode 100644 index 000000000..227132bf0 --- /dev/null +++ b/lib/filters/forward-limit.ts @@ -0,0 +1,63 @@ +import type { FilterRule } from '@/lib/jmap/sieve-types'; + +/* + * A server lets one message trigger only so many redirects + * (maxNumberRedirects, 1 for user scripts on Stalwart by default) and skips + * the ones over that. The rules run in order, and a rule that stops ("Stop + * processing", or the Stop action) ends the script for the messages it + * matches: the forwards of the rules below it never add to its own. What + * counts is the most forwards a single message can collect, not how many + * forwards there are. + */ + +type RuleForwards = Pick; + +/** The forwards of a rule that run: those ahead of a Stop action. */ +export function ruleForwards(rule: Pick): number { + let forwards = 0; + for (const action of rule.actions) { + if (action.type === 'stop') break; + if (action.type === 'forward') forwards++; + } + return forwards; +} + +/** The rule ends the script for the messages it matches. */ +export function ruleStops(rule: Pick): boolean { + return rule.stopProcessing || rule.actions.some((action) => action.type === 'stop'); +} + +/** The most forwards one message can collect going through `rules`, in their order. */ +export function worstCaseForwards(rules: RuleForwards[]): number { + // From the rules so far that let the message go on. + let open = 0; + let worst = 0; + for (const rule of rules) { + if (!rule.enabled) continue; + const forwards = ruleForwards(rule); + if (ruleStops(rule)) worst = Math.max(worst, open + forwards); + else open += forwards; + } + return Math.max(worst, open); +} + +/** + * For the rule at `index` of `rules` (in the order they run): the forwards a + * message can have collected when it reaches the rule, and the most it can + * still collect below it when the rule lets it go on. `replaces`: the rule + * is at `index` already, rather than about to be put there. + */ +export function forwardsAround( + rules: RuleForwards[], + index: number, + replaces: boolean, +): { before: number; after: number } { + // A rule above that stops lets only the messages it does not match go on, + // so its forwards never meet this rule's. + const before = rules + .slice(0, index) + .filter((rule) => rule.enabled && !ruleStops(rule)) + .reduce((n, rule) => n + ruleForwards(rule), 0); + const after = worstCaseForwards(rules.slice(replaces ? index + 1 : index)); + return { before, after }; +}