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

EZP-32308: Fixed evaluating permissions on non prepared targets #3083

Merged
merged 5 commits into from
Feb 12, 2021

Conversation

ViniTou
Copy link
Contributor

@ViniTou ViniTou commented Jan 28, 2021

Question Answer
JIRA issue EZP-32308
Bug/Improvement yes
New feature no
Target version 7.5
BC breaks no
Tests pass yes
Doc needed no

Instead of requiring that all targets must be instance of Location, at least one valid is needed.

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

  1. It's a bit unclear to me what is present in $targets which causes the issue. Based on the description there should be no such issue, on the contrary - only Location should be present
  2. This requires integration test coverage (we already have similar tests).
  3. Commit message & PR title needs to be rephrased to follow the conventions.

@ViniTou
Copy link
Contributor Author

ViniTou commented Jan 29, 2021

It's a bit unclear to me what is present in $targets which causes the issue. Based on the description there should be no such issue, on the contrary - only Location should be present.

Only \eZ\Publish\SPI\Limitation\Target\Version was provided inside adminUI checks.

Copy link
Contributor

@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.

I'm confused. Location limitation is probably one of the most used limitations in the system. How did it not explode earlier? Is it really the very first occurrence of mixed $targets (which could be pretty common actually)?

@ViniTou
Copy link
Contributor Author

ViniTou commented Feb 2, 2021

@Nattfarinn

Is it really the very first occurrence of mixed $targets (which could be pretty common actually)?

No it is not, and all other places were systematically patched up and fixed (@mikadamczyk).
You may even see that same logic below in that same class
https://github.com/ezsystems/ezpublish-kernel/pull/3083/files#diff-a2d05e3e6c6fceef94a01e793d3e02b31b4d8d320f0535181fc5855c8db43f85R178

Why it came from clients now and not long time ago? 🤷 But it is with us since 2.5.1

@ViniTou ViniTou changed the title EZP-32308: At least one valid Location is required in targets EZP-32308: Fixed evaluating permissions on non prepared targets Feb 2, 2021
@mikadamczyk
Copy link
Contributor

mikadamczyk commented Feb 3, 2021

I'm not sure if this limitation change should actually be removed. From what I can see in PR EZP-30344: Allowed limiting Content management to specific translations (# 2585) then when adding the method prepareTargetsForType to PermissionResolver https://github.com/ezsystems/ezpublish-kernel/pull/2585/files#diff-0f692381531f1f40b194e997c1e98f1c72bd6ca3f97de35274be3cfeca8d5e6fR424 was added https://github.com/ezsystems/ezpublish-kernel/pull/2585/files#diff-23d89diff-23d89diff-23d89diffed965cf6b531diff-23d89diffed965c

For me, if we allow the possibility that Limitation can get different targets, then it should not throw this exception. Because in every place (old and new) you will need to know/remember that before doing LimitationType::evaluate you have to filter the targets

So both commit should be added

Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on eZPlatform 2.5 & v2.5.16 with patch & diff.

@adamwojs adamwojs merged commit fe9b584 into 7.5 Feb 12, 2021
@adamwojs adamwojs deleted the ezp-32308-location-limitations-publish branch February 12, 2021 20:36
@adamwojs
Copy link
Member

Could you please merge up changes @ViniTou?

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.

6 participants