[IMP] mail: clean activities access rights

This commit improves code of access rights checks when creating or updating
activities. Several points triggers this commit

 * we would like to use _filter_access_rules and override it on activities
   model in order to be more standard;
 * we would like to get rid of sudo used in various CRUD overrides as using
   standard ORM method allows to correctly use current user;
 * we would like to get rid of create_user_id field once we can keep the
   current user as create_uid;

Access rights are a bit updated. They are now the following

 * you can create activities for documents like posting messages (using
   _mail_post_access attribute; if not defined, write access on document
   is required);
 * you can read activities for documents you can read;
 * you can write and unlink activities either following the defined access
   rule (created OR assigned), and otherwise like posting messages (
   using _mail_post_access attribute; if not defined, write access on
   document is required);

In this commit we also hide edit / mark as done / cancel buttons for
activities the current user cannot modify.

This commit is linked to task ID 1856417 and PR #27619.

Co-Authored-By: Sébastien Theys <seb@odoo.com>
Co-Authored-By: Thibault Delavallée <tde@odoo.com>

addons/mail/models/mail_activity.py

@@ -113,9 +113,9 @@ class MailActivity(models.Model):
         index=True, ondelete='cascade', required=True)
     res_model = fields.Char(
         'Related Document Model',
-        index=True, related='res_model_id.model', store=True, readonly=True)
+        index=True, related='res_model_id.model', compute_sudo=True, store=True, readonly=True)
     res_name = fields.Char(
-        'Document Name', compute='_compute_res_name', store=True,
+        'Document Name', compute='_compute_res_name', compute_sudo=True, store=True,
         help="Display name of the related document.", readonly=True)
     # activity
     activity_type_id = fields.Many2one(
@@ -153,6 +153,8 @@ class MailActivity(models.Model):
         help='Technical field for UX purpose')
     mail_template_ids = fields.Many2many(related='activity_type_id.mail_template_ids', readonly=False)
     force_next = fields.Boolean(related='activity_type_id.force_next', readonly=False)
+    # access
+    can_write = fields.Boolean(compute='_compute_can_write', help='Technical field to hide buttons if the current user has no access.')
 
     @api.multi
     @api.onchange('previous_activity_type_id')
@@ -196,6 +198,12 @@ class MailActivity(models.Model):
         else:
             return 'planned'
 
+    @api.depends('res_model', 'res_id', 'user_id')
+    def _compute_can_write(self):
+        valid_records = self._filter_access_rules('write')
+        for record in self:
+            record.can_write = record in valid_records
+
     @api.onchange('activity_type_id')
     def _onchange_activity_type_id(self):
         if self.activity_type_id:
@@ -213,34 +221,60 @@ class MailActivity(models.Model):
             self.activity_type_id = self.recommended_activity_type_id
 
     @api.multi
-    def _check_access(self, operation):
-        """ Rule to access activities
+    def _filter_access_rules(self, operation):
+        """ Return the subset of ``self`` for which ``operation`` is allowed.
+        A custom implementation is done on activities as this document has some
+        access rules and is based on related document for activities that are
+        not covered by those rules.
 
-         * create: check write rights on related document;
-         * write: rule OR write rights on document;
-         * unlink: rule OR write rights on document;
+        Access on activities are the following :
+
+          * create: (``mail_post_access`` or write) right on related documents;
+          * read: read rights on related documents;
+          * write: access rule OR
+                   (``mail_post_access`` or write) rights on related documents);
+          * unlink: access rule OR
+                    (``mail_post_access`` or write) rights on related documents);
         """
-        self.check_access_rights(operation, raise_exception=True)  # will raise an AccessError
+        if self.env.user._is_superuser():
+            return self
+        if not self.check_access_rights(operation, raise_exception=False):
+            return self.env[self._name]
 
+        # write / unlink: valid for creator / assigned
         if operation in ('write', 'unlink'):
-            try:
-                self.check_access_rule(operation)
-            except exceptions.AccessError:
-                pass
-            else:
-                return
-        doc_operation = 'read' if operation == 'read' else 'write'
+            valid = super(MailActivity, self)._filter_access_rules(operation)
+            if valid and valid == self:
+                return self
+        else:  # create / read: linked to document only, no access rules defined
+            valid = self.env[self._name]
+
+        # compute remaining for hand-tailored rules
+        remaining = self - valid
+        remaining_sudo = remaining.sudo()
+
+        # fall back on related document access right checks. Use the same as defined for mail.thread
+        # if available; otherwise fall back on read for read, write for other operations.
         activity_to_documents = dict()
-        for activity in self.sudo():
+        for activity in remaining_sudo:
+            # write / unlink: if not updating self or assigned, limit to automated activities to avoid
+            # updating other people's activities. As unlinking a document bypasses access rights checks
+            # on related activities this will not prevent people from deleting documents with activities
+            # create / read: just check rights on related document
             activity_to_documents.setdefault(activity.res_model, list()).append(activity.res_id)
-        for model, res_ids in activity_to_documents.items():
-            self.env[model].check_access_rights(doc_operation, raise_exception=True)
-            try:
-                self.env[model].browse(res_ids).check_access_rule(doc_operation)
-            except exceptions.AccessError:
-                raise exceptions.AccessError(
-                    _('The requested operation cannot be completed due to security restrictions. Please contact your system administrator.\n\n(Document type: %s, Operation: %s)') %
-                    (self._description, operation))
+        for doc_model, doc_ids in activity_to_documents.items():
+            if hasattr(self.env[doc_model], '_mail_post_access'):
+                doc_operation = self.env[doc_model]._mail_post_access
+            elif operation == 'read':
+                doc_operation = 'read'
+            else:
+                doc_operation = 'write'
+            right = self.env[doc_model].check_access_rights(doc_operation, raise_exception=False)
+            if right:
+                valid_doc_ids = self.env[doc_model].browse(doc_ids)._filter_access_rules(doc_operation)
+                valid += remaining.filtered(lambda activity: activity.res_model == doc_model and activity.res_id in valid_doc_ids.ids)
+
+        return valid
 
     @api.multi
     def _check_access_assignation(self):
@@ -266,37 +300,29 @@ class MailActivity(models.Model):
 
     @api.model
     def create(self, values):
-        # already compute default values to be sure those are computed using the current user
-        values_w_defaults = self.default_get(self._fields.keys())
-        values_w_defaults.update(values)
-
-        # continue as sudo because activities are somewhat protected
-        activity = super(MailActivity, self.sudo()).create(values_w_defaults)
-        activity_user = activity.sudo(self.env.user)
-        activity_user._check_access('create')
+        activity = super(MailActivity, self).create(values)
 
         # send a notification to assigned user; in case of manually done activity also check
         # target has rights on document otherwise we prevent its creation. Automated activities
         # are checked since they are integrated into business flows that should not crash.
-        if activity_user.user_id != self.env.user:
-            if not activity_user.automated:
-                activity_user._check_access_assignation()
+        if activity.user_id != self.env.user:
+            if not activity.automated:
+                activity._check_access_assignation()
             if not self.env.context.get('mail_activity_quick_update', False):
-                activity_user.action_notify()
+                activity.action_notify()
 
-        self.env[activity_user.res_model].browse(activity_user.res_id).message_subscribe(partner_ids=[activity_user.user_id.partner_id.id])
+        self.env[activity.res_model].browse(activity.res_id).message_subscribe(partner_ids=[activity.user_id.partner_id.id])
         if activity.date_deadline <= fields.Date.today():
             self.env['bus.bus'].sendone(
                 (self._cr.dbname, 'res.partner', activity.user_id.partner_id.id),
                 {'type': 'activity_updated', 'activity_created': True})
-        return activity_user
+        return activity
 
     @api.multi
     def write(self, values):
-        self._check_access('write')
         if values.get('user_id'):
             pre_responsibles = self.mapped('user_id.partner_id')
-        res = super(MailActivity, self.sudo()).write(values)
+        res = super(MailActivity, self).write(values)
 
         if values.get('user_id'):
             if values['user_id'] != self.env.uid:
@@ -320,13 +346,12 @@ class MailActivity(models.Model):
 
     @api.multi
     def unlink(self):
-        self._check_access('unlink')
         for activity in self:
             if activity.date_deadline <= fields.Date.today():
                 self.env['bus.bus'].sendone(
                     (self._cr.dbname, 'res.partner', activity.user_id.partner_id.id),
                     {'type': 'activity_updated', 'activity_deleted': True})
-        return super(MailActivity, self.sudo()).unlink()
+        return super(MailActivity, self).unlink()
 
     @api.multi
     def action_notify(self):

addons/mail/security/mail_security.xml

@@ -41,9 +41,9 @@
         </record>
 
         <record id="mail_activity_rule_user" model="ir.rule">
-            <field name="name">mail.activity: user: own only</field>
+            <field name="name">mail.activity: user: write/unlink only (created or assigned)</field>
             <field name="model_id" ref="model_mail_activity"/>
-            <field name="domain_force">[('user_id', '=', user.id)]</field>
+            <field name="domain_force">['|', ('user_id', '=', user.id), ('create_user_id', '=', user.id)]</field>
             <field name="groups" eval="[(4, ref('base.group_user'))]"/>
             <field name="perm_create" eval="False"/>
             <field name="perm_read" eval="False"/>

addons/mail/static/src/xml/activity.xml

@@ -74,7 +74,7 @@
                                 </t>
                             </div>
                         </t>
-                        <div class="o_thread_message_tools btn-group">
+                        <div t-if="activity.can_write" class="o_thread_message_tools btn-group">
                             <span class="o_mark_as_done" data-toggle="popover" t-att-data-activity-id="activity.id" t-att-data-force-next-activity="activity.force_next" t-att-data-previous-activity-type-id="activity.activity_type_id[0]">
                                 <a role="button" href="#" class="btn btn-link btn-success text-muted o_activity_link mr8">
                                 <i class="fa fa-check"/> Mark Done </a>

addons/mail/static/src/xml/web_kanban_activity.xml

@@ -40,7 +40,7 @@
                             <div t-attf-class="o_activity_title_entry {{! log.force_next ? 'align-items-center' : 'mb-1'}}">
                                 <span t-attf-class="fa #{log.icon ? log.icon : 'fa-bell' } fa-fw mr-2 text-center text-muted" role="img" aria-label="Log" title="Log"/>
                                 <strong class="text-dark"><t t-esc="log.title_action or log.activity_type_id[1]"/></strong>
-                                <button t-if="! log.force_next" class="btn btn-sm btn-link py-0 o_edit_button"><i class="fa fa-pencil"/></button>
+                                <button t-if="! log.force_next and log.can_write" class="btn btn-sm btn-link py-0 o_edit_button"><i class="fa fa-pencil"/></button>
                             </div>
                             <div class="o_activity_title_entry mt-1" t-if="log.state != 'today'">
                                 <span class="fa fa-clock-o fa-fw mr-2 text-center text-muted" role="img" aria-label="Deadline" title="Deadline"/>
@@ -62,7 +62,7 @@
                                 <span><t t-esc="log.user_id[1]" /></span>
                             </div>
                         </div>
-                        <div class="flex-grow-1 text-right">
+                        <div t-if="log.can_write" class="flex-grow-1 text-right">
                         <a  t-att-data-force-next-activity="log.force_next"
                             t-att-data-previous-activity-type-id="log.activity_type_id[0]"
                             t-att-data-activity-id="log.id"

addons/mail/static/tests/activity_tests.js

@@ -99,6 +99,7 @@ QUnit.module('activity view', {
                     activity_type_id: { string: "Activity type", type: "many2one", relation: "mail.activity.type" },
                     display_name: { string: "Display name", type: "char" },
                     date_deadline: { string: "Due Date", type: "date" },
+                    can_write: { string: "Can write", type: "boolean" },
                     state: {
                         string: 'State',
                         type: 'selection',
@@ -112,6 +113,8 @@ QUnit.module('activity view', {
                         id: 1,
                         display_name: "An activity",
                         date_deadline: moment().add(3, "days").format("YYYY-MM-DD"), // now
+                        can_write: true,
+
                         state: "planned",
                         activity_type_id: 1,
                         mail_template_ids: [8, 9],
@@ -120,6 +123,7 @@ QUnit.module('activity view', {
                         id: 2,
                         display_name: "An activity",
                         date_deadline: moment().format("YYYY-MM-DD"), // now
+                        can_write: true,
                         state: "today",
                         activity_type_id: 1,
                         mail_template_ids: [8, 9],
@@ -128,6 +132,7 @@ QUnit.module('activity view', {
                         id: 3,
                         display_name: "An activity",
                         date_deadline: moment().subtract(2, "days").format("YYYY-MM-DD"), // now
+                        can_write: true,
                         state: "overdue",
                         activity_type_id: 2,
                         mail_template_ids: [],

addons/mail/static/tests/chatter_tests.js

@@ -74,6 +74,7 @@ QUnit.module('Chatter', {
                     activity_type_id: { string: "Activity type", type: "many2one", relation: "mail.activity.type" },
                     create_uid: { string: "Assigned to", type: "many2one", relation: 'partner' },
                     create_user_id: { string: "Creator", type: "many2one", relation: 'partner' },
+                    can_write: { string: "Can write", type: "boolean" },
                     display_name: { string: "Display name", type: "char" },
                     date_deadline: { string: "Due Date", type: "date" },
                     user_id: { string: "Assigned to", type: "many2one", relation: 'partner' },
@@ -228,7 +229,7 @@ QUnit.test('Activity Done keep feedback on blur', function (assert) {
     var done = assert.async();
 
     this.data['mail.activity'].records = [
-        {activity_type_id: 1, id: 1, user_id: 2, state: 'today', note: 'But I\'m talkin\' about Shaft'},
+        {activity_type_id: 1, id: 1, can_write: true, user_id: 2, state: 'today', note: 'But I\'m talkin\' about Shaft'},
     ];
     this.data.partner.records[0].activity_ids = [1];
 
@@ -512,6 +513,7 @@ QUnit.test('kanban activity widget with an activity', function (assert) {
         id: 1,
         display_name: "An activity",
         date_deadline: moment().format("YYYY-MM-DD"), // now
+        can_write: true,
         state: "today",
         user_id: 2,
         create_user_id: 2,
@@ -586,6 +588,7 @@ QUnit.test('kanban activity widget popover test', function (assert) {
         id: 1,
         display_name: "An activity",
         date_deadline: moment().format("YYYY-MM-DD"), // now
+        can_write: true,
         state: "today",
         user_id: 2,
         create_user_id: 2,
@@ -1477,6 +1480,7 @@ QUnit.test('form activity widget: read RPCs', function (assert) {
         id: 1,
         display_name: "An activity",
         date_deadline: moment().format("YYYY-MM-DD"), // now
+        can_write: true,
         state: "today",
         user_id: 2,
         create_user_id: 2,
@@ -1562,6 +1566,7 @@ QUnit.test('form activity widget with another x2many field in view', function (a
         id: 1,
         display_name: "An activity",
         date_deadline: moment().format("YYYY-MM-DD"), // now
+        can_write: true,
         state: "today",
         user_id: 2,
         create_user_id: 2,
@@ -1596,6 +1601,7 @@ QUnit.test('form activity widget: schedule next activity', function (assert) {
         id: 1,
         display_name: "An activity",
         date_deadline: moment().format("YYYY-MM-DD"), // now
+        can_write: true,
         state: "today",
         user_id: 2,
         create_user_id: 2,
@@ -1652,6 +1658,7 @@ QUnit.test('form activity widget: edit next activity', function (assert) {
         id: 1,
         display_name: "An activity",
         date_deadline: moment().format("YYYY-MM-DD"), // now
+        can_write: true,
         state: "today",
         user_id: 2,
         create_user_id: 2,
@@ -1715,6 +1722,7 @@ QUnit.test('form activity widget: clic mail template', function (assert) {
         id: 1,
         display_name: "An activity",
         date_deadline: moment().format("YYYY-MM-DD"), // now
+        can_write: true,
         state: "today",
         user_id: 2,
         activity_type_id: 2,
@@ -1833,6 +1841,7 @@ QUnit.test('form activity widget: mark as done and remove', function (assert) {
         id: 1,
         display_name: "An activity",
         date_deadline: moment().format("YYYY-MM-DD"), // now
+        can_write: true,
         state: "today",
         user_id: 2,
         create_user_id: 2,
@@ -1841,6 +1850,7 @@ QUnit.test('form activity widget: mark as done and remove', function (assert) {
         id: 2,
         display_name: "A second activity",
         date_deadline: moment().format("YYYY-MM-DD"), // now
+        can_write: true,
         state: "today",
         user_id: 2,
         create_user_id: 2,

addons/test_mail/tests/test_mail_activity.py

@@ -31,13 +31,18 @@ class TestActivityCommon(BaseFunctionalTest):
 @tests.tagged('mail_activity')
 class TestActivityRights(TestActivityCommon):
 
-    def test_activity_security_user_access(self):
-        activity = self.test_record.activity_schedule(
+    def test_activity_security_user_access_other(self):
+        activity = self.test_record.sudo(self.user_employee).activity_schedule(
             'test_mail.mail_act_test_todo',
-            user_id=self.user_employee.id)
+            user_id=self.user_admin.id)
+        self.assertTrue(activity.can_write)
+        activity.write({'user_id': self.user_employee.id})
 
-        activity2 = self.test_record.activity_schedule('test_mail.mail_act_test_todo')
-        activity2.write({'user_id': self.user_employee.id})
+    def test_activity_security_user_access_own(self):
+        activity = self.test_record.sudo(self.user_employee).activity_schedule(
+            'test_mail.mail_act_test_todo')
+        self.assertTrue(activity.can_write)
+        activity.write({'user_id': self.user_admin.id})
 
     def test_activity_security_user_noaccess_automated(self):
         def _employee_crash(*args, **kwargs):
@@ -63,6 +68,7 @@ class TestActivityRights(TestActivityCommon):
                 raise exceptions.AccessError('Hop hop hop Ernest, please step back.')
             return DEFAULT
 
+        # cannot create activities for people that cannot access record
         with patch.object(MailTestActivity, 'check_access_rights', autospec=True, side_effect=_employee_crash):
             with self.assertRaises(exceptions.UserError):
                 activity = self.env['mail.activity'].create({
@@ -72,13 +78,12 @@ class TestActivityRights(TestActivityCommon):
                     'user_id': self.user_employee.id,
                 })
 
-            activity2 = self.env['mail.activity'].create({
-                'activity_type_id': self.env.ref('test_mail.mail_act_test_todo').id,
-                'res_model_id': self.env.ref('test_mail.model_mail_test_activity').id,
-                'res_id': self.test_record.id,
-            })
-            with self.assertRaises(exceptions.UserError):
-                activity2.write({'user_id': self.user_employee.id})
+        # cannot create activities if no access to the document
+        with patch.object(MailTestActivity, 'check_access_rights', autospec=True, side_effect=_employee_crash):
+            with self.assertRaises(exceptions.AccessError):
+                activity = self.test_record.sudo(self.user_employee).activity_schedule(
+                    'test_mail.mail_act_test_todo',
+                    user_id=self.user_admin.id)
 
 
 @tests.tagged('mail_activity')

addons/test_mail/tests/test_performance.py

@@ -172,7 +172,7 @@ class TestAdvMailPerformance(TransactionCase):
             'default_res_model': 'mail.test.activity',
         })
 
-        with self.assertQueryCount(__system__=10, emp=15):  # com runbot: 9 - 15 // test_mail only: 10 - 14
+        with self.assertQueryCount(__system__=10, emp=16):  # com runbot: 9 - 15 // test_mail only: 10 - 14
             activity = MailActivity.create({
                 'summary': 'Test Activity',
                 'res_id': record.id,
@@ -182,7 +182,7 @@ class TestAdvMailPerformance(TransactionCase):
             #voip module read activity_type during create leading to one less query in enterprise on action_feedback
             category = activity.activity_type_id.category
 
-        with self.assertQueryCount(__system__=26, emp=46):  # com runbot: 25 - 46 // test_mail only: 26 - 46
+        with self.assertQueryCount(__system__=26, emp=50):  # com runbot: 25 - 46 // test_mail only: 26 - 46
             activity.action_feedback(feedback='Zizisse Done !')
 
     @users('__system__', 'emp')
@@ -191,7 +191,7 @@ class TestAdvMailPerformance(TransactionCase):
     def test_adv_activity_mixin(self):
         record = self.env['mail.test.activity'].create({'name': 'Test'})
 
-        with self.assertQueryCount(__system__=10, emp=15):  # com runbot: 9 - 15 // test_mail only: 10 - 14
+        with self.assertQueryCount(__system__=10, emp=16):  # com runbot: 9 - 15 // test_mail only: 10 - 14
             activity = record.action_start('Test Start')
             #read activity_type to normalize cache between enterprise and community
             #voip module read activity_type during create leading to one less query in enterprise on action_close
@@ -199,7 +199,7 @@ class TestAdvMailPerformance(TransactionCase):
 
         record.write({'name': 'Dupe write'})
 
-        with self.assertQueryCount(__system__=28, emp=48):  # com runbot: 27 - 86 // test_mail only: 28 - 48
+        with self.assertQueryCount(__system__=28, emp=52):  # com runbot: 27 - 86 // test_mail only: 28 - 48
             record.action_close('Dupe feedback')
 
         self.assertEqual(record.activity_ids, self.env['mail.activity'])