expanded acceptance tests around rbac in alerts

gmmorris · Jun 9, 2020 · ee79c9c · ee79c9c
1 parent 445710f
commit ee79c9c
Show file tree

Hide file tree

Showing 18 changed files with 1,282 additions and 79 deletions.
diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts
@@ -675,20 +675,24 @@ export class AlertsClient {
         producer: authorization.actions.alerting.get(alertTypeId, alertType.producer, operation),
       };
 
+      // We special case the Alerts Management `consumer` as we don't want to have to
+      // manually authorize each alert type in the management UI
+      const shouldAuthorizeConsumer = consumer !== AlertsFeatureId;
+
       const checkPrivileges = authorization.checkPrivilegesDynamicallyWithRequest(this.request);
       const { hasAllRequested, privileges } = await checkPrivileges(
-        consumer === AlertsFeatureId || consumer === alertType.producer
+        shouldAuthorizeConsumer && consumer !== alertType.producer
           ? [
-              // skip consumer privilege checks under `alerts` as all alert types can
-              // be created under `alerts` if you have producer level privileges
-              requiredPrivilegesByScope.producer,
-            ]
-          : [
               // check for access at consumer level
               requiredPrivilegesByScope.consumer,
               // check for access at producer level
               requiredPrivilegesByScope.producer,
             ]
+          : [
+              // skip consumer privilege checks under `alerts` as all alert types can
+              // be created under `alerts` if you have producer level privileges
+              requiredPrivilegesByScope.producer,
+            ]
       );
 
       if (!hasAllRequested) {
@@ -703,7 +707,9 @@ export class AlertsClient {
 
         throw Boom.forbidden(
           `Unauthorized to ${operation} a "${alertTypeId}" alert ${
-            unauthorizedScopes.consumer ? `for "${consumer}"` : `by "${alertType.producer}"`
+            shouldAuthorizeConsumer && unauthorizedScopes.consumer
+              ? `for "${consumer}"`
+              : `by "${alertType.producer}"`
           }`
         );
       }

diff --git a/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/plugin.ts b/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/plugin.ts
@@ -69,6 +69,7 @@ export class FixturePlugin implements Plugin<void, void, FixtureSetupDeps, Fixtu
               'test.onlyContextVariables',
               'test.onlyStateVariables',
               'test.noop',
+              'test.unrestricted-noop',
             ],
           },
           ui: [],

diff --git a/.../test/alerting_api_integration/common/fixtures/plugins/alerts_restricted/server/plugin.ts b/.../test/alerting_api_integration/common/fixtures/plugins/alerts_restricted/server/plugin.ts
@@ -46,7 +46,7 @@ export class FixturePlugin implements Plugin<void, void, FixtureSetupDeps, Fixtu
             read: ['alert'],
           },
           alerting: {
-            read: ['test.restricted-noop', 'test.unrestricted-noop'],
+            read: ['test.restricted-noop', 'test.unrestricted-noop', 'test.noop'],
           },
           ui: [],
         },

diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/scenarios.ts b/x-pack/test/alerting_api_integration/security_and_spaces/scenarios.ts
@@ -50,6 +50,7 @@ const GlobalRead: User = {
           alerts: ['read'],
           actions: ['read'],
           alertsFixture: ['read'],
+          alertsRestrictedFixture: ['read'],
         },
         spaces: ['*'],
       },

diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts
@@ -239,7 +239,11 @@ export default function createAlertTests({ getService }: FtrProviderContext) {
               expect(response.statusCode).to.eql(403);
               expect(response.body).to.eql({
                 error: 'Forbidden',
-                message: getConsumerUnauthorizedErrorMessage('create', 'test.noop', 'alerts'),
+                message: getProducerUnauthorizedErrorMessage(
+                  'create',
+                  'test.noop',
+                  'alertsFixture'
+                ),
                 statusCode: 403,
               });
               break;

diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/delete.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/delete.ts
@@ -222,7 +222,11 @@ export default function createDeleteTests({ getService }: FtrProviderContext) {
               expect(response.statusCode).to.eql(403);
               expect(response.body).to.eql({
                 error: 'Forbidden',
-                message: getConsumerUnauthorizedErrorMessage('delete', 'test.noop', 'alerts'),
+                message: getProducerUnauthorizedErrorMessage(
+                  'delete',
+                  'test.noop',
+                  'alertsFixture'
+                ),
                 statusCode: 403,
               });
               objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');

diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/disable.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/disable.ts
@@ -103,6 +103,7 @@ export default function createDisableAlertTests({ getService }: FtrProviderConte
               })
             )
             .expect(200);
+          objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
 
           const response = await alertUtils.getDisableRequest(createdAlert.id);
 
@@ -121,9 +122,6 @@ export default function createDisableAlertTests({ getService }: FtrProviderConte
                 ),
                 statusCode: 403,
               });
-              objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
-              // Ensure task still exists
-              await getScheduledTask(createdAlert.scheduledTaskId);
               break;
             case 'superuser at space1':
             case 'space_1_all_with_restricted_fixture at space1':
