[#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 <cjohns@slashdotmedia.com>
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 @@
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:
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 @@
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)],
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 @@
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):
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 @@
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)()
