This is an automated email from the ASF dual-hosted git repository. brondsem pushed a commit to branch db/8556-breaking-removal-2 in repository https://gitbox.apache.org/repos/asf/allura.git
commit bd805e630383df3e6340c60b1ccf2f662d0d0772 Author: Dave Brondsema <[email protected]> AuthorDate: Wed Jun 12 11:30:09 2024 -0400 [#8556] remove TruthyCallable, has_access() now returns a normal bool --- Allura/allura/controllers/auth.py | 6 +- Allura/allura/controllers/basetest_project_root.py | 2 +- Allura/allura/controllers/rest.py | 2 +- Allura/allura/lib/security.py | 134 ++++++++++----------- Allura/allura/lib/utils.py | 25 ---- Allura/allura/tests/test_plugin.py | 1 - Allura/allura/tests/test_utils.py | 21 ---- .../033-change-comment-anon-permissions.py | 2 +- 8 files changed, 73 insertions(+), 120 deletions(-) diff --git a/Allura/allura/controllers/auth.py b/Allura/allura/controllers/auth.py index 1699f6447..23565d98c 100644 --- a/Allura/allura/controllers/auth.py +++ b/Allura/allura/controllers/auth.py @@ -523,9 +523,9 @@ class AuthController(BaseController): log.info("Can't find repo at %s on repo_path %s", rest[0], repo_path) return disallow - return dict(allow_read=bool(has_access(c.app, 'read', user)), - allow_write=bool(has_access(c.app, 'write', user)), - allow_create=bool(has_access(c.app, 'create', user))) + return dict(allow_read=has_access(c.app, 'read', user), + allow_write=has_access(c.app, 'write', user), + allow_create=has_access(c.app, 'create', user)) @expose('jinja:allura:templates/pwd_expired.html') @without_trailing_slash diff --git a/Allura/allura/controllers/basetest_project_root.py b/Allura/allura/controllers/basetest_project_root.py index 90dc7e88c..a110a962f 100644 --- a/Allura/allura/controllers/basetest_project_root.py +++ b/Allura/allura/controllers/basetest_project_root.py @@ -181,7 +181,7 @@ class SecurityTest: @expose() def needs_project_access_ok(self): pred = has_access(c.project, 'read') - if not pred(): + if not pred: log.info('Inside needs_project_access, c.user = %s' % c.user) require(pred) return '' diff --git a/Allura/allura/controllers/rest.py b/Allura/allura/controllers/rest.py index b42605eca..f5d1400d6 100644 --- a/Allura/allura/controllers/rest.py +++ b/Allura/allura/controllers/rest.py @@ -555,7 +555,7 @@ def rest_has_access(obj, user, perm): resp = {'result': False} user = M.User.by_username(user) if user: - resp['result'] = bool(security.has_access(obj, perm, user=user)) + resp['result'] = security.has_access(obj, perm, user=user) return resp diff --git a/Allura/allura/lib/security.py b/Allura/allura/lib/security.py index 0fbacf6d6..1db9f19c2 100644 --- a/Allura/allura/lib/security.py +++ b/Allura/allura/lib/security.py @@ -33,8 +33,6 @@ from itertools import chain from ming.utils import LazyProperty import tg -from allura.lib.utils import TruthyCallable - if typing.TYPE_CHECKING: from allura.model import M @@ -306,7 +304,7 @@ def debug_obj(obj) -> str: return str(obj) -def has_access(obj, permission: str, user: M.User | None = None, project: M.Project | None = None) -> TruthyCallable: +def has_access(obj, permission: str, user: M.User | None = None, project: M.Project | None = None, roles = None) -> bool: '''Return whether the given user has the permission name on the given object. - First, all the roles for a user in the given project context are computed. @@ -349,69 +347,65 @@ def has_access(obj, permission: str, user: M.User | None = None, project: M.Proj DEBUG = False - def predicate(obj=obj, user=user, project=project, roles=None) -> bool: - if obj is None: - if DEBUG: - log.debug(f'{user} denied {permission} on {debug_obj(obj)} ({debug_obj(project)})') - return False - if roles is None: - if user is None: - user = c.user - assert user, 'c.user should always be at least M.User.anonymous()' - cred = Credentials.get() - if project is None: - if isinstance(obj, M.Neighborhood): - project = obj.neighborhood_project - if project is None: - log.error('Neighborhood project missing for %s', obj) - return False - elif isinstance(obj, M.Project): - project = obj.root_project - else: - project = getattr(obj, 'project', None) or c.project - project = project.root_project - roles: RoleCache = cred.user_roles(user_id=user._id, project_id=project._id).reaching_roles - - # TODO: move deny logic into loop below; see ticket [#6715] - if is_denied(obj, permission, user, project): - if DEBUG: - log.debug(f"{user.username} '{permission}' denied on {debug_obj(obj)} ({debug_obj(project)})") - return False - - chainable_roles = [] - for role in roles: - for ace in obj.acl: - if M.ACE.match(ace, role['_id'], permission): - if ace.access == M.ACE.ALLOW: - # access is allowed - if DEBUG: - log.debug( - f"{user.username} '{permission}' granted on {debug_obj(obj)} ({debug_obj(project)})") - return True - else: - # access is denied for this particular role - if DEBUG: - log.debug(f"{user.username} '{permission}' denied for role={role['name'] or role['_id']} (BUT continuing to see if other roles permit) on {debug_obj(obj)} ({debug_obj(project)})") - break + if obj is None: + if DEBUG: + log.debug(f'{user} denied {permission} on {debug_obj(obj)} ({debug_obj(project)})') + return False + if roles is None: + if user is None: + user = c.user + assert user, 'c.user should always be at least M.User.anonymous()' + cred = Credentials.get() + if project is None: + if isinstance(obj, M.Neighborhood): + project = obj.neighborhood_project + if project is None: + log.error('Neighborhood project missing for %s', obj) + return False + elif isinstance(obj, M.Project): + project = obj.root_project else: - # access neither allowed or denied, may chain to parent context - chainable_roles.append(role) - parent = obj.parent_security_context() - if parent and chainable_roles: - result = has_access(parent, permission, user=user, project=project)( - roles=tuple(chainable_roles)) - elif not isinstance(obj, M.Neighborhood): - result = has_access(project.neighborhood, 'admin', user=user) - if not (result or isinstance(obj, M.Project)): - result = has_access(project, 'admin', user=user) - else: - result = False - result = bool(result) + project = getattr(obj, 'project', None) or c.project + project = project.root_project + roles: RoleCache = cred.user_roles(user_id=user._id, project_id=project._id).reaching_roles + + # TODO: move deny logic into loop below; see ticket [#6715] + if is_denied(obj, permission, user, project): if DEBUG: - log.debug( - f"{user.username} '{permission}' {result} from parent(s) on {debug_obj(obj)} ({debug_obj(project)})") - return result - return TruthyCallable(predicate) + log.debug(f"{user.username} '{permission}' denied on {debug_obj(obj)} ({debug_obj(project)})") + return False + + chainable_roles = [] + for role in roles: + for ace in obj.acl: + if M.ACE.match(ace, role['_id'], permission): + if ace.access == M.ACE.ALLOW: + # access is allowed + if DEBUG: + log.debug( + f"{user.username} '{permission}' granted on {debug_obj(obj)} ({debug_obj(project)})") + return True + else: + # access is denied for this particular role + if DEBUG: + log.debug(f"{user.username} '{permission}' denied for role={role['name'] or role['_id']} (BUT continuing to see if other roles permit) on {debug_obj(obj)} ({debug_obj(project)})") + break + else: + # access neither allowed or denied, may chain to parent context + chainable_roles.append(role) + parent = obj.parent_security_context() + if parent and chainable_roles: + result = has_access(parent, permission, user=user, project=project, roles=tuple(chainable_roles)) + elif not isinstance(obj, M.Neighborhood): + result = has_access(project.neighborhood, 'admin', user=user) + if not (result or isinstance(obj, M.Project)): + result = has_access(project, 'admin', user=user) + else: + result = False + if DEBUG: + log.debug( + f"{user.username} '{permission}' {result} from parent(s) on {debug_obj(obj)} ({debug_obj(project)})") + return result def all_allowed(obj, user_or_role=None, project=None): @@ -495,14 +489,20 @@ def require(predicate, message=None): ''' Example: ``require(has_access(c.app, 'read'))`` - :param callable predicate: truth function to call + :param callable|bool predicate: truth function to call, or truth value :param str message: message to show upon failure :raises: HTTPForbidden or HTTPUnauthorized ''' from allura import model as M - if predicate(): - return + + if callable(predicate): + if predicate(): + return + else: + if predicate: + return + if not message: message = """You don't have permission to do that. You must ask a project administrator for rights to perform this task. diff --git a/Allura/allura/lib/utils.py b/Allura/allura/lib/utils.py index bba85e618..d025f4f67 100644 --- a/Allura/allura/lib/utils.py +++ b/Allura/allura/lib/utils.py @@ -396,31 +396,6 @@ class AntiSpam: return before_validate(antispam_hook) -class TruthyCallable: - ''' - Wraps a callable to make it truthy in a boolean context. - - Assumes the callable returns a truthy value and can be called with no args. - ''' - - def __init__(self, callable): - self.callable = callable - - def __call__(self, *args, **kw): - return self.callable(*args, **kw) - - def __bool__(self): - return self.callable() - - def __eq__(self, other): - if other is True and bool(self): - return True - elif other is False and not bool(self): - return True - else: - return NotImplemented - - class TransformedDict(MutableMapping): """ diff --git a/Allura/allura/tests/test_plugin.py b/Allura/allura/tests/test_plugin.py index c57e9e4a4..085e3a143 100644 --- a/Allura/allura/tests/test_plugin.py +++ b/Allura/allura/tests/test_plugin.py @@ -30,7 +30,6 @@ from allura import model as M from allura.lib import plugin from allura.lib import phone from allura.lib import helpers as h -from allura.lib.utils import TruthyCallable from allura.lib.plugin import ProjectRegistrationProvider from allura.lib.plugin import ThemeProvider from allura.lib.exceptions import ProjectConflict, ProjectShortnameInvalid diff --git a/Allura/allura/tests/test_utils.py b/Allura/allura/tests/test_utils.py index 131e9bf95..57b1b830b 100644 --- a/Allura/allura/tests/test_utils.py +++ b/Allura/allura/tests/test_utils.py @@ -169,27 +169,6 @@ class TestAntispam(unittest.TestCase): return encrypted_form -class TestTruthyCallable(unittest.TestCase): - - def test_everything(self): - def wrapper_func(bool_flag): - def predicate(bool_flag=bool_flag): - return bool_flag - return utils.TruthyCallable(predicate) - true_predicate = wrapper_func(True) - false_predicate = wrapper_func(False) - assert true_predicate(True) is True - assert false_predicate(False) is False - assert true_predicate() is True - assert false_predicate() is False - assert bool(true_predicate) is True - assert bool(false_predicate) is False - - t, f = True, False # use variables because '== True' would generate warnings, and we do want '==' not 'is' - assert true_predicate == t - assert false_predicate == f - - class TestCaseInsensitiveDict(unittest.TestCase): def test_everything(self): diff --git a/scripts/migrations/033-change-comment-anon-permissions.py b/scripts/migrations/033-change-comment-anon-permissions.py index 7285c479f..2cfb824d9 100644 --- a/scripts/migrations/033-change-comment-anon-permissions.py +++ b/scripts/migrations/033-change-comment-anon-permissions.py @@ -47,7 +47,7 @@ def main(): for chunk in utils.chunked_find(ForumPost, {'app_config_id': tool._id}): for p in chunk: - has_access = bool(security.has_access(p, 'moderate', M.User.anonymous())) + has_access = security.has_access(p, 'moderate', M.User.anonymous()) if has_access: anon_role_id = None
