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][FIX] mis_builder: use compute_sudo on field source_aml_model_id to prevent errors due to missing permissions. #596

Merged

Conversation

JordiBForgeFlow
Copy link
Sponsor Member

@JordiBForgeFlow JordiBForgeFlow commented Feb 7, 2024

The field source_aml_model_id refers to the ir.model, which is a technical in nature, and users should not have naturally permission to read them directly.

Test => PENDING

Don't forget to add unit tests. If your PR fixes a bug, prefer creating a separate
commit for the test so we one see that your test reproduces the bug and is fixed by the
PR.

Target branch => PENDING TO CHECK IF PREVIOUS BRANCHES ARE AFFECTED

MIS Builder is actively maintained for Odoo versions 9, 10, 11 and 12.

If your feature is applicable with the same implementation to all these versions, please
target branch 10.0. Maintainers will port it to 9, 11 and 12 soon after merging.

In the rare cases your feature or implementation is specific to an Odoo version, then
target the corresponding branch.

Changelog entry => DONE

@OCA-git-bot
Copy link
Contributor

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

@JordiBForgeFlow JordiBForgeFlow force-pushed the 16.0-mis_builder-fix-permissions-ir-model branch from 40ec864 to a3cf25a Compare February 7, 2024 12:01
@sbidoul
Copy link
Member

sbidoul commented Feb 7, 2024

Hi @JordiBForgeFlow. Sounds good but normally compute_sudo is true by default for stored computed fields: https://github.com/odoo/odoo/blame/60ee2dc2601c1fe7440d69d01844cc7eca4d39df/odoo/fields.py#L448

So I'm curious about the circumstances that trigger a problem related to this.

@JordiBForgeFlow
Copy link
Sponsor Member Author

@sbidoul hmm good point. It failed in two different environments... will check further.

@JordiBForgeFlow
Copy link
Sponsor Member Author

@JordiBForgeFlow
Copy link
Sponsor Member Author

The field is certainly compute_sudo = True

image

@JordiBForgeFlow
Copy link
Sponsor Member Author

When I add compute_sudo explicity in the field definition, the error does not appear. It's a mistery! Perhaps @MiquelRForgeFlow , the master mistery solver, can help here...

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 16.0-mis_builder-fix-permissions-ir-model branch from a3cf25a to 6d738e8 Compare February 7, 2024 16:51
@MiquelRForgeFlow
Copy link

The problem was not in the compute, but in the _check_source_aml_model_id contraint, when accessing technical field field_id. Fixed.

Comment on lines 1 to 2
The user may not have permission to access to the field field_id in ir.model, which corresponds to an ir.model.fields record.
It's a technical issue in nature, so we should use sudo to access it without error.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The user may not have permission to access to the field field_id in ir.model, which corresponds to an ir.model.fields record.
It's a technical issue in nature, so we should use sudo to access it without error.
Resolve a permission issue when creating report periods with a user without admin rights.

Let's have a user friendly change log.

Choose a reason for hiding this comment

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

Done

Resolve a permission issue when creating report periods with a user without admin rights.
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 16.0-mis_builder-fix-permissions-ir-model branch from 6d738e8 to c221650 Compare February 7, 2024 17:02
@sbidoul
Copy link
Member

sbidoul commented Feb 8, 2024

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-596-by-sbidoul-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 1a38f7c into OCA:16.0 Feb 8, 2024
5 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 25d65be. Thanks a lot for contributing to OCA. ❤️

@JordiBForgeFlow
Copy link
Sponsor Member Author

Thanks @sbidoul !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants