From b9c8ba83e6b367275eca28255dd7df9b8ac84aa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Theys?= Date: Fri, 20 Jul 2018 12:02:16 +0000 Subject: [PATCH 1/2] [IMP] mail: clean activities access rights MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-Authored-By: Thibault Delavallée --- addons/mail/models/mail_activity.py | 109 +++++++++++------- addons/mail/security/mail_security.xml | 4 +- addons/mail/static/src/xml/activity.xml | 2 +- .../static/src/xml/web_kanban_activity.xml | 4 +- addons/mail/static/tests/activity_tests.js | 5 + addons/mail/static/tests/chatter_tests.js | 12 +- addons/test_mail/tests/test_mail_activity.py | 29 +++-- addons/test_mail/tests/test_performance.py | 8 +- 8 files changed, 109 insertions(+), 64 deletions(-) diff --git a/addons/mail/models/mail_activity.py b/addons/mail/models/mail_activity.py index b925f59b1be..3bddb647bde 100644 --- a/addons/mail/models/mail_activity.py +++ b/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): diff --git a/addons/mail/security/mail_security.xml b/addons/mail/security/mail_security.xml index 7a97925b42b..79f6f44bae1 100644 --- a/addons/mail/security/mail_security.xml +++ b/addons/mail/security/mail_security.xml @@ -41,9 +41,9 @@ - mail.activity: user: own only + mail.activity: user: write/unlink only (created or assigned) - [('user_id', '=', user.id)] + ['|', ('user_id', '=', user.id), ('create_user_id', '=', user.id)] diff --git a/addons/mail/static/src/xml/activity.xml b/addons/mail/static/src/xml/activity.xml index cb7f8a5ae28..80618f7024c 100644 --- a/addons/mail/static/src/xml/activity.xml +++ b/addons/mail/static/src/xml/activity.xml @@ -74,7 +74,7 @@ -
+
Mark Done diff --git a/addons/mail/static/src/xml/web_kanban_activity.xml b/addons/mail/static/src/xml/web_kanban_activity.xml index 8f63b51cdfe..9ebed7ebce4 100644 --- a/addons/mail/static/src/xml/web_kanban_activity.xml +++ b/addons/mail/static/src/xml/web_kanban_activity.xml @@ -40,7 +40,7 @@
- +
@@ -62,7 +62,7 @@
-
+
Date: Wed, 10 Oct 2018 13:29:03 +0000 Subject: [PATCH 2/2] [REF] mail: remove create_user_id field on activity As activities are created using the current user there is no need anymore to have another field to store the activity creator. We can therefore remove create_user_id and replace its use by the magic create_uid field. This commit is linked to task ID 1856417 and PR #27619. --- addons/crm/data/crm_lead_demo.xml | 24 +++++++++---------- .../data/hr_recruitment_demo.xml | 14 +++++------ addons/mail/data/mail_data.xml | 2 +- addons/mail/models/mail_activity.py | 4 ---- addons/mail/security/mail_security.xml | 2 +- addons/mail/static/src/xml/activity.xml | 8 +++---- addons/mail/static/tests/chatter_tests.js | 19 +++++++-------- addons/mail/views/mail_activity_views.xml | 2 +- addons/test_mail/tests/test_mail_activity.py | 4 ++-- 9 files changed, 37 insertions(+), 42 deletions(-) diff --git a/addons/crm/data/crm_lead_demo.xml b/addons/crm/data/crm_lead_demo.xml index 6f3191d39fb..4f638a42f0e 100644 --- a/addons/crm/data/crm_lead_demo.xml +++ b/addons/crm/data/crm_lead_demo.xml @@ -1176,7 +1176,7 @@ Andrew

]]> Meeting to go over pricing information. - + @@ -1185,7 +1185,7 @@ Andrew

]]>
Send Catalog by Email - + @@ -1194,7 +1194,7 @@ Andrew

]]>
Call to get system requirements - + @@ -1203,7 +1203,7 @@ Andrew

]]>
Convert to quote - + @@ -1213,7 +1213,7 @@ Andrew

]]>
Send our service pricelist - + @@ -1221,7 +1221,7 @@ Andrew

]]>
Call to get training needs - +
@@ -1231,7 +1231,7 @@ Andrew

]]>
Followup on the proposal - + @@ -1250,7 +1250,7 @@ Andrew

]]>
- +
@@ -1258,7 +1258,7 @@ Andrew

]]>
- + @@ -1266,7 +1266,7 @@ Andrew

]]>
- + @@ -1275,7 +1275,7 @@ Andrew

]]>
Conf call with technical service - + @@ -1285,7 +1285,7 @@ Andrew

]]>
Send Catalog by Email - + diff --git a/addons/hr_recruitment/data/hr_recruitment_demo.xml b/addons/hr_recruitment/data/hr_recruitment_demo.xml index 82c6165549e..bd1bbdc00a8 100644 --- a/addons/hr_recruitment/data/hr_recruitment_demo.xml +++ b/addons/hr_recruitment/data/hr_recruitment_demo.xml @@ -359,7 +359,7 @@ Send mail regarding our interview - + @@ -368,7 +368,7 @@ Send mail for first interview - + @@ -377,7 +377,7 @@ Send mail regarding our interview - + @@ -386,7 +386,7 @@ Call to define real needs about training - + @@ -395,7 +395,7 @@ Call to define real needs about training - + @@ -404,7 +404,7 @@ Call to schedule a second interview - + @@ -413,7 +413,7 @@ Call to propose a contract - + diff --git a/addons/mail/data/mail_data.xml b/addons/mail/data/mail_data.xml index 9553cc5b69e..ec4868a9a75 100644 --- a/addons/mail/data/mail_data.xml +++ b/addons/mail/data/mail_data.xml @@ -327,7 +327,7 @@