Merge "Reduce duplicated code related to policies"

5 files changed, 337 insertions, 107 deletions

diff --git a/openstack/common/fileutils.py b/openstack/common/fileutils.py
index b988ad0..bb2ef61 100644
--- a/openstack/common/fileutils.py
+++ b/openstack/common/fileutils.py
@@ -19,6 +19,13 @@
 import errno
 import os
 
+from openstack.common.gettextutils import _
+from openstack.common import log as logging
+
+LOG = logging.getLogger(__name__)
+
+_FILE_CACHE = {}
+
 
 def ensure_tree(path):
     """Create a directory (and any ancestor directories required)
@@ -33,3 +40,28 @@ def ensure_tree(path):
                 raise
         else:
             raise
+
+
+def read_cached_file(filename, force_reload=False):
+    """Read from a file if it has been modified.
+
+    :param force_reload: Whether to reload the file.
+    :returns: A tuple with a boolean specifying if the data is fresh
+              or not.
+    """
+    global _FILE_CACHE
+
+    if force_reload and filename in _FILE_CACHE:
+        del _FILE_CACHE[filename]
+
+    reloaded = False
+    mtime = os.path.getmtime(filename)
+    cache_info = _FILE_CACHE.setdefault(filename, {})
+
+    if not cache_info or mtime > cache_info.get('mtime', 0):
+        LOG.debug(_("Reloading cached file %s") % filename)
+        with open(filename) as fap:
+            cache_info['data'] = fap.read()
+        cache_info['mtime'] = mtime
+        reloaded = True
+    return (reloaded, cache_info['data'])
diff --git a/openstack/common/policy.py b/openstack/common/policy.py
index cd6dcfc..3669f9b 100644
--- a/openstack/common/policy.py
+++ b/openstack/common/policy.py
@@ -60,21 +60,39 @@ import abc
 import re
 import urllib
 
+from oslo.config import cfg
 import six
 import urllib2
 
 from openstack.common.gettextutils import _
+from openstack.common import fileutils
 from openstack.common import jsonutils
 from openstack.common import log as logging
 
+policy_opts = [
+    cfg.StrOpt('policy_file',
+               default='policy.json',
+               help=_('JSON file containing policy')),
+    cfg.StrOpt('policy_default_rule',
+               default='default',
+               help=_('Rule enforced when requested rule is not found')),
+]
 
-LOG = logging.getLogger(__name__)
+CONF = cfg.CONF
+CONF.register_opts(policy_opts)
 
+LOG = logging.getLogger(__name__)
 
-_rules = None
 _checks = {}
 
 
+class PolicyNotAuthorized(Exception):
+
+    def __init__(self, rule):
+        msg = _("Policy doesn't allow %s to be performed.") % rule
+        super(PolicyNotAuthorized, self).__init__(msg)
+
+
 class Rules(dict):
     """
     A store for rules.  Handles the default_rule setting directly.
@@ -124,65 +142,146 @@ class Rules(dict):
         return jsonutils.dumps(out_rules, indent=4)
 
 
-# Really have to figure out a way to deprecate this
-def set_rules(rules):
-    """Set the rules in use for policy checks."""
+class Enforcer(object):
+    """
+    Responsible for loading and enforcing rules
 
-    global _rules
+    :param policy_file: Custom policy file to use, if none is
+                        specified, `CONF.policy_file` will be
+                        used.
+    :param rules: Default dictionary / Rules to use. It will be
+                  considered just in the first instantiation. If
+                  `load_rules(True)`, `clear()` or `set_rules(True)`
+                  is called this will be overwritten.
+    :param default_rule: Default rule to use, CONF.default_rule will
+                         be used if none is specified.
+    """
 
-    _rules = rules
+    def __init__(self, policy_file=None, rules=None, default_rule=None):
+        self.rules = Rules(rules)
+        self.default_rule = default_rule or CONF.policy_default_rule
 
+        self.policy_path = None
+        self.policy_file = policy_file or CONF.policy_file
 
-# Ditto
-def reset():
-    """Clear the rules used for policy checks."""
+    def set_rules(self, rules, overwrite=True):
+        """
+        Create a new Rules object based on the provided dict of rules
 
-    global _rules
+        :param rules: New rules to use. It should be an instance of dict.
+        :param overwrite: Whether to overwrite current rules or update them
+                          with the new rules.
+        """
 
-    _rules = None
+        if not isinstance(rules, dict):
+            raise TypeError(_("Rules must be an instance of dict or Rules, "
+                            "got %s instead") % type(rules))
 
+        if overwrite:
+            self.rules = Rules(rules)
+        else:
+            self.update(rules)
 
-def check(rule, target, creds, exc=None, *args, **kwargs):
-    """
-    Checks authorization of a rule against the target and credentials.
+    def clear(self):
+        """
+        Clears Enforcer rules,  policy's cache
+        and policy's path.
+        """
+        self.set_rules({})
+        self.policy_path = None
 
-    :param rule: The rule to evaluate.
-    :param target: As much information about the object being operated
-                   on as possible, as a dictionary.
-    :param creds: As much information about the user performing the
-                  action as possible, as a dictionary.
-    :param exc: Class of the exception to raise if the check fails.
-                Any remaining arguments passed to check() (both
-                positional and keyword arguments) will be passed to
-                the exception class.  If exc is not provided, returns
-                False.
+    def load_rules(self, force_reload=False):
+        """
+        Loads policy_path's rules. Policy file is cached
+        and will be reloaded if modified.
 
-    :return: Returns False if the policy does not allow the action and
-             exc is not provided; otherwise, returns a value that
-             evaluates to True.  Note: for rules using the "case"
-             expression, this True value will be the specified string
-             from the expression.
-    """
+        :param force_reload: Whether to overwrite current rules.
+        """
 
-    # Allow the rule to be a Check tree
-    if isinstance(rule, BaseCheck):
-        result = rule(target, creds)
-    elif not _rules:
-        # No rules to reference means we're going to fail closed
-        result = False
-    else:
-        try:
-            # Evaluate the rule
-            result = _rules[rule](target, creds)
-        except KeyError:
-            # If the rule doesn't exist, fail closed
+        if not self.policy_path:
+            self.policy_path = self._get_policy_path()
+
+        reloaded, data = fileutils.read_cached_file(self.policy_path,
+                                                    force_reload=force_reload)
+
+        if reloaded:
+            rules = Rules.load_json(data, self.default_rule)
+            self.set_rules(rules)
+            LOG.debug(_("Rules successfully reloaded"))
+
+    def _get_policy_path(self):
+        """
+        Locate the policy json data file
+
+        :param policy_file: Custom policy file to locate.
+
+        :returns: The policy path
+
+        :raises: ConfigFilesNotFoundError if the file couldn't
+                 be located.
+        """
+        policy_file = CONF.find_file(self.policy_file)
+
+        if policy_file:
+            return policy_file
+
+        raise cfg.ConfigFilesNotFoundError(path=CONF.policy_file)
+
+    def enforce(self, rule, target, creds, do_raise=False,
+                exc=None, *args, **kwargs):
+        """
+        Checks authorization of a rule against the target and credentials.
+
+        :param rule: A string or BaseCheck instance specifying the rule
+                    to evaluate.
+        :param target: As much information about the object being operated
+                    on as possible, as a dictionary.
+        :param creds: As much information about the user performing the
+                    action as possible, as a dictionary.
+        :param do_raise: Whether to raise an exception or not if check
+                        fails.
+        :param exc: Class of the exception to raise if the check fails.
+                    Any remaining arguments passed to check() (both
+                    positional and keyword arguments) will be passed to
+                    the exception class. If not specified, PolicyNotAuthorized
+                    will be used.
+
+        :return: Returns False if the policy does not allow the action and
+                exc is not provided; otherwise, returns a value that
+                evaluates to True.  Note: for rules using the "case"
+                expression, this True value will be the specified string
+                from the expression.
+        """
+
+        # NOTE(flaper87): Not logging target or creds to avoid
+        # potential security issues.
+        LOG.debug(_("Rule %s will be now enforced") % rule)
+
+        self.load_rules()
+
+        # Allow the rule to be a Check tree
+        if isinstance(rule, BaseCheck):
+            result = rule(target, creds, self)
+        elif not self.rules:
+            # No rules to reference means we're going to fail closed
             result = False
+        else:
+            try:
+                # Evaluate the rule
+                result = self.rules[rule](target, creds, self)
+            except KeyError:
+                LOG.debug(_("Rule [%s] doesn't exist") % rule)
+                # If the rule doesn't exist, fail closed
+                result = False
+
+        # If it is False, raise the exception if requested
+        if do_raise and not result:
+            if exc:
+                raise exc(*args, **kwargs)
 
-    # If it is False, raise the exception if requested
-    if exc and result is False:
-        raise exc(*args, **kwargs)
+            raise PolicyNotAuthorized(rule)
 
-    return result
+        return result
 
 
 class BaseCheck(object):
@@ -392,7 +491,7 @@ def _parse_check(rule):
     try:
         kind, match = rule.split(':', 1)
     except Exception:
-        LOG.exception(_("Failed to understand rule %(rule)s") % locals())
+        LOG.exception(_("Failed to understand rule %s") % rule)
         # If the rule is invalid, we'll fail closed
         return FalseCheck()
 
@@ -723,13 +822,13 @@ def register(name, func=None):
 
 @register("rule")
 class RuleCheck(Check):
-    def __call__(self, target, creds):
+    def __call__(self, target, creds, enforcer):
         """
         Recursively checks credentials based on the defined rules.
         """
 
         try:
-            return _rules[self.match](target, creds)
+            return enforcer.rules[self.match](target, creds, enforcer)
         except KeyError:
             # We don't have any matching rule; fail closed
             return False
@@ -737,7 +836,7 @@ class RuleCheck(Check):
 
 @register("role")
 class RoleCheck(Check):
-    def __call__(self, target, creds):
+    def __call__(self, target, creds, enforcer):
         """Check that there is a matching role in the cred dict."""
 
         return self.match.lower() in [x.lower() for x in creds['roles']]
@@ -745,7 +844,7 @@ class RoleCheck(Check):
 
 @register('http')
 class HttpCheck(Check):
-    def __call__(self, target, creds):
+    def __call__(self, target, creds, enforcer):
         """
         Check http: rules by calling to a remote server.
 
@@ -763,7 +862,7 @@ class HttpCheck(Check):
 
 @register(None)
 class GenericCheck(Check):
-    def __call__(self, target, creds):
+    def __call__(self, target, creds, enforcer):
         """
         Check an individual match.
 
diff --git a/tests/unit/test_fileutils.py b/tests/unit/test_fileutils.py
index c0bf0ac..7cd067c 100644
--- a/tests/unit/test_fileutils.py
+++ b/tests/unit/test_fileutils.py
@@ -15,10 +15,13 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import __builtin__
 import os
 import shutil
 import tempfile
 
+import mox
+
 from openstack.common import fileutils
 from tests import utils
 
@@ -34,3 +37,48 @@ class EnsureTree(utils.BaseTestCase):
         finally:
             if os.path.exists(tmpdir):
                 shutil.rmtree(tmpdir)
+
+
+class TestCachedFile(utils.BaseTestCase):
+
+    def setUp(self):
+        super(TestCachedFile, self).setUp()
+        self.mox = mox.Mox()
+        self.addCleanup(self.mox.UnsetStubs)
+
+    def test_read_cached_file(self):
+        self.mox.StubOutWithMock(os.path, "getmtime")
+        os.path.getmtime(mox.IgnoreArg()).AndReturn(1)
+        self.mox.ReplayAll()
+
+        fileutils._FILE_CACHE = {
+            '/this/is/a/fake': {"data": 1123, "mtime": 1}
+        }
+        fresh, data = fileutils.read_cached_file("/this/is/a/fake")
+        fdata = fileutils._FILE_CACHE['/this/is/a/fake']["data"]
+        self.assertEqual(fdata, data)
+
+    def test_read_modified_cached_file(self):
+        self.mox.StubOutWithMock(os.path, "getmtime")
+        self.mox.StubOutWithMock(__builtin__, 'open')
+        os.path.getmtime(mox.IgnoreArg()).AndReturn(2)
+
+        fake_contents = "lorem ipsum"
+        fake_file = self.mox.CreateMockAnything()
+        fake_file.read().AndReturn(fake_contents)
+        fake_context_manager = self.mox.CreateMockAnything()
+        fake_context_manager.__enter__().AndReturn(fake_file)
+        fake_context_manager.__exit__(mox.IgnoreArg(),
+                                      mox.IgnoreArg(),
+                                      mox.IgnoreArg())
+
+        __builtin__.open(mox.IgnoreArg()).AndReturn(fake_context_manager)
+
+        self.mox.ReplayAll()
+        fileutils._FILE_CACHE = {
+            '/this/is/a/fake': {"data": 1123, "mtime": 1}
+        }
+
+        fresh, data = fileutils.read_cached_file("/this/is/a/fake")
+        self.assertEqual(data, fake_contents)
+        self.assertTrue(fresh)
diff --git a/tests/unit/test_policy.py b/tests/unit/test_policy.py
index 7e56796..582021b 100644
--- a/tests/unit/test_policy.py
+++ b/tests/unit/test_policy.py
@@ -17,16 +17,25 @@
 
 """Test of Policy Engine"""
 
