Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMP] core: more details in debug mode access error messages #30144

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
10 participants
@xmo-odoo
Copy link
Collaborator

commented Jan 11, 2019

Access rights issues are currently hard to diagnose (especially from
ir.rule) due to how little details they provide.

This makes a pass over various sources of access errors and:

  • improve/clarify the base message
  • provide an alternative message with more details in debug mode
  • ir.model.access
  • ir.rule
  • related fields
  • fields with groups
  • close #28451 (provide both _name and _description in access error messages)

Task 1886746

@robodoo robodoo added the seen 🙂 label Jan 11, 2019

@xmo-odoo

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 11, 2019

@robodoo rebase-ff

@xmo-odoo

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 11, 2019

@robodoo rebase-merge

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

Merge method set to rebase and fast-forward

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

Merge method set to rebase and merge, using the PR as merge commit message

@fmdl

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

@xmo-odoo I have work on the same think about the description on AccesError message, please find my PR : #28451 (and if need you can close).

@gustavovalverde

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

I know I'm asking the hardest question but, can this be backported? :)

@xmo-odoo

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 11, 2019

@fmdl I guess I'll close #28451 as this one does more, but I didn't really think about the _name v _description bit, so thanks for the pointer: I'll have to fix up some of my editions to put the same _description (_name) in the messages.

@gustavovalverde technically probably, I just work on master by default. You want to talk to @rim-odoo as they're the "product owner" and may themselves want it on older version (since it's for support reasons).

@rim-odoo

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

@xmo-odoo If your branch passes the "stable rules check", you would make me very happy by backporting this to 10.0 🙏

@fmdl

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

@xmo-odoo thanks

@xmo-odoo xmo-odoo force-pushed the odoo-dev:master-access-feedback-xmo branch 2 times, most recently Jan 14, 2019

@odony
Copy link
Contributor

left a comment

Looks nice (did not test yet, though), thanks! I mostly have comments about translations/placeholders at this point, and the possibly cardinality/time taken to process and log the blocked records.

