diff --git a/backend/__tests__/services/usageEnableDisableRace.test.js b/backend/__tests__/services/usageEnableDisableRace.test.js new file mode 100644 index 00000000..f387859c --- /dev/null +++ b/backend/__tests__/services/usageEnableDisableRace.test.js @@ -0,0 +1,111 @@ +/** + * /disable overlapping an in-flight /enable (#1110). + * + * While activation generates an identity and writes its binding file the row + * still reads `disabled`, so disable()'s conditional update matched nothing + * and the lease conflict from its tick() was swallowed. The admin was told + * participation was off, and the activation then completed and left it on — + * an opt-out silently ignored, which is the one thing this feature cannot do. + * + * enable() now claims its state with a single conditional UPDATE that also + * tests the cancellation flag, so whichever lands first wins outright. + */ +const knex = require('knex'); +const { UsageService } = require('../../src/usage/UsageService'); + +const SECRET = 'z'.repeat(48); + +async function bootDb() { + const db = knex({ + client: 'sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await db.schema.createTable('product_usage_state', (t) => { + t.integer('id').primary(); + t.string('status', 30).notNullable().defaultTo('disabled'); + t.boolean('notice_dismissed').notNullable().defaultTo(false); + t.string('installation_id', 64); + t.string('public_key', 59); + t.text('private_key_encrypted'); + t.string('instance_binding', 64); + t.bigInteger('sequence').notNullable().defaultTo(0); + t.text('pending_packet'); + t.text('last_packet'); + t.text('last_receipt'); + t.string('last_report_date', 10); + t.string('last_error', 80); + t.text('feedback_preferences'); + t.string('lease_token', 36); + t.bigInteger('lease_until').notNullable().defaultTo(0); + t.boolean('cancel_requested').notNullable().defaultTo(false); + }); + await db.schema.createTable('product_usage_markers', (t) => { + t.string('feature', 60).primary(); + }); + await db('product_usage_state').insert({ id: 1 }); + return db; +} + +/** A service whose binding() is slow, so the race window is controllable. */ +function makeService(db, { onBinding } = {}) { + const service = new UsageService(db, { + secret: SECRET, + endpoint: 'http://127.0.0.1:9/', + fetch: async () => { throw new Error('collector must not be reached'); }, + }); + const realBinding = service.binding.bind(service); + service.binding = async (create = false) => { + if (create && onBinding) await onBinding(); + return realBinding === undefined ? 'x'.repeat(64) : 'b'.repeat(64); + }; + return service; +} + +describe('withdrawal during an in-flight activation', () => { + let db; + afterEach(async () => { if (db) await db.destroy(); db = null; }); + + it('honours a /disable that lands while /enable is still generating its identity', async () => { + db = await bootDb(); + let disableDone; + const service = makeService(db, { + // Fires inside enable(), before it claims the row — exactly the window + // where the status still reads `disabled`. + onBinding: async () => { disableDone = await service.disable(); }, + }); + + await service.enable('usage-consent.v1'); + + const row = await db('product_usage_state').where({ id: 1 }).first(); + expect(row.status).toBe('disabled'); + // Nothing was registered, so there is no identity and nothing to delete. + expect(row.installation_id).toBeNull(); + expect(row.pending_packet).toBeNull(); + expect(disableDone.status).toBe('disabled'); + }); + + it('activates normally when no withdrawal arrives', async () => { + db = await bootDb(); + const service = makeService(db); + await service.enable('usage-consent.v1'); + + const row = await db('product_usage_state').where({ id: 1 }).first(); + // The collector is unreachable here, so it stops at activation_pending — + // the point is that the claim succeeded and an identity exists. + expect(row.status).toBe('activation_pending'); + expect(row.installation_id).not.toBeNull(); + }); + + it('does not let a stale cancellation veto a later deliberate opt-in', async () => { + db = await bootDb(); + await db('product_usage_state').where({ id: 1 }).update({ cancel_requested: true }); + + const service = makeService(db); + await service.enable('usage-consent.v1'); + + const row = await db('product_usage_state').where({ id: 1 }).first(); + expect(row.status).toBe('activation_pending'); + expect(row.installation_id).not.toBeNull(); + }); +}); diff --git a/backend/migrations/core/202_product_usage_cancel_requested.js b/backend/migrations/core/202_product_usage_cancel_requested.js new file mode 100644 index 00000000..97b3a1f0 --- /dev/null +++ b/backend/migrations/core/202_product_usage_cancel_requested.js @@ -0,0 +1,22 @@ +// Separate from 201 deliberately. 201 already shipped on this branch, and +// knex records it as applied — so folding the column into it would silently +// skip every database that had already run it, and the first /disable would +// fail on a missing column. Its own migration runs everywhere. +exports.up = async function (knex) { + if (!(await knex.schema.hasTable('product_usage_state'))) return; + if (await knex.schema.hasColumn('product_usage_state', 'cancel_requested')) return; + await knex.schema.alterTable('product_usage_state', (t) => { + // Set by /disable so an activation still generating its identity — during + // which the row still reads `disabled` — cannot go on to complete after + // the admin has asked to withdraw. + t.boolean('cancel_requested').notNullable().defaultTo(false); + }); +}; + +exports.down = async function (knex) { + if (!(await knex.schema.hasTable('product_usage_state'))) return; + if (!(await knex.schema.hasColumn('product_usage_state', 'cancel_requested'))) return; + await knex.schema.alterTable('product_usage_state', (t) => { + t.dropColumn('cancel_requested'); + }); +}; diff --git a/backend/src/usage/UsageService.js b/backend/src/usage/UsageService.js index ef74b1f7..705f73ca 100644 --- a/backend/src/usage/UsageService.js +++ b/backend/src/usage/UsageService.js @@ -220,29 +220,63 @@ class UsageService { 'Finish the current participation before rejoining' ); this.collectorUrl(); + // A cancellation left over from an earlier participation must not veto + // this deliberate opt-in, so the flag is cleared before the slow work + // starts. Anything set from here on is a withdrawal aimed at THIS + // activation. + await this.db('product_usage_state') + .where({ id: 1 }) + .update({ cancel_requested: formatBoolean(false) }); + const identity = generateIdentity(); const pending = makePacket(identity, 'register', 0, { consent_version: consent }); - await this.db('product_usage_state') - .where({ id: 1 }) + // Identity generation and the binding file are the slow part, and the + // row still reads `disabled` throughout — which is why /disable could + // not see an activation in flight and its conditional update matched + // nothing. + const instanceBinding = await this.binding(true); + + // The claim itself carries the check. Re-reading the flag and then + // updating would only move the window rather than close it: this is one + // conditional UPDATE, so a /disable that lands first makes it match no + // rows and the registration is never sent. + const claimed = await this.db('product_usage_state') + .where({ id: 1, status: 'disabled' }) + .whereNot({ cancel_requested: formatBoolean(true) }) .update({ status: 'activation_pending', notice_dismissed: formatBoolean(true), installation_id: identity.installation_id, public_key: identity.public_key, private_key_encrypted: this.encrypt(identity.private_key), - instance_binding: await this.binding(true), + instance_binding: instanceBinding, sequence: 0, pending_packet: JSON.stringify(pending), last_error: null }); + // Withdrawn while activating. Participation stays off and nothing was + // registered, so there is nothing to delete remotely either. + if (!claimed) return; + await this.deliver(await this.state()); }); return this.status(); } async disable() { + // Recorded unconditionally and first, because the interesting case is the + // one where there is seemingly nothing to stop: while /enable is still + // generating an identity the row reads `disabled`, so the conditional + // update below matches nothing and the lease conflict from tick() is + // swallowed — the admin was told participation was off while the + // activation went on to complete. enable() claims its state conditionally + // on this flag, so a withdrawal that lands during that window wins. + await this.db('product_usage_state') + .where({ id: 1 }) + .update({ cancel_requested: formatBoolean(true) }); + // Stop collection before waiting for an in-flight send. The sender checks // state again before delivery and preserves this stop after its response. await this.db('product_usage_state')