From 62c9dedafda0cbdc618641eaa80c49535ab0b5f6 Mon Sep 17 00:00:00 2001 From: Yannick Tivisse Date: Tue, 26 Mar 2019 12:51:22 +0000 Subject: [PATCH] [IMP] base: Remove useless ACL rules Now that portal users don't have access to the backend, it doesn't make sense to give the read access on attachments, as this is the role of the specific controllers to provide this access according to the business logic. For the record, these rules have been introduced at: https://github.com/odoo/odoo/commit/61065b6d04248aa496765e1035c4c90cbdc38de7 https://github.com/odoo/odoo/commit/f3fa266d115ac9d4723164af10f8dee40821b290 closes odoo/odoo#32134 Signed-off-by: Yannick Tivisse (yti) --- addons/mail/models/ir_attachment.py | 2 +- addons/mail/models/mail_thread.py | 2 +- addons/test_mail/tests/test_odoobot.py | 7 +++- addons/website_sale_digital/models/product.py | 8 ++--- odoo/addons/base/models/ir_attachment.py | 33 +++++++++++++++---- odoo/addons/base/models/ir_http.py | 18 +++++++--- odoo/addons/base/security/ir.model.access.csv | 3 +- odoo/models.py | 2 +- 8 files changed, 53 insertions(+), 22 deletions(-) diff --git a/addons/mail/models/ir_attachment.py b/addons/mail/models/ir_attachment.py index 4330059c495..31e59d680c9 100644 --- a/addons/mail/models/ir_attachment.py +++ b/addons/mail/models/ir_attachment.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. -from odoo import api, fields, models +from odoo import api, models class IrAttachment(models.Model): diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 79cfb230a6b..766d0a05d55 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -109,7 +109,7 @@ class MailThread(models.AbstractModel): message_has_error_counter = fields.Integer( 'Number of error', compute='_compute_message_has_error', help="Number of messages with delivery error") - message_attachment_count = fields.Integer('Attachment Count', compute='_compute_message_attachment_count') + message_attachment_count = fields.Integer('Attachment Count', compute='_compute_message_attachment_count', groups="base.group_user") message_main_attachment_id = fields.Many2one(string="Main Attachment", comodel_name='ir.attachment', index=True, copy=False) @api.one diff --git a/addons/test_mail/tests/test_odoobot.py b/addons/test_mail/tests/test_odoobot.py index 07cdfaa0d3a..900e7b62689 100644 --- a/addons/test_mail/tests/test_odoobot.py +++ b/addons/test_mail/tests/test_odoobot.py @@ -61,7 +61,12 @@ class TestOdoobot(BaseFunctionalTest, MockEmails, TestRecipients): answer=("attachment",) ) kwargs['body'] = '' - kwargs['attachment_ids'] = [1] + attachment = self.env['ir.attachment'].sudo(self.user_employee).create({ + 'datas': 'bWlncmF0aW9uIHRlc3Q=', + 'datas_fname': 'catalog.doc', + 'name': 'picture_of_your_dog.doc', + }) + kwargs['attachment_ids'] = [attachment.id] last_message = self.assertNextMessage( channel.message_post(**kwargs), sender=self.odoobot, diff --git a/addons/website_sale_digital/models/product.py b/addons/website_sale_digital/models/product.py index cfbd3c33918..23a2004ea22 100644 --- a/addons/website_sale_digital/models/product.py +++ b/addons/website_sale_digital/models/product.py @@ -41,11 +41,11 @@ class Product(models.Model): @api.multi def _compute_attachment_count(self): - IrAttachment = self.env['ir.attachment'] for product in self: - prod_tmpl_attach_count = IrAttachment.search_count([('res_model', '=', 'product.template'), ('res_id', 'in', product.product_tmpl_id.ids), ('product_downloadable', '=', True)]) - prod_attach_count = IrAttachment.search_count([('res_model', '=', 'product.product'), ('res_id', 'in', product.ids), ('product_downloadable', '=', True)]) - product.attachment_count = prod_tmpl_attach_count + prod_attach_count + product.attachment_count = self.env['ir.attachment'].search_count([ + '|', + ('res_model', '=', 'product.template'), ('res_id', '=', product.product_tmpl_id.id), ('product_downloadable', '=', True), + ('res_model', '=', 'product.product'), ('res_id', '=', product.id), ('product_downloadable', '=', True)]) @api.multi def action_open_attachments(self): diff --git a/odoo/addons/base/models/ir_attachment.py b/odoo/addons/base/models/ir_attachment.py index d7672d815ed..b0944e5ebd4 100644 --- a/odoo/addons/base/models/ir_attachment.py +++ b/odoo/addons/base/models/ir_attachment.py @@ -330,8 +330,10 @@ class IrAttachment(models.Model): model_ids = defaultdict(set) # {model_name: set(ids)} require_employee = False if self: - self._cr.execute('SELECT res_model, res_id, create_uid, public FROM ir_attachment WHERE id IN %s', [tuple(self.ids)]) - for res_model, res_id, create_uid, public in self._cr.fetchall(): + self._cr.execute('SELECT res_model, res_id, create_uid, public, res_field FROM ir_attachment WHERE id IN %s', [tuple(self.ids)]) + for res_model, res_id, create_uid, public, res_field in self._cr.fetchall(): + if self.env.user.id != SUPERUSER_ID and not self.env.user._is_system() and res_field: + raise AccessError(_("Sorry, you are not allowed to access this document.")) if public and mode == 'read': continue if not (res_model and res_id): @@ -367,14 +369,24 @@ class IrAttachment(models.Model): if not (self.env.user._is_admin() or self.env.user.has_group('base.group_user')): raise AccessError(_("Sorry, you are not allowed to access this document.")) + def _read_group_allowed_fields(self): + return ['type', 'company_id', 'res_id', 'create_date', 'create_uid', 'res_name', 'name', 'mimetype', 'id', 'url', 'datas_fname', 'res_field', 'res_model'] + @api.model def read_group(self, domain, fields, groupby, offset=0, limit=None, orderby=False, lazy=True): """Override read_group to add res_field=False in domain if not present.""" + if not fields: + raise AccessError(_("Sorry, you must provide fields to read on attachments")) + if any('(' in field for field in fields + groupby): + raise AccessError(_("Sorry, the syntax 'name:agg(field)' is not available for attachments")) if not any(item[0] in ('id', 'res_field') for item in domain): domain.insert(0, ('res_field', '=', False)) - return super(IrAttachment, self).read_group(domain, fields, groupby, - offset=offset, limit=limit, - orderby=orderby, lazy=lazy) + groupby = [groupby] if isinstance(groupby, str) else groupby + allowed_fields = self._read_group_allowed_fields() + fields_set = set(field.split(':')[0] for field in fields + groupby) + if not self.env.user._is_system() and (not fields or fields_set.difference(allowed_fields)): + raise AccessError(_("Sorry, you are not allowed to access these fields on attachments.")) + return super().read_group(domain, fields, groupby, offset=offset, limit=limit, orderby=orderby, lazy=lazy) @api.model def _search(self, args, offset=0, limit=None, order=None, count=False, access_rights_uid=None): @@ -386,7 +398,7 @@ class IrAttachment(models.Model): ids = super(IrAttachment, self)._search(args, offset=offset, limit=limit, order=order, count=False, access_rights_uid=access_rights_uid) - if self._uid == SUPERUSER_ID: + if self.env.user._is_system(): # rules do not apply for the superuser return len(ids) if count else ids @@ -404,12 +416,19 @@ class IrAttachment(models.Model): # Use pure SQL rather than read() as it is about 50% faster for large dbs (100k+ docs), # and the permissions are checked in super() and below anyway. model_attachments = defaultdict(lambda: defaultdict(set)) # {res_model: {res_id: set(ids)}} - self._cr.execute("""SELECT id, res_model, res_id, public FROM ir_attachment WHERE id IN %s""", [tuple(ids)]) + binary_fields_attachments = set() + self._cr.execute("""SELECT id, res_model, res_id, public, res_field FROM ir_attachment WHERE id IN %s""", [tuple(ids)]) for row in self._cr.dictfetchall(): if not row['res_model'] or row['public']: continue # model_attachments = {res_model: {res_id: set(ids)}} model_attachments[row['res_model']][row['res_id']].add(row['id']) + # Should not retrieve binary fields attachments + if row['res_field']: + binary_fields_attachments.add(row['id']) + + if binary_fields_attachments: + ids.difference_update(binary_fields_attachments) # To avoid multiple queries for each attachment found, checks are # performed in batch as much as possible. diff --git a/odoo/addons/base/models/ir_http.py b/odoo/addons/base/models/ir_http.py index 293269d61d1..ccd7518005f 100644 --- a/odoo/addons/base/models/ir_http.py +++ b/odoo/addons/base/models/ir_http.py @@ -260,11 +260,19 @@ class IrHttp(models.AbstractModel): if not record or not record.exists() or field not in record: return None, 404 - # access token grant access - if model == 'ir.attachment' and access_token: - record = record.sudo() - if not consteq(record.access_token or '', access_token): + if model == 'ir.attachment': + record_sudo = record.sudo() + if access_token and not consteq(record_sudo.access_token or '', access_token): return None, 403 + elif (access_token and consteq(record_sudo.access_token or '', access_token)): + record = record_sudo + elif record_sudo.public: + record = record_sudo + elif self.env.user.has_group('base.group_portal'): + # Check the read access on the record linked to the attachment + # eg: Allow to download an attachment on a task from /my/task/task_id + record.check('read') + record = record_sudo # check read access try: @@ -316,7 +324,7 @@ class IrHttp(models.AbstractModel): field_def = record._fields[field] if field_def.type == 'binary' and field_def.attachment: - field_attachment = self.env['ir.attachment'].search_read(domain=[('res_model', '=', model), ('res_id', '=', record.id), ('res_field', '=', field)], fields=['datas', 'mimetype', 'checksum'], limit=1) + field_attachment = self.env['ir.attachment'].sudo().search_read(domain=[('res_model', '=', model), ('res_id', '=', record.id), ('res_field', '=', field)], fields=['datas', 'mimetype', 'checksum'], limit=1) if field_attachment: mimetype = field_attachment[0]['mimetype'] content = field_attachment[0]['datas'] diff --git a/odoo/addons/base/security/ir.model.access.csv b/odoo/addons/base/security/ir.model.access.csv index 2c5edba27c5..445caa026e3 100644 --- a/odoo/addons/base/security/ir.model.access.csv +++ b/odoo/addons/base/security/ir.model.access.csv @@ -1,7 +1,6 @@ "id","name","model_id:id","group_id:id","perm_read","perm_write","perm_create","perm_unlink" -"access_ir_attachment_all","ir_attachment all","model_ir_attachment",,1,0,0,0 "access_ir_attachment_group_user","ir_attachment group_user","model_ir_attachment","group_user",1,1,1,1 -"access_ir_attachment_portal","ir.attachment.portal","model_ir_attachment","group_portal",1,0,1,0 +"access_ir_attachment_group_portal_public","ir_attachment group_portal_public","model_ir_attachment",,0,0,0,0 "access_ir_cron_group_cron","ir_cron group_cron","model_ir_cron","group_system",1,1,1,1 "access_ir_exports_group_system","ir_exports group_system","model_ir_exports","base.group_user",1,1,1,1 "access_ir_exports_line_group_system","ir_exports_line group_system","model_ir_exports_line","base.group_user",1,1,1,1 diff --git a/odoo/models.py b/odoo/models.py index 6a09ce54453..b1cb70599c9 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -3189,7 +3189,7 @@ Fields: cr.execute(query, (self._name, sub_ids)) attachments = Attachment.browse([row[0] for row in cr.fetchall()]) if attachments: - attachments.unlink() + attachments.sudo().unlink() if any(field.translate for field in self._fields.values()): # For the same reason, remove the relevant records in ir_translation