diff --git a/CHANGELOG b/CHANGELOG index a454fef974..e72a778170 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,4 @@ +* Display ids and rule names on access error * Move field definition from Model to Field * Use number of verbose flag as log level * Add coroutine concurrency option diff --git a/trytond/ir/message.xml b/trytond/ir/message.xml index a2a25a17f4..d8447712bf 100644 --- a/trytond/ir/message.xml +++ b/trytond/ir/message.xml @@ -84,11 +84,27 @@ this repository contains the full copyright notices and license terms. --> You are not allowed to access "%(model)s.%(field)s". + + You are not allowed to create records of "%(model)s" because they fail on at least one of these rules: +%(rules)s + + + You are not allowed to read records "%(ids)s" of "%(model)s" because of at least one of these rules: +%(rules)s + - You are trying to read records of "%(model)s" that don't exist anymore. + You are trying to read records "%(ids)s" of "%(model)s" that don't exist anymore. + + + You are not allowed to write to records "%(ids)s" of "%(model)s" because of at least one of these rules: +%(rules)s - You are trying to write to records of "%(model)s" that don't exist anymore. + You are trying to write to records "%(ids)s" of "%(model)s" that don't exist anymore. + + + You are not allowed to delete records "%(ids)s" of "%(model)s" because of at lease one of those rules: +%(rules)s Invalid domain in schema "%(schema)s". diff --git a/trytond/ir/rule.py b/trytond/ir/rule.py index c7d2580045..714c5aead4 100644 --- a/trytond/ir/rule.py +++ b/trytond/ir/rule.py @@ -1,5 +1,7 @@ # This file is part of Tryton. The COPYRIGHT file at the top level of # this repository contains the full copyright notices and license terms. +from collections import defaultdict + from trytond.i18n import gettext from trytond.model.exceptions import ValidationError from ..model import ModelView, ModelSQL, fields, EvalEnvironment, Check @@ -20,7 +22,9 @@ class DomainError(ValidationError): class RuleGroup(ModelSQL, ModelView): "Rule group" __name__ = 'ir.rule.group' - name = fields.Char('Name', select=True) + name = fields.Char( + "Name", select=True, translate=True, required=True, + help="Displayed to users when access error is raised for this rule.") model = fields.Many2One('ir.model', 'Model', select=True, required=True, ondelete='CASCADE') global_p = fields.Boolean('Global', select=True, @@ -104,6 +108,8 @@ class Rule(ModelSQL, ModelView): '- "user" as the current user') _domain_get_cache = Cache('ir_rule.domain_get', context=False) + modes = {'read', 'write', 'create', 'delete'} + @classmethod def validate(cls, rules): super(Rule, cls).validate(rules) @@ -144,35 +150,29 @@ class Rule(ModelSQL, ModelView): return (Transaction().user, Transaction().context.get('_datetime')) @classmethod - def domain_get(cls, model_name, mode='read'): - assert mode in ['read', 'write', 'create', 'delete'], \ - 'Invalid domain mode for security' - - # root user above constraint - if Transaction().user == 0: - if not Transaction().context.get('user'): - return - with Transaction().set_user(Transaction().context['user']): - return cls.domain_get(model_name, mode=mode) - - key = (model_name, mode) + cls._get_cache_key() - domain = cls._domain_get_cache.get(key, False) - if domain is not False: - return domain - + def get(cls, model_name, mode='read'): + "Return dictionary of non-global and global rules" pool = Pool() RuleGroup = pool.get('ir.rule.group') Model = pool.get('ir.model') RuleGroup_Group = pool.get('ir.rule.group-res.group') User_Group = pool.get('res.user-res.group') - - cursor = Transaction().connection.cursor() rule_table = cls.__table__() rule_group = RuleGroup.__table__() rule_group_group = RuleGroup_Group.__table__() user_group = User_Group.__table__() model = Model.__table__() - user_id = Transaction().user + transaction = Transaction() + + assert mode in cls.modes + + cursor = transaction.connection.cursor() + user_id = transaction.user + # root user above constraint + if user_id == 0: + user_id = transaction.context.get('user') + if not user_id: + return {}, {} cursor.execute(*rule_table.join(rule_group, condition=rule_group.id == rule_table.rule_group ).join(model, @@ -191,26 +191,7 @@ class Rule(ModelSQL, ModelView): | (rule_group.default_p == True) | (rule_group.global_p == True) ))) - ids = [x[0] for x in cursor.fetchall()] - if not ids: - cls._domain_get_cache.set(key, None) - return - clause = {} - clause_global = {} - ctx = cls._get_context() - # Use root user without context to prevent recursion - with Transaction().set_user(0), \ - Transaction().set_context(user=0): - for rule in cls.browse(ids): - assert rule.domain, ('Rule domain empty,' - 'check if migration was done') - dom = PYSONDecoder(ctx).decode(rule.domain) - if rule.rule_group.global_p: - clause_global.setdefault(rule.rule_group.id, ['OR']) - clause_global[rule.rule_group.id].append(dom) - else: - clause.setdefault(rule.rule_group.id, ['OR']) - clause[rule.rule_group.id].append(dom) + ids = [x for x, in cursor] # Test if there is no rule_group that have no rule cursor.execute(*rule_group.join(model, @@ -222,10 +203,47 @@ class Rule(ModelSQL, ModelView): condition=rule_group_group.group == user_group.group ).select(rule_group_group.rule_group, where=user_group.user == user_id)))) - fetchone = cursor.fetchone() - if fetchone: - group_id = fetchone[0] - clause[group_id] = [] + no_rules = cursor.fetchone() + + clause = defaultdict(lambda: ['OR']) + clause_global = defaultdict(lambda: ['OR']) + decoder = PYSONDecoder(cls._get_context()) + # Use root user without context to prevent recursion + with transaction.set_user(0), transaction.set_context(user=0): + for rule in cls.browse(ids): + assert rule.domain, ('Rule domain empty,' + 'check if migration was done') + dom = decoder.decode(rule.domain) + if rule.rule_group.global_p: + clause_global[rule.rule_group].append(dom) + else: + clause[rule.rule_group].append(dom) + + if no_rules: + group_id = no_rules[0] + clause[cls(group_id)] = [] + + return clause, clause_global + + @classmethod + def domain_get(cls, model_name, mode='read'): + transaction = Transaction() + # root user above constraint + if transaction.user == 0: + if not transaction.context.get('user'): + return + with transaction.set_user(Transaction().context['user']): + return cls.domain_get(model_name, mode=mode) + + assert mode in cls.modes + + key = (model_name, mode) + cls._get_cache_key() + domain = cls._domain_get_cache.get(key, False) + if domain is not False: + return domain + + clause, clause_global = cls.get(model_name, mode=mode) + clause = list(clause.values()) if clause: clause.insert(0, 'OR') diff --git a/trytond/model/modelsql.py b/trytond/model/modelsql.py index ac0056c138..04680053af 100644 --- a/trytond/model/modelsql.py +++ b/trytond/model/modelsql.py @@ -748,23 +748,10 @@ class ModelSQL(ModelStorage): order_by=history_order, limit=history_limit)) fetchall = list(cursor_dict(cursor)) if not len(fetchall) == len({}.fromkeys(sub_ids)): - if domain: - where = red_sql - if history_clause: - where &= history_clause - where &= dom_exp - cursor.execute(*from_.select(table.id, where=where, - order_by=history_order, limit=history_limit)) - rowcount = cursor.rowcount - if rowcount == -1 or rowcount is None: - rowcount = len(cursor.fetchall()) - if rowcount == len({}.fromkeys(sub_ids)): - raise AccessError( - gettext('ir.msg_access_rule_error', - model=cls.__name__)) - raise AccessError( - gettext('ir.msg_read_error', - model=cls.__name__)) + cls.__check_domain_rule( + ids, 'read', nodomain='ir.msg_read_error') + cls.__check_domain_rule(ids, 'read') + raise RuntimeError("Undetected access error") result.extend(fetchall) else: result = [{'id': x} for x in ids] @@ -1154,32 +1141,85 @@ class ModelSQL(ModelStorage): def __check_domain_rule(cls, ids, mode, nodomain=None): pool = Pool() Rule = pool.get('ir.rule') + Model = pool.get('ir.model') table = cls.__table__() - cursor = Transaction().connection.cursor() + transaction = Transaction() + in_max = transaction.database.IN_MAX + history_clause = None + limit = None + if (mode == 'read' + and cls._history + and transaction.context.get('_datetime') + and not callable(cls.table_query)): + in_max = 1 + table = cls.__table_history__() + column = Coalesce(table.write_date, table.create_date) + history_clause = (column <= Transaction().context['_datetime']) + limit = 1 + cursor = transaction.connection.cursor() + assert mode in Rule.modes - domain = Rule.domain_get(cls.__name__, mode=mode) - tables = {None: (table, None)} - if domain or nodomain: + def test_domain(ids, domain): + result = [] + tables = {None: (table, None)} if domain: tables, dom_exp = cls.search_domain( domain, active_test=False, tables=tables) from_ = convert_from(None, tables) - for sub_ids in grouped_slice(ids): - sub_ids = list(set(sub_ids)) + for sub_ids in grouped_slice(ids, in_max): + sub_ids = set(sub_ids) where = reduce_ids(table.id, sub_ids) + if history_clause: + where &= history_clause if domain: where &= dom_exp - cursor.execute(*from_.select(table.id, where=where)) + cursor.execute( + *from_.select(table.id, where=where, limit=limit)) rowcount = cursor.rowcount if rowcount == -1 or rowcount is None: rowcount = len(cursor.fetchall()) if rowcount != len(sub_ids): - if domain: - msg = 'ir.msg_access_rule_error' - else: - msg = nodomain - raise AccessError( - gettext(msg, model=cls.__name__)) + cursor.execute( + *from_.select(table.id, where=where, limit=limit)) + result.extend( + sub_ids.difference([x for x, in cursor])) + return result + + domain = Rule.domain_get(cls.__name__, mode=mode) + if not domain and not nodomain: + return + wrong_ids = test_domain(ids, domain) + if wrong_ids: + model = cls.__name__ + if Model: + models = Model.search([ + ('model', '=', cls.__name__), + ], limit=1) + if models: + model, = models + model = model.name + ids = ', '.join(map(str, ids[:5])) + if len(wrong_ids) > 5: + ids += '...' + if domain: + rules = [] + clause, clause_global = Rule.get(cls.__name__, mode=mode) + if clause: + dom = list(clause.values()) + dom.insert(0, 'OR') + if test_domain(wrong_ids, dom): + rules.extend(clause.keys()) + + for rule, dom in clause_global.items(): + if test_domain(wrong_ids, dom): + rules.append(rule) + + msg = gettext( + 'ir.msg_%s_rule_error' % mode, ids=ids, model=model, + rules='\n'.join(r.name for r in rules)) + else: + msg = gettext(nodomain, ids=ids, model=model) + raise AccessError(msg) @classmethod def search(cls, domain, offset=0, limit=None, order=None, count=False, diff --git a/trytond/res/ir.xml b/trytond/res/ir.xml index 183c328377..8d3adae00c 100644 --- a/trytond/res/ir.xml +++ b/trytond/res/ir.xml @@ -447,6 +447,7 @@ this repository contains the full copyright notices and license terms. --> + User in groups @@ -461,6 +462,7 @@ this repository contains the full copyright notices and license terms. --> + User in groups @@ -475,6 +477,7 @@ this repository contains the full copyright notices and license terms. --> + User in groups @@ -490,6 +493,7 @@ this repository contains the full copyright notices and license terms. --> + User in groups @@ -505,6 +509,7 @@ this repository contains the full copyright notices and license terms. --> + User in groups @@ -520,6 +525,7 @@ this repository contains the full copyright notices and license terms. --> + User in groups @@ -535,6 +541,7 @@ this repository contains the full copyright notices and license terms. --> + User in groups @@ -768,6 +775,7 @@ this repository contains the full copyright notices and license terms. --> + User in groups @@ -780,6 +788,7 @@ this repository contains the full copyright notices and license terms. --> + User in groups @@ -831,6 +840,7 @@ this repository contains the full copyright notices and license terms. --> + Own view search @@ -848,6 +858,7 @@ this repository contains the full copyright notices and license terms. --> + Any view search @@ -864,6 +875,7 @@ this repository contains the full copyright notices and license terms. --> + Own session @@ -876,6 +888,7 @@ this repository contains the full copyright notices and license terms. --> + Own session @@ -888,6 +901,7 @@ this repository contains the full copyright notices and license terms. --> + Own favorite diff --git a/trytond/res/user.xml b/trytond/res/user.xml index 751b934a87..e378c827ef 100644 --- a/trytond/res/user.xml +++ b/trytond/res/user.xml @@ -106,6 +106,7 @@ this repository contains the full copyright notices and license terms. --> + Own warning @@ -128,6 +129,7 @@ this repository contains the full copyright notices and license terms. --> + Own user application @@ -141,6 +143,7 @@ this repository contains the full copyright notices and license terms. --> + Any user application diff --git a/trytond/tests/test_rule.py b/trytond/tests/test_rule.py index 4829531ddf..fcd525f590 100644 --- a/trytond/tests/test_rule.py +++ b/trytond/tests/test_rule.py @@ -33,6 +33,7 @@ class ModelRuleTestCase(unittest.TestCase): model, = Model.search([('model', '=', 'test.rule')]) rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", 'model': model.id, 'global_p': True, 'perm_read': False, @@ -57,6 +58,7 @@ class ModelRuleTestCase(unittest.TestCase): model, = Model.search([('model', '=', 'test.rule')]) rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", 'model': model.id, 'global_p': True, 'perm_read': False, @@ -69,7 +71,34 @@ class ModelRuleTestCase(unittest.TestCase): }])], }]) - with self.assertRaises(AccessError): + with self.assertRaisesRegex(AccessError, "Field different from foo"): + test, = TestRule.create([{'field': 'foo'}]) + + @with_transaction() + def test_perm_create_with_default_rule_fail(self): + "Test create with default rule fail" + pool = Pool() + TestRule = pool.get('test.rule') + RuleGroup = pool.get('ir.rule.group') + Model = pool.get('ir.model') + + model, = Model.search([('model', '=', 'test.rule')]) + rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", + 'model': model.id, + 'global_p': False, + 'default_p': True, + 'perm_read': False, + 'perm_create': True, + 'perm_write': False, + 'perm_delete': False, + 'rules': [('create', [{ + 'domain': json.dumps( + [('field', '!=', 'foo')]), + }])], + }]) + + with self.assertRaisesRegex(AccessError, "Field different from foo"): test, = TestRule.create([{'field': 'foo'}]) @with_transaction() @@ -92,6 +121,7 @@ class ModelRuleTestCase(unittest.TestCase): model, = Model.search([('model', '=', 'test.rule')]) rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", 'model': model.id, 'global_p': True, 'perm_read': False, @@ -117,6 +147,7 @@ class ModelRuleTestCase(unittest.TestCase): model, = Model.search([('model', '=', 'test.rule')]) rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", 'model': model.id, 'global_p': True, 'perm_read': False, @@ -130,7 +161,7 @@ class ModelRuleTestCase(unittest.TestCase): }]) test, = TestRule.create([{'field': 'foo'}]) - with self.assertRaises(AccessError): + with self.assertRaisesRegex(AccessError, "Field different from foo"): TestRule.write([test], {'field': 'bar'}) @with_transaction() @@ -143,6 +174,7 @@ class ModelRuleTestCase(unittest.TestCase): model, = Model.search([('model', '=', 'test.rule')]) rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", 'model': model.id, 'global_p': True, 'perm_read': False, @@ -156,7 +188,7 @@ class ModelRuleTestCase(unittest.TestCase): }]) test, = TestRule.create([{'field': 'bar'}]) - with self.assertRaises(AccessError): + with self.assertRaisesRegex(AccessError, "Field different from foo"): TestRule.write([test], {'field': 'foo'}) @with_transaction() @@ -179,6 +211,7 @@ class ModelRuleTestCase(unittest.TestCase): model, = Model.search([('model', '=', 'test.rule')]) rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", 'model': model.id, 'global_p': True, 'perm_read': False, @@ -204,6 +237,7 @@ class ModelRuleTestCase(unittest.TestCase): model, = Model.search([('model', '=', 'test.rule')]) rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", 'model': model.id, 'global_p': True, 'perm_read': False, @@ -217,7 +251,7 @@ class ModelRuleTestCase(unittest.TestCase): }]) test, = TestRule.create([{'field': 'foo'}]) - with self.assertRaises(AccessError): + with self.assertRaisesRegex(AccessError, "Field different from foo"): TestRule.delete([test]) @with_transaction() @@ -240,6 +274,7 @@ class ModelRuleTestCase(unittest.TestCase): model, = Model.search([('model', '=', 'test.rule')]) rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", 'model': model.id, 'global_p': True, 'perm_read': True, @@ -265,6 +300,7 @@ class ModelRuleTestCase(unittest.TestCase): model, = Model.search([('model', '=', 'test.rule')]) rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", 'model': model.id, 'global_p': True, 'perm_read': True, @@ -278,7 +314,7 @@ class ModelRuleTestCase(unittest.TestCase): }]) test, = TestRule.create([{'field': 'foo'}]) - with self.assertRaises(AccessError): + with self.assertRaisesRegex(AccessError, "Field different from foo"): TestRule.read([test.id], ['field']) @with_transaction() @@ -301,6 +337,7 @@ class ModelRuleTestCase(unittest.TestCase): model, = Model.search([('model', '=', 'test.rule')]) rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", 'model': model.id, 'global_p': True, 'perm_read': True, @@ -326,6 +363,7 @@ class ModelRuleTestCase(unittest.TestCase): model, = Model.search([('model', '=', 'test.rule')]) rule_group, = RuleGroup.create([{ + 'name': "Field different from foo", 'model': model.id, 'global_p': True, 'perm_read': True,