Respect permission value for utility factory registrations

[dataflake]

Fixes #54

[jamadden]

That seemed like a curious omission, so I wondered why.

In the earliest version of the code in the repository (3.4.0), a factory was immediately instantiated at ZCML action time, so there was no need to do anything further with it:

zope.component/src/zope/component/zcml.py

Lines 403 to 406 in 40e438c

	if factory:
	if component:
	raise TypeError("Can't specify factory and component.")
	component = factory()

Later, in response to https://bugs.launchpad.net/zope3/+bug/240631, version 3.5 added the ability to save the factory as part of the arguments so that it was sent in the event, and so that later it could be inspected through registeredUtilities(). Even in the initial version of that commit, no security was applied; see 98216e5#diff-1022d9172e052cc0c5fa64e50f4d0733fedf2bb2bc975e1c592888d7146121bc

This PR wraps the original factory that gets passed to registerUtility in an inner function. That raises two concerns.

First, I think this may break the original use case of detecting identical factories. (From the bug report: " For tools like GenericSetup that need to round-trip utility registration information that is critical.")

Second, because the factory is stored in Components._utility_registrations, I think this will break persistent registries, because inner functions can't be pickled.

>>> import pickle
>>> def wrap(obj):
...     def wrapper():
...         return obj
...     return wrapper
...
>>> wrapper = wrap('abc')
>>> pickle.dumps(wrapper)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: Can't pickle local object 'wrap.<locals>.wrapper'

[dataflake]

I was just trying to apply what I had seen elsewhere in the code, that's all. You know this code a lot more, so I may very well have produced a complete dud. Feel free to close or suggest a different implementation if there is one.

[jamadden]

Concern (2) isn't really a practical problem. While it's possible to arrange to execute ZCML against a non-global SiteManager (through z3c.baseregistry IIRC), it's rare-to-nonexistant to do so, I think, with a persistent SiteManager. (I don't know that for sure, but things like adapter registrations with permissions would already be broken).

Concern (1) is the main issue, I suspect. I know nothing about the Zope2/4/5/Plone GenericSetup architecture or similar tools and so I don't know what to make of the original bug report, specifically "For tools like GenericSetup that need to round-trip utility registration information that is critical." If you or someone else who does know about that thinks this is fine, then it's fine by me too! (I'm guessing the reason for giving the inner function a .factory attribute is to match what the LocatingTrustedAdapterFactory et al from zope.security do, so I'm also guessing that it may be fine?)

Other than that, I would suggest this is a fairly substantial change that might break some things so it would need to increment the version to at least 4.7.0.

[dataflake]

Thank you for taking all that time. I can't judge the potential for breakage well, I'm trying to stay away from GenericSetup. It worries me and makes me wonder if the best course would be to drop this change and also drop any documentation that claims you can use a utility registration that specifies a factory and permissions. Basically, permissions is only usable if you register a utility and pass a component. Obviously no one has ever noticed or complained in all those years, except for one person who sent email to the Plone folks.

[jamadden]

Starting from scratch, wrapping the factory with the permission factory seems like it should be the right thing to do.

What if we take this change, and call the version that releases it 5.0? That should give plenty of notice that there might be breakage, and GenericSetup or whatever can adapt if needed.

Actually, with #53, I'd like to be able to call it 5.0. That way, we can bump the zope.interface dependency (currently on 4.something) to the latest zope.interface 5.3 as well (generally semver specifies that if you bump a semver dependency you must also bump your version at the same level). That way I can drop the caveats about "well, this only works if you have the latest zope.interface".

We could also drop some of the least useful, previously deprecated, things in this package, such as BWC imports.

[dataflake]

I don't mind cleaning out things that have a DeprecationWarning - do they? ;-)

[jamadden]

Off the top of my head, I know at least one place that does. I feel sure that other places are at least docs-deprecated, but I haven't gone looking in more detail.

zope.component/src/zope/component/interfaces.py

Lines 25 to 29 in 72d0664

	import zope.deferredimport
	zope.deferredimport.deprecatedFrom(
	"Import from zope.interface.interfaces",
	"zope.interface.interfaces",
	'ComponentLookupError',

[dataflake]

From my POV this is ready for merging. I have bumped the version number to 5.0 and ran the following tests in conjunction with this branch:

• unit tests for Products.GenericSetup
• unit tests for Products.CMFCore
• unit, functional and integration tests from Plone's buildout.coredev, which combines a total of over 9000 tests. I did not run the 150 or so browser tests that want to launch Firefox.
• eyeballing a GenericSetup snapshot of a stock Plone 5.2 site with tons of utility registrations that use factory, all those dotted names were fine.

[jamadden]

LGTM.