From 8e892f1a1f2b503f9d0965c41a605bd391ac9a4e Mon Sep 17 00:00:00 2001 From: Anurag Mittal <1321012+anurag4DSB@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:13:59 +0100 Subject: [PATCH 1/2] CLDSRV-1068: test destination Delete Add unit tests for the Delete handler on the in-memory backend, on a stored record. They check: - the record removed with no body, and Get then answering NoSuchDestination - only the named record removed - an IAM user deleting for its account - an unknown name, a second Delete and another account's name: 404 with no delete call, and the record kept - a record gone between the read and the delete - invalid names: 400 before any read - store read and delete errors, returned unchanged - no record value in the logs Add functional tests through Create, Get and List. They check: - Delete 204 with no body, then Get 404 - an unknown name and a second Delete: 404 - the name created again after a Delete - List no longer showing a deleted name - a bad name 400 - anonymous 403, with the record kept - with a real Vault: an IAM user with no policy gets 403 on Get, List and Delete, with the record kept (Create and Update have blocks of their own) - with a real Vault: a user allowed to delete one destination deletes it and gets 403 on another When each test of the route tests ends, it deletes the destinations that it created, with the credentials of their account. Each IAM test also deletes its IAM user. Thus a run leaves no record. The per-run name suffix stays, for runs that share a server. The api tests no longer say that the destination handlers do not exist yet, and drop the case of a destination method with no handler. --- .../test/routes/notificationDestination.js | 199 ++++++++++++++++- tests/unit/api/api.js | 16 +- .../unit/api/notificationDestinationDelete.js | 205 ++++++++++++++++++ 3 files changed, 402 insertions(+), 18 deletions(-) create mode 100644 tests/unit/api/notificationDestinationDelete.js diff --git a/tests/functional/raw-node/test/routes/notificationDestination.js b/tests/functional/raw-node/test/routes/notificationDestination.js index c4065e5d81..8e38ab104a 100644 --- a/tests/functional/raw-node/test/routes/notificationDestination.js +++ b/tests/functional/raw-node/test/routes/notificationDestination.js @@ -208,8 +208,8 @@ const saslSslBody = '' + ''; -// No Delete exists yet to remove what a run creates, so each run uses names -// of its own. +// Each run uses names of its own. Thus runs that share a server, or a run +// that stopped before its clean-up, never meet each other's names. const runId = crypto.randomBytes(4).toString('hex'); const uniqueName = prefix => `${prefix}-${runId}`; @@ -227,9 +227,44 @@ function destinationRequest(method, queryObj, requestBody, credentials = authCre ); } -const createDestination = (name, body, credentials) => destinationRequest('PUT', { name }, body, credentials); const getDestination = (name, credentials) => destinationRequest('GET', { name }, undefined, credentials); const updateDestination = (name, body, credentials) => destinationRequest('POST', { name }, body, credentials); +const deleteDestination = (name, credentials) => destinationRequest('DELETE', { name }, undefined, credentials); + +// Every destination that a Create made, with the credentials of its +// account. Thus each test deletes what it created when it ends. +let createdDestinations = []; + +async function createDestination(name, body, credentials) { + const result = await destinationRequest('PUT', { name }, body, credentials); + if (!result.err && result.res.statusCode === 200) { + createdDestinations.push({ name, credentials }); + } + return result; +} + +// Delete what the test created, with no throw, so that the other clean-up +// steps of the test still run. A test may have deleted some already, so +// NoSuchDestination is fine. Keep any other answer for the check of the +// root after hook below. +const cleanUpFailures = []; + +async function deleteCreatedDestinations() { + const created = createdDestinations; + createdDestinations = []; + for (const { name, credentials } of created) { + const { err, res } = await deleteDestination(name, credentials); + if (err && err.code !== 'NoSuchDestination') { + cleanUpFailures.push(`${name}: ${err.code} ${res?.statusCode}`); + } + } +} + +// This after hook is at the top level, so Mocha runs it on its root suite: +// once, after the whole run, not at the end of this file. +after(() => { + assert.deepStrictEqual(cleanUpFailures, [], 'some destinations could not be deleted'); +}); const escapeRegExp = text => text.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); @@ -273,6 +308,8 @@ describeOff('notification destination calls with the API off', () => { }); describeOn('CreateNotificationDestination', () => { + afterEach(deleteCreatedDestinations); + it('should create a destination and answer 200 with no body', async () => { const mtlsBody = saslSslBody.replace( /.*<\/Auth>/, @@ -368,6 +405,8 @@ describeOn('CreateNotificationDestination', () => { }); describeOn('GetNotificationDestination', () => { + afterEach(deleteCreatedDestinations); + // Strings that only a secret field holds, so that finding one in a body // means a secret leaked. const secrets = { @@ -461,15 +500,17 @@ describeOn('ListNotificationDestinations', () => { const listedNames = body => [...body.matchAll(/([^<]*)<\/Name>/g)].map(match => match[1]); // The other tests of this file create names of their own in the same - // account and never delete them. Thus the names here share one prefix - // that no other test uses, and each page starts after that prefix. Each - // test gets a prefix and destinations of its own. + // account, and another run may share the server. Thus the names here + // share one prefix that no other test uses, and each page starts after + // that prefix. Each test gets a prefix and destinations of its own. const lisa = getCredentials('lisa'); const lisaCredentials = { accessKey: lisa.accessKeyId, secretKey: lisa.secretAccessKey }; let prefix; let names; let lisaName; + afterEach(deleteCreatedDestinations); + beforeEach(async () => { prefix = testName('zz-list'); names = ['a', 'b', 'c'].map(suffix => `${prefix}-${suffix}`); @@ -560,6 +601,8 @@ describeOn('ListNotificationDestinations', () => { }); describeOn('UpdateNotificationDestination', () => { + afterEach(deleteCreatedDestinations); + const newBody = saslSslBody .replace( /.*<\/Brokers>/, @@ -621,6 +664,109 @@ describeOn('UpdateNotificationDestination', () => { }); }); +describeOn('DeleteNotificationDestination', () => { + afterEach(deleteCreatedDestinations); + + const listDestinations = queryObj => destinationRequest('GET', queryObj); + const listedNames = body => + [...body.matchAll(/([^<]*)<\/Name>/g)].map(match => match[1]); + + it('should delete the destination and answer 204 with no body, then Get answers 404', async () => { + const name = uniqueName('delete'); + assert.ifError((await createDestination(name, saslSslBody)).err); + const { err, res } = await deleteDestination(name); + assert.ifError(err); + assert.strictEqual(res.statusCode, 204); + assert.strictEqual(res.body, ''); + assertError(await getDestination(name), 'NoSuchDestination', 404); + }); + + it('should answer NoSuchDestination to an unknown name', async () => { + assertError(await deleteDestination(uniqueName('unknown')), 'NoSuchDestination', 404); + }); + + it('should answer NoSuchDestination to a second Delete of the name', async () => { + const name = uniqueName('delete-twice'); + assert.ifError((await createDestination(name, saslSslBody)).err); + assert.ifError((await deleteDestination(name)).err); + assertError(await deleteDestination(name), 'NoSuchDestination', 404); + }); + + it('should let the name be created again after a Delete', async () => { + const name = uniqueName('delete-again'); + assert.ifError((await createDestination(name, saslSslBody)).err); + assert.ifError((await deleteDestination(name)).err); + const body = saslSslBody.replace('bucket-events', 'again-events'); + assert.notStrictEqual(body, saslSslBody); + const { err, res } = await createDestination(name, body); + assert.ifError(err); + assert.strictEqual(res.statusCode, 200); + const got = await getDestination(name); + assert.ifError(got.err); + assert.deepStrictEqual(elementValues(got.res.body, 'Topic'), ['again-events']); + }); + + it('should no longer list the name after a Delete', async () => { + // A prefix of its own, so that the other names of the account never + // show in the page. + const prefix = uniqueName('zz-delete-list'); + const [kept, deleted] = [`${prefix}-a`, `${prefix}-b`]; + assert.ifError((await createDestination(kept, saslSslBody)).err); + assert.ifError((await createDestination(deleted, saslSslBody)).err); + const page = async () => { + const { err, res } = await listDestinations({ 'max-items': '1000', 'marker': `${prefix}-` }); + assert.ifError(err); + return listedNames(res.body).filter(name => name.startsWith(prefix)); + }; + assert.deepStrictEqual(await page(), [kept, deleted]); + assert.ifError((await deleteDestination(deleted)).err); + assert.deepStrictEqual(await page(), [kept]); + }); + + it('should answer InvalidDestination to a name outside the grammar', async () => { + assertError(await deleteDestination('bad/name'), 'InvalidDestination', 400); + }); + + it('should answer AccessDenied to an anonymous request, and keep the destination', async () => { + const name = uniqueName('delete-anonymous'); + assert.ifError((await createDestination(name, saslSslBody)).err); + assertError(await deleteDestination(name, null), 'AccessDenied', 403); + assert.ifError((await getDestination(name)).err); + }); +}); + +// Create and Update by a user with no policy have blocks of their own. +describeIam('Get, List and Delete by an IAM user with no policy', () => { + const iamClient = makeIamClient(); + const userName = uniqueName('destination-all-no-policy'); + let name; + let credentials; + + beforeEach(async () => { + name = testName('no-policy'); + assert.ifError((await createDestination(name, saslSslBody)).err); + credentials = await createIamCaller(iamClient, userName); + }); + + afterEach(async () => { + await deleteCreatedDestinations(); + await removeIamCaller(iamClient, userName); + }); + + [ + ['GetNotificationDestination', () => getDestination(name, credentials)], + ['ListNotificationDestinations', () => destinationRequest('GET', {}, undefined, credentials)], + ['DeleteNotificationDestination', () => deleteDestination(name, credentials)], + ].forEach(([operation, call]) => { + it(`should answer AccessDenied to ${operation}, and keep the destination`, async () => { + assertError(await call(), 'AccessDenied', 403); + const got = await getDestination(name); + assert.ifError(got.err); + assert.deepStrictEqual(elementValues(got.res.body, 'Topic'), ['bucket-events']); + }); + }); +}); + describeIam('CreateNotificationDestination by an IAM user', () => { const iamClient = makeIamClient(); const userName = uniqueName('destination-no-policy'); @@ -631,6 +777,7 @@ describeIam('CreateNotificationDestination by an IAM user', () => { }); afterEach(async () => { + await deleteCreatedDestinations(); await removeIamCaller(iamClient, userName); }); @@ -658,6 +805,7 @@ describeIam('GetNotificationDestination by an IAM user', () => { }); afterEach(async () => { + await deleteCreatedDestinations(); await removeIamCaller(iamClient, userName); }); @@ -701,6 +849,7 @@ describeIam('ListNotificationDestinations by an IAM user', () => { }); afterEach(async () => { + await deleteCreatedDestinations(); for (const userName of Object.values(userNames)) { await removeIamCaller(iamClient, userName); } @@ -739,6 +888,7 @@ describeIam('UpdateNotificationDestination by an IAM user', () => { }); afterEach(async () => { + await deleteCreatedDestinations(); await removeIamCaller(iamClient, userName); }); @@ -770,6 +920,7 @@ describeIam('UpdateNotificationDestination by an IAM user allowed on one destina }); afterEach(async () => { + await deleteCreatedDestinations(); await removeIamCaller(iamClient, userName); }); @@ -791,3 +942,39 @@ describeIam('UpdateNotificationDestination by an IAM user allowed on one destina assert.deepStrictEqual(elementValues(got.res.body, 'Topic'), ['bucket-events']); }); }); + +describeIam('DeleteNotificationDestination by an IAM user allowed on one destination', () => { + const iamClient = makeIamClient(); + const userName = uniqueName('destination-delete-one'); + let allowedName; + let otherName; + let credentials; + + beforeEach(async () => { + allowedName = testName('delete-allowed'); + otherName = testName('delete-other'); + const allowedArn = await createDestinationArn(allowedName); + assert.ifError((await createDestination(otherName, saslSslBody)).err); + credentials = await createIamCaller(iamClient, userName, { + action: 'scality:DeleteBucketNotificationDestination', + resource: allowedArn, + }); + }); + + afterEach(async () => { + await deleteCreatedDestinations(); + await removeIamCaller(iamClient, userName); + }); + + it('should delete the destination that the policy names', async () => { + const { err, res } = await deleteDestination(allowedName, credentials); + assert.ifError(err); + assert.strictEqual(res.statusCode, 204); + assertError(await getDestination(allowedName), 'NoSuchDestination', 404); + }); + + it('should answer AccessDenied to another destination, and keep it', async () => { + assertError(await deleteDestination(otherName, credentials), 'AccessDenied', 403); + assert.ifError((await getDestination(otherName)).err); + }); +}); diff --git a/tests/unit/api/api.js b/tests/unit/api/api.js index 3318a10db6..1562209c57 100644 --- a/tests/unit/api/api.js +++ b/tests/unit/api/api.js @@ -454,8 +454,8 @@ describe('api.callApiMethod', () => { let existingHandler; beforeEach(() => { - // No handler exists yet for these methods: add a stub for the - // test, and put back whatever was there afterwards. + // Stub the handler for the test. After the test, restore + // whatever was there before. existingHandler = api[method]; handler = sandbox.stub().callsFake((userInfo, _request, _log, cb) => cb()); api[method] = handler; @@ -521,8 +521,8 @@ describe('api.callApiMethod', () => { let stubbed; - // No handler exists yet for these methods. Add a stub for the test. - // After the test, restore whatever was there before. + // Stub each handler for the test. After the test, restore whatever + // was there before. beforeEach(() => { stubbed = {}; destinationMethods.forEach(method => { @@ -592,14 +592,6 @@ describe('api.callApiMethod', () => { assert.strictEqual(authServer.doAuth.calledOnce, true); assert.strictEqual(api[method].called, false); }); - - it(`should answer NotImplemented to ${method} while it has no handler`, async () => { - delete api[method]; - request.query = method === 'notificationDestinationList' ? {} : { name: 'my-destination' }; - const err = await callApi(method); - assert.strictEqual(err?.message, 'NotImplemented'); - assert.strictEqual(authServer.doAuth.called, false); - }); }); it('should let a bucket method through', async () => { diff --git a/tests/unit/api/notificationDestinationDelete.js b/tests/unit/api/notificationDestinationDelete.js new file mode 100644 index 0000000000..2206dd9792 --- /dev/null +++ b/tests/unit/api/notificationDestinationDelete.js @@ -0,0 +1,205 @@ +const assert = require('assert'); +const sinon = require('sinon'); +const { errors, models, storage } = require('@scality/arsenal'); +const AuthInfo = require('@scality/arsenal').auth.AuthInfo; + +const notificationDestinationDelete = require('../../../lib/api/notificationDestinationDelete'); +const notificationDestinationGet = require('../../../lib/api/notificationDestinationGet'); +const recordStore = require('../../../lib/api/apiUtils/notificationDestination/recordStore'); +const { config } = require('../../../lib/Config'); +const constants = require('../../../constants'); +const { cleanup, DummyRequestLogger, makeAuthInfo } = require('../helpers'); + +const { metadata } = storage.metadata.inMemory.metadata; + +const { accountResourcesBucket: recordsBucket, notificationDestinationKeyTag: tag, splitter } = constants; +// the account of accessKey1 in makeAuthInfo +const accountId = '123456789098'; +const keyOf = name => `${accountId}${splitter}${tag}${splitter}${name}`; + +// A stored record that each test deletes, with values that only it holds, +// so that finding one in a log means the record leaked. +const storedSaslSsl = { + modelVersion: 1, + Name: 'orders', + AccountId: accountId, + Arn: `arn:scality:bucketnotif::${accountId}:orders`, + Type: 'kafka', + Connection: { + Brokers: ['broker1.example.com:9093'], + Topic: 'bucket-events', + RequiredAcks: 1, + CompressionType: 'zstd', + }, + Auth: { + Type: 'sasl_ssl', + Sasl: { Mechanism: 'SCRAM-SHA-512', Username: 'sentinel-user-7a3f', Password: 'sentinel-pass-7a3f' }, + Tls: { CaCertificate: 'sentinel-ca-7a3f' }, + }, + CreatedAt: '2020-01-01T00:00:00Z', + UpdatedAt: '2020-01-01T00:00:00Z', +}; + +const accountAuthInfo = makeAuthInfo('accessKey1'); +// An IAM user whose short ID, the user ID, is not the account ID, as Vault +// gives it. makeAuthInfo gives a user the short ID of its account. +const userAuthInfo = new AuthInfo({ + canonicalID: accountAuthInfo.getCanonicalID(), + shortid: 'AIDAUSERID0000000000', + arn: `arn:aws:iam::${accountId}:user/alice`, +}); +const otherAuthInfo = makeAuthInfo('accessKey2'); + +function makeRequest(name, method = 'DELETE') { + return { + method, + url: `/?notification-destination&name=${encodeURIComponent(name)}`, + query: { 'notification-destination': '', name }, + headers: { host: 's3.amazonaws.com' }, + actionImplicitDenies: false, + }; +} + +// Resolve with the callback arguments, so that a failed assertion fails the +// test at once instead of in a metadata callback. +function deleteDestination(authInfo, request, log) { + return new Promise(resolve => notificationDestinationDelete(authInfo, request, log, (...args) => resolve(args))); +} + +function getDestination(authInfo, name, log) { + return new Promise(resolve => + notificationDestinationGet(authInfo, makeRequest(name, 'GET'), log, (...args) => resolve(args)), + ); +} + +function seed(record, log) { + return new Promise((resolve, reject) => + recordStore.putDestination(accountId, record.Name, structuredClone(record), log, err => + err ? reject(err) : resolve(), + ), + ); +} + +function storedRecord(name) { + const keys = metadata.keyMaps.get(recordsBucket); + return keys ? keys.get(keyOf(name)) : undefined; +} + +describe('notificationDestinationDelete', () => { + let log; + let savedBackend; + + beforeEach(async () => { + // The tests read the in-memory store, so the record store runs on + // mem, whatever S3BACKEND says. stop() drops its client after each + // test. + savedBackend = config.backends.metadata; + config.backends.metadata = 'mem'; + recordStore.stop(); + cleanup(); + log = new DummyRequestLogger(); + await seed(storedSaslSsl, log); + }); + + afterEach(() => { + recordStore.stop(); + config.backends.metadata = savedBackend; + sinon.restore(); + }); + + it('should delete the record and answer with no body, then Get answers NoSuchDestination', async () => { + const args = await deleteDestination(accountAuthInfo, makeRequest('orders'), log); + assert.strictEqual(args.length, 1, 'Delete answers callback(err)'); + assert.ifError(args[0]); + assert.strictEqual(storedRecord('orders'), undefined); + + const [getErr] = await getDestination(accountAuthInfo, 'orders', log); + assert.strictEqual(getErr?.message, 'NoSuchDestination'); + assert.strictEqual(getErr.code, 404); + }); + + it('should delete only the named record', async () => { + await seed({ ...storedSaslSsl, Name: 'payments', Arn: `arn:scality:bucketnotif::${accountId}:payments` }, log); + const [err] = await deleteDestination(accountAuthInfo, makeRequest('orders'), log); + assert.ifError(err); + assert.strictEqual(storedRecord('orders'), undefined); + assert.strictEqual(storedRecord('payments').Name, 'payments'); + }); + + it('should delete the destination of the account of an IAM user, never its user ID', async () => { + const [err] = await deleteDestination(userAuthInfo, makeRequest('orders'), log); + assert.ifError(err); + assert.strictEqual(storedRecord('orders'), undefined); + }); + + it('should answer NoSuchDestination to an unknown name, and delete nothing', async () => { + const del = sinon.spy(recordStore, 'deleteDestination'); + const [err] = await deleteDestination(accountAuthInfo, makeRequest('unknown'), log); + assert.strictEqual(err?.message, 'NoSuchDestination'); + assert.strictEqual(err.code, 404); + assert.strictEqual(del.called, false); + }); + + it('should answer NoSuchDestination to a name deleted already', async () => { + assert.ifError((await deleteDestination(accountAuthInfo, makeRequest('orders'), log))[0]); + const [err] = await deleteDestination(accountAuthInfo, makeRequest('orders'), log); + assert.strictEqual(err?.message, 'NoSuchDestination'); + }); + + it('should answer NoSuchDestination to the name of another account, and keep the record', async () => { + const del = sinon.spy(recordStore, 'deleteDestination'); + const [err] = await deleteDestination(otherAuthInfo, makeRequest('orders'), log); + assert.strictEqual(err?.message, 'NoSuchDestination'); + assert.strictEqual(del.called, false); + assert.deepStrictEqual(storedRecord('orders'), storedSaslSsl); + }); + + it('should answer NoSuchDestination when the record goes between the read and the delete', async () => { + // Another Delete of the name may land between the two calls. + sinon.stub(recordStore, 'deleteDestination').callsArgWith(3, errors.NoSuchDestination); + const [err] = await deleteDestination(accountAuthInfo, makeRequest('orders'), log); + assert.strictEqual(err?.message, 'NoSuchDestination'); + }); + + ['bad/name', 'a'.repeat(models.NotificationDestination.nameMaxLength + 1), '*', ''].forEach(name => { + it(`should answer InvalidDestination to the name '${name.slice(0, 20)}', and read nothing`, async () => { + const get = sinon.spy(recordStore, 'getDestination'); + const del = sinon.spy(recordStore, 'deleteDestination'); + const [err] = await deleteDestination(accountAuthInfo, makeRequest(name), log); + assert.strictEqual(err?.message, 'InvalidDestination'); + assert.strictEqual(err.code, 400); + assert.strictEqual(get.called, false); + assert.strictEqual(del.called, false); + }); + }); + + it('should pass a record store read error through, and delete nothing', async () => { + sinon.stub(recordStore, 'getDestination').callsArgWith(3, errors.InternalError); + const del = sinon.spy(recordStore, 'deleteDestination'); + const [err] = await deleteDestination(accountAuthInfo, makeRequest('orders'), log); + assert.strictEqual(err?.message, 'InternalError'); + assert.strictEqual(del.called, false); + assert.deepStrictEqual(storedRecord('orders'), storedSaslSsl); + }); + + it('should pass a record store delete error through', async () => { + sinon.stub(recordStore, 'deleteDestination').callsArgWith(3, errors.InternalError); + const [err] = await deleteDestination(accountAuthInfo, makeRequest('orders'), log); + assert.strictEqual(err?.message, 'InternalError'); + }); + + it('should never log the record', async () => { + const spyLog = new DummyRequestLogger(); + const levels = ['trace', 'debug', 'info', 'warn', 'error']; + levels.forEach(level => sinon.spy(spyLog, level)); + await deleteDestination(otherAuthInfo, makeRequest('orders'), spyLog); + await deleteDestination(accountAuthInfo, makeRequest('orders'), spyLog); + await deleteDestination(accountAuthInfo, makeRequest('orders'), spyLog); + const logged = JSON.stringify(levels.flatMap(level => spyLog[level].args)); + ['sentinel-user-7a3f', 'sentinel-pass-7a3f', 'sentinel-ca-7a3f', 'broker', 'events'].forEach(value => + assert.doesNotMatch(logged, new RegExp(value)), + ); + assert.match(logged, /orders/); + assert.match(logged, new RegExp(accountId)); + }); +}); From ebc0ad70bf6737949857022f2c0428988a405317 Mon Sep 17 00:00:00 2001 From: Anurag Mittal <1321012+anurag4DSB@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:14:07 +0100 Subject: [PATCH 2/2] CLDSRV-1068: add destination Delete notificationDestinationDelete: - checks the name - reads the record of the requester's account. NoSuchDestination stops the delete. - deletes the record - answers with no body, which the router sends as 204 - logs the name and the account only - reports its call to the Prometheus metrics, as the bucket APIs do Another request can remove the record between the read and the delete. On mem and bucketd, the backend reports the missing key, so the answer is NoSuchDestination. On file, the delete of a missing key succeeds, so the answer is 204. The end state is the same. All five destination handlers now exist. Thus callApiHandler no longer answers NotImplemented to a destination method with no handler. --- lib/api/api.js | 12 +++-- lib/api/notificationDestinationDelete.js | 61 ++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) create mode 100644 lib/api/notificationDestinationDelete.js diff --git a/lib/api/api.js b/lib/api/api.js index 18a4ac28e0..9a3e4889b9 100644 --- a/lib/api/api.js +++ b/lib/api/api.js @@ -52,6 +52,7 @@ const metadataSearch = require('./metadataSearch'); const { multiObjectDelete } = require('./multiObjectDelete'); const multipartDelete = require('./multipartDelete'); const notificationDestinationCreate = require('./notificationDestinationCreate'); +const notificationDestinationDelete = require('./notificationDestinationDelete'); const notificationDestinationGet = require('./notificationDestinationGet'); const notificationDestinationList = require('./notificationDestinationList'); const notificationDestinationUpdate = require('./notificationDestinationUpdate'); @@ -256,12 +257,12 @@ function callApiHandler(apiMethod, apiHandler, request, response, log, callback) // The router always sends destination requests here. Thus its 405 and 400 // answers do not depend on the flag. The destination requests get - // NotImplemented, before any Vault call, while the flag is off, or while - // a method has no handler yet. On a metadata backend that does not serve - // the API, the flag is always off. The answer comes before any auth, so - // it never names the metadata backend. + // NotImplemented, before any Vault call, while the flag is off. On a + // metadata backend that does not serve the API, the flag is always off. + // The answer comes before any auth, so it never names the metadata + // backend. if (constants.notificationDestinationApiMethods.has(apiMethod)) { - if (!config.notificationDestinationApi.enabled || typeof apiHandler !== 'function') { + if (!config.notificationDestinationApi.enabled) { log.debug('bucket notification destination method not available', { method: 'api.callApiMethod', apiMethod, @@ -646,6 +647,7 @@ const api = { multiObjectDelete, multipartDelete, notificationDestinationCreate, + notificationDestinationDelete, notificationDestinationGet, notificationDestinationList, notificationDestinationUpdate, diff --git a/lib/api/notificationDestinationDelete.js b/lib/api/notificationDestinationDelete.js new file mode 100644 index 0000000000..6e31ec67e1 --- /dev/null +++ b/lib/api/notificationDestinationDelete.js @@ -0,0 +1,61 @@ +const recordStore = require('./apiUtils/notificationDestination/recordStore'); +const { nameError, requesterAccountId } = require('./apiUtils/notificationDestination/request'); +const monitoring = require('../utilities/monitoringHandler'); + +const METRICS_ACTION = 'deleteNotificationDestination'; + +/** + * notificationDestinationDelete - delete a bucket notification destination + * of the account of the requester + * @param {AuthInfo} authInfo - Instance of AuthInfo class with requester's info + * @param {object} request - http request object + * @param {object} log - Werelogs logger + * @param {function} callback - callback(err) + * @return {undefined} + */ +function notificationDestinationDelete(authInfo, request, log, callback) { + log.debug('processing request', { method: 'notificationDestinationDelete' }); + const name = request.query.name; + + const invalidName = nameError(name); + if (invalidName) { + log.debug('invalid destination name', { method: 'notificationDestinationDelete' }); + monitoring.promMetrics('DELETE', request.bucketName, 400, METRICS_ACTION); + return process.nextTick(callback, invalidName); + } + const accountId = requesterAccountId(authInfo); + const logFields = { method: 'notificationDestinationDelete', destinationName: name, accountId }; + + return recordStore.getDestination(accountId, name, log, getErr => { + if (getErr) { + if (getErr.is?.NoSuchDestination) { + log.debug('destination not found', logFields); + } else { + log.error('could not read the destination record', { ...logFields, error: getErr }); + } + monitoring.promMetrics('DELETE', request.bucketName, getErr.code, METRICS_ACTION); + return callback(getErr); + } + // Read, then delete. Another request can remove the record between + // the two calls. On mem and bucketd, the backend reports the missing + // key, so the answer is NoSuchDestination. On file, the delete of a + // missing key succeeds, so the answer is 204. The end state is the + // same. + return recordStore.deleteDestination(accountId, name, log, deleteErr => { + if (deleteErr) { + if (deleteErr.is?.NoSuchDestination) { + log.debug('destination not found', logFields); + } else { + log.error('could not delete the destination record', { ...logFields, error: deleteErr }); + } + monitoring.promMetrics('DELETE', request.bucketName, deleteErr.code, METRICS_ACTION); + return callback(deleteErr); + } + log.debug('destination deleted', logFields); + monitoring.promMetrics('DELETE', request.bucketName, '204', METRICS_ACTION); + return callback(null); + }); + }); +} + +module.exports = notificationDestinationDelete;