Contributor can grant himself (and others) Reviewer locally

[glenfant]

• Create a contributor "johndoe"

• Login with "johndoe" credential

• Add any content item (say a Folder)

• Open the "sharing" tab of that item

• Search "johndoe" and check "Can review"

-> Yes, it works. "johndoe" is now reviewer of that folder as well as future inner content, can publish anything in that folder and add keywords (and perhaps other things, I didn't check all)

Through the "sharing" tab, an user should never be able to grant roles (thus potential new permissions) he has not.

In a Plone site with a business specific security policy with new roles, this could lead to critical security holes.

[limi]

I'm not sure I would term this a security issue — it's more a policy error, but I agree that it need to be fixed.

There's another related bug in the policy too:

Currently, when you share the Editor local role on an item, that person can't add additional people to share it with — which kind of makes the whole Sharing process dysfunctional. It's the common case — and although I can see sites that will want that only the original author can control the sharing, I think this is the exception (and those people can create their own workflow setting for it).

I assume adding "Change local roles" to the Editor role will fix this.

Any objections?

[mj]

Replying to limi:

I'm not sure I would term this a security issue — it's more a policy error, but I agree that it need to be fixed.

It most definitely is a security issue, it's a privilege escalation.

[limi]

But he only gets control of the item he created himself. While inconvenient, it's unlikely to cause much damage.

Let's fix both of these this weekend? I'm not sure how to update existing objects, but I have a local fix for both when if comes to the WF definitions.

[wichert]

That's not the point. There should be code that explicitly checks that users can only hand out rules they themselves have.

If you consider local roles on a folder you quickly see that this can affect much more than just the single object.

[glenfant]

I agree with Wichert. Letting a user grant any role to anybody in the folder he owns may have lots of other unwanted side effects in sites with deeply customized security policy (new roles/permissions, new workflows).

I think (maybe I'm wrong) the owner (or delegated owner) of an item should never be in position to grant a role he has not in the context of that item. This (more general) solution should fit with most of those deeply customized security schemas.

Cheers.

[optilude]

I think it's a mistake to let Owner delegate Reviewer. However, it's *not* a mistake to let Owner delegate Viewer, Editor and Contributor as he sees fit. The Owner implicitly has "view", "edit" and "add" (in folders) rights anyway, and should be able to delegate those to others as he sees fit, separately. Delegating Owner (what people used to do) causes other problems and makes it impossible to let someone view a private item but not edit it, say.

I don't think we need explicit low-level checks here. In plone.app.workflow.localroles, you can protect each delegatable role with a permission. If we had a couple of new permissions, this would work fine. I think they should be: "Delegate basic permissions" given to Owner and Manager, controlling Viewer, Editor and Contributor, and "Delegate review permissions", given to Reviewer and Manager only.

If we want people with Editor role to be able to delegate View/Edit/Contribute permissions further, then we could give "Delegate basic permissions" to Editor as well.

We could set these using rolemap.xml at the site root (and also in a migration step).

Martin

[optilude]

By the way, if you invent a new permission, you'll need to call setDefaultRoles('Delegate basic permissions', Manager?) on them in localroles.py. Then give this to Owner, Manager and Editor in rolemap.xml (don't forget migration step) and then change localroles.py like this:

class ReaderRole(object):
    implements(ISharingPageRole)
    
    title = _(u"title_can_view", default=u"Can view")
    required_permission ="Delegate basic permissions"

and ditto for the other roles + permissions.

Martin

[optilude]

(In [19763]) Only let users with roles delegated via the Sharing page delegate roles they actually have, by introducing more fine-grained permissions over who can delegate what. This allows for a flexible policy on individual sites, but the defaults should be sane now. Fixes #7652.

[optilude]

(In [19764]) Set Sharing action permission to the right one. Refs #7652.

[optilude]

(In [19851]) Backport the setup for this from 3.1 branch. Refs #7652. We may need a migration step for this as well, but arguably we could let people keep control over the sharing page action permission.

[wichert]

(In [21290]) Properly register the permissions with Zope.

Mark the security names as public so they can be imported everywhere and register them with Zope on startup so you can manage them via the ZMI or a GenericSetup profile. refs #7652.