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

[BUG] remove checks on quantity value convert actions #536

Merged

Conversation

aMH-techsigns
Copy link
Contributor

Resolves: #434

Remove checkPermission method on specific rights for quantity value convert endpoints.

Additional info

It seems not necessary to provide specific checks while using convert functions for quanity values. Futhermore if you giving the user the quantityValueUnits access right the user can edit all scale units. That seems not correct at least in our use case where just users with higher level access rights should be able to edit scale units.

Copy link

github-actions bot commented May 21, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@aMH-techsigns
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@robertSt7 robertSt7 self-assigned this May 31, 2024
@robertSt7 robertSt7 added this to the 1.4.3 milestone May 31, 2024
@herbertroth herbertroth modified the milestones: 1.4.3, 1.4.4 Jun 4, 2024
@robertSt7
Copy link
Contributor

Hi @aMH-techsigns I think we should check the objects permission here. What is your opinion?

@aMH-techsigns
Copy link
Contributor Author

aMH-techsigns commented Jun 11, 2024

Hi @aMH-techsigns I think we should check the objects permission here. What is your opinion?

Hi @robertSt7 you mean to check the user permission for the underlaying data object tree, right? If so my thinking is if this is not handeled somewhere else maybe some steps before this call can even happen? So we might do stuff we shouldnt do on this position (might be unnecessary)? As the controller is inheriting from the admin controller where a admin user should be guaranteed i feel some kind of safe :-D.
Furthermore these two methods don't do any harm or update on the underlying data as far as i can see.

But ok i got you. You mean there is missing some kind of check (security-wise).
I can add the check on objects on both methods. It should not harm the process as the object permission needs to be granted to come this far.

@herbertroth herbertroth modified the milestones: 1.4.4, 1.4.5 Jun 18, 2024
@aMH-techsigns
Copy link
Contributor Author

@robertSt7 I followed your suggestion and added the two permission checks. Thx

@robertSt7 robertSt7 merged commit f12d81d into pimcore:1.4 Jun 28, 2024
7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 28, 2024
@robertSt7
Copy link
Contributor

@aMH-techsigns Thanks a lot for the fix. There is still a problem when the quantity value is changed the auto-save will be triggered and then it shows another error. I have alread created a follow- up for this. #544

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

Successfully merging this pull request may close these issues.

4 participants