From 0569476adc8f5adab8f7a3b41fb8aac5fb5434e0 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 22 Apr 2022 06:46:10 +0000 Subject: [PATCH] [FIX] calendar: read([]) as a non-admin would not return all fields Because private fields would automatically add a bunch of fields to the user's request (in order to do their own post-treatment), unlike normal behavior `read([])` would be completed to `read(['privacy', 'user_id'])` and would fail to trigger the "select all the fields" behavior of `BaseModel.read`. Move the privacy management to a `_read` override instead. Also update the code to be more linear and straightforward, without a bunch of intermediate helper function. This is also a small performance optimisation, although apparently much more minor for 14 (best case of about 5%) than on 15.0 (where it seemed to reach 30). The gain is mostly for private events being read for non-participants: because the non-public fields get neutered before computation (rather than after), fields are computed on a neutered basis and thus largely do nothing. This is especially salient with computations relying on relations (in this case `attendee_status`), as from an empty starting point they essentially do not do anything. This leads to the effort (and cost) of private events being read by non-participants to be about the same as the cost of public events, whereas before the change there is a visible overhead. The signal is quite noisy though. NOTE: backported the addition of `privacy` to the public fields from 2f4a91c0ce5ab36fcb80004a45e89c9f9b02be98 as that is better behavior and makes the tests more logical (and easier to forward port probably) OPW-2831113 closes odoo/odoo#89936 X-original-commit: b0c6f2969596fb715e0a6c62865b5c7dbc0bcc78 Related: odoo/enterprise#26699 Signed-off-by: Xavier Morel (xmo) --- addons/calendar/models/calendar_event.py | 78 ++++++++------------- addons/calendar/tests/test_access_rights.py | 70 ++++++++++++------ 2 files changed, 78 insertions(+), 70 deletions(-) diff --git a/addons/calendar/models/calendar_event.py b/addons/calendar/models/calendar_event.py index 93d955ef85a..473094ab60d 100644 --- a/addons/calendar/models/calendar_event.py +++ b/addons/calendar/models/calendar_event.py @@ -1,10 +1,12 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +import logging +import math from collections import defaultdict from datetime import timedelta -import math -import logging +from itertools import repeat + import pytz import uuid @@ -461,56 +463,34 @@ class Meeting(models.Model): return events - def read(self, fields=None, load='_classic_read'): - def hide(field, value): - """ - :param field: field name - :param value: field value - :return: obfuscated field value - """ - if field in {'name', 'display_name'}: - return _('Busy') - return [] if isinstance(value, list) else False + def _read(self, fields): + if self.env.is_system(): + super()._read(fields) + return - def split_privacy(events): - """ - :param events: list of event values (dict) - :return: tuple(private events, public events) - """ - private = [event for event in events if event.get('privacy') == 'private'] - public = [event for event in events if event.get('privacy') != 'private'] - return private, public + fields = set(fields) + private_fields = fields - self._get_public_fields() + if not private_fields: + super()._read(fields) + return - def split_visibility(events): - """ - :param events: list of event values (dict) - :return: tuple(my events, other events) - """ - current_partner_id = self.env.user.partner_id.id - visible_events = [] - other_events = [] - for event in events: - if (event.get('user_id') and event.get('user_id')[0] == self.env.uid) or current_partner_id in event.get('partner_ids'): - visible_events.append(event) - else: - other_events.append(event) - return visible_events, other_events + private_fields.add('partner_ids') + super()._read(fields | {'privacy', 'user_id', 'partner_ids'}) + current_partner_id = self.env.user.partner_id + others_private_events = self.filtered( + lambda e: e.privacy == 'private' \ + and e.user_id != self.env.user \ + and current_partner_id not in e.partner_ids + ) + if not others_private_events: + return - def obfuscated(events): - """ - :param events: list of event values (dict) - :return: events with private field values obfuscated - """ - public_fields = self._get_public_fields() - return [{ - field: hide(field, value) if field not in public_fields else value - for field, value in event.items() - } for event in events] - - events = super().read(fields=fields + ['privacy', 'user_id', 'partner_ids'], load=load) - private_events, public_events = split_privacy(events) - my_private_events, others_private_events = split_visibility(private_events) - return public_events + my_private_events + obfuscated(others_private_events) + for field_name in private_fields: + field = self._fields[field_name] + replacement = field.convert_to_cache( + _('Busy') if field_name == 'name' else False, + others_private_events) + self.env.cache.update(others_private_events, field, repeat(replacement)) def write(self, values): detached_events = self.env['calendar.event'] diff --git a/addons/calendar/tests/test_access_rights.py b/addons/calendar/tests/test_access_rights.py index bb37b2e55a6..acbf9a30c30 100644 --- a/addons/calendar/tests/test_access_rights.py +++ b/addons/calendar/tests/test_access_rights.py @@ -5,54 +5,82 @@ from datetime import datetime from odoo.tests.common import TransactionCase, new_test_user from odoo.exceptions import AccessError +from odoo.tools import mute_logger class TestAccessRights(TransactionCase): @classmethod + @mute_logger('odoo.tests', 'odoo.addons.auth_signup.models.res_users') def setUpClass(cls): super().setUpClass() cls.john = new_test_user(cls.env, login='john', groups='base.group_user') cls.raoul = new_test_user(cls.env, login='raoul', groups='base.group_user') + cls.george = new_test_user(cls.env, login='george', groups='base.group_user') cls.portal = new_test_user(cls.env, login='pot', groups='base.group_portal') def create_event(self, user, **values): - return self.env['calendar.event'].with_user(user).create(dict({ + return self.env['calendar.event'].with_user(user).create({ 'name': 'Event', 'start': datetime(2020, 2, 2, 8, 0), 'stop': datetime(2020, 2, 2, 18, 0), 'user_id': user.id, - }, **values)) + 'partner_ids': [(4, self.george.partner_id.id, 0)], + **values + }) def read_event(self, user, events, field): data = events.with_user(user).read([field]) if len(events) == 1: return data[0][field] - mapped_data = {record['id']: record for record in data} - # Keep the same order - return [mapped_data[eid][field] for eid in events.ids] + return [r[field] for r in data] - def test_private_read_name(self): + # don't spam logs with ACL failures from portal + @mute_logger('odoo.addons.base.models.ir_rule') + def test_privacy(self): event = self.create_event( self.john, privacy='private', name='my private event', + location='in the Sky' ) - self.assertEqual(self.read_event(self.john, event, 'name'), 'my private event', "Owner should be able to read the event") - self.assertEqual(self.read_event(self.raoul, event, 'name'), 'Busy', "Private value should be obfuscated") - with self.assertRaises(AccessError): - self.read_event(self.portal, event, 'name') - - def test_private_other_field(self): - event = self.create_event( - self.john, - privacy='private', - location='in the Sky', - ) - self.assertEqual(self.read_event(self.john, event, 'location'), 'in the Sky', "Owner should be able to read the event") - self.assertEqual(self.read_event(self.raoul, event, 'location'), False, "Private value should be obfuscated") - with self.assertRaises(AccessError): - self.read_event(self.portal, event, 'location') + for user, field, expect, error in [ + # public field, any employee can read + (self.john, 'privacy', 'private', None), + (self.george, 'privacy', 'private', None), + (self.raoul, 'privacy', 'private', None), + (self.portal, 'privacy', None, AccessError), + # substituted private field, only owner and invitees can read, other + # employees get substitution + (self.john, 'name', 'my private event', None), + (self.george, 'name', 'my private event', None), + (self.raoul, 'name', 'Busy', None), + (self.portal, 'name', None, AccessError), + # computed from private field + (self.john, 'display_name', 'my private event', None), + (self.george, 'display_name', 'my private event', None), + (self.raoul, 'display_name', 'Busy', None), + (self.portal, 'display_name', None, AccessError), + # non-substituted private field, only owner and invitees can read, + # other employees get an empty field + (self.john, 'location', 'in the Sky', None), + (self.george, 'location', 'in the Sky', None), + (self.raoul, 'location', False, None), + (self.portal, 'location', None, AccessError), + # non-substituted sequence field + (self.john, 'partner_ids', self.george.partner_id, None), + (self.george, 'partner_ids', self.george.partner_id, None), + (self.raoul, 'partner_ids', self.env['res.partner'], None), + (self.portal, 'partner_ids', None, AccessError), + ]: + event.invalidate_cache() + with self.subTest("private read", user=user.display_name, field=field, error=error): + e = event.with_user(user) + if error: + with self.assertRaises(error): + _ = e[field] + else: + self.assertEqual(e[field], expect) def test_private_and_public(self): private = self.create_event(