From 725c54e60ac8b6e6c236fdcdc0d76be373337ecd Mon Sep 17 00:00:00 2001 From: He Jie Xu Date: Mon, 2 Mar 2015 08:42:11 +0800 Subject: [PATCH] Remove db layer hard-code permission checks for quota_class_create/update This patch removes db layer hard-code permission checks for quota_class_create/update. For v2 API, this patch adds back-comptiable permission checks at REST API layer. For v2.1 API, this patch adds new rule for update method. Partially implements bp nova-api-policy-final-part SecurityImpact UpgradeImpact: Due to the db layer permission checks deleted, the policy rule "os_compute_api:os-quota-class-sets:update" was updated with a default that match the permission as before. Admin should be notified to update their policy configuration to keep permission as before. Change-Id: Icddc7e5cc1c11ab3d272f61a2ef623d3f750030c --- etc/nova/policy.json | 2 +- .../compute/contrib/quota_classes.py | 11 +++++-- .../compute/plugins/v3/quota_classes.py | 7 ++--- nova/db/sqlalchemy/api.py | 2 -- .../compute/contrib/test_quota_classes.py | 30 ++++++++++--------- nova/tests/unit/fake_policy.py | 2 +- 6 files changed, 29 insertions(+), 25 deletions(-) diff --git a/etc/nova/policy.json b/etc/nova/policy.json index bc9d76914b40..4b666ecbc08c 100644 --- a/etc/nova/policy.json +++ b/etc/nova/policy.json @@ -337,7 +337,7 @@ "os_compute_api:os-quota-sets:update": "rule:admin_api", "os_compute_api:os-quota-sets:delete": "rule:admin_api", "os_compute_api:os-quota-sets:detail": "rule:admin_api", - "os_compute_api:os-quota-class-sets": "", + "os_compute_api:os-quota-class-sets:update": "rule:admin_api", "os_compute_api:os-quota-class-sets:show": "is_admin:True or quota_class:%(quota_class)s", "os_compute_api:os-quota-class-sets:discoverable": "", "os_compute_api:os-rescue": "", diff --git a/nova/api/openstack/compute/contrib/quota_classes.py b/nova/api/openstack/compute/contrib/quota_classes.py index 225c29d68267..9229a353a27e 100644 --- a/nova/api/openstack/compute/contrib/quota_classes.py +++ b/nova/api/openstack/compute/contrib/quota_classes.py @@ -94,13 +94,20 @@ class QuotaClassSetsController(wsgi.Controller): msg = _("Bad key(s) %s in quota_set") % ",".join(bad_keys) raise webob.exc.HTTPBadRequest(explanation=msg) + try: + # NOTE(alex_xu): back-compatible with db layer hard-code admin + # permission checks. This has to be left only for API v2.0 because + # this version has to be stable even if it means that only admins + # can call this method while the policy could be changed. + nova.context.require_admin_context(context) + except exception.AdminRequired: + raise webob.exc.HTTPForbidden() + for key, value in quota_class_set.items(): try: db.quota_class_update(context, quota_class, key, value) except exception.QuotaClassNotFound: db.quota_class_create(context, quota_class, key, value) - except exception.AdminRequired: - raise webob.exc.HTTPForbidden() values = QUOTAS.get_class_quotas(context, quota_class) return self._format_quota_set(None, values) diff --git a/nova/api/openstack/compute/plugins/v3/quota_classes.py b/nova/api/openstack/compute/plugins/v3/quota_classes.py index 3c5b4d1e85e7..6c27694a55e3 100644 --- a/nova/api/openstack/compute/plugins/v3/quota_classes.py +++ b/nova/api/openstack/compute/plugins/v3/quota_classes.py @@ -14,7 +14,6 @@ # under the License. import six -import webob from nova.api.openstack.compute.schemas.v3 import quota_classes from nova.api.openstack import extensions @@ -68,11 +67,11 @@ class QuotaClassSetsController(wsgi.Controller): values = QUOTAS.get_class_quotas(context, id) return self._format_quota_set(id, values) - @extensions.expected_errors((403)) + @extensions.expected_errors(()) @validation.schema(quota_classes.update) def update(self, req, id, body): context = req.environ['nova.context'] - authorize(context) + authorize(context, action='update', target={'quota_class': id}) quota_class = id for key, value in six.iteritems(body['quota_class_set']): @@ -80,8 +79,6 @@ class QuotaClassSetsController(wsgi.Controller): db.quota_class_update(context, quota_class, key, value) except exception.QuotaClassNotFound: db.quota_class_create(context, quota_class, key, value) - except exception.AdminRequired: - raise webob.exc.HTTPForbidden() values = QUOTAS.get_class_quotas(context, quota_class) return self._format_quota_set(None, values) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 318188e8175b..76c65f3f0271 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -3166,7 +3166,6 @@ def quota_class_get_all_by_name(context, class_name): return result -@require_admin_context def quota_class_create(context, class_name, resource, limit): quota_class_ref = models.QuotaClass() quota_class_ref.class_name = class_name @@ -3176,7 +3175,6 @@ def quota_class_create(context, class_name, resource, limit): return quota_class_ref -@require_admin_context def quota_class_update(context, class_name, resource, limit): result = model_query(context, models.QuotaClass, read_deleted="no").\ filter_by(class_name=class_name).\ diff --git a/nova/tests/unit/api/openstack/compute/contrib/test_quota_classes.py b/nova/tests/unit/api/openstack/compute/contrib/test_quota_classes.py index f418effeb57a..553dbda75e4f 100644 --- a/nova/tests/unit/api/openstack/compute/contrib/test_quota_classes.py +++ b/nova/tests/unit/api/openstack/compute/contrib/test_quota_classes.py @@ -105,20 +105,6 @@ class QuotaClassSetsTestV21(test.TestCase): self.assertEqual(res_dict, body) - def test_quotas_update_as_user(self): - body = {'quota_class_set': {'instances': 50, 'cores': 50, - 'ram': 51200, 'floating_ips': 10, - 'fixed_ips': -1, 'metadata_items': 128, - 'injected_files': 5, - 'injected_file_content_bytes': 10240, - 'security_groups': 10, - 'security_group_rules': 20, - 'key_pairs': 100, - }} - - self.assertRaises(webob.exc.HTTPForbidden, self.controller.update, - self.req, 'test_class', body=body) - def test_quotas_update_with_empty_body(self): body = {} self.assertRaises(self.validation_error, self.controller.update, @@ -162,6 +148,11 @@ class QuotaClassSetsTestV2(QuotaClassSetsTestV21): self.assertRaises(webob.exc.HTTPForbidden, self.controller.show, self.req, 'test_class') + def test_quotas_update_as_user(self): + body = {'quota_class_set': {}} + self.assertRaises(webob.exc.HTTPForbidden, self.controller.update, + self.req, 'test_class', body=body) + class QuotaClassesPolicyEnforcementV21(test.NoDBTestCase): @@ -181,3 +172,14 @@ class QuotaClassesPolicyEnforcementV21(test.NoDBTestCase): self.assertEqual( "Policy doesn't allow %s to be performed." % rule_name, exc.format_message()) + + def test_update_policy_failed(self): + rule_name = "os_compute_api:os-quota-class-sets:update" + self.policy.set_rules({rule_name: "quota_class:non_fake"}) + exc = self.assertRaises( + exception.PolicyNotAuthorized, + self.controller.update, self.req, fakes.FAKE_UUID, + body={'quota_class_set': {}}) + self.assertEqual( + "Policy doesn't allow %s to be performed." % rule_name, + exc.format_message()) diff --git a/nova/tests/unit/fake_policy.py b/nova/tests/unit/fake_policy.py index d4b39a787910..e83e323491bf 100644 --- a/nova/tests/unit/fake_policy.py +++ b/nova/tests/unit/fake_policy.py @@ -299,7 +299,7 @@ policy_data = """ "os_compute_api:os-quota-sets:detail": "", "os_compute_api:os-quota-sets:defaults": "", "compute_extension:quota_classes": "", - "os_compute_api:os-quota-class-sets": "", + "os_compute_api:os-quota-class-sets:update": "", "os_compute_api:os-quota-class-sets:show": "", "compute_extension:rescue": "", "os_compute_api:os-rescue": "",