fix allowedRolesAndUsers by using 'Access contents information'

[agitator]

fixes #260

[ale-rt]

As discussed in the Alpine city sprint we merge this

[jaroel]

Can we get the discussion in here as well?

I doubt if this is correct, as allowedRolesAndUsers is used to filter out objects that the user cannot view.
If you also want to check for Access contents information as well, that's great!

[ale-rt]

I see your point and we can even use both permission or even list permissions in a registry record so that everybody can do what he prefers.
I think the point here is that once the proper permission for getting catalog items was the "Access contents information" and then it was changed to "View", at least this is what I heard so this is resetting the behavior this permission was introduced for.
I would appreciate some more insights about this PR by other people and some example of how this commit can cause problems.

[jaroel]

Correct, the `Access contents information` permission is for getting to the properties of an object, or allowing `getObject` on brains. This `allowedRolesAndUsers` index is used to filter out objects that users don’t have the `View` permission for, as noted in the docstring of the function. Here is how this will break: 1. Have `access contents information` for an object, but not `view`. 2. Search for the content, or list the objects in the folder 3. See a link to the object 4. Click it and get an Unauthorised. Here’s how it currently works for example: https://www.jaroel.nl/some-public-folder/ <https://www.jaroel.nl/some-public-folder/> contains a document https://www.jaroel.nl/some-public-folder/some-do/ <https://www.jaroel.nl/some-public-folder/some-do/> with Access Contents Information for Anonymous, but View for owner only. https://www.jaroel.nl/some-public-folder/some-do/Title <https://www.jaroel.nl/some-public-folder/some-do/Title> does work, but https://www.jaroel.nl/some-public-folder/some-do/ <https://www.jaroel.nl/some-public-folder/some-do/> doesn’t. As you can see https://www.jaroel.nl/some-public-folder/ <https://www.jaroel.nl/some-public-folder/> doesn’t contains a link as we don’t have View. I’m guessing a test might be missing that wouldn’t cover this.

…

On 6 Feb 2019, at 11:54, Alessandro Pisa ***@***.***> wrote: I see your point and we can even use both permission or even list permissions in a registry record so that everybody can do what he prefers. I think the point here is that once the proper permission for getting catalog items was the "Access contents information" and then it was changed to "View", at least this is what I heard so this is resetting the behavior this permission was introduced for. I would appreciate some more insights about this PR by other people and some example of how this commit can cause problems. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2723 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEULNN7BLv7-PwAe0TBTHMHsbTnbbacks5vKrRJgaJpZM4akziU>.

[agitator]

Which workflow within Plone doesn't use those two permissions in combination?

[jaroel]

How is that relevant? It’s the wrong permission to check at that point. Please revert this, then write tests, then issue a new PR.

…

On 6 Feb 2019, at 12:10, agitator ***@***.***> wrote: Which workflow within Plone doesn't use those two permissions in combination? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2723 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEULPbk22mpCbvomzp69DCk20EIXLIeks5vKrgYgaJpZM4akziU>.

[jensens]

@jaroel I dont agree. -1 on revert.

If you used the permission in a different way as it was intended to use, then you need to fix the customization.

Access Content Information is meant for metadata such as in listings. View is meant for object access and viewing it. Both was mixed in past. And that was very bad. Now, with this change, View is free for the real intended meaning to use.

In case of your customization you either need to check if you want to set a link to it if you do not have view access or you need to redirect the user to a login page with came_from to the page. This could be done in core as well, but I am pretty sure there is no way satisfying all use-cases.

[jaroel]

I’m saying that `View` is the the permission to check in `allowedRolesAndUsers` as that index is used to not return objects that the user cannot View via a link. If you need them anyhow, you’ll need to use the catalog’s `unrestrictedSearchResults`.

…

On 6 Feb 2019, at 12:26, Jens W. Klein ***@***.***> wrote: @jaroel <https://github.com/jaroel> I dont agree. -1 on revert. If you used the permission in a different way as it was intended to use, then you need to fix the customization. Access Content Information is meant for metadata such as in listings. View is meant for object access and viewing it. Both was mixed in past. And that was very bad. Now, with this change, View is free for the real intended meaning to use. In case of your customization you either need to check if you want to set a link to it if you do not have view access or you need to redirect the user to a login page with came_from to the page. This could be done in core as well, but I am pretty sure there is no way satisfying all use-cases. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2723 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEULGa_1Q33aY4ytBxWzeVtCA28jf6fks5vKrvXgaJpZM4akziU>.

