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

IBX-6592: Allowed Location to be a part of permission check for Object State assignment #2112

Closed
wants to merge 3 commits into from

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Nov 7, 2023

Question Answer
Tickets IBX-6592
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Requires: ezsystems/ezplatform-kernel#391

Currently, Content Object State assignment is done utilizing ContentInfo, which in my opinion is wrong, as that leaves Subtree Limitation pretty much useless, and yet, this limitation is available for a choosing when we are dealing with the state/assign policy.

Therefore, ContentInfo was replaced with Location everywhere where it touches Object States assignment.

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

Copy link

sonarcloud bot commented Nov 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.6% 2.6% Duplication

@barw4 barw4 marked this pull request as ready for review November 8, 2023 14:42
@barw4 barw4 requested a review from a team November 8, 2023 14:44
@@ -821,7 +821,7 @@ ezplatform.object_state.state.bulk_delete:
_controller: 'EzSystems\EzPlatformAdminUiBundle\Controller\ObjectStateController::bulkDeleteAction'

ezplatform.object_state.contentstate.update:
path: /state/contentstate/update/{contentInfoId}/group/{objectStateGroupId}
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a bc break, I guess we should create new route and deprecate that one.

);
}),
'choice_loader' => new CallbackChoiceLoader(
function () use ($objectStateGroup, $location) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function () use ($objectStateGroup, $location) {
function () use ($objectStateGroup, $location): array {

function () use ($objectStateGroup, $location) {
return array_filter(
$this->objectStateService->loadObjectStates($objectStateGroup),
function (ObjectState $objectState) use ($location) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function (ObjectState $objectState) use ($location) {
function (ObjectState $objectState) use ($location): bool {

Copy link
Member

@Nattfarinn Nattfarinn 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 approach. We can't make our permission layer to behave differently if used from UI and used as API.

@barw4
Copy link
Member Author

barw4 commented Nov 30, 2023

This is not correct approach. We can't make our permission layer to behave differently if used from UI and used as API.

@Nattfarinn where exactly does it behave differently than what we have in the API?

@Nattfarinn
Copy link
Member

Nattfarinn commented Dec 1, 2023

@Nattfarinn where exactly does it behave differently than what we have in the API?

This additional argument is used only in ibexa/admin-ui. I know this was done to keep BC, but existing API calls that won't use Location will behave differently and would force everyone to update all their API calls (what about REST calls?) to use additional parameter to make it work the same. If we insist on keeping BC here (ping @alongosz), we should at least be able to do that without changes to API.

This could/should be solved in https://github.com/ezsystems/ezplatform-kernel/blob/1.3/eZ/Publish/Core/Limitation/SubtreeLimitationType.php by handling special case (maybe check $targets for ObjectState?) but this would need at least @alongosz decision/expertise/omniscience.

@webhdx
Copy link
Contributor

webhdx commented Dec 19, 2023

We put more thought into this change and we can't change the API to facilitate Locations only to be used as a part of Permission check. I think the proper solution is to remove LocationSubtree limitation from state:assign policy and fix the documentation. It was never working correctly and I believe it was added by a mistake since Object State API doesn't work with Locations at all.

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

Successfully merging this pull request may close these issues.

7 participants