@@ -153,6 +151,7 @@ export default function createDisableAlertTests({ getService }: FtrProviderConte
               })
             )
             .expect(200);
+          objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
 
           const response = await alertUtils.getDisableRequest(createdAlert.id);
 
@@ -170,9 +169,6 @@ export default function createDisableAlertTests({ getService }: FtrProviderConte
                 ),
                 statusCode: 403,
               });
-              objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
-              // Ensure task still exists
-              await getScheduledTask(createdAlert.scheduledTaskId);
               break;
             case 'space_1_all at space1':
               expect(response.statusCode).to.eql(403);
@@ -185,9 +181,6 @@ export default function createDisableAlertTests({ getService }: FtrProviderConte
                 ),
                 statusCode: 403,
               });
-              objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
-              // Ensure task still exists
-              await getScheduledTask(createdAlert.scheduledTaskId);
               break;
             case 'superuser at space1':
             case 'space_1_all_with_restricted_fixture at space1':
@@ -217,6 +210,7 @@ export default function createDisableAlertTests({ getService }: FtrProviderConte
               })
             )
             .expect(200);
+          objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
 
           const response = await alertUtils.getDisableRequest(createdAlert.id);
 
@@ -227,12 +221,13 @@ export default function createDisableAlertTests({ getService }: FtrProviderConte
               expect(response.statusCode).to.eql(403);
               expect(response.body).to.eql({
                 error: 'Forbidden',
-                message: getConsumerUnauthorizedErrorMessage('disable', 'test.noop', 'alerts'),
+                message: getProducerUnauthorizedErrorMessage(
+                  'disable',
+                  'test.noop',
+                  'alertsFixture'
+                ),
                 statusCode: 403,
               });
-              objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
-              // Ensure task still exists
-              await getScheduledTask(createdAlert.scheduledTaskId);
               break;
             case 'superuser at space1':
             case 'space_1_all at space1':

diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/enable.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/enable.ts
@@ -108,6 +108,7 @@ export default function createEnableAlertTests({ getService }: FtrProviderContex
               })
             )
             .expect(200);
+          objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
 
           const response = await alertUtils.getEnableRequest(createdAlert.id);
 
@@ -126,9 +127,6 @@ export default function createEnableAlertTests({ getService }: FtrProviderContex
                 ),
                 statusCode: 403,
               });
-              objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
-              // Ensure task still exists
-              await getScheduledTask(createdAlert.scheduledTaskId);
               break;
             case 'superuser at space1':
             case 'space_1_all_with_restricted_fixture at space1':
@@ -158,6 +156,7 @@ export default function createEnableAlertTests({ getService }: FtrProviderContex
               })
             )
             .expect(200);
+          objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
 
           const response = await alertUtils.getEnableRequest(createdAlert.id);
 
@@ -175,9 +174,6 @@ export default function createEnableAlertTests({ getService }: FtrProviderContex
                 ),
                 statusCode: 403,
               });
-              objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
-              // Ensure task still exists
-              await getScheduledTask(createdAlert.scheduledTaskId);
               break;
             case 'space_1_all at space1':
               expect(response.statusCode).to.eql(403);
@@ -190,20 +186,11 @@ export default function createEnableAlertTests({ getService }: FtrProviderContex
                 ),
                 statusCode: 403,
               });
-              objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
-              // Ensure task still exists
-              await getScheduledTask(createdAlert.scheduledTaskId);
               break;
             case 'superuser at space1':
             case 'space_1_all_with_restricted_fixture at space1':
               expect(response.statusCode).to.eql(204);
               expect(response.body).to.eql('');
-              try {
-                await getScheduledTask(createdAlert.scheduledTaskId);
-                throw new Error('Should have removed scheduled task');
-              } catch (e) {
-                expect(e.status).to.eql(404);
-              }
               break;
             default:
               throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
@@ -222,6 +209,7 @@ export default function createEnableAlertTests({ getService }: FtrProviderContex
               })
             )
             .expect(200);
+          objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
 
           const response = await alertUtils.getEnableRequest(createdAlert.id);
 