[jensens]

so, @ale-rt just brought up the idea to make this configureable - for special cases like yours - in the security controlpanel. What about that?

[jensens]

and View is not the correct permission check, there I strongly disagree.

[jaroel]

@jensens View is not the right permission to check if a user can view an object??

[jensens]

View is not the right permission to check to get the metadata. What we do with the catalog. Your assumption is wrong or at least different than ours here.

[jaroel]

`allowedRolesAndUsers` is not that use case - it’s not for getting metadata. It is used to filter out objects *the* *user* *cannot* *view*. See for instance - https://www.fourdigits.nl/blog/unrestrictedsearchresults-vs-allowedrolesandusers/ <https://www.fourdigits.nl/blog/unrestrictedsearchresults-vs-allowedrolesandusers/> - https://books.google.nl/books?id=a5ZCXilclkkC&pg=PA168&lpg=PA168&dq=plone+%22allowedRolesAndUsers%22&source=bl&ots=ylYat_1ASX&sig=ACfU3U1G6Ab4U_yuydWRGwPc1LOz2qqD_w&hl=en&sa=X&ved=2ahUKEwidq7uqhqfgAhXSZ1AKHabYB_0Q6AEwBXoECAQQAQ#v=onepage&q=plone%20%22allowedRolesAndUsers%22&f=false <https://books.google.nl/books?id=a5ZCXilclkkC&pg=PA168&lpg=PA168&dq=plone+%22allowedRolesAndUsers%22&source=bl&ots=ylYat_1ASX&sig=ACfU3U1G6Ab4U_yuydWRGwPc1LOz2qqD_w&hl=en&sa=X&ved=2ahUKEwidq7uqhqfgAhXSZ1AKHabYB_0Q6AEwBXoECAQQAQ#v=onepage&q=plone "allowedRolesAndUsers"&f=false> - https://docs.plone.org/develop/addons/components/genericsetup.html#portal-atct-xml

…

On 6 Feb 2019, at 12:51, Jens W. Klein ***@***.***> wrote: View is not the right permission to check to get the metadata. What we do with the catalog. Your assumption is wrong or at least different than ours here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2723 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEULO7PPHc85rycpShW2W3Wh1CLBzG8ks5vKsHSgaJpZM4akziU>.

[jensens]

so, allowedRolesAndUsers is used in the filtered searchResults. Which returns catalog brains. And catalog brains are metadata. We may need two indexes then, one for searchResults and one specific for for object access. But I dont like to blow up the catalog for a case the standard Plone anyways would not run in. Since you use it different, I propose to make the cataloged permission configurable for your specific use-case, but stick to what one would expect to get from the catalog: brains one is allowed to see by quering the catalog, not objects to access.

[jaroel]

so, allowedRolesAndUsers is used in the filtered searchResults.

Correct, so users don't get links to objects they cannot view.

Which returns catalog brains. And catalog brains are metadata.

Correct, but not relevant. We're not permission checking the brain, but the actual object.

We may need two indexes then, one for searchResults and one specific for for object access. But I dont like to blow up the catalog for a case the standard Plone anyways would not run in. Since you use it different, I propose to make the cataloged permission configurable for your specific use-case, but stick to what one would expect to get from the catalog: brains one is allowed to see by quering the catalog, not objects to access.

For object acces one can use unrestrictedSearchResults at the point where you need it.

I'd like allowedRolesAndUsers to be the same as it was since the introduction as it never was broken to begin with.

[lukasgraf]

I completely agree with @jaroel here.

This change is not correct in my opinion. It breaks a contract that has been around for what seems like forever. The docstring (which hasn't even been updated in this PR) clearly states:

Return a list of roles and users with View permission.
Used to filter out items you're not allowed to see.

allowedRolesAndUsers is used to limit the set of objects that are returned from the filtered searchResults. If you now want to control access to metadata (on the brains) using Access contents information - why not.

+1 for revert from me.

[MrTango]

one common use case to use "Access content information" is, to show folders in th navigation and when the user clicks on it having a login form because the user does not have the view permission.

[jensens]

Better open a new issue than discussing on an closed PR.