Draft: [16.0][MIG] #10965 auditlog_security #3029
Conversation
|
@KKamaa @gfcapalbo before doing an in-depth review, an architecture question: Why do you do complex stuff with creating and managing ir.rules on the fly, instead of:
This should make the module much simpler, and I don't see any reason why this shouldn't work or impact performance. |
Rules need to be dynamic, because of this: which is an overkill in my opinion, and also this code part is untested. If we can ignore this condition, then what you suggested is the way to go |
|
even if you want to keep the fields-per-group functionality, you don't need to write specific rules. In that case keep the auditlog.line.access.rule model, and create a computed field allowed_group_ids on auditlog.log{,.line} whose search function looks at those records. My reading was that the field-level functionality is covered by fields_to_exclude_ids from the base addon, that's why I considered that part obsolete |
|
@hbrunn I went ahead and implemented your second suggestion. Let's pretend for the time being that unit tests are not significant at all - their time will come. Some further refinements are needed, though the bulk of the work is there and in tandem with what you said. Please take a look and let me know. |
| |badge1| |badge2| |badge3| |badge4| |badge5| | ||
|
|
||
| This module allows extends auditlog, allowing specific log lines to be viewed only | ||
| by users belonging to specific views, while all other lines are allowed only to |
There was a problem hiding this comment.
| by users belonging to specific views, while all other lines are allowed only to | |
| by users belonging to specific groups, while all other lines are allowed only to |
| ) | ||
| res_id = fields.Integer(compute="_compute_res_id", store=True, index=True) | ||
| allowed_group_ids = fields.Many2many( | ||
| "res.groups", compute="_compute_allowed_group_ids", store=True |
There was a problem hiding this comment.
| "res.groups", compute="_compute_allowed_group_ids", store=True | |
| "res.groups", compute="_compute_allowed_group_ids", search="_search_allowed_group_ids" |
I expect storing things here will come with quite a performance penalty, writing/creating logs and log lines should be as fast as possible. _search_allowed_group_ids would look like
def _search_allowed_group_ids(self, operator, value):
access_rules = self.env['auditlog.line.access.rule'].search(['|', ('group_ids', operator, value), ('group_ids', '=', False)])
domains = []
for access_rule in access_rules:
domain = [('log_id.model_id', '=', access_rule.auditlog_rule_id.id)]
if access_rule.field_ids:
domain.append(('field_id', 'in', access_rule.field_ids.ids))
domains.append(domain)
return osv.expression.OR(domains)and an analogue search function for auditlog.log#allowed_group_ids - this way you offload all the work to the sql server at view time, and have no impact at creation time.
And given that we don't actually care for the value of this field, the compute function can be lambda self: self.update({'allowed_group_ids': False}) I think.
In the same vain, I suggest to replace all computed fields above with nonstored related fields, Odoo is smart enough by now to generate proper joins for that.
There was a problem hiding this comment.
amazing, thanks for the wisdom shared here
There was a problem hiding this comment.
though this _search method should return a domain for searching on res.groups
| @api.constrains("model_id") | ||
| def unique_model(self): | ||
| if self.search_count([("model_id", "=", self.model_id.id)]) > 1: | ||
| raise ValidationError(_("A rule for this model already exists")) |
There was a problem hiding this comment.
the base module does this already
| <field name="inherit_id" ref="auditlog.view_auditlog_log_tree" /> | ||
| <field name="arch" type="xml"> | ||
| <xpath expr="//tree" position="attributes"> | ||
| <attribute name="delete">false</attribute> |
There was a problem hiding this comment.
permissions should take care of that, right?
There was a problem hiding this comment.
Agreed, though I suspect that that's there because no human should be allowed to delete anything from the ui
There was a problem hiding this comment.
but that should be up to administrators fiddling with the permissions, not to some module making this choice i think
| <record id="group_can_view_audit_logs" model="res.groups"> | ||
| <field name="name">View Audit Logs</field> | ||
| </record> | ||
| </odoo> |
There was a problem hiding this comment.
this group is not used anywhere and unnecessary given you'll assign groups in the access rules record I think?
There was a problem hiding this comment.
@ntsirintanis The View Audit Logs group is still being used in the Inuka instances.
Currently, it has around 11 users.
It's also being used in approx. 21 record rules.
This prevents to module from being upgraded.
There was a problem hiding this comment.
the rules from your legacy module should be deleted anyways, as you don't need them any more with this approach. for easier transition, mark them as noupdate in your legacy code, then those records won't be touched on module upgrade
a42e691 to
ed84245
Compare
|
@hbrunn resume with your review please |
|
@thomaspaulb you might want to take a look too. The module looks radically different than before now, and things appear to be fine during my personal tests |
| <field name="perm_unlink" eval="True" /> | ||
| </record> | ||
|
|
||
| <record id="auditlog_log_line_rule_auditlog_user" model="ir.rule"> |
There was a problem hiding this comment.
why restrict auditlog_user's permissions? I'd add this group below to the rule lifting the restrictions imposed on base.group_user.
The auditlog module defines this: https://github.com/OCA/server-tools/blob/16.0/auditlog/security/ir.model.access.csv - so I think after installing auditlog_security, those groups should end up with the same permissions.
| <field name="perm_read" eval="True" /> | ||
| <field name="perm_write" eval="True" /> | ||
| <field name="perm_create" eval="True" /> | ||
| <field name="perm_unlink" eval="True" /> |
There was a problem hiding this comment.
| <field name="perm_read" eval="True" /> | |
| <field name="perm_write" eval="True" /> | |
| <field name="perm_create" eval="True" /> | |
| <field name="perm_unlink" eval="True" /> |
that's the default, I only mentioned those being true because there was perm_read=False before, which was wrong
| <field name="inherit_id" ref="auditlog.view_auditlog_log_tree" /> | ||
| <field name="arch" type="xml"> | ||
| <xpath expr="//tree" position="attributes"> | ||
| <attribute name="delete">false</attribute> |
There was a problem hiding this comment.
but that should be up to administrators fiddling with the permissions, not to some module making this choice i think
| "model_id": self.model_id.id, | ||
| "state": "code", | ||
| "code": code, | ||
| "groups_id": [(6, 0, allowed_groups.ids)], |
There was a problem hiding this comment.
| "groups_id": [(6, 0, allowed_groups.ids)], | |
| "groups_id": [(6, 0, allowed_groups.ids)] if not any(not x.group_ids for x in self.auditlog_line_access_rule_ids), |
if there's an unrestricted access rule and a restricted one, all users should see the action. ir.rules will take care of what they see then
There was a problem hiding this comment.
In that sense then we do not need to assign any groups_id here. We just need to make sure that the user is also an auditlog user
There was a problem hiding this comment.
... which is admin work
| for log in self: | ||
| log.rule_id = self.env["auditlog.rule"].search( | ||
| [("model_id", "=", log.model_id.id)] | ||
| ) |
There was a problem hiding this comment.
do we need this whole file after all?
There was a problem hiding this comment.
Yeah, basically the rule_id computation can also happen in the line model, so I'll see into removing this
| @api.onchange("model_id") | ||
| def onchange_model_id(self): | ||
| # if model changes we must wipe out all field ids | ||
| self.auditlog_line_access_rule_ids.unlink() |
There was a problem hiding this comment.
| self.auditlog_line_access_rule_ids.unlink() | |
| self.auditlog_line_access_rule_ids = False |
onchange methods should work on the memory, not the database
|
|
||
| user_id = fields.Many2one(related="log_id.user_id") | ||
| method = fields.Char(related="log_id.method") | ||
| model_id = fields.Many2one(related="log_id.model_id", store=True) |
There was a problem hiding this comment.
Actually we need it stored for filtering purposes in views etc
|
@hbrunn it appears that we are almost there |
|
|
||
| user_id = fields.Many2one(related="log_id.user_id") | ||
| method = fields.Char(related="log_id.method") | ||
| model_id = fields.Many2one(related="log_id.model_id", store=True, reaonly=True) |
There was a problem hiding this comment.
@ntsirintanis Typo readonly
| <record id="group_can_view_audit_logs" model="res.groups"> | ||
| <field name="name">View Audit Logs</field> | ||
| </record> | ||
| </odoo> |
There was a problem hiding this comment.
@ntsirintanis The View Audit Logs group is still being used in the Inuka instances.
Currently, it has around 11 users.
It's also being used in approx. 21 record rules.
This prevents to module from being upgraded.
3100b44 to
bc76353
Compare
| if self._register_hook(): | ||
| modules.registry.Registry(self.env.cr.dbname).signal_changes() |
There was a problem hiding this comment.
super does this already. my suggestion is to replace line 62 with
modules.registry.Registry(self.env.cr.dbname).signal_changes()|
|
||
| user_id = fields.Many2one(related="log_id.user_id") | ||
| method = fields.Char(related="log_id.method") | ||
| model_id = fields.Many2one(related="log_id.model_id", store=True, reaonly=True) |
There was a problem hiding this comment.
| model_id = fields.Many2one(related="log_id.model_id", store=True, reaonly=True) | |
| model_id = fields.Many2one(related="log_id.model_id") |
I still strongly think those fields shouldn't be stored, and related fields are readonly by default
| method = fields.Char(related="log_id.method") | ||
| model_id = fields.Many2one(related="log_id.model_id", store=True, reaonly=True) | ||
| res_id = fields.Integer(related="log_id.res_id") | ||
| rule_id = fields.Many2one("auditlog.rule", compute="_compute_rule_id", store=True) |
There was a problem hiding this comment.
| rule_id = fields.Many2one("auditlog.rule", compute="_compute_rule_id", store=True) |
see below
| ) | ||
| domains = [] | ||
| for access_rule in access_rules: | ||
| domain = [("rule_id", "=", access_rule.auditlog_rule_id.id)] |
There was a problem hiding this comment.
| domain = [("rule_id", "=", access_rule.auditlog_rule_id.id)] | |
| domain = [("log_id.model_id", "=", access_rule.auditlog_rule_id.model_id.id)] |
|
|
||
| @api.depends("model_id") | ||
| def _compute_rule_id(self): | ||
| for line in self: | ||
| line.rule_id = self.env["auditlog.rule"].search( | ||
| [("model_id", "=", line.model_id.id)] | ||
| ) |
There was a problem hiding this comment.
| @api.depends("model_id") | |
| def _compute_rule_id(self): | |
| for line in self: | |
| line.rule_id = self.env["auditlog.rule"].search( | |
| [("model_id", "=", line.model_id.id)] | |
| ) |
| <record id="group_can_view_audit_logs" model="res.groups"> | ||
| <field name="name">View Audit Logs</field> | ||
| </record> | ||
| </odoo> |
There was a problem hiding this comment.
the rules from your legacy module should be deleted anyways, as you don't need them any more with this approach. for easier transition, mark them as noupdate in your legacy code, then those records won't be touched on module upgrade
No description provided.