diff --git a/src/device-sync/__tests__/contacts/groups.test.ts b/src/device-sync/__tests__/contacts/groups.test.ts index f29efe9b..c83c2637 100644 --- a/src/device-sync/__tests__/contacts/groups.test.ts +++ b/src/device-sync/__tests__/contacts/groups.test.ts @@ -20,6 +20,15 @@ async function localGroup(h: Harness, groupId: string) { return g; } +/** A group card in an address book this user may only read, on the device. */ +async function readOnlyGroup(h: Harness, members: Record = {}) { + const shared = h.server.addAddressBook(JMAP, { name: 'Shared' }); + h.readOnlyBooks.add(shared); + const gid = h.addCard({ uid: 'urn:uuid:group-shared', kind: 'group', name: { full: 'Shared' }, members, addressBookIds: { [shared]: true } }); + await h.download(gid); + return gid; +} + /** A group card holding Anna (apple card), both on the device. */ async function withGroup(members: Record = { [appleCard().uid as string]: true }) { const h = new Harness(); @@ -127,6 +136,71 @@ describe('contacts groups: membership edits', () => { expect(h.planner.planMembershipUploads([await h.contact(rawId)], await h.groups(), h.ctx)).toEqual([]); }); + it('puts back an addition to a group in read-only books instead of keeping the contact dirty', async () => { + const h = new Harness(); + const gid = await readOnlyGroup(h, { 'urn:uuid:someone': true }); + const { rawId } = await h.seed(appleCard()); + h.device.user.addToGroup(rawId, (await localGroup(h, gid)).groupId); + expect((await h.upload(rawId)).kind).toBe('revert'); + expect(h.membershipGroups(rawId)).toEqual([]); + expect(h.dirty(rawId)).toBe(false); + expect(h.planner.planMembershipUploads([await h.contact(rawId)], await h.groups(), h.ctx)).toEqual([]); + }); + + it('puts back a removal from a group in read-only books', async () => { + const h = new Harness(); + const gid = await readOnlyGroup(h, { [appleCard().uid as string]: true }); + const { rawId } = await h.seed(appleCard()); + const membership = (await h.contact(rawId)).rows.find((r) => r.mimetype === MimeType.GROUP_MEMBERSHIP)!; + h.device.user.deleteData(membership.id); + expect((await h.upload(rawId)).kind).toBe('revert'); + expect(h.membershipGroups(rawId)).toEqual([`${JMAP}/${gid}`]); + expect(h.dirty(rawId)).toBe(false); + }); + + it('puts back an addition to a group of another JMAP account', async () => { + const h = new Harness(); + h.server.addAccount('t', { name: 'Team' }); + const book = h.server.addAddressBook('t', { name: 'Team contacts' }); + const team = h.server.addCard('t', { uid: 'urn:uuid:team', kind: 'group', name: { full: 'Team' }, addressBookIds: { [book]: true } }); + const card = h.server.get('ContactCard', 't', team) as unknown as ContactCardWire; + await h.applyOk(h.planner.planGroupDownload(card, null, { ...h.ctx, jmapAccountId: 't' }).ops); + const { rawId } = await h.seed(appleCard()); + h.device.user.addToGroup(rawId, (await h.groups()).find((g) => g.sourceId === `t/${team}`)!.groupId); + expect(h.membershipGroups(rawId)).toEqual([`t/${team}`]); + expect((await h.upload(rawId)).kind).toBe('revert'); + expect(h.membershipGroups(rawId)).toEqual([]); + expect(h.dirty(rawId)).toBe(false); + }); + + it('still uploads a read-only contact joining a writable group (the group card is what changes)', async () => { + const h = new Harness(); + const other = h.server.addAddressBook(JMAP, { name: 'Shared' }); + h.readOnlyBooks.add(other); + const gid = h.addCard({ uid: 'urn:uuid:g5', kind: 'group', name: { full: 'Mine' }, members: { 'urn:uuid:someone': true } }); + await h.download(gid); + const { rawId } = await h.seed({ ...appleCard(), addressBookIds: { [other]: true } }); + h.device.user.addToGroup(rawId, (await localGroup(h, gid)).groupId); + expect((await h.upload(rawId)).kind).toBe('revert'); + expect(h.membershipGroups(rawId)).toEqual([`${JMAP}/${gid}`]); + expect(h.dirty(rawId)).toBe(true); + expect(h.planner.planMembershipUploads([await h.contact(rawId)], await h.groups(), h.ctx)).toEqual([ + { kind: 'update', id: gid, patch: { [`members/${appleCard().uid as string}`]: true } }, + ]); + }); + + it('uploads the edits it can next to a membership it puts back, and settles', async () => { + const h = new Harness(); + const gid = await readOnlyGroup(h); + const { id, rawId } = await h.seed(appleCard()); + h.device.user.addToGroup(rawId, (await localGroup(h, gid)).groupId); + h.device.user.updateData(h.rowsByKey(rawId)['emails:k1']._id as number, { data1: 'anna@new.example' }); + const plan = await h.upload(rawId); + expect(actionsOf(plan)).toEqual([{ kind: 'update', id, patch: { 'emails/k1/address': 'anna@new.example' } }]); + expect(h.membershipGroups(rawId)).toEqual([]); + expect(h.dirty(rawId)).toBe(false); + }); + it('does not take a server-side addition for a local removal while the contact is dirty', async () => { const { h, gid } = await withGroup({}); const { rawId } = await h.seed(googleCard()); diff --git a/src/device-sync/__tests__/contacts/harness.ts b/src/device-sync/__tests__/contacts/harness.ts index 095a1c9b..663dfbd0 100644 --- a/src/device-sync/__tests__/contacts/harness.ts +++ b/src/device-sync/__tests__/contacts/harness.ts @@ -6,6 +6,7 @@ */ import { GroupMembership, Groups, RawContacts } from '../../android-columns'; import { makeKeyMinter, parseObjectRef, uuidFrom } from '../../common/ids'; +import type { GroupRights } from '../../contacts/members'; import { contactsPlanner } from '../../contacts/planner'; import type { ContactCardWire, @@ -48,7 +49,7 @@ export class Harness { readonly unselected = new Set(); /** When false the context has no `groupsOf` (memberships not synced). */ groupIndex = true; - readonly ctx: ContactsContext; + readonly ctx: ContactsContext & GroupRights; constructor() { this.server.addAccount(JMAP, { name: ACCOUNT }); @@ -79,14 +80,20 @@ export class Harness { .filter((c) => c.kind === 'group' && (c.members as Record | undefined)?.[uid] === true) .map((c) => `${JMAP}/${c.id as string}`); }, + groupReadOnly(sourceId: string) { + const ref = parseObjectRef(sourceId); + const card = ref ? self.server.get('ContactCard', ref.accountId, ref.id) : undefined; + const books = Object.keys((card?.addressBookIds as Record | undefined) ?? {}); + return books.length > 0 && books.every((id) => self.readOnlyBooks.has(id)); + }, }; } /** The context as the engine may build it without a group index. */ - context(): ContactsContext { + context(): ContactsContext & GroupRights { if (this.groupIndex) return this.ctx; const { groupsOf: _drop, ...rest } = this.ctx; - return rest as ContactsContext; + return rest as ContactsContext & GroupRights; } async apply(group: OpGroup): Promise { diff --git a/src/device-sync/__tests__/contacts/real-engine.test.ts b/src/device-sync/__tests__/contacts/real-engine.test.ts index f42ba6ae..79207b23 100644 --- a/src/device-sync/__tests__/contacts/real-engine.test.ts +++ b/src/device-sync/__tests__/contacts/real-engine.test.ts @@ -3,16 +3,21 @@ // themselves are tested in the other files of this folder. import { describe, expect, it } from 'vitest'; +import { MimeType } from '../../android-columns'; import { calendarPlanner } from '../../calendar/planner'; import { contactsPlanner } from '../../contacts/planner'; -import { addServerCards, createHarness, type Harness } from '../engine/harness'; +import { addServerCards, CONTACTS_AUTHORITY, createHarness, type Harness } from '../engine/harness'; -function real(): Harness { - const h = createHarness(); +function real(options: Parameters[0] = {}): Harness { + const h = createHarness(options); h.deps.planners = { contacts: contactsPlanner, calendar: calendarPlanner }; return h; } +function memberships(h: Harness, rawContactId: number) { + return h.device.rows('data').filter((d) => Number(d.raw_contact_id) === rawContactId && d.mimetype === MimeType.GROUP_MEMBERSHIP); +} + /** An address book of the personal account that this device does not sync. */ function unsyncedBook(h: Harness, name = 'Archive'): string { const id = h.server.addAddressBook('a', { name }); @@ -51,4 +56,24 @@ describe('contacts through the engine', () => { expect(h.server.get('ContactCard', 'a', group)).toMatchObject({ addressBookIds: { [archive]: true } }); expect(h.device.rows('groups')).toEqual([]); }); + + it('puts back a membership in a group of another account, so no change waits for ever', async () => { + const h = real({ accounts: 'withShared' }); + const book = h.server.all('AddressBook', 'team')[0].id as string; + h.prefs.contactsSelection[`team/${book}`] = true; + const team = h.server.addCard('team', { uid: 'g-team', kind: 'group', name: { full: 'Team' }, addressBookIds: { [book]: true } }); + addServerCards(h, ['Ada Lovelace']); + expect((await h.run()).outcome).toBe('ok'); + + const ada = h.contactNamed('Ada Lovelace')!.id; + h.device.user.addToGroup(ada, Number(h.device.rows('groups').find((g) => g.sourceid === `team/${team}`)!._id)); + const report = await h.run(); + + expect(report).toMatchObject({ outcome: 'ok', stats: { skipped: 1 } }); + expect(h.contactNamed('Ada Lovelace')).toMatchObject({ dirty: false }); + expect(memberships(h, ada)).toEqual([]); + expect(h.server.get('ContactCard', 'team', team)?.members).toBeUndefined(); + // Turning sync off finds nothing waiting. + expect(await h.teardown(CONTACTS_AUTHORITY)).toEqual({ pending: 0 }); + }); }); diff --git a/src/device-sync/contacts/download.ts b/src/device-sync/contacts/download.ts index 066605f6..feacece2 100644 --- a/src/device-sync/contacts/download.ts +++ b/src/device-sync/contacts/download.ts @@ -14,13 +14,13 @@ */ import { Data, MimeType, RawContacts } from '../android-columns'; import { canonicalJson } from '../common/json'; -import { objectRef } from '../common/ids'; +import { objectRef, parseCollectionKey, parseObjectRef } from '../common/ids'; import type { ContactCardWire, ContactsContext, DownloadPlan, LocalContact, LocalDataRow } from '../planner'; import type { ProviderOp, WriteRow } from '../types'; import { changedFromBaseline, encodeBaseline, sameCell } from './cells'; import { localChanges, sameUnit } from './diff'; import { deletionsInferred, inPlaceEvidence, matchKind, reconcileKeys, type KindMatch } from './matching'; -import { planMemberships, remoteMemberships, shadowMemberOf, withMemberOf, withoutMemberOf } from './members'; +import { editableGroups, planMemberships, remoteMemberships, shadowMemberOf, withMemberOf, withoutMemberOf } from './members'; import { assertContact, deleteData, insertUnit, updateData, updateRawContact, updateUnit } from './ops'; import { shadowOf } from './photo'; import { KIND_ORDER, projectCard, rowMatchesUnit, SPECS, type Unit } from './project'; @@ -224,10 +224,16 @@ export interface CleanWriteOptions { accepted?: boolean; } +/** The JMAP account a local contact belongs to. */ +export function accountOf(local: LocalContact, ctx: Ctx): string { + return parseObjectRef(local.sourceId)?.accountId ?? parseCollectionKey(local.pending?.target)?.accountId ?? ctx.jmapAccountId; +} + /** * The ops that make a local contact's rows, shadow and raw columns the * server card's, behind the VERSION assert. Membership changes that the - * server does not have yet stay (and keep the contact dirty). + * server does not have yet stay (and keep the contact dirty), unless it can + * never take them: those are put back (`revertedMembers`). */ export function cleanWrite(local: LocalContact, card: ContactCardWire, ctx: Ctx, options: CleanWriteOptions) { const server = withoutMemberOf(card); @@ -243,6 +249,7 @@ export function cleanWrite(local: LocalContact, card: ContactCardWire, ctx: Ctx, restore: true, parent: local.rawContactId, groupRowIdBySourceId: (g) => ctx.groupRowIdBySourceId(g), + editable: editableGroups(ctx, accountOf(local, ctx)), }); const shadow = withMemberOf(shadowOf(server), members.memberOf); const raw = changedRawColumns(local, { @@ -258,6 +265,7 @@ export function cleanWrite(local: LocalContact, card: ContactCardWire, ctx: Ctx, ops: ops.length ? [assertContact(local, options.assertDirty), ...ops] : [], writes: ops.length, pendingMembers: members.pending, + revertedMembers: members.reverted, }; } diff --git a/src/device-sync/contacts/members.ts b/src/device-sync/contacts/members.ts index 0a07983b..118f3cb7 100644 --- a/src/device-sync/contacts/members.ts +++ b/src/device-sync/contacts/members.ts @@ -13,9 +13,12 @@ * Per group, a local change wins where the server did not change, the server * wins elsewhere; both sides can only flip a membership the same way, so * memberships never conflict. A local change stays in the base until the - * server has it, which keeps it visible to `planMembershipUploads`. + * server has it, which keeps it visible to `planMembershipUploads`; one the + * server can't take (a group in read-only books, or in another JMAP account) + * is put back by the next clean write instead. */ import { Data, GroupMembership, MimeType } from '../android-columns'; +import { parseObjectRef } from '../common/ids'; import type { ContactCardWire, ContactsContext, LocalDataRow } from '../planner'; import type { ProviderOp } from '../types'; import { text } from './cells'; @@ -23,6 +26,24 @@ import { entryKey } from './keys'; import { deleteData, insertUnit, updateData } from './ops'; import { MEMBER_OF, type Unit } from './project'; +/** + * Group rights the engine can add to the context (not part of + * ContactsContext yet): whether a group card, by SOURCE_ID, is in read-only + * address books only. Without it every synced group counts as writable. + */ +export interface GroupRights { + groupReadOnly?(sourceId: string): boolean; +} + +/** + * The groups whose members an upload for a contact of `account` can change: + * that account's groups outside read-only books. A membership change of any + * other group can never reach the server (`planMembershipUploads` skips it). + */ +export function editableGroups(ctx: ContactsContext & GroupRights, account: string): (sourceId: string) => boolean { + return (sourceId) => parseObjectRef(sourceId)?.accountId === account && !(ctx.groupReadOnly?.(sourceId) ?? false); +} + /** Remote memberships of a card, or null when the engine can't tell (memberships are then left alone). */ export function remoteMemberships(card: ContactCardWire, ctx: ContactsContext): string[] | null { if (typeof ctx.groupsOf !== 'function' || typeof card.uid !== 'string' || !card.uid) return null; @@ -71,6 +92,8 @@ export interface MemberPlan { /** Groups the device added the contact to, or removed it from, that the server does not show yet. */ added: string[]; removed: string[]; + /** Local changes of groups that are not `editable` were put back to what the server has. */ + reverted: boolean; } export interface MemberInput { @@ -83,13 +106,19 @@ export interface MemberInput { restore: boolean; parent: number | { ref: number }; groupRowIdBySourceId(sourceId: string): number | null; + /** + * Groups whose members the device may change (see `editableGroups`); a local + * change of another group is put back to what the server has rather than + * waiting for an upload that never comes. Without it every group counts. + */ + editable?(sourceId: string): boolean; } export function planMemberships(input: MemberInput): MemberPlan { const live = input.rows.filter((r) => r.mimetype === MimeType.GROUP_MEMBERSHIP); if (input.remote === null) { // Without the engine's group index memberships are not synced: nothing is written or kept pending. - return { ops: [], writes: 0, memberOf: input.base, pending: false, added: [], removed: [] }; + return { ops: [], writes: 0, memberOf: input.base, pending: false, added: [], removed: [], reverted: false }; } const ops: ProviderOp[] = []; const rowOf = new Map(); @@ -119,11 +148,17 @@ export function planMemberships(input: MemberInput): MemberPlan { after.add(g); }; + let reverted = false; for (const g of [...new Set([...base, ...remote, ...rowOf.keys()])].sort()) { const inBase = base.has(g); const inRemote = remote.has(g); const row = rowOf.get(g); - const change = inBase ? (!row && deletions ? 'removed' : null) : row ? 'added' : null; + let change = inBase ? (!row && deletions ? 'removed' : null) : row ? 'added' : null; + if (change && input.editable && !input.editable(g)) { + // A change the server can't take (a group in read-only books, or another account's): its state comes back. + if (change === 'added' ? !inRemote : inRemote) reverted = true; + change = null; + } if (change === 'added') { keep(g, row!); if (!inRemote) pendingAdded.add(g); @@ -145,5 +180,6 @@ export function planMemberships(input: MemberInput): MemberPlan { pending: unsynced || pendingAdded.size > 0 || pendingRemoved.size > 0, added: [...pendingAdded], removed: [...pendingRemoved], + reverted, }; } diff --git a/src/device-sync/contacts/upload.ts b/src/device-sync/contacts/upload.ts index debff260..34430e1b 100644 --- a/src/device-sync/contacts/upload.ts +++ b/src/device-sync/contacts/upload.ts @@ -23,7 +23,7 @@ import type { import type { ProviderOp } from '../types'; import { encodeBaseline } from './cells'; import { localChanges } from './diff'; -import { cardUnits, cleanWrite } from './download'; +import { accountOf, cardUnits, cleanWrite } from './download'; import { newEntries, unitEdits, unitEntries, type Edit, type NewEntry } from './entries'; import { parseEntryKey } from './keys'; import { deletionsInferred, inPlaceEvidence, matchKind, type KindMatch } from './matching'; @@ -263,18 +263,15 @@ export function planContactUpload(local: LocalContact, ctx: Ctx): UploadPlan[] = [{ kind: 'update', id: parseObjectRef(local.sourceId)!.id, patch }]; if (backedOff(local, ctx, actions)) return { kind: 'skip', reason: `poisoned:${local.poison!.type}` }; return { kind: 'upload', actions }; } -/** The JMAP account a local contact belongs to. */ -function accountOf(local: LocalContact, ctx: Ctx): string { - return parseObjectRef(local.sourceId)?.accountId ?? parseCollectionKey(local.pending?.target)?.accountId ?? ctx.jmapAccountId; -} - export function planContactAccepted(local: LocalContact, server: ContactCardWire, ctx: Ctx): AcceptedPlan { if (local.deleted) { const purge = group(local, [deleteWhereId('raw_contacts', local.rawContactId)]);