You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@allura.apache.org by jo...@apache.org on 2013/03/01 16:58:04 UTC
[6/24] git commit: [#5647] Fixed escalation of rights on private
tickets for developers and creator
[#5647] Fixed escalation of rights on private tickets for developers and creator
Signed-off-by: Cory Johns <jo...@geek.net>
Project: http://git-wip-us.apache.org/repos/asf/incubator-allura/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-allura/commit/36b8c925
Tree: http://git-wip-us.apache.org/repos/asf/incubator-allura/tree/36b8c925
Diff: http://git-wip-us.apache.org/repos/asf/incubator-allura/diff/36b8c925
Branch: refs/heads/cj/5411
Commit: 36b8c925ff51cf52531b9d07a58c8cbb80dd1119
Parents: 5db659a
Author: Cory Johns <jo...@geek.net>
Authored: Wed Feb 27 18:51:46 2013 +0000
Committer: Cory Johns <jo...@geek.net>
Committed: Wed Feb 27 18:52:17 2013 +0000
----------------------------------------------------------------------
Allura/allura/lib/security.py | 65 ++++++++++
Allura/allura/tests/test_security.py | 94 +++++++++++++++
ForgeTracker/forgetracker/model/ticket.py | 16 ++--
.../forgetracker/tests/unit/test_ticket_model.py | 18 ++-
4 files changed, 180 insertions(+), 13 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/36b8c925/Allura/allura/lib/security.py
----------------------------------------------------------------------
diff --git a/Allura/allura/lib/security.py b/Allura/allura/lib/security.py
index 8f73209..0ca803f 100644
--- a/Allura/allura/lib/security.py
+++ b/Allura/allura/lib/security.py
@@ -297,6 +297,71 @@ def has_access(obj, permission, user=None, project=None):
return result
return TruthyCallable(predicate)
+def all_allowed(obj, user_or_role=None, project=None):
+ '''
+ List all the permission names that a given user or named role
+ is allowed for a given object. This list reflects the permissions
+ for which has_access() would return True for the user (or a user
+ in the given named role, e.g. Developer).
+
+ Example:
+
+ Given a tracker with the following ACL (pseudo-code):
+ [
+ ACE.allow(ProjectRole.by_name('Developer'), 'create'),
+ ACE.allow(ProjectRole.by_name('Member'), 'post'),
+ ACE.allow(ProjectRole.by_name('*anonymous'), 'read'),
+ ]
+
+ And user1 is in the Member group, then all_allowed(tracker, user1)
+ will return:
+
+ set(['post', 'read'])
+
+ And all_allowed(tracker, ProjectRole.by_name('Developer')) will return:
+
+ set(['create', 'post', 'read'])
+ '''
+ from allura import model as M
+ anon = M.ProjectRole.anonymous(project)
+ auth = M.ProjectRole.authenticated(project)
+ if user_or_role is None:
+ user_or_role = c.user
+ if user_or_role is None:
+ user_or_role = anon
+ if isinstance(user_or_role, M.User):
+ user_or_role = M.ProjectRole.by_user(user_or_role, project)
+ if user_or_role is None:
+ user_or_role = auth # user is not member of project, treat as auth
+ roles = [user_or_role]
+ if user_or_role == anon:
+ pass # anon inherits nothing
+ elif user_or_role == auth:
+ roles += [anon] # auth inherits from anon
+ else:
+ roles += [auth, anon] # named group or user inherits from auth + anon
+ role_ids = RoleCache(Credentials.get(), roles).reaching_ids # match rules applicable to us
+ perms = set()
+ denied = defaultdict(set)
+ while obj: # traverse parent contexts
+ for role_id in role_ids:
+ for ace in obj.acl:
+ if ace.permission in denied[role_id]:
+ # don't consider permissions that were denied for this role
+ continue
+ if M.ACE.match(ace, role_id, ace.permission):
+ if ace.access == M.ACE.ALLOW:
+ perms.add(ace.permission)
+ else:
+ # explicit DENY overrides any ALLOW for this permission
+ # for this role_id in this ACL or parent(s) (but an ALLOW
+ # for a different role could still grant this permission)
+ denied[role_id].add(ace.permission)
+ obj = obj.parent_security_context()
+ if M.ALL_PERMISSIONS in perms:
+ return set([M.ALL_PERMISSIONS])
+ return perms
+
def require(predicate, message=None):
'''
Example: require(has_access(c.app, 'read'))
http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/36b8c925/Allura/allura/tests/test_security.py
----------------------------------------------------------------------
diff --git a/Allura/allura/tests/test_security.py b/Allura/allura/tests/test_security.py
index a88467b..ddb1a39 100644
--- a/Allura/allura/tests/test_security.py
+++ b/Allura/allura/tests/test_security.py
@@ -1,6 +1,25 @@
+from pylons import tmpl_context as c
+from nose.tools import assert_equal
+
+from ming.odm import ThreadLocalODMSession
from allura.tests import decorators as td
from allura.tests import TestController
+from allura.lib.security import Credentials, all_allowed, has_access
+from allura import model as M
+from forgewiki import model as WM
+
+
+def _deny(obj, role, perm):
+ obj.acl.insert(0, M.ACE.deny(role._id, perm))
+ ThreadLocalODMSession.flush_all()
+ Credentials.get().clear()
+
+def _add_to_group(user, role):
+ user.project_role().roles.append(role._id)
+ ThreadLocalODMSession.flush_all()
+ Credentials.get().clear()
+
class TestSecurity(TestController):
validate_skip = True
@@ -25,3 +44,78 @@ class TestSecurity(TestController):
self.app.get('/security/test-admin/needs_artifact_access_fail', status=200)
self.app.get('/security/test-admin/needs_artifact_access_ok', status=200)
+ @td.with_wiki
+ def test_all_allowed(self):
+ wiki = c.project.app_instance('wiki')
+ page = WM.Page.query.get(app_config_id=wiki.config._id)
+ admin_role = M.ProjectRole.by_name('Admin')
+ dev_role = M.ProjectRole.by_name('Developer')
+ member_role = M.ProjectRole.by_name('Member')
+ auth_role = M.ProjectRole.by_name('*authenticated')
+ anon_role = M.ProjectRole.by_name('*anonymous')
+ test_user = M.User.by_username('test-user')
+
+ assert_equal(all_allowed(wiki, admin_role), set(['configure', 'read', 'create', 'edit', 'unmoderated_post', 'post', 'moderate', 'admin', 'delete']))
+ assert_equal(all_allowed(wiki, dev_role), set(['read', 'create', 'edit', 'unmoderated_post', 'post', 'moderate', 'delete']))
+ assert_equal(all_allowed(wiki, member_role), set(['read', 'create', 'edit', 'unmoderated_post', 'post']))
+ assert_equal(all_allowed(wiki, auth_role), set(['read', 'post', 'unmoderated_post']))
+ assert_equal(all_allowed(wiki, anon_role), set(['read']))
+ assert_equal(all_allowed(wiki, test_user), set(['read', 'post', 'unmoderated_post']))
+
+ _add_to_group(test_user, member_role)
+
+ assert_equal(all_allowed(wiki, test_user), set(['read', 'create', 'edit', 'unmoderated_post', 'post']))
+
+ _deny(wiki, auth_role, 'unmoderated_post')
+
+ assert_equal(all_allowed(wiki, member_role), set(['read', 'create', 'edit', 'post']))
+ assert_equal(all_allowed(wiki, test_user), set(['read', 'create', 'edit', 'post']))
+
+ @td.with_wiki
+ def test_weird_allow_vs_deny(self):
+ '''
+ Test weird interaction of DENYs and ALLOWs in has_access.
+ '''
+ wiki = c.project.app_instance('wiki')
+ page = WM.Page.query.get(app_config_id=wiki.config._id)
+ auth_role = M.ProjectRole.by_name('*authenticated')
+ test_user = M.User.by_username('test-user')
+
+
+ # DENY for auth_role on page prevents chaining of auth_role for 'read'
+ # but anon_role still chains so ALLOW read for anon_role on wiki applies
+ # and authed user can still read. 'post' and 'unmoderated_post' don't
+ # match DENY rule so they chain as normal.
+ #
+ # This behavior seems wrong and should probably be fixed at some point,
+ # but this test is here to confirm that all_allowed matches has_access.
+ assert has_access(page, 'read', test_user)()
+ assert has_access(page, 'post', test_user)()
+ assert has_access(page, 'unmoderated_post', test_user)()
+ assert_equal(all_allowed(page, test_user), set(['read', 'post', 'unmoderated_post']))
+
+ _deny(page, auth_role, 'read')
+
+ assert has_access(page, 'read', test_user)()
+ assert has_access(page, 'post', test_user)()
+ assert has_access(page, 'unmoderated_post', test_user)()
+ assert_equal(all_allowed(page, test_user), set(['read', 'post', 'unmoderated_post']))
+
+
+ # Same thing applies to ALLOW vs DENY on the same ACL;
+ # an ALLOW on any applicable role overrides a DENY on any other.
+ #
+ # In this case it's reasonable since you might want to DENY read for
+ # *anon but ALLOW it for *auth. *anon ALLOW overriding *auth DENY is
+ # just an unfortunate side-effect of not having a true heiarchy of roles.
+ assert has_access(wiki, 'read', test_user)()
+ assert has_access(wiki, 'post', test_user)()
+ assert has_access(wiki, 'unmoderated_post', test_user)()
+ assert_equal(all_allowed(wiki, test_user), set(['read', 'post', 'unmoderated_post']))
+
+ _deny(wiki, auth_role, 'read')
+
+ assert has_access(wiki, 'read', test_user)()
+ assert has_access(wiki, 'post', test_user)()
+ assert has_access(wiki, 'unmoderated_post', test_user)()
+ assert_equal(all_allowed(wiki, test_user), set(['read', 'post', 'unmoderated_post']))
http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/36b8c925/ForgeTracker/forgetracker/model/ticket.py
----------------------------------------------------------------------
diff --git a/ForgeTracker/forgetracker/model/ticket.py b/ForgeTracker/forgetracker/model/ticket.py
index b01782a..dbfda81 100644
--- a/ForgeTracker/forgetracker/model/ticket.py
+++ b/ForgeTracker/forgetracker/model/ticket.py
@@ -409,14 +409,14 @@ class Ticket(VersionedArtifact, ActivityObject, VotableArtifact):
def _set_private(self, bool_flag):
if bool_flag:
- role_developer = ProjectRole.by_name('Developer')._id
- role_creator = self.reported_by.project_role()._id
- self.acl = [
- ACE.allow(role_developer, ALL_PERMISSIONS),
- ACE.allow(role_creator, 'read'),
- ACE.allow(role_creator, 'post'),
- ACE.allow(role_creator, 'unmoderated_post'),
- DENY_ALL]
+ role_developer = ProjectRole.by_name('Developer')
+ role_creator = self.reported_by.project_role()
+ _allow_all = lambda role, perms: [ACE.allow(role._id, perm) for perm in perms]
+ # maintain existing access for developers and the ticket creator,
+ # but revoke all access for everyone else
+ self.acl = _allow_all(role_developer, security.all_allowed(self, role_developer)) \
+ + _allow_all(role_creator, security.all_allowed(self, role_creator)) \
+ + [DENY_ALL]
else:
self.acl = []
private = property(_get_private, _set_private)
http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/36b8c925/ForgeTracker/forgetracker/tests/unit/test_ticket_model.py
----------------------------------------------------------------------
diff --git a/ForgeTracker/forgetracker/tests/unit/test_ticket_model.py b/ForgeTracker/forgetracker/tests/unit/test_ticket_model.py
index 0799b91..ad9af77 100644
--- a/ForgeTracker/forgetracker/tests/unit/test_ticket_model.py
+++ b/ForgeTracker/forgetracker/tests/unit/test_ticket_model.py
@@ -81,11 +81,19 @@ class TestTicketModel(TrackerTestWithModel):
cred = Credentials.get().clear()
t.private = True
- assert t.acl == [ACE.allow(role_developer, ALL_PERMISSIONS),
- ACE.allow(role_creator, 'read'),
- ACE.allow(role_creator, 'post'),
- ACE.allow(role_creator, 'unmoderated_post'),
- DENY_ALL]
+ assert_equal(t.acl, [
+ ACE.allow(role_developer, 'save_searches'),
+ ACE.allow(role_developer, 'read'),
+ ACE.allow(role_developer, 'create'),
+ ACE.allow(role_developer, 'update'),
+ ACE.allow(role_developer, 'unmoderated_post'),
+ ACE.allow(role_developer, 'post'),
+ ACE.allow(role_developer, 'moderate'),
+ ACE.allow(role_developer, 'delete'),
+ ACE.allow(role_creator, 'read'),
+ ACE.allow(role_creator, 'post'),
+ ACE.allow(role_creator, 'unmoderated_post'),
+ DENY_ALL])
assert has_access(t, 'read', user=admin)()
assert has_access(t, 'create', user=admin)()
assert has_access(t, 'update', user=admin)()