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/07/29 22:48:13 UTC
git commit: [#6460] Fixed security checks sometimes using incorrect
roles
Updated Branches:
refs/heads/cj/6460 [created] 2484e6c4d
[#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/2484e6c4
Tree: http://git-wip-us.apache.org/repos/asf/incubator-allura/tree/2484e6c4
Diff: http://git-wip-us.apache.org/repos/asf/incubator-allura/diff/2484e6c4
Branch: refs/heads/cj/6460
Commit: 2484e6c4db62a32e489bdbbdc056a22dd422bad0
Parents: 70d4cdb
Author: Cory Johns <cj...@slashdotmedia.com>
Authored: Mon Jul 29 20:40:35 2013 +0000
Committer: Cory Johns <cj...@slashdotmedia.com>
Committed: Mon Jul 29 20:40:40 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/2484e6c4/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/2484e6c4/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/2484e6c4/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/2484e6c4/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)()