[19.0][FIX] base_tier_validation: AccessError in systray when reviewer lacks model ACL#30
Open
bosd wants to merge 1 commit into
Open
Conversation
…odel ACL
The reviewer systray endpoint res.users.review_user_count() iterates
over the user's pending tier reviews, groups them by model, and for
each model runs Model.with_user(user).search(records_domain) to gather
the records the user can still act on.
When the tier definition targets a model the reviewer has no read
access to (e.g. a definition on account.move assigned to a user who
is not in any accounting group), that search raises AccessError -- and
because the systray fetches the count on every page mount, every
refresh of the reviewer's session blew up with:
You are not allowed to access 'Journal Entry' (account.move) records.
Reproduction (runboat):
1. As admin, create a tier definition on account.move with the demo
user as reviewer.
2. Log in as demo in another browser.
3. As admin, request validation on a journal entry.
4. Refresh demo's browser -> AccessError on every page.
Fix: catch AccessError per model in the loop and skip silently with a
debug log. The user couldn't have acted on those records anyway
without read access, so silently dropping them from the count is the
right behaviour -- and it stops the systray from breaking every page.
Includes a regression test that revokes the validated model's
ir.model.access for non-admins, requests validation, and asserts
review_user_count() returns [] instead of raising.
Contributor
|
Hi @LoisRForgeFlow, |
OCA-git-bot
added
mod:base_tier_validation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The reviewer systray endpoint
res.users.review_user_count()iterates the current user's pending tier reviews, groups them by model, and for each model does:When the user is a reviewer on a model they have no
ir.model.accessread on (e.g. a tier definition onaccount.moveassigned to a non-accounting user), thatsearch()raisesAccessError. Because the systray fetches the count on every component mount, every page refresh in that user's session crashes with:Repro on runboat
account.movewith the demo user as reviewer.Fix
Catch
AccessErrorper model inside the loop and skip silently (debug log). The user can't act on records they have no read access to anyway, so the right behaviour is to drop the model from the count rather than crash the whole systray.Test plan
test_16b_review_user_count_no_model_access: it unlinks the tester model'sir.model.access, requests validation, and assertsreview_user_count()returns[](was raisingAccessErrorbefore).