collective / collective.nitf

A Dexterity-based content type inspired on the News Industry Text Format specification
8 stars 3 forks source link

Private content populating NITF tile is rendered for anonymous users #185

Closed idgserpro closed 7 years ago

idgserpro commented 7 years ago
# Commit: a65675559866b8e8123c4235a9b46aaaa2c259cb
git clone https://github.com/collective/collective.nitf.git
python bootstrap.py
# Edit the buildout adding the last collective.cover release as an egg at version 1.4b1. 
# Remember to add it's dependencies pinnings as well from
# https://github.com/collective/collective.cover/blob/f8e68cad0d7024a155bcb86009805cac1838cbc0/versions-4.3.x.cfg
bin/buildout

Create a Plone site, install both addons. Create a nitf content with an image, don't publish it.

Go to @@cover-settings, select "News Article Tile" as available.

Create a new cover content having a nitf tile in "Layout", save, publish it, and add a nitf tile selecting the nitf content you created earlier in "Compose".

This is the cover for a logged in user:

selecao_010

See the cover with an anonymous user: the tile shouldn't be rendered for anonymous users, but it is.

selecao_011

When you click on the link, it's http://localhost:8080/Plone/home/None.

rodfersou commented 7 years ago

I think this is a collective.cover issue, it copy any content type dropped into the tile without check if it is published or not.

We should add this check there, and just allow drop item when it is published (or maybe even didn't show the item at content chooser if not published)

idgserpro commented 7 years ago

Should I repost this at collective.cover @hvelarde?

hvelarde commented 7 years ago

no, having private content in a tile is a totally valid use case: tile content should not be rendered if the user doesn't have permission to see them; that's the way Plone works.

it would be nice to show something indicating content is in private state, but that's a different issue.

idgserpro commented 7 years ago

Usually Plone adds a state-private class that sets color: red !important to private content. It seems simple but I think it's going to be really complicated to add this behavior to cover. But I don't think this is priority right now, the most important issue here is to avoid showing private content to anonymous users.

This is what happens when I add state-private to tile-container:

image

rodfersou commented 7 years ago

@hvelarde ok, add private content into a tile is a valid use case, and show this content to everyone? AFAIK collective.cover don't keep the state of the content inserted into the tile, in almost all tile it just copy the tile content instead of keep reference of original data. This happen because the tile content can be modified at compose tab.

The problem here is bigger than it looks.

idgserpro commented 7 years ago

collective.cover don't keep the state of the content inserted into the tile The problem here is bigger than it looks.

Yes, it's a big problem. I already said in my comment that to add this behavior to collective.cover would be really complicated. :)

The idea here is just to try the way Plone does things: if it's too much work, then we do our way. No big deal, was just a suggestion.

But as I said, the most important thing here is the private content being shown to anonymous users bug: people may be disclosing information that shouldn't be available at all to anonymos people. Showing that a content is private like mentioned in https://github.com/collective/collective.nitf/issues/185#issuecomment-274800949 is just the icing on the cake.

hvelarde commented 7 years ago

let's not lose the focus: the only issue here is the link to the original object is broken and we only need to fix that.

an user trying to visit a private object will be redirected to the usual login page.

idgserpro commented 7 years ago

So, what should we do to fix? Create a new method, for example, isAllowedToView and call this method here changing tal:condition="not:is_empty" to tal:condition="pythoni:is_empty and isAllowedToView"?

So, would it be nice to have this method in collective.cover itself, just to be used from tile developers that would need it in a template? Actually, we're starting to rethink if this whole fix shouldn't be in collective.cover itself :laughing:

rodfersou commented 7 years ago

The problem happen in this method. It looks like the object UID is just indexed when it is published.

ipdb> uuid
'4218edbadbce49578852aedb3065b86f'
ipdb> catalog(UID=uuid)
[]
ipdb> portal = api.portal.get()
ipdb> n1 = portal['teste-1']
ipdb> n1
<NITF at /Plone/teste-1>
ipdb> n1.UID()
'4218edbadbce49578852aedb3065b86f'
rodfersou commented 7 years ago

@hvelarde any idea on how to fix it? maybe other tiles have the same problem

idgserpro commented 7 years ago

@hvelarde What do you think? We can help on this but we would like to know the best approach.

hvelarde commented 7 years ago

first we need to find out if vanilla Plone does not index the UUID until publishing an item; I doubt it.

idgserpro commented 7 years ago

Fixed upstream in https://github.com/collective/collective.cover/issues/721.