(allura) 11/33: [#8607] check before mailbox sub too

[br...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

asf-gitbox-commits pushed a commit to branch db/8607
in repository https://gitbox.apache.org/repos/asf/allura.git

commit 36609a8622417f682d9c4b3ecad47311b13155b0
Author: Dave Brondsema <da...@brondsema.net>
AuthorDate: Fri May 15 18:16:58 2026 -0400

    [#8607] check before mailbox sub too
---
 Allura/allura/controllers/auth.py           | 10 +++++----
 Allura/allura/tests/functional/test_auth.py | 32 +++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/Allura/allura/controllers/auth.py b/Allura/allura/controllers/auth.py
index 6ff0c7577..385cc6c08 100644
--- a/Allura/allura/controllers/auth.py
+++ b/Allura/allura/controllers/auth.py
@@ -1373,10 +1373,12 @@ def index(self, **kw):
     def update_subscriptions(self, subscriptions=None, email_format=None, **kw):
         for s in subscriptions:
             if s['subscribed']:
-                if s['tool_id'] and s['project_id']:
-                    M.Mailbox.subscribe(
-                        project_id=bson.ObjectId(s['project_id']),
-                        app_config_id=bson.ObjectId(s['tool_id']))
+                if s['tool_id']:
+                    app_config = M.AppConfig.query.get(_id=bson.ObjectId(s['tool_id']))
+                    if app_config and has_access(app_config, 'read'):
+                        M.Mailbox.subscribe(
+                            project_id=app_config.project_id,
+                            app_config_id=app_config._id)
             else:
                 mbox = s['subscription_id']
                 if mbox is not None and mbox.user_id == c.user._id:
diff --git a/Allura/allura/tests/functional/test_auth.py b/Allura/allura/tests/functional/test_auth.py
index 347ec996e..31cb3e413 100644
--- a/Allura/allura/tests/functional/test_auth.py
+++ b/Allura/allura/tests/functional/test_auth.py
@@ -984,6 +984,38 @@ def test_prefs_subscriptions_unsubscribe(self):
         s = M.Mailbox.query.get(_id=s_id)
         assert not s, "User still has subscription with Mailbox._id %s" % s_id
 
+    @td.with_user_project('test-admin')
+    @patch('allura.controllers.auth.has_access')
+    def test_update_subscriptions_subscribe_requires_read_access(self, has_access_mock):
+        # Subscribe must be rejected when the user lacks read access to the tool.
+        has_access_mock.side_effect = lambda obj, *a, **kw: not isinstance(obj, M.AppConfig)
+        project = M.Project.query.get(shortname='test')
+        app_config = project.app_configs[0]
+        attacker = M.User.by_username('test-admin')
+        M.Mailbox.query.remove(dict(
+            user_id=attacker._id,
+            app_config_id=app_config._id,
+            artifact_index_id=None))
+        ThreadLocalODMSession.flush_all()
+
+        self.app.get('/').follow()
+        self.app.post('/auth/subscriptions/update_subscriptions',
+                      params={
+                          'subscriptions-0.subscribed': 'on',
+                          'subscriptions-0.tool_id': str(app_config._id),
+                          'subscriptions-0.project_id': str(project._id),
+                          'subscriptions-0.subscription_id': '',
+                          'subscriptions-0.topic': '',
+                          'subscriptions-0.artifact_index_id': '',
+                          '_csrf_token': self.app.cookies['_csrf_token'],
+                      })
+
+        mbox = M.Mailbox.query.get(
+            user_id=attacker._id,
+            app_config_id=app_config._id,
+            artifact_index_id=None)
+        assert mbox is None
+
     @td.with_user_project('test-admin')
     def test_update_subscriptions_rejects_foreign_mailbox(self):
         # Create a Mailbox owned by test-user-1 (the victim).