Remove admin context check, update unit tests

In manila/api/v1/security_service.py, the context.is_admin check is
removed, allowing the subsequent policy check to determine whether the
user can retrieve all security services. Authorization is determined by
the RBAC policy "security_services:get_all_security_services".

In manila/tests/api/v1/test_security_service.py, unit tests for listing
security services based on admin context were replaced with unit tests
for listing security services based on whether the user is authorized or
not.

The unit test test_security_services_list_all_tenants_policy_authorized
asserts that the security services are retrieved when
policy.check_policy returns True.

The unit test
test_security_services_list_all_tenants_policy_not_authorized asserts
that security services are not retrieved when policy.check_policy
raises a NotAuthorized exception.

Closes-Bug: #1916102

Change-Id: I6cce61237f5ee3ce60d8165f6fac5e7e5a63b4dd
Depends-On: https://review.opendev.org/c/openstack/manila-tempest-plugin/+/840727

manila/api/v1/security_service.py

@@ -108,11 +108,15 @@ class SecurityServiceController(wsgi.Controller):
             security_services = share_nw['security_services']
             del search_opts['share_network_id']
         else:
-            if context.is_admin and utils.is_all_tenants(search_opts):
-                policy.check_policy(context, RESOURCE_NAME,
-                                    'get_all_security_services')
+            # ignore all_tenants if not authorized to use it.
+            security_services = None
+            if utils.is_all_tenants(search_opts):
+                allowed_to_list_all_tenants = policy.check_policy(
+                    context, RESOURCE_NAME, 'get_all_security_services',
+                    do_raise=False)
+                if allowed_to_list_all_tenants:
                     security_services = db.security_service_get_all(context)
-            else:
+            if security_services is None:
                 security_services = db.security_service_get_all_by_project(
                     context, context.project_id)
         search_opts.pop('all_tenants', None)

manila/tests/api/v1/test_security_service.py

@@ -23,6 +23,7 @@ from manila.api.v1 import security_service
 from manila.common import constants
 from manila import db
 from manila import exception
+from manila import policy
 from manila import test
 from manila.tests.api import fakes
 
@@ -339,31 +340,38 @@ class ShareApiTest(test.TestCase):
             req.environ['manila.context'],
             sn['id'])
 
+    @mock.patch.object(db, 'security_service_get_all_by_project', mock.Mock())
     @mock.patch.object(db, 'security_service_get_all', mock.Mock())
-    def test_security_services_list_all_tenants_admin_context(self):
+    def test_security_services_list_all_tenants_policy_authorized(self):
         self.check_policy_patcher.stop()
         db.security_service_get_all.return_value = [
             self.ss_active_directory,
             self.ss_ldap,
         ]
-        req = fakes.HTTPRequest.blank(
-            '/security-services?all_tenants=1&name=fake-name',
-            use_admin_context=True)
-        res_dict = self.controller.index(req)
-        self.assertEqual(self.security_service_list_expected_resp, res_dict)
-        db.security_service_get_all.assert_called_once_with(
-            req.environ['manila.context'])
-
-    @mock.patch.object(db, 'security_service_get_all_by_project', mock.Mock())
-    def test_security_services_list_all_tenants_non_admin_context(self):
-        db.security_service_get_all_by_project.return_value = []
         req = fakes.HTTPRequest.blank(
             '/security-services?all_tenants=1')
+        self.mock_object(policy, "check_policy", mock.Mock(return_value=True))
         fake_context = req.environ['manila.context']
         self.controller.index(req)
-        db.security_service_get_all_by_project.assert_called_once_with(
-            fake_context, fake_context.project_id
-        )
+        db.security_service_get_all_by_project.assert_not_called()
+        db.security_service_get_all.assert_called_once_with(fake_context)
+
+    @mock.patch.object(db, 'security_service_get_all_by_project', mock.Mock())
+    @mock.patch.object(db, 'security_service_get_all', mock.Mock())
+    def test_security_services_list_all_tenants_policy_not_authorized(self):
+        self.check_policy_patcher.stop()
+        db.security_service_get_all.return_value = [
+            self.ss_active_directory,
+            self.ss_ldap,
+        ]
+        req = fakes.HTTPRequest.blank(
+            '/security-services?all_tenants=1')
+        self.mock_object(policy,
+                         "check_policy",
+                         mock.Mock(side_effect=exception.NotAuthorized()))
+        self.assertRaises(exception.NotAuthorized, self.controller.index, req)
+        db.security_service_get_all_by_project.assert_not_called()
+        db.security_service_get_all.assert_not_called()
 
     @mock.patch.object(db, 'security_service_get_all', mock.Mock())
     def test_security_services_list_all_tenants_with_invalid_value(self):

releasenotes/notes/bug-1916102-fix-security-service-policy-check-8e72254fa9fedc9e.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Decoupled the RBAC ``share:get_all_security_services`` from
+    ``context_is_admin``, potentially allowing the use of the
+    ``all_tenants`` query by non-administrators.