# HG changeset patch # User Sylvain Thénault # Date 1484932624 -3600 # Node ID 3a6746dfc57f654a3d06e8ed164d4764726dc06e # Parent a88101bf9f87e05d526511da4f7513de82e2c3cb Change hooks control (deny_all_hooks_but / allow_all_hooks_but) to be more predictable Prior to this, if one execute code like: with cnx.hooks.deny_all_hooks_but('metadata'): with cnx.hooks.deny_all_hooks_but(): # mycode 'metadata' hooks will be activated anyway in the inner block, which is rather unexpected (of course in real life you only see the latest hooks control statement, the former being higher in the call stack). This is due to the underlying usage of old `enable_hook_categories` / `disable_hook_categories` methods, which were introduced much before the now official context manager based API (with `cnx.[deny|all]_all_hooks_but(...)`). To move on, this patch drop the two legacy methods, rename and privatize related internal state on the connection (`hooks_mode` becomes `_hooks_mode`, `disabled_hook_cats` and `enabled_hook_cats` become `_hooks_categories`) and reimplement the `_hooks_control` context manager to simply update them. See the added unit test for details. diff -r a88101bf9f87 -r 3a6746dfc57f cubicweb/server/session.py --- a/cubicweb/server/session.py Tue Jan 24 14:09:13 2017 +0100 +++ b/cubicweb/server/session.py Fri Jan 20 18:17:04 2017 +0100 @@ -1,4 +1,4 @@ -# copyright 2003-2016 LOGILAB S.A. (Paris, FRANCE), all rights reserved. +# copyright 2003-2017 LOGILAB S.A. (Paris, FRANCE), all rights reserved. # contact http://www.logilab.fr/ -- mailto:contact@logilab.fr # # This file is part of CubicWeb. @@ -109,27 +109,19 @@ assert mode in (HOOKS_ALLOW_ALL, HOOKS_DENY_ALL) self.cnx = cnx self.mode = mode - self.categories = categories - self.oldmode = None - self.changes = () + self.categories = set(categories) + self.old_mode = None + self.old_categories = None def __enter__(self): - self.oldmode = self.cnx.hooks_mode - self.cnx.hooks_mode = self.mode - if self.mode is HOOKS_DENY_ALL: - self.changes = self.cnx.enable_hook_categories(*self.categories) - else: - self.changes = self.cnx.disable_hook_categories(*self.categories) + self.old_mode = self.cnx._hooks_mode + self.old_categories = self.cnx._hooks_categories + self.cnx._hooks_mode = self.mode + self.cnx._hooks_categories = self.categories def __exit__(self, exctype, exc, traceback): - try: - if self.categories: - if self.mode is HOOKS_DENY_ALL: - self.cnx.disable_hook_categories(*self.categories) - else: - self.cnx.enable_hook_categories(*self.categories) - finally: - self.cnx.hooks_mode = self.oldmode + self.cnx._hooks_mode = self.old_mode + self.cnx._hooks_categories = self.old_categories @deprecated('[3.17] use .security_enabled instead') @@ -238,13 +230,8 @@ Hooks controls: - :attr:`hooks_mode`, may be either `HOOKS_ALLOW_ALL` or `HOOKS_DENY_ALL`. - - :attr:`enabled_hook_cats`, when :attr:`hooks_mode` is - `HOOKS_DENY_ALL`, this set contains hooks categories that are enabled. - - :attr:`disabled_hook_cats`, when :attr:`hooks_mode` is - `HOOKS_ALLOW_ALL`, this set contains hooks categories that are disabled. + .. automethod:: cubicweb.server.session.Connection.deny_all_hooks_but + .. automethod:: cubicweb.server.session.Connection.allow_all_hooks_but Security level Management: @@ -283,9 +270,13 @@ self.commit_state = None # hook control attribute - self.hooks_mode = HOOKS_ALLOW_ALL - self.disabled_hook_cats = set() - self.enabled_hook_cats = set() + # `_hooks_mode`, may be either `HOOKS_ALLOW_ALL` or `HOOKS_DENY_ALL`. + self._hooks_mode = HOOKS_ALLOW_ALL + # `_hooks_categories`, when :attr:`_hooks_mode` is `HOOKS_DENY_ALL`, + # this set contains hooks categories that are enabled ; + # when :attr:`_hooks_mode` is `HOOKS_ALLOW_ALL`, it contains hooks + # categories that are disabled. + self._hooks_categories = set() self.pruned_hooks_cache = {} # security control attributes @@ -674,60 +665,26 @@ @_open_only def allow_all_hooks_but(self, *categories): + """Context manager to enable all hooks but those in the given + categories. + """ return _hooks_control(self, HOOKS_ALLOW_ALL, *categories) @_open_only def deny_all_hooks_but(self, *categories): - return _hooks_control(self, HOOKS_DENY_ALL, *categories) - - @_open_only - def disable_hook_categories(self, *categories): - """disable the given hook categories: - - - on HOOKS_DENY_ALL mode, ensure those categories are not enabled - - on HOOKS_ALLOW_ALL mode, ensure those categories are disabled + """Context manager to disable all hooks but those in the given + categories. """ - changes = set() - self.pruned_hooks_cache.clear() - categories = set(categories) - if self.hooks_mode is HOOKS_DENY_ALL: - enabledcats = self.enabled_hook_cats - changes = enabledcats & categories - enabledcats -= changes # changes is small hence faster - else: - disabledcats = self.disabled_hook_cats - changes = categories - disabledcats - disabledcats |= changes # changes is small hence faster - return tuple(changes) - - @_open_only - def enable_hook_categories(self, *categories): - """enable the given hook categories: - - - on HOOKS_DENY_ALL mode, ensure those categories are enabled - - on HOOKS_ALLOW_ALL mode, ensure those categories are not disabled - """ - changes = set() - self.pruned_hooks_cache.clear() - categories = set(categories) - if self.hooks_mode is HOOKS_DENY_ALL: - enabledcats = self.enabled_hook_cats - changes = categories - enabledcats - enabledcats |= changes # changes is small hence faster - else: - disabledcats = self.disabled_hook_cats - changes = disabledcats & categories - disabledcats -= changes # changes is small hence faster - return tuple(changes) + return _hooks_control(self, HOOKS_DENY_ALL, *categories) @_open_only def is_hook_category_activated(self, category): """return a boolean telling if the given category is currently activated or not """ - if self.hooks_mode is HOOKS_DENY_ALL: - return category in self.enabled_hook_cats - return category not in self.disabled_hook_cats + if self._hooks_mode is HOOKS_DENY_ALL: + return category in self._hooks_categories + return category not in self._hooks_categories @_open_only def is_hook_activated(self, hook): diff -r a88101bf9f87 -r 3a6746dfc57f cubicweb/server/test/unittest_session.py --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/cubicweb/server/test/unittest_session.py Fri Jan 20 18:17:04 2017 +0100 @@ -0,0 +1,54 @@ +# copyright 2017 LOGILAB S.A. (Paris, FRANCE), all rights reserved. +# contact http://www.logilab.fr/ -- mailto:contact@logilab.fr +# +# This file is part of CubicWeb. +# +# CubicWeb is free software: you can redistribute it and/or modify it under the +# terms of the GNU Lesser General Public License as published by the Free +# Software Foundation, either version 2.1 of the License, or (at your option) +# any later version. +# +# CubicWeb is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS +# FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more +# details. +# +# You should have received a copy of the GNU Lesser General Public License along +# with CubicWeb. If not, see . + +from cubicweb.devtools.testlib import CubicWebTC +from cubicweb.server import session + + +class HooksControlTC(CubicWebTC): + + def test_hooks_control(self): + with self.admin_access.repo_cnx() as cnx: + self.assertEqual(cnx._hooks_mode, session.HOOKS_ALLOW_ALL) + self.assertEqual(cnx._hooks_categories, set()) + + with cnx.deny_all_hooks_but('metadata'): + self.assertEqual(cnx._hooks_mode, session.HOOKS_DENY_ALL) + self.assertEqual(cnx._hooks_categories, set(['metadata'])) + + with cnx.deny_all_hooks_but(): + self.assertEqual(cnx._hooks_categories, set()) + self.assertEqual(cnx._hooks_categories, set(['metadata'])) + + with cnx.deny_all_hooks_but('integrity'): + self.assertEqual(cnx._hooks_categories, set(['integrity'])) + self.assertEqual(cnx._hooks_categories, set(['metadata'])) + + with cnx.allow_all_hooks_but('integrity'): + self.assertEqual(cnx._hooks_mode, session.HOOKS_ALLOW_ALL) + self.assertEqual(cnx._hooks_categories, set(['integrity'])) + self.assertEqual(cnx._hooks_mode, session.HOOKS_DENY_ALL) + self.assertEqual(cnx._hooks_categories, set(['metadata'])) + + self.assertEqual(cnx._hooks_mode, session.HOOKS_ALLOW_ALL) + self.assertEqual(cnx._hooks_categories, set()) + + +if __name__ == '__main__': + import unittest + unittest.main() diff -r a88101bf9f87 -r 3a6746dfc57f flake8-ok-files.txt --- a/flake8-ok-files.txt Tue Jan 24 14:09:13 2017 +0100 +++ b/flake8-ok-files.txt Fri Jan 20 18:17:04 2017 +0100 @@ -47,6 +47,7 @@ cubicweb/server/test/data-schema2sql/__init__.py cubicweb/server/test/unittest_checkintegrity.py cubicweb/server/test/unittest_ldapsource.py +cubicweb/server/test/unittest_session.py cubicweb/sobjects/test/unittest_notification.py cubicweb/sobjects/test/unittest_register_user.py cubicweb/sobjects/textparsers.py