@@ -232,12 +220,13 @@ export default function createEnableAlertTests({ getService }: FtrProviderContex
               expect(response.statusCode).to.eql(403);
               expect(response.body).to.eql({
                 error: 'Forbidden',
-                message: getConsumerUnauthorizedErrorMessage('enable', 'test.noop', 'alerts'),
+                message: getProducerUnauthorizedErrorMessage(
+                  'enable',
+                  'test.noop',
+                  'alertsFixture'
+                ),
                 statusCode: 403,
               });
-              objectRemover.add(space.id, createdAlert.id, 'alert', 'alerts');
-              // Ensure task still exists
-              await getScheduledTask(createdAlert.scheduledTaskId);
               break;
             case 'superuser at space1':
             case 'space_1_all at space1':

diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/find.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/find.ts
@@ -125,7 +125,6 @@ export default function createFindTests({ getService }: FtrProviderContext) {
               expect(response.body.total).to.equal(0);
               expect(response.body.data.length).to.equal(0);
               break;
-            case 'global_read at space1':
             case 'space_1_all at space1':
               expect(response.body.page).to.equal(1);
               expect(response.body.perPage).to.be.equal(3);
@@ -140,6 +139,7 @@ export default function createFindTests({ getService }: FtrProviderContext) {
                 expect(response.body.data.map((alert: any) => alert.id)).to.eql(firstPage);
               }
               break;
+            case 'global_read at space1':
             case 'superuser at space1':
             case 'space_1_all_with_restricted_fixture at space1':
               expect(response.body.page).to.equal(1);

diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/get.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/get.ts
@@ -103,7 +103,6 @@ export default function createGetTests({ getService }: FtrProviderContext) {
           switch (scenario.id) {
             case 'no_kibana_privileges at space1':
             case 'space_1_all at space2':
-            case 'global_read at space1':
             case 'space_1_all at space1':
               expect(response.statusCode).to.eql(403);
               expect(response.body).to.eql({
@@ -116,6 +115,7 @@ export default function createGetTests({ getService }: FtrProviderContext) {
                 statusCode: 403,
               });
               break;
+            case 'global_read at space1':
             case 'superuser at space1':
             case 'space_1_all_with_restricted_fixture at space1':
               expect(response.statusCode).to.eql(200);
@@ -145,7 +145,6 @@ export default function createGetTests({ getService }: FtrProviderContext) {
           switch (scenario.id) {
             case 'no_kibana_privileges at space1':
             case 'space_1_all at space2':
-            case 'global_read at space1':
               expect(response.statusCode).to.eql(403);
               expect(response.body).to.eql({
                 error: 'Forbidden',
@@ -170,6 +169,7 @@ export default function createGetTests({ getService }: FtrProviderContext) {
               });
               break;
             case 'superuser at space1':
+            case 'global_read at space1':
             case 'space_1_all_with_restricted_fixture at space1':
               expect(response.statusCode).to.eql(200);
               break;
@@ -199,18 +199,18 @@ export default function createGetTests({ getService }: FtrProviderContext) {
             case 'no_kibana_privileges at space1':
             case 'space_1_all at space2':
             case 'space_1_all at space1':
-            case 'global_read at space1':
               expect(response.statusCode).to.eql(403);
               expect(response.body).to.eql({
                 error: 'Forbidden',
-                message: getConsumerUnauthorizedErrorMessage(
+                message: getProducerUnauthorizedErrorMessage(
                   'get',
                   'test.restricted-noop',
-                  'alerts'
+                  'alertsRestrictedFixture'
                 ),
                 statusCode: 403,
               });
               break;
+            case 'global_read at space1':
             case 'superuser at space1':
             case 'space_1_all_with_restricted_fixture at space1':
               expect(response.statusCode).to.eql(200);

diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/get_alert_state.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/get_alert_state.ts
@@ -62,13 +62,13 @@ export default function createGetAlertStateTests({ getService }: FtrProviderCont
           }
         });
 
-        it('should handle getAlertState alert request appropriately', async () => {
+        it('should handle getAlertState alert request appropriately when unauthorized', async () => {
           const { body: createdAlert } = await supertest
             .post(`${getUrlPrefix(space.id)}/api/alerts/alert`)
             .set('kbn-xsrf', 'foo')
             .send(
               getTestAlertData({
-                alertTypeId: 'test.restricted-noop',
+                alertTypeId: 'test.unrestricted-noop',
                 consumer: 'alertsFixture',
               })
             )
@@ -85,7 +85,11 @@ export default function createGetAlertStateTests({ getService }: FtrProviderCont
               expect(response.statusCode).to.eql(403);
               expect(response.body).to.eql({
                 error: 'Forbidden',
-                message: getConsumerUnauthorizedErrorMessage('get', 'test.noop', 'alertsFixture'),
+                message: getConsumerUnauthorizedErrorMessage(
+                  'get',
+                  'test.unrestricted-noop',
+                  'alertsFixture'
+                ),
                 statusCode: 403,
               });
               break;
@@ -95,7 +99,7 @@ export default function createGetAlertStateTests({ getService }: FtrProviderCont
                 error: 'Forbidden',
                 message: getProducerUnauthorizedErrorMessage(
                   'get',
-                  'test.noop',
+                  'test.unrestricted-noop',
                   'alertsRestrictedFixture'
                 ),
                 statusCode: 403,
@@ -111,6 +115,7 @@ export default function createGetAlertStateTests({ getService }: FtrProviderCont
               throw new Error(`Scenario untested: ${JSON.stringify(scenario)}`);
           }
         });
+
         it(`shouldn't getAlertState for an alert from another space`, async () => {
           const { body: createdAlert } = await supertest
             .post(`${getUrlPrefix(space.id)}/api/alerts/alert`)