+import os
 import StringIO
 import urllib
 
 import mock
 import urllib2
+from oslo.config import cfg
 
 from openstack.common import jsonutils
 from openstack.common import policy
 from tests import utils
 
+CONF = cfg.CONF
+
+TEST_VAR_DIR = os.path.abspath(os.path.join(os.path.dirname(__file__),
+                                            '..', 'var'))
+
+ENFORCER = policy.Enforcer()
+
 
 class TestException(Exception):
     def __init__(self, *args, **kwargs):
@@ -35,6 +44,7 @@ class TestException(Exception):
 
 
 class RulesTestCase(utils.BaseTestCase):
+
     def test_init_basic(self):
         rules = policy.Rules()
 
@@ -104,24 +114,50 @@ class RulesTestCase(utils.BaseTestCase):
         self.assertEqual(str(rules), exemplar)
 
 
-class PolicySetAndResetTestCase(utils.BaseTestCase):
+class PolicyBaseTestCase(utils.BaseTestCase):
 
     def setUp(self):
-        super(PolicySetAndResetTestCase, self).setUp()
+        super(PolicyBaseTestCase, self).setUp()
+        CONF(args=['--config-dir', TEST_VAR_DIR])
+        self.enforcer = ENFORCER
+
+    def tearDown(self):
+        super(PolicyBaseTestCase, self).tearDown()
         # Make sure the policy rules are reset for remaining tests
-        self.addCleanup(setattr, policy, '_rules', None)
+        self.enforcer.clear()
+
+
+class EnforcerTest(PolicyBaseTestCase):
+
+    def test_load_file(self):
+        self.enforcer.load_rules(True)
+        self.assertIsNotNone(self.enforcer.rules)
+        self.assertIn('default', self.enforcer.rules)
+        self.assertIn('admin', self.enforcer.rules)
+
+    def test_reload(self):
+        self.enforcer.set_rules({'test': 'test'})
+        self.enforcer.load_rules(force_reload=True)
+
+        self.assertNotEquals(self.enforcer.rules, {'test': 'test'})
+        self.assertIn('default', self.enforcer.rules)
 
     def test_set_rules(self):
         # Make sure the rules are set properly
-        policy._rules = None
-        policy.set_rules('spam')
-        self.assertEqual(policy._rules, 'spam')
+        self.enforcer.rules = None
+        self.enforcer.set_rules({'test': 1})
+        self.assertEqual(self.enforcer.rules, {'test': 1})
+
+    def test_set_rules_type(self):
+        self.assertRaises(TypeError,
+                          self.enforcer.set_rules,
+                          'dummy')
 
-    def test_reset(self):
+    def test_clear(self):
         # Make sure the rules are reset
-        policy._rules = 'spam'
-        policy.reset()
-        self.assertEqual(policy._rules, None)
+        self.enforcer.rules = 'spam'
+        self.enforcer.clear()
+        self.assertEqual(self.enforcer.rules, {})
 
 
 class FakeCheck(policy.BaseCheck):
@@ -129,59 +165,58 @@ class FakeCheck(policy.BaseCheck):
         self.result = result
 
     def __str__(self):