As for backporting, given that this is an improvement, not a bugfix, it's not an ideal candidate.
Are we sure that the risk/benefit ratio is good enough?

  • it changes the RR API slightly (return value of _compute_domain, if I'm not mistaken?)
  • it changes translated terms, so will cause some lost translations for some people (updating them requires translators to fix + forcing -u, even for the non-debug versions)
  • the names of the RR are not currently very meaningful, so it could look confusing to end users who use the dev mode - could be worth some work in master, not sure?
  • it cannot be considered risk-free: processing the failed records can become costly if it happens too often in some pathological case (imagine a case where our business code catches the expected error). Logging/displaying the error could also cause indirect issues (think of the cardinality of the records), etc.
    So I would at least start by merging it in master only, and wait until that part is stabilized and tested in the next freeze (and then some more), before considering backporting it.

@nim-odoo and @mart-e: what do you guys think?

Show resolved Hide resolved odoo/addons/base/models/ir_rule.py Outdated
Show resolved Hide resolved odoo/addons/base/models/ir_rule.py Outdated
Show resolved Hide resolved odoo/addons/base/models/ir_rule.py Outdated
Show resolved Hide resolved odoo/addons/base/models/ir_model.py Outdated
Show resolved Hide resolved odoo/models.py Outdated
Show resolved Hide resolved odoo/models.py Outdated
Show resolved Hide resolved odoo/models.py Outdated
Show resolved Hide resolved odoo/fields.py Outdated
Show resolved Hide resolved odoo/addons/base/models/ir_rule.py Outdated
Show resolved Hide resolved odoo/addons/base/models/ir_rule.py Outdated
@mart-e
Copy link
Contributor

left a comment

I also don't think it is a good idea in stable, too risky.
Also you are making unstable changes in test modules. While these modules have no reason to be on a production database, it is not something you can exclude...

Show resolved Hide resolved odoo/addons/base/models/ir_model.py Outdated

@robodoo robodoo added the CI 🤖 label Jan 14, 2019

@Yenthe666

This comment has been minimized.

Copy link
Collaborator

commented Jan 14, 2019

Thank god, this will be a great improvement, even if it lands in master.
Perhaps we could do a minor backport for the most important functionalities (to atleast 12.0) though?

@xmo-odoo

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 15, 2019

  • it changes the RR API slightly (return value of _compute_domain, if I'm not mistaken?)

True, I'll change it back to returning an empty list, probably forgot to do so in the early return.

  • it changes translated terms, so will cause some lost translations for some people (updating them requires translators to fix + forcing -u, even for the non-debug versions)

Yeah that's pretty inevitable given the entire point is trying to improve them.

  • the names of the RR are not currently very meaningful, so it could look confusing to end users who use the dev mode - could be worth some work in master, not sure?

That was one of the concerns / future work being mentioned (clarify names to make clearer what rules are for), but I guess short term the assumption was that it at least provided a pointer. The same issue exists (to a lower extent) with model descriptions.

  • it cannot be considered risk-free: processing the failed records can become costly if it happens too often in some pathological case (imagine a case where our business code catches the expected error). Logging/displaying the error could also cause indirect issues (think of the cardinality of the records), etc.

Sure that's a fair point. A backport option would be to simply make it clear that it's a record rules issue, and optionally list all rules (without the non-trivial filtering bit).

@xmo-odoo

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 15, 2019

@odony found out about the _compute_domain thing: it could already return None or a list, and would return None for the superuser. So I figured returning None whether superuser or no relevant rules wouldn't make a difference: the None result had to be handled by the caller either way. Could change it to always return an empty list, should make no difference.

@xmo-odoo xmo-odoo force-pushed the odoo-dev:master-access-feedback-xmo branch Jan 15, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 15, 2019

@rim-odoo

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

@odony I'm arriving late in the chat, but regarding the value/ratio risk, I can explain you the value: with that kind of information available, most of the access rights errors are to be debugged by people with functional knowledge and won't require debugging skills anymore.

If it lands in master, I'll be super happy anyway :)

@Yenthe666

This comment has been minimized.

Copy link
Collaborator

commented Jan 16, 2019

@rim-odoo I can actually imagine that it would help the support desk of Odoo S.A. itself a lot, wouldn't it?

@rim-odoo

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

@Yenthe666

Show resolved Hide resolved odoo/addons/base/models/ir_model.py Outdated
'document_kind': description,
'document_model': model,
'rules_list': '\n'.join('- %s' % rule.name for rule in rules),
'multi_company_warning': ('\n' + _('Note: this might be a multi-company issue.') + '\n') if any(

This comment has been minimized.

Copy link
@rim-odoo

rim-odoo Feb 4, 2019

Contributor

👍 😍

@xmo-odoo xmo-odoo force-pushed the odoo-dev:master-access-feedback-xmo branch Feb 5, 2019

@robodoo robodoo removed the CI 🤖 label Feb 5, 2019

xmo-odoo added some commits Jan 10, 2019

[FIX] core, base: rules detail on access error
In debug mode, if an access fails due to access rules, try to provide
a clearer error message & include a list of the rules which fail for
the record-set.

Note: for read() operations it'll be a bit misleading as failed
records will get a list of all the rules failing for the recordset,
only some of which might apply to that specific record.
[IMP] access errors through related fields
When an access error bubbles through a related field (aka the user
doesn't have access to the delegate or the delegate's field), append
the "intermediate step" so that it's easier to understand e.g. that
the access error to a partner really comes from accessing a user or
somesuch.

@xmo-odoo xmo-odoo force-pushed the odoo-dev:master-access-feedback-xmo branch to 3c4ae46 Feb 6, 2019

@xmo-odoo

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 6, 2019

robodoo r+

robodoo added a commit that referenced this pull request Feb 6, 2019

[IMP] core: more details in debug mode access error messages
Access rights issues are currently hard to diagnose (especially from
ir.rule) due to how little details they provide.

This makes a pass over various sources of access errors and:
* improve/clarify the base message
* provide an alternative message with more details in debug mode

- [x] ir.model.access
- [x] ir.rule
- [x] related fields
- [x] fields with groups
- [x] close #28451 (provide both _name and _description in access error messages)

Task [1886746](https://www.odoo.com/web?debug#id=1886746&model=project.task&view_type=form&menu_id=4720)

closes #30144
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.