incubator-allura-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From john...@apache.org
Subject [1/3] git commit: [#5647] Fixed escalation of rights on private tickets for developers and creator
Date Wed, 27 Feb 2013 18:52:46 GMT
[#5647] Fixed escalation of rights on private tickets for developers and creator

Signed-off-by: Cory Johns <johnsca@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/5647
Commit: 36b8c925ff51cf52531b9d07a58c8cbb80dd1119
Parents: 5db659a
Author: Cory Johns <johnsca@geek.net>
Authored: Wed Feb 27 18:51:46 2013 +0000
Committer: Cory Johns <johnsca@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)()


Mime
View raw message