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

[FIX] product_cost_security: ORM-level security #1538

Merged

Conversation

yajo
Copy link
Member

@yajo yajo commented Mar 6, 2024

Before this patch, inheriting modules had to manually add some logic related to guessing wether the user can access cost information. Also, smartypants users could still obtain the desired information through API calls.

Now:

  • You can't read/write cost fields via API anymore.
  • Views automatically set those fields to readonly if the user has only read permissions (and the model inherits from the new mixin).
  • You don't need to enable debug mode anymore to follow configuration instructions.
  • Instructions improved.

@moduon MT-5158

@OCA-git-bot
Copy link
Contributor

Hi @sergio-teruel,
some modules you are maintaining are being modified, check this out!

@yajo yajo force-pushed the 16.0-product_cost_security-real_security branch from 95d1f34 to b56309b Compare March 7, 2024 10:10
Copy link
Contributor

@Shide Shide left a comment

Choose a reason for hiding this comment

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

Code LGTM

@yajo yajo marked this pull request as draft March 7, 2024 11:50
@yajo yajo force-pushed the 16.0-product_cost_security-real_security branch from b56309b to 0803eae Compare March 7, 2024 12:06
@yajo yajo marked this pull request as ready for review March 7, 2024 12:06
Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

@yajo ,

  • As a Inventory "user" with our permisions in sales, purchase, etc., I get "Access Error" in common operations

You don't have permission to update product costs. Allowed group: Product costs / Modify product costs

imagen

Before this patch, inheriting modules had to manually add some logic related to guessing wether the user can access cost information. Also, smartypants users could still obtain the desired information through API calls.

Now:
- You can't read/write cost fields via API anymore.
- Views automatically set those fields to readonly if the user has only read permissions (and the model inherits from the new mixin).
- You don't need to enable debug mode anymore to follow configuration instructions.
- Instructions improved.

@moduon MT-5158
@yajo yajo force-pushed the 16.0-product_cost_security-real_security branch from 0803eae to d53def1 Compare March 7, 2024 13:57
@yajo
Copy link
Member Author

yajo commented Mar 7, 2024

Oops... I forgot to drop the security on sudo mode. Fixed now, please review again.

yajo added a commit to moduon/margin-analysis that referenced this pull request Mar 8, 2024
Both modules do essentially the same thing: apply a restriction over product costs. Thus, it makes sense to share the same permission groups.

This refactor, that depends on OCA/product-attribute#1538, improves the module readme and makes `sale_margin_security` auto-installable addon when `product_cost_security` is found.

A migration script is provided to make sure the same users still retain the same permissions.

@moduon MT-5158
yajo added a commit to moduon/margin-analysis that referenced this pull request Mar 8, 2024
Following OCA/product-attribute#1538, the security is now done via mixin and the view can be removed.

@moduon MT-5158
@yajo
Copy link
Member Author

yajo commented Mar 8, 2024

Now OCA/margin-analysis#198 depends on this PR to also unify the permissions interface with sale_margin_security and sale_margin_delivered_security.

yajo added a commit to moduon/margin-analysis that referenced this pull request Mar 8, 2024
Both modules do essentially the same thing: apply a restriction over product costs. Thus, it makes sense to share the same permission groups.

This refactor, that depends on OCA/product-attribute#1538, improves the module readme and makes `sale_margin_security` auto-installable addon when `product_cost_security` is found.

A migration script is provided to make sure the same users still retain the same permissions.

@moduon MT-5158
yajo added a commit to moduon/margin-analysis that referenced this pull request Mar 8, 2024
Following OCA/product-attribute#1538, the security is now done via mixin and the view can be removed.

@moduon MT-5158
Copy link

@amarcosg amarcosg left a comment

Choose a reason for hiding this comment

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

Code review LGTM

@legalsylvain
Copy link
Contributor

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1538-by-legalsylvain-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit d9f6d78 into OCA:16.0 Mar 11, 2024
9 checks passed
@OCA-git-bot
Copy link
Contributor

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

@yajo yajo deleted the 16.0-product_cost_security-real_security branch March 13, 2024 07:48
yajo added a commit to moduon/margin-analysis that referenced this pull request Mar 13, 2024
Both modules do essentially the same thing: apply a restriction over product costs. Thus, it makes sense to share the same permission groups.

This refactor, that depends on OCA/product-attribute#1538, improves the module readme and makes `sale_margin_security` auto-installable addon when `product_cost_security` is found.

A migration script is provided to make sure the same users still retain the same permissions.

@moduon MT-5158
yajo added a commit to moduon/margin-analysis that referenced this pull request Mar 13, 2024
Following OCA/product-attribute#1538, the security is now done via mixin and the view can be removed.

@moduon MT-5158
yajo added a commit to moduon/margin-analysis that referenced this pull request Mar 25, 2024
Following OCA/product-attribute#1538, the security is now done via mixin and the view can be removed.

@moduon MT-5158
jdidderen-nsi pushed a commit to jdidderen-nsi/margin-analysis that referenced this pull request Jul 12, 2024
Both modules do essentially the same thing: apply a restriction over product costs. Thus, it makes sense to share the same permission groups.

This refactor, that depends on OCA/product-attribute#1538, improves the module readme and makes `sale_margin_security` auto-installable addon when `product_cost_security` is found.

A migration script is provided to make sure the same users still retain the same permissions.

@moduon MT-5158
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.

6 participants