CLDSRV-1068: DeleteNotificationDestination API

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,

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;

tests/functional/raw-node/test/routes/notificationDestination.js

@@ -208,8 +208,8 @@ const saslSslBody =
     '</Auth>' +
     '</NotificationDestination>';
 
-// 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>.*<\/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(/<NotificationDestination><Name>([^<]*)<\/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>.*<\/Brokers>/,
@@ -621,6 +664,109 @@ describeOn('UpdateNotificationDestination', () => {
     });
 });
 
+describeOn('DeleteNotificationDestination', () => {
+    afterEach(deleteCreatedDestinations);
+
+    const listDestinations = queryObj => destinationRequest('GET', queryObj);
+    const listedNames = body =>
+        [...body.matchAll(/<NotificationDestination><Name>([^<]*)<\/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);
+    });
+});