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

AccessInactivePortalContent in catalog queries #1952

Merged
merged 1 commit into from
Feb 23, 2017

Conversation

thet
Copy link
Member

@thet thet commented Feb 22, 2017

Checked for AccessInactivePortalContent for each path in a catalog query.
This solves a problem, where Editors couldn't see inactive content, even though they had the required permission on a subpath of the portal (e.g. a subsite).

Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

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

Overall I think this is fine. I would like to have someone else to look at it before merging.


paths = query_kw.get('path', False)
if not paths:
return []
Copy link
Member

Choose a reason for hiding this comment

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

Why an empty list here?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a leftover from refactoring this method a couple of times... I'll change to False.

allow = True
for ob in objs:
allow = allow and\
_checkPermission(AccessInactivePortalContent, ob)
Copy link
Member

Choose a reason for hiding this comment

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

If you have more than one paths given, one not allowed will result in not showing the other path as well. Is it intended? Overall I think it's impossible (or at least difficult to do in a fast way) to implement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's indented - I think it's better to be conservative here instead of showing too much content which shouldn't be shown.
Also, it's quite uncommon that two paths are used.

@jensens
Copy link
Member

jensens commented Feb 22, 2017

@mauritsvanrees since your also deeper into security, what do you think of this change?

@thet thet force-pushed the thet-accessinactiveportalcontent branch from dc6b242 to 2822b6e Compare February 22, 2017 14:19
Checked for ``AccessInactivePortalContent`` for each path in a catalog query.
This solves a problem, where Editors couldn't see inactive content, even though they had the required permission on a subpath of the portal (e.g. a subsite).
@thet
Copy link
Member Author

thet commented Feb 23, 2017

As discussed with @jensens this might need a broader discussion and therefore a PLIP.

As a prerequisite, that this change has any effect to editors is, that the editor role gets the AccessInactivePortalContent permission set. That way, they can see contents from other editors, which are expired or inactive. AFAIK, the Owner role has the AccessInactivePortalContent permisson. This has following impacts:

  • Editors can see inactive portal content from other editors in the folder_contents view (which is actually very handy. Imagine someone creating a content or an automated import where the creator is set to the user who imported and another editor is editing the content and setting the publication date to the future. That another editor isn't able to find the content anymore).

  • Inactive content will show up in listings (custom listings, collections, folder_summary_view, etc). If the number of items is limited (e.g. 10 batch items) it might push other content out of the view. The resulting listing will not correspond to the listing other persons will see, e.g. anonymous visitors to a page. This is actually a problem.

  • Also, inactive content should be visually marked as such - like with workflow states.

Part of the solution could be to provide an additional parameter to searchResults to show inactive content, if the user has permission for it and use it in plone.app.content for the folder_contents listing.

@jensens
Copy link
Member

jensens commented Feb 23, 2017

While the overall new behavior is correct we have some side effects in existing projects needing some more discussion.

So please do NOT merge.

@thet
Copy link
Member Author

thet commented Feb 23, 2017

Ideas to visually mark inactive content in folder_contents:

  • not-effecive conten in italic: Title of the content item
  • expired content striked through: Title of the content item

At some time in the future we might be able to use: https://developer.mozilla.org/de/docs/Web/CSS/text-decoration

Not sure, how to explain that to the user. We currently do not have a legend for such visual markings in folder_contents.

@jensens
Copy link
Member

jensens commented Feb 23, 2017

Overall after merging this, the only behavioral change is that editors in local roles are seeing in their section the same as editors in global roles which I think is a good thing.

The other problems are still there and we already have them for global roles editors.

I propose to fix these problems in a different PR.

@jensens jensens merged commit 9807e39 into master Feb 23, 2017
@jensens jensens deleted the thet-accessinactiveportalcontent branch February 23, 2017 10:57
@mauritsvanrees
Copy link
Member

I didn't have time previously to look at this, but I had a look now, and I agree. Thanks for the fix!

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.

4 participants