-        return self.result
+        return str(self.result)
 
-    def __call__(self, target, creds):
+    def __call__(self, target, creds, enforcer):
         if self.result is not None:
             return self.result
-        return (target, creds)
-
+        return (target, creds, enforcer)
 
-class CheckFunctionTestCase(utils.BaseTestCase):
 
-    def setUp(self):
-        super(CheckFunctionTestCase, self).setUp()
-        # Make sure the policy rules are reset for remaining tests
-        self.addCleanup(setattr, policy, '_rules', None)
+class CheckFunctionTestCase(PolicyBaseTestCase):
 
     def test_check_explicit(self):
-        policy._rules = None
+        self.enforcer.load_rules()
+        self.enforcer.rules = None
         rule = FakeCheck()
-        result = policy.check(rule, "target", "creds")
+        result = self.enforcer.enforce(rule, "target", "creds")
 
-        self.assertEqual(result, ("target", "creds"))
-        self.assertEqual(policy._rules, None)
+        self.assertEqual(result, ("target", "creds", self.enforcer))
+        self.assertEqual(self.enforcer.rules, None)
 
     def test_check_no_rules(self):
-        policy._rules = None
-        result = policy.check('rule', "target", "creds")
+        self.enforcer.load_rules()
+        self.enforcer.rules = None
+        result = self.enforcer.enforce('rule', "target", "creds")
 
         self.assertEqual(result, False)
-        self.assertEqual(policy._rules, None)
+        self.assertEqual(self.enforcer.rules, None)
 
     def test_check_missing_rule(self):
-        policy._rules = {}
-        result = policy.check('rule', 'target', 'creds')
+        self.enforcer.rules = {}
+        result = self.enforcer.enforce('rule', 'target', 'creds')
 
         self.assertEqual(result, False)
 
     def test_check_with_rule(self):
-        policy._rules = dict(default=FakeCheck())
-        result = policy.check("default", "target", "creds")
+        self.enforcer.load_rules()
+        self.enforcer.rules = dict(default=FakeCheck())
+        result = self.enforcer.enforce("default", "target", "creds")
 
-        self.assertEqual(result, ("target", "creds"))
+        self.assertEqual(result, ("target", "creds", self.enforcer))
 
     def test_check_raises(self):
-        policy._rules = None
+        self.enforcer.rules = None
 
         try:
-            policy.check('rule', 'target', 'creds', TestException,
-                         "arg1", "arg2", kw1="kwarg1", kw2="kwarg2")
+            self.enforcer.enforce('rule', 'target', 'creds',
+                                  True, TestException, "arg1",
+                                  "arg2", kw1="kwarg1", kw2="kwarg2")
         except TestException as exc:
             self.assertEqual(exc.args, ("arg1", "arg2"))
             self.assertEqual(exc.kwargs, dict(kw1="kwarg1", kw2="kwarg2"))
         else:
-            self.fail("policy.check() failed to raise requested exception")
+            self.fail("enforcer.enforce() failed to raise requested exception")
 
 
 class FalseCheckTestCase(utils.BaseTestCase):
@@ -209,7 +244,7 @@ class TrueCheckTestCase(utils.BaseTestCase):
 
 
 class CheckForTest(policy.Check):
-    def __call__(self, target, creds):
+    def __call__(self, target, creds, enforcer):
         pass
 
 
@@ -693,42 +728,48 @@ class CheckRegisterTestCase(utils.BaseTestCase):
 
 
 class RuleCheckTestCase(utils.BaseTestCase):
-    @mock.patch.object(policy, '_rules', {})
+    @mock.patch.object(ENFORCER, 'rules', {})
     def test_rule_missing(self):
         check = policy.RuleCheck('rule', 'spam')
 
-        self.assertEqual(check('target', 'creds'), False)
+        self.assertEqual(check('target', 'creds', ENFORCER), False)
 
