You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@allura.apache.org by tv...@apache.org on 2013/07/31 16:19:20 UTC
[1/2] git commit: [#6460] Fixed security checks sometimes using
incorrect roles
Updated Branches:
refs/heads/master 53e35eb9f -> 625bf2ec9
[#6460] Fixed security checks sometimes using incorrect roles
When doing a has_access() check for a given user against a given
artifact without explicitly specifying the project, c.project was being
used to get the list of user's roles instead of the artifact's project
attribute. If c.project was not the project to which the artifact
belonged, the the wrong set of role_ids were being used, resulting in
access being denied. It's a bit nonsensical to use an unrelated
project's role_ids to check access to an artifact, and this was breaking
notifications, which fire all pending notifications, regardless of the
context under which fire_ready() was called.
Signed-off-by: Cory Johns <cj...@slashdotmedia.com>
Project: http://git-wip-us.apache.org/repos/asf/incubator-allura/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-allura/commit/4b2d8c17
Tree: http://git-wip-us.apache.org/repos/asf/incubator-allura/tree/4b2d8c17
Diff: http://git-wip-us.apache.org/repos/asf/incubator-allura/diff/4b2d8c17
Branch: refs/heads/master
Commit: 4b2d8c171aedf12c525d659470b0d807732afb9d
Parents: 53e35eb
Author: Cory Johns <cj...@slashdotmedia.com>
Authored: Mon Jul 29 20:40:35 2013 +0000
Committer: Tim Van Steenburgh <tv...@gmail.com>
Committed: Wed Jul 31 13:50:42 2013 +0000
----------------------------------------------------------------------
Allura/allura/lib/security.py | 4 +-
Allura/allura/model/notification.py | 6 +-
Allura/allura/tests/model/test_notification.py | 86 +++++++++++++++++++++
Allura/allura/tests/test_security.py | 21 +++++
4 files changed, 113 insertions(+), 4 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/4b2d8c17/Allura/allura/lib/security.py
----------------------------------------------------------------------
diff --git a/Allura/allura/lib/security.py b/Allura/allura/lib/security.py
index 21ffb0a..a39e68d 100644
--- a/Allura/allura/lib/security.py
+++ b/Allura/allura/lib/security.py
@@ -287,7 +287,9 @@ def has_access(obj, permission, user=None, project=None):
elif isinstance(obj, M.Project):
project = obj.root_project
else:
- project = c.project.root_project
+ project = getattr(obj, 'project', None)
+ if project is None:
+ project = c.project.root_project
roles = cred.user_roles(user_id=user._id, project_id=project._id).reaching_ids
chainable_roles = []
for rid in roles:
http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/4b2d8c17/Allura/allura/model/notification.py
----------------------------------------------------------------------
diff --git a/Allura/allura/model/notification.py b/Allura/allura/model/notification.py
index d4b0755..f1449fe 100644
--- a/Allura/allura/model/notification.py
+++ b/Allura/allura/model/notification.py
@@ -250,10 +250,10 @@ class Notification(MappedClass):
not security.has_access(artifact, 'read', user)():
log.debug("Skipping notification - User %s doesn't have read "
"access to artifact %s" % (user_id, str(self.ref_id)))
- log.debug("User roles [%s]; artifact ACL [%s]; project ACL [%s]",
- ', '.join([str(r) for r in security.Credentials.get().user_roles(user_id=user_id, project_id=c.project._id).reaching_ids]),
+ log.debug("User roles [%s]; artifact ACL [%s]; PSC ACL [%s]",
+ ', '.join([str(r) for r in security.Credentials.get().user_roles(user_id=user_id, project_id=artifact.project._id).reaching_ids]),
', '.join([str(a) for a in artifact.acl]),
- ', '.join([str(a) for a in c.project.acl]))
+ ', '.join([str(a) for a in artifact.parent_security_context().acl]))
return
allura.tasks.mail_tasks.sendmail.post(
destinations=[str(user_id)],
http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/4b2d8c17/Allura/allura/tests/model/test_notification.py
----------------------------------------------------------------------
diff --git a/Allura/allura/tests/model/test_notification.py b/Allura/allura/tests/model/test_notification.py
index 4e9dd5b..1e1c273 100644
--- a/Allura/allura/tests/model/test_notification.py
+++ b/Allura/allura/tests/model/test_notification.py
@@ -68,6 +68,92 @@ class TestNotification(unittest.TestCase):
assert len(subscriptions) == 0
assert M.Mailbox.query.find().count() == 0
+ @mock.patch('allura.tasks.mail_tasks.sendmail')
+ def test_send_direct(self, sendmail):
+ c.user = M.User.query.get(username='test-user')
+ wiki = c.project.app_instance('wiki')
+ page = WM.Page.query.get(app_config_id=wiki.config._id)
+ notification = M.Notification(
+ _id='_id',
+ ref=page.ref,
+ from_address='from_address',
+ reply_to_address='reply_to_address',
+ in_reply_to='in_reply_to',
+ subject='subject',
+ text='text',
+ )
+ notification.footer = lambda: ' footer'
+ notification.send_direct(c.user._id)
+ sendmail.post.assert_called_once_with(
+ destinations=[str(c.user._id)],
+ fromaddr='from_address',
+ reply_to='reply_to_address',
+ subject='subject',
+ message_id='_id',
+ in_reply_to='in_reply_to',
+ text='text footer',
+ )
+
+ @mock.patch('allura.tasks.mail_tasks.sendmail')
+ def test_send_direct_no_access(self, sendmail):
+ c.user = M.User.query.get(username='test-user')
+ wiki = c.project.app_instance('wiki')
+ page = WM.Page.query.get(app_config_id=wiki.config._id)
+ page.parent_security_context().acl = []
+ ThreadLocalORMSession.flush_all()
+ ThreadLocalORMSession.close_all()
+ notification = M.Notification(
+ _id='_id',
+ ref=page.ref,
+ from_address='from_address',
+ reply_to_address='reply_to_address',
+ in_reply_to='in_reply_to',
+ subject='subject',
+ text='text',
+ )
+ notification.footer = lambda: ' footer'
+ notification.send_direct(c.user._id)
+ assert_equal(sendmail.post.call_count, 0)
+
+ @mock.patch('allura.tasks.mail_tasks.sendmail')
+ def test_send_direct_wrong_project_context(self, sendmail):
+ """
+ Test that Notification.send_direct() works as expected even
+ if c.project is wrong.
+
+ This can happen when a notify task is triggered on project A (thus
+ setting c.project to A) and then calls Mailbox.fire_ready() which fires
+ pending Notifications on any waiting Mailbox, regardless of project,
+ but doesn't update c.project.
+ """
+ project1 = c.project
+ project2 = M.Project.query.get(shortname='test2')
+ assert_equal(project1.shortname, 'test')
+ c.user = M.User.query.get(username='test-user')
+ wiki = project1.app_instance('wiki')
+ page = WM.Page.query.get(app_config_id=wiki.config._id)
+ notification = M.Notification(
+ _id='_id',
+ ref=page.ref,
+ from_address='from_address',
+ reply_to_address='reply_to_address',
+ in_reply_to='in_reply_to',
+ subject='subject',
+ text='text',
+ )
+ notification.footer = lambda: ' footer'
+ c.project = project2
+ notification.send_direct(c.user._id)
+ sendmail.post.assert_called_once_with(
+ destinations=[str(c.user._id)],
+ fromaddr='from_address',
+ reply_to='reply_to_address',
+ subject='subject',
+ message_id='_id',
+ in_reply_to='in_reply_to',
+ text='text footer',
+ )
+
class TestPostNotifications(unittest.TestCase):
def setUp(self):
http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/4b2d8c17/Allura/allura/tests/test_security.py
----------------------------------------------------------------------
diff --git a/Allura/allura/tests/test_security.py b/Allura/allura/tests/test_security.py
index 40f9b8b..10ae614 100644
--- a/Allura/allura/tests/test_security.py
+++ b/Allura/allura/tests/test_security.py
@@ -136,3 +136,24 @@ class TestSecurity(TestController):
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']))
+
+ @td.with_wiki
+ def test_implicit_project(self):
+ '''
+ Test that relying on implicit project context does the Right Thing.
+
+ If you call has_access(artifact, perm), it should use the roles from
+ the project to which artifact belongs, even in c.project is something
+ else. If you really want to use the roles from an unrelated project,
+ you should have to be very explicit about it, not just set c.project.
+ '''
+ project1 = c.project
+ project2 = M.Project.query.get(shortname='test2')
+ wiki = project1.app_instance('wiki')
+ page = WM.Page.query.get(app_config_id=wiki.config._id)
+ test_user = M.User.by_username('test-user')
+
+ assert_equal(project1.shortname, 'test')
+ assert has_access(page, 'read', test_user)()
+ c.project = project2
+ assert has_access(page, 'read', test_user)()
[2/2] git commit: [#6460] Get root_project of artifact
Posted by tv...@apache.org.
[#6460] Get root_project of artifact
Signed-off-by: Tim Van Steenburgh <tv...@gmail.com>
Project: http://git-wip-us.apache.org/repos/asf/incubator-allura/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-allura/commit/625bf2ec
Tree: http://git-wip-us.apache.org/repos/asf/incubator-allura/tree/625bf2ec
Diff: http://git-wip-us.apache.org/repos/asf/incubator-allura/diff/625bf2ec
Branch: refs/heads/master
Commit: 625bf2ec9dfe3765d7d8813324b8a61986aba51f
Parents: 4b2d8c1
Author: Tim Van Steenburgh <tv...@gmail.com>
Authored: Wed Jul 31 14:18:47 2013 +0000
Committer: Tim Van Steenburgh <tv...@gmail.com>
Committed: Wed Jul 31 14:18:47 2013 +0000
----------------------------------------------------------------------
Allura/allura/lib/security.py | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-allura/blob/625bf2ec/Allura/allura/lib/security.py
----------------------------------------------------------------------
diff --git a/Allura/allura/lib/security.py b/Allura/allura/lib/security.py
index a39e68d..a0497ca 100644
--- a/Allura/allura/lib/security.py
+++ b/Allura/allura/lib/security.py
@@ -287,9 +287,8 @@ def has_access(obj, permission, user=None, project=None):
elif isinstance(obj, M.Project):
project = obj.root_project
else:
- project = getattr(obj, 'project', None)
- if project is None:
- project = c.project.root_project
+ project = getattr(obj, 'project', None) or c.project
+ project = project.root_project
roles = cred.user_roles(user_id=user._id, project_id=project._id).reaching_ids
chainable_roles = []
for rid in roles: