From b6dd538569ebf0f1580c8e1fadc5e0f8054c9b08 Mon Sep 17 00:00:00 2001
From: Thomas Leaman <thomas.leaman@hp.com>
Date: Wed, 22 Jan 2014 16:02:56 +0000
Subject: [PATCH] Check first matching rule for protected properties

When using roles to define protected properties, the first matching rule
in the config file should be used to grant/deny access. This change
enforces that behaviour.

Fixes bug 1271426

Change-Id: I11ece25ae85ff868516bcd1839a4e430e9c51370
---
 glance/common/property_utils.py               | 28 +++++++++-------
 .../etc/property-protections-policies.conf    | 12 +++++++
 glance/tests/etc/property-protections.conf    | 12 +++++++
 .../tests/unit/common/test_property_utils.py  | 32 +++++++++++++++++++
 4 files changed, 72 insertions(+), 12 deletions(-)

diff --git a/glance/common/property_utils.py b/glance/common/property_utils.py
index 6384edff56..803e63c4fa 100644
--- a/glance/common/property_utils.py
+++ b/glance/common/property_utils.py
@@ -173,16 +173,20 @@ class PropertyRules(object):
 
         for rule_exp, rule in self.rules:
             if rule_exp.search(str(property_name)):
-                rule_roles = rule.get(action)
-                if rule_roles:
-                    if '!' in rule_roles:
-                        return False
-                    elif '@' in rule_roles:
-                        return True
-                    if self.prop_prot_rule_format == 'policies':
-                        prop_exp_key = self.prop_exp_mapping[rule_exp]
-                        return self._check_policy(prop_exp_key, action,
-                                                  context)
-                    if set(roles).intersection(set(rule_roles)):
-                        return True
+                break
+        else:  # no matching rules
+            return False
+
+        rule_roles = rule.get(action)
+        if rule_roles:
+            if '!' in rule_roles:
+                return False
+            elif '@' in rule_roles:
+                return True
+            if self.prop_prot_rule_format == 'policies':
+                prop_exp_key = self.prop_exp_mapping[rule_exp]
+                return self._check_policy(prop_exp_key, action,
+                                          context)
+            if set(roles).intersection(set(rule_roles)):
+                return True
         return False
diff --git a/glance/tests/etc/property-protections-policies.conf b/glance/tests/etc/property-protections-policies.conf
index 7c1893989a..4be3656d79 100644
--- a/glance/tests/etc/property-protections-policies.conf
+++ b/glance/tests/etc/property-protections-policies.conf
@@ -40,6 +40,18 @@ read = context_is_admin
 update = context_is_admin
 delete = !
 
+[x_foo_matcher]
+create = context_is_admin
+read = context_is_admin
+update = context_is_admin
+delete = context_is_admin
+
+[x_foo_*]
+create = @
+read = @
+update = @
+delete = @
+
 [.*]
 create = context_is_admin
 read = context_is_admin
diff --git a/glance/tests/etc/property-protections.conf b/glance/tests/etc/property-protections.conf
index 5f0e27b80b..97ef1f642d 100644
--- a/glance/tests/etc/property-protections.conf
+++ b/glance/tests/etc/property-protections.conf
@@ -70,6 +70,18 @@ read = admin,member
 update = admin,member
 delete = !
 
+[x_foo_matcher]
+create = admin
+read = admin
+update = admin
+delete = admin
+
+[x_foo_*]
+create = @
+read = @
+update = @
+delete = @
+
 [.*]
 create = admin
 read = admin
diff --git a/glance/tests/unit/common/test_property_utils.py b/glance/tests/unit/common/test_property_utils.py
index 484debe93f..a8a69e12a7 100644
--- a/glance/tests/unit/common/test_property_utils.py
+++ b/glance/tests/unit/common/test_property_utils.py
@@ -32,6 +32,8 @@ CONFIG_SECTIONS = [
     'x_none_read',
     'x_none_update',
     'x_none_delete',
+    'x_foo_matcher',
+    'x_foo_*',
     '.*'
 ]
 
@@ -287,6 +289,21 @@ class TestPropertyRulesWithRoles(base.IsolatedUnitTest):
             'x_none_delete', 'delete',
             create_context(self.policy, [''])))
 
+    def test_check_return_first_match(self):
+        self.rules_checker = property_utils.PropertyRules()
+        self.assertFalse(self.rules_checker.check_property_rules(
+            'x_foo_matcher', 'create',
+            create_context(self.policy, [''])))
+        self.assertFalse(self.rules_checker.check_property_rules(
+            'x_foo_matcher', 'read',
+            create_context(self.policy, [''])))
+        self.assertFalse(self.rules_checker.check_property_rules(
+            'x_foo_matcher', 'update',
+            create_context(self.policy, [''])))
+        self.assertFalse(self.rules_checker.check_property_rules(
+            'x_foo_matcher', 'delete',
+            create_context(self.policy, [''])))
+
 
 class TestPropertyRulesWithPolicies(base.IsolatedUnitTest):
 
@@ -441,3 +458,18 @@ class TestPropertyRulesWithPolicies(base.IsolatedUnitTest):
         self.assertFalse(self.rules_checker.check_property_rules(
             'x_none_delete', 'delete',
             create_context(self.policy, [''])))
+
+    def test_check_return_first_match(self):
+        self.rules_checker = property_utils.PropertyRules()
+        self.assertFalse(self.rules_checker.check_property_rules(
+            'x_foo_matcher', 'create',
+            create_context(self.policy, [''])))
+        self.assertFalse(self.rules_checker.check_property_rules(
+            'x_foo_matcher', 'read',
+            create_context(self.policy, [''])))
+        self.assertFalse(self.rules_checker.check_property_rules(
+            'x_foo_matcher', 'update',
+            create_context(self.policy, [''])))
+        self.assertFalse(self.rules_checker.check_property_rules(
+            'x_foo_matcher', 'delete',
+            create_context(self.policy, [''])))