[Zope-dev] Re: PermissionGeddon

[Florent Guillaume]

On 26 Nov 2005, at 09:28, Hanno Schlichting wrote:
> Florent Guillaume wrote:
>> I've done a big checkin to switch practically everything to the  
>> new- style (actually they're 5 years old) security declarations.
>> I'd appreciate if another set of eyes could double-check  
>> everything;  while I've taken a number of steps to ensure I didn't  
>> make mistakes  you never know...
>> http://mail.zope.org/pipermail/zope-checkins/2005-November/ 
>> 030103.html
>> Thanks,
>> Florent
>
> Hi Florent.
>
> I have spent some time to run all unit tests of Plone (2.1.1) and  
> all it's core products on Zope 2.9 and look into the failing tests.

Thanks a lot!

> I hope to have tracked the ~200 failing tests down to two of your  
> changes in OFS.CopySupport.
>
> The first change is in the manage_pasteObjects method of  
> CopyContainer. There are some _setObject and _delObject calls which  
> grew a new suppress_events parameter. This breaks the reference  
> implementation of Archetypes because it uses something based on  
> BTreeFolder2 to store references and BTreeFolder2 overwrites both  
> _setObject and _delObject with its own versions.

The BTreeFolder2 inside Zope 2.9 has been modified to also take this  
suppress_events parameter. I'm afraid Plone/AT will have to modify  
its _setObject and _delObject implementations to take this optional  
parameter too.

> As I'm quite new to Zope internals I suppose fixing BTreeFolder2 is  
> the right thing here (adding the new parameter on both of its  
> methods). But this change might effect some more products which  
> have overwritten these methods as CopySupport is a base class of  
> OFS.Folder and so probably of every folderish type out there.

This was the minimal necessary change to have events work. These  
products will have to be modified to work with Zope 2.9, there's no  
avoiding it. Fortunately the changes are obvious and minimal. BTW  
what BTreeFolder2 are you talking about? BTreeFolder2 has been  
included in Zope for a while.


> The second change is actually related to your permission work.  
> First of all I have to thank you for your great work :) But I have  
> found one nasty thing.
>
> CopySupport had the following security declaration:
>
> __ac_permissions__=(('Copy or Move', (), ('Anonymous', 'Manager',)),)
> ...
> Globals.default__class_init__(CopySource)
>
> which changed into:
>
> security = ClassSecurityInfo()
> security.setPermissionDefault(copy_or_move, ('Anonymous', 'Manager'))
> ...
> InitializeClass(CopySource)

That's correct.

> Now the InitializeClass call is actually an alias for the former  
> Globals call, so no change here. But as you wrote yourself, you had  
> some trouble with the mysterious __ac_permissions format.
>
> Looking at the actual code in App.class_init in the last paragraph  
> I'm quite sure that the former code did effectivly nothing so far.  
> The actual setattr call is inside a 'for mname in mnames:' loop  
> where mnames is the second element of each security tuple - in this  
> special case the mysterious () which results in not going through  
> the 'for mname in mnames:' loop at all.

Yes the previous code did nothing, and it just left the  
__ac_permissions__ alone.

The new code goes through SecurityInfo.ClassSecurityInfo.apply and,  
because there has been a setPermissionDefault, has an available  
self.roles dict... Hm I see what's wrong, the SecurityInfo code only  
generates permission default roles for permissions which are *also*  
mentioned in a method permission setting (the for loop). That code is  
wrong, I'll have to fix it.

> In the Plone unit tests this results in about 190 failing tests

Could you give me the reference to one or two of these failing tests  
so that I can check things are ok after the fix? Is this with a  
standard Plone-2.1.1 tarball? Do you run the Plone tests using the  
standard Zope method?

> as inserting an object into a folder isn't allowed anymore for  
> normal members because this triggers some code in a Referenceable  
> class which is based on CopySupport directly, which required no  
> extra permission so far but now is protected through the 'Copy or  
> Move' permission.
>
> While I guess that protecting the class with this permission has  
> been the intent it was not in effect so far. So this should be  
> either reconsidered or at least noted in the changelog. Changing  
> this in Archetypes shouldn't be a problem (though I'm very new to  
> Zope's permissions system).

The class is not protected by copy_or_move, the effect of the  
declaration is supposed to be that copy_or_move is by default granted  
to Anonymous and Manager. But with my changes because of the  
aforementioned bug it's not anymore.

Thanks again for reviewing this! I'll prepare a fix soon.

Florent

-- 
Florent Guillaume, Nuxeo (Paris, France)   Director of R&D
+33 1 40 33 71 59   http://nuxeo.com   fg at nuxeo.com