Incorrect permission checks due to caching with nested DelegatingImpersonatedAuthenticationPolicy.impersonate_user calls

[cutz]

I've been digging into test failure in the socket code. This test validates that a note sent over the socket to it's sharedWith targets doesn't have an edit link. The test has a failing assertion because the note created by sjohnson has an edit link when sent to over the wire jason.

I believe this was introduced by the effective_princpal caching that went in as part of PR #422. Nested use of the context manager returned from IImpersonatedAuthenticationPolicy.impersonating_userid (https://github.com/nextthought/nti.dataserver/blob/master/src/nti/dataserver/authentication.py#L316) seems to be the likely culprit. When externalizing objects to go over the socket sessions, we setup an IImpersonatedAuthenticationPolicy context for the user that is getting the object. We want the object to render as if the recipient is rendering it. However during the render pipeline the edit link's is_writable permission check is passing when it shouldn't be.

Note the test first creates an impersonation context then it sends the object to the owner session, then it sends the object to a sharingTarget's session. When externalizing the data as part of sending data to the sharingTarget's session, permission checks are getting cached effective principals off the request that are the owner's effective principals, not the sharingTarget's principals.

The cache takes into account the active authentication policy when deciding to use the cached values via an is check. In this case despite the policy being the same, the state of the authentication policy has changed in a way that invalidates the previously provided cached effective principals. The first pass through that cached the data the policy was impersonating the owner, the second pass through the policy is now impersonating a different user.

    327 def _get_effective_principals(request):
    328     """
    329     Return the principals as a set, plus the auth policy and registry (optimization)
    330     """
    331     # Make sure we return a set here, the membership checks are much, much faster
    332     # than iteration.
    333     # not pyramid.threadlocal.get_current_registry or request.registry, it
    334     # ignores the site
    335     reg = component.getSiteManager()
    336 
    337     active_authn_policy = reg.queryUtility(IAuthenticationPolicy)
    338     if active_authn_policy is None or request is None:
    339         return (psec.Everyone,), None, reg
    340     try:
    341         cached_policy, principals = request._v_nti_appserver_effective_principals
    342     except AttributeError:
    343         pass
    344     else:
    345         # This may change within the request (e.g. authentication)
--> 346         if active_authn_policy is cached_policy:
    347             return principals, active_authn_policy, reg
    348 
    349     principals = active_authn_policy.effective_principals(request)
    350     if request.authenticated_userid:
    351         # Only cache if we have an authenticated user
    352         request._v_nti_appserver_effective_principals = active_authn_policy, principals
    353     return principals, active_authn_policy, reg
    354 

ipdb> active_authn_policy
<nti.dataserver.authentication.DelegatingImpersonatedAuthenticationPolicy object at 0x129a36dd0>
ipdb> [x.__dict__ for x in cached_policy._locals.stack]
[{'auth_user': 'sjohnson'}, {'auth_user': 'jason'}]
ipdb> cached_policy
<nti.dataserver.authentication.DelegatingImpersonatedAuthenticationPolicy object at 0x129a36dd0>

Note the active and current policy are the same utility, but that utility is backed by a thread local stack of delegate policies, the state of which was changed when the nested impersonation context was set up.

ipdb> principals #cached principals
frozenset([_StringPrincipal('tag:nextthought.com,2011-10:system-NamedEntity:User-sjohnson'), _StringPrincipal('Everyone'), _StringPrincipal('sjohnson'), <zope.principalregistry.principalregistry.EverybodyGroup object at 0x1220d2f50>, u'system.Everyone', <zope.principalregistry.principalregistry.AuthenticatedGroup object at 0x1221a2cd0>, u'Everyone'])
ipdb> active_authn_policy.effective_principals(request)
frozenset([_StringPrincipal('tag:nextthought.com,2011-10:system-NamedEntity:User-jason'), _StringPrincipal('Everyone'), _StringPrincipal('jason'), <zope.principalregistry.principalregistry.EverybodyGroup object at 0x1220d2f50>, u'system.Everyone', <zope.principalregistry.principalregistry.AuthenticatedGroup object at 0x1221a2cd0>, u'Everyone'])

Seems like we could let policies opt out of having their results cached, or maybe we need to delegate the instance/equality checks of DelegatingImpersonatedAuthenticationPolicy to the top item in it's stack?

[cutz]

@jamadden thoughts here?

[jamadden]

In previous implementations (BlueBox) we've just forbidden impersonating while impersonating, i.e., no nesting. That seems like generally the correct approach to me.