diff --git a/backend/__tests__/routes/adminUsage.test.js b/backend/__tests__/routes/adminUsage.test.js index 75fba5f0..187f4d4b 100644 --- a/backend/__tests__/routes/adminUsage.test.js +++ b/backend/__tests__/routes/adminUsage.test.js @@ -171,8 +171,33 @@ test('public/gallery paths and failed/unauthenticated admin operations never set simulate('/quotes', { id: 1 }, 403); expect(service.markUsed).not.toHaveBeenCalled(); simulate('/customers/42/hour-entries', { id: 1 }, 200); + // The second argument tells markUsed whether this operation writes to the + // configured backup destination; a CRM route never does. expect(service.markUsed).toHaveBeenCalledWith( - expect.arrayContaining(['crm', 'crm_hours']) + expect.arrayContaining(['crm', 'crm_hours']), + expect.objectContaining({ destinationBackup: false }) ); expect(JSON.stringify(service.markUsed.mock.calls)).not.toContain('42'); }); + +test('only a backup that writes to the configured destination flags S3', () => { + // /database-backup/* and /backup/picpeak/export produce a local file, so + // they must not imply S3 use just because S3 is the configured destination. + const seen = []; + const simulate = (pathname) => { + service.markUsed.mockClear(); + const res = new (require('events').EventEmitter)(); + res.statusCode = 200; + productUsage({ path: pathname, admin: { id: 1 } }, res, () => {}); + res.emit('finish'); + seen.push([pathname, service.markUsed.mock.calls[0]?.[1]?.destinationBackup]); + }; + simulate('/backup/run'); + simulate('/database-backup/backup'); + simulate('/backup/picpeak/export'); + expect(seen).toEqual([ + ['/backup/run', true], + ['/database-backup/backup', false], + ['/backup/picpeak/export', false], + ]); +}); diff --git a/backend/__tests__/services/usageSnapshotSignals.test.js b/backend/__tests__/services/usageSnapshotSignals.test.js index b31efcf4..c575bdf1 100644 --- a/backend/__tests__/services/usageSnapshotSignals.test.js +++ b/backend/__tests__/services/usageSnapshotSignals.test.js @@ -150,3 +150,45 @@ describe('status survives a misconfigured collector URL', () => { expect(status.collector_url).toBe('https://usage.picpeak.app'); }); }); + +describe('S3 use is only implied by backups that write to the destination', () => { + let db; + afterEach(async () => { if (db) await db.destroy(); db = null; }); + + const withS3Destination = async (database) => { + await database('app_settings').insert({ + setting_key: 'backup_destination_type', + setting_value: JSON.stringify('s3'), + }); + await database('product_usage_state').where({ id: 1 }).update({ status: 'active' }); + }; + + it('marks S3 for a backup that uses the configured destination', async () => { + db = await bootDb(); + await withS3Destination(db); + await service(db).markUsed(['backup'], { destinationBackup: true }); + expect((await db('product_usage_markers').pluck('feature')).sort()) + .toEqual(['backup', 's3_storage']); + }); + + it('does NOT mark S3 for a local backup, even with S3 configured', async () => { + // /database-backup/* and /backup/picpeak/export produce a local file. They + // count as `backup`, but claiming S3 was used for them made merely + // configuring S3 and downloading an export report s3_storage.used. + db = await bootDb(); + await withS3Destination(db); + await service(db).markUsed(['backup']); + expect(await db('product_usage_markers').pluck('feature')).toEqual(['backup']); + }); + + it('does not mark S3 when the destination is not S3', async () => { + db = await bootDb(); + await db('app_settings').insert({ + setting_key: 'backup_destination_type', + setting_value: JSON.stringify('local'), + }); + await db('product_usage_state').where({ id: 1 }).update({ status: 'active' }); + await service(db).markUsed(['backup'], { destinationBackup: true }); + expect(await db('product_usage_markers').pluck('feature')).toEqual(['backup']); + }); +}); diff --git a/backend/src/middleware/productUsage.js b/backend/src/middleware/productUsage.js index 7bbd7eb7..a8d41c14 100644 --- a/backend/src/middleware/productUsage.js +++ b/backend/src/middleware/productUsage.js @@ -28,6 +28,12 @@ const RULES = [ [/^\/email\/(?:test|send)(?:\/|$)/, ['smtp']], [/^\/external-media(?:\/|$)/, ['share_mounts']] ]; +// Backup operations that write to the CONFIGURED destination, and so imply +// S3 use when that destination is S3. Deliberately excludes +// /backup/picpeak/export and everything under /database-backup/, which +// produce a local file regardless of where scheduled backups go. +const DESTINATION_BACKUP = /^\/backup\/(?:run|backup|create|start|test)(?:\/|$)/; + function productUsage(req, res, next) { const pathname = req.path; res.once('finish', () => { @@ -42,9 +48,11 @@ function productUsage(req, res, next) { features.push('s3_storage'); if (features.length) service - .markUsed(features) + .markUsed(features, { + destinationBackup: DESTINATION_BACKUP.test(pathname) + }) .catch(() => logger.warn('Product usage marker could not be recorded')); }); next(); } -module.exports = { productUsage, RULES }; +module.exports = { productUsage, RULES, DESTINATION_BACKUP }; diff --git a/backend/src/routes/adminUsage.js b/backend/src/routes/adminUsage.js index 20cb1b38..9506dddc 100644 --- a/backend/src/routes/adminUsage.js +++ b/backend/src/routes/adminUsage.js @@ -4,10 +4,18 @@ const { adminAuth } = require('../middleware/auth'); const { requirePermission } = require('../middleware/permissions'); const { ValidationError } = require('../utils/errors'); const service = require('../services/productUsageService'); +const { ProtocolError } = require('../usage/protocol.cjs'); const router = express.Router(); const wrap = (fn) => (req, res, next) => Promise.resolve(fn(req, res)).catch((error) => { - if (error.name === 'ProtocolError') + // instanceof, not `error.name`: ProtocolError extends Error without + // setting `name`, so every instance reports 'Error' and this branch never + // ran. A malformed vote or feedback payload fell through to the global + // handler, which logs it as an unhandled programming error and answers + // INTERNAL_ERROR in production — losing the validation code the caller + // needs. protocol.cjs is vendored byte-identical with picpeak-usage, so + // the fix belongs here rather than in the class. + if (error instanceof ProtocolError) return res .status(400) .json({ error: 'Invalid usage request', code: error.code }); diff --git a/backend/src/usage/UsageService.js b/backend/src/usage/UsageService.js index d34568b4..c87584e7 100644 --- a/backend/src/usage/UsageService.js +++ b/backend/src/usage/UsageService.js @@ -572,7 +572,7 @@ class UsageService { return this.status(); } - async markUsed(features) { + async markUsed(features, { destinationBackup = false } = {}) { const allowed = [...new Set(features)].filter((f) => FEATURE_KEYS.includes(f) ); @@ -583,7 +583,12 @@ class UsageService { if (this.db.client.config.client === 'pg') query.forUpdate(); const state = await query.first(); if (!state || state.status !== 'active') return; - if (allowed.includes('backup')) { + // Only when the operation actually writes to the configured backup + // destination. Deriving this from "a backup ran while S3 is configured" + // marked S3 as USED for a local database backup or a .picpeak export, + // which the middleware also counts as `backup` — so merely configuring + // S3 and downloading a local export claimed S3 was in use. + if (destinationBackup && allowed.includes('backup')) { const destination = await tx('app_settings') .where({ setting_key: 'backup_destination_type' }) .first();