search

collective / collective.cover

A sane, working, editor-friendly way of creating front pages and other composite pages. Working now, for mere mortals.
48 stars 55 forks source link

@@jsonbytype permission after upgrading from versions older than 1.3b1 #857

Open shogunbr opened 5 years ago

shogunbr commented 5 years ago

After upgrading collective.cover from versions older than 1.3b1, compose content tree is not filled with content to users who doesn't have Site Admin or global Editor rights. @@jsonbytype gives an unauthorized error.

Steps to reproduce: 1 - Create a new Plone site (i've tested on a clean 4.3.18 instance) and install collective.cover 1.2b (or older) add-on 2 - Upgrade collective.cover to 1.3b (or newer up to 1.8b2) 3 - Run the upgrade steps 4 - Create a regular user (do not add him to any group) 5 - Create a Cover 6 - At the sharing tab of the cover, add permissions, so this user can compose (i've added all permissions). 7 - Login as the user and click on Compose tab of this cover. Try to add content from the Content Tree 8 - The Content tree will be empty

The error doesn't happen to the Site Admin or to Editor role, only to Member users. The error doesn't happen if the Plone site have never had older collective.cover versions prior to 1.3b installed.

Any ideas how it can be fixed?

error-jsonbytype

shogunbr commented 5 years ago

I've done more tests and it looks like it doesn't happen only when upgrade. Just install collective.cover >=1.3b (I've tested on the latest 1.x: 1.8b2) And try the steps 4 to 8 from the previous post.

idgserpro commented 5 years ago

Any ideas how it can be fixed?

I've done more tests and it looks like it doesn't happen only when upgrade.

Usually, for problems between upgrades we suggest reading changelogs and see which implementation may have introduced the problem you're describing.

github has a feature that can help you in this endeavor: using https://help.github.com/en/articles/comparing-commits-across-time you can compare all commits between the versions you used in your test.

In the scenario of this issue: if you take 1.0a6 version and compare to the newest commit, @@jsonbytype permission changed from:

https://github.com/collective/collective.cover/blob/6fb4bff46f57aa9855a4b7fde14cda1de06ed37f/src/collective/cover/configure.zcml#L45

to

https://github.com/collective/collective.cover/blob/fa35c561bb21ff8cf46cfc367112035a98e9c33f/src/collective/cover/browser/configure.zcml#L24

I don't know if by changing permissions in configure.zcml other modifications are needed. I suggest comparing both implementations of @@jsonbytype view to check if an upgradeStep is missing.

shogunbr commented 5 years ago

At first I thought it happened only when upgrading, but on my later tests it also happens on a fresh install. So I guess it's not required an upgrade step. Changing the permission back to "zope2.View" seems to solve the problem, but i don't know if it implies on the security. Why @@jsonbytype permissions were changed to "cmf.ModifyPortalContent" ? Security issue?

idgserpro commented 5 years ago

It was changed from (Click on Files tab):

https://github.com/collective/collective.cover/compare/1.2b1...1.3b1#diff-d874ab145d94829c27c5b7431db4d17aL57

to (Click on Files tab):

https://github.com/collective/collective.cover/compare/1.2b1...1.3b1#diff-f549427a7e7f3e61f0d313f1e942048cR25

And said in the changelog:

https://github.com/collective/collective.cover/blob/master/CHANGES.rst#13b1-2016-09-12

A huge code refactoring was made as part of the removal of the dependency on five.grok. The following unused views were removed: AddCTWidget, AddTileWidget, SetWidgetMap, UpdateWidget and RemoveTileWidget. All Compose tab helper views use now cmf.ModifyPortalContent permission. All Layout tab helper views use now collective.cover.CanEditLayout permission. The BaseGrid class is now located in the collective.cover.grids module.

IMHO there's no error here. If you want diferent behavior, you should add to a policy package in your installation. We've had a similar discussion about these differences and rolemaps in https://github.com/collective/collective.cover/issues/855#issuecomment-486228531.

shogunbr commented 5 years ago

But in this example, the user does have Local Editor permission only at this specific Cover. I guess the problem is that @@jsonbytype is called at the root of the site and this user doesn't have Edit permission at the root of the site. So i think it's a bug, it's not correct to add site wide edit permission to all users that only need to edit specific covers.

idgserpro commented 3 years ago

Changing the permission back to "zope2.View" seems to solve the problem, but i don't know if it implies on the security. Why @@jsonbytype permissions were changed to "cmf.ModifyPortalContent" ? Security issue?

@shogunbr As a workaround you can use overrides.zcml and unconfigure the views/pages content-chooser and configure again using a different permission in a policy package.

Security wise, this method returns a catalog, so the user is seeing what was expected to see. But if you're using zope2.View and an attacker knows this url and plans to overload your server this could be an issue.

I recommend in this case to use cmf.SetOwnPassword, since every authenticated user have it. This way, if logged in users directly call this url no data which it's not supposed to be available will be shown (since it's a portal_catalog call) and you avoid the problem of anonymous users abusing the url. And since you still need cmf.ModifyPortalContent to use Compose anyway, I see no risks involved.