-    @mock.patch.object(policy, '_rules',
+    @mock.patch.object(ENFORCER, 'rules',
                        dict(spam=mock.Mock(return_value=False)))
     def test_rule_false(self):
+        enforcer = ENFORCER
+
         check = policy.RuleCheck('rule', 'spam')
 
-        self.assertEqual(check('target', 'creds'), False)
-        policy._rules['spam'].assert_called_once_with('target', 'creds')
+        self.assertEqual(check('target', 'creds', enforcer), False)
+        enforcer.rules['spam'].assert_called_once_with('target', 'creds',
+                                                       enforcer)
 
-    @mock.patch.object(policy, '_rules',
+    @mock.patch.object(ENFORCER, 'rules',
                        dict(spam=mock.Mock(return_value=True)))
     def test_rule_true(self):
+        enforcer = ENFORCER
         check = policy.RuleCheck('rule', 'spam')
 
-        self.assertEqual(check('target', 'creds'), True)
-        policy._rules['spam'].assert_called_once_with('target', 'creds')
+        self.assertEqual(check('target', 'creds', enforcer), True)
+        enforcer.rules['spam'].assert_called_once_with('target', 'creds',
+                                                       enforcer)
 
 
-class RoleCheckTestCase(utils.BaseTestCase):
+class RoleCheckTestCase(PolicyBaseTestCase):
     def test_accept(self):
         check = policy.RoleCheck('role', 'sPaM')
 
-        self.assertEqual(check('target', dict(roles=['SpAm'])), True)
+        self.assertEqual(check('target', dict(roles=['SpAm']),
+                               self.enforcer), True)
 
     def test_reject(self):
         check = policy.RoleCheck('role', 'spam')
 
-        self.assertEqual(check('target', dict(roles=[])), False)
+        self.assertEqual(check('target', dict(roles=[]), self.enforcer), False)
 
 
-class HttpCheckTestCase(utils.BaseTestCase):
+class HttpCheckTestCase(PolicyBaseTestCase):
     def decode_post_data(self, post_data):
         result = {}
         for item in post_data.split('&'):
@@ -743,7 +784,8 @@ class HttpCheckTestCase(utils.BaseTestCase):
         check = policy.HttpCheck('http', '//example.com/%(name)s')
 
         self.assertEqual(check(dict(name='target', spam='spammer'),
-                               dict(user='user', roles=['a', 'b', 'c'])),
+                               dict(user='user', roles=['a', 'b', 'c']),
+                               self.enforcer),
                          True)
         self.assertEqual(mock_urlopen.call_count, 1)
 
@@ -761,7 +803,8 @@ class HttpCheckTestCase(utils.BaseTestCase):
         check = policy.HttpCheck('http', '//example.com/%(name)s')
 
         self.assertEqual(check(dict(name='target', spam='spammer'),
-                               dict(user='user', roles=['a', 'b', 'c'])),
+                               dict(user='user', roles=['a', 'b', 'c']),
+                               self.enforcer),
                          False)
         self.assertEqual(mock_urlopen.call_count, 1)
 
@@ -774,18 +817,22 @@ class HttpCheckTestCase(utils.BaseTestCase):
         ))
 
 
-class GenericCheckTestCase(utils.BaseTestCase):
+class GenericCheckTestCase(PolicyBaseTestCase):
     def test_no_cred(self):
         check = policy.GenericCheck('name', '%(name)s')
 
-        self.assertEqual(check(dict(name='spam'), {}), False)
+        self.assertEqual(check(dict(name='spam'), {}, self.enforcer), False)
 
     def test_cred_mismatch(self):
         check = policy.GenericCheck('name', '%(name)s')
 
-        self.assertEqual(check(dict(name='spam'), dict(name='ham')), False)
+        self.assertEqual(check(dict(name='spam'),
+                               dict(name='ham'),
+                               self.enforcer), False)
 
     def test_accept(self):
         check = policy.GenericCheck('name', '%(name)s')
 
-        self.assertEqual(check(dict(name='spam'), dict(name='spam')), True)
+        self.assertEqual(check(dict(name='spam'),
+                               dict(name='spam'),
+                               self.enforcer), True)
diff --git a/tests/var/policy.json b/tests/var/policy.json
new file mode 100644
index 0000000..73730ae
--- /dev/null
+++ b/tests/var/policy.json
@@ -0,0 +1,4 @@
+{
+    "default": "rule:admin",
+    "admin": "is_admin:True"
+}