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

[16.0][IMP] mis_builder: add read access to model and fields to group account manager #644

Closed
wants to merge 1 commit into from

Conversation

AnizR
Copy link
Contributor

@AnizR AnizR commented Oct 18, 2024

Description

Editing a query in a template can throw an access error.

This happens when a 'Billing administrator' (group_account_manager) hasn't the group 'Access Rights' (group_erp_manager).
In fact, only the group 'Access Rights' gives a read access to ir_model and ir_model_fields.
But, on a mis.report.query, the user needs those access to be able to set model_id and field_ids

@OCA-git-bot
Copy link
Contributor

Hi @sbidoul,
some modules you are maintaining are being modified, check this out!

@AnizR AnizR changed the title [IMP] mis_builder: add read access to model and fields to group account manager [16.0][IMP] mis_builder: add read access to model and fields to group account manager Oct 18, 2024
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. We shouldn't grant that permission, but bypass the restriction when applicable. I know it's a direct m2o, but we should find another way. Giving permissions is not the way.

@pedrobaeza pedrobaeza added this to the 16.0 milestone Oct 18, 2024
@AnizR AnizR marked this pull request as draft October 18, 2024 11:48
@AnizR
Copy link
Contributor Author

AnizR commented Oct 18, 2024

This is not correct. We shouldn't grant that permission, but bypass the restriction when applicable. I know it's a direct m2o, but we should find another way. Giving permissions is not the way.

Ok, how would you do such thing then?

I had the idea to 'dynamically build a selection containing the name of models/fields'. This selection would be computed in sudo and would set the m2o. What do you think?

Do you have any other idea?

@pedrobaeza
Copy link
Member

Yes, that's what we have done in other places.

@sbidoul
Copy link
Member

sbidoul commented Oct 18, 2024

I'm not sure we can solve that in mis_builder, because any solution will weaken the security baseline by exposing the list of models and fields.

@AnizR
Copy link
Contributor Author

AnizR commented Oct 18, 2024

This wasn't a good idea.
Let's discuss the best way to tackle this issue: #415

@AnizR AnizR closed this Oct 18, 2024
@AnizR AnizR deleted the imp-security-ir-model-fields branch October 18, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants