Add pyramid_authstack.AuthenticationStackPolicy as a core pyramid policy

[ergo]

Reference: https://github.com/wichert/pyramid_authstack

I've found it very useful to have multiple authentication methods in my pyramid applications, I always used pyramid_authstack for this purpose.
After talking with Wichert he was enthusiastic to the idea of making it core pyramid auth.
In my opinion it would make sense to have an out-of-the box option to have stackable policies.

This addition would not require any changes to pyramid tutorial, just documentation of new policy + some mentions in authentication section.

Alternatives include Mozilla's https://github.com/mozilla-services/pyramid_multiauth - however pyramid_authstack is a simple implementation that is easy to understand and reason about so I would consider it as the primary candidate for implementation.

[stevepiercy]

At the very least, it should be included in the list of add-ons on trypyramid.com.

Use this link to create a new issue:
Pylons/trypyramid.com#202

[digitalresistor]

@ergo why add it to Pyramid directly? That increases the maintenance burden for core devs, adds to our testing requirements, and we have been telling people to use sub-classing of the authentication policy rather than using the callback mechanism as the callback mechanism confuses people and makes the code less readable/understandable than modifying the policy directly.

[ergo]

I think the case where you want user to authenticate via more than one policy at same time: for example via auth_tkt and/or token/apikey is common enough scenario warrant this functionality provided out of the box.

@bertjwregeer Have you seen how small the code - it is 40 LOC, I think it should not be a dependency but part of pyramid itself. I raised the subject of incorporating this as another policy because that would be a non-breaking change.
I'm not sure how should I relate callback mechanism to having multiple policies in general - how would subclassing solve this particular problem?

[digitalresistor]

We have been telling people that want to write custom authentication policies that they should sub-class one of the existing ones and override authenticated_userid and effective_principals:

https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/security.html#extending-default-authentication-policies

Other noticeable issues I see: all of my custom authentication policies have authenticated_userid do the heavy lifting of verifying things are correct, yet with the current implementation that is only ever checked with the callback, or once effective_principals is called my code could call into authenticated_userid on my policy alone, potentially doing double the work.

My policies also always return None for unauthenticated_userid since it's not a feature they support.

It steps through the sub-policies for effective_principals and merges the groups it gets from them, but if both policies also have a callback function (or are sub-classing another policy and have their own effective_principals) then you end up with double the database calls or calls to external services depending on how the group information is found/pulled.

Do note that the current example shows AuthTkt using the same secret, so the cookie it sets is the exact same, and there is no disambiguation using a different cookie name and thus both policies would always return effective_principals, even if the "secure" policy was never "remembered" and the user would gain full access because auth:sensitive would always be set.

from pyramid.authentication import AuthTktAuthenticationPolicy
from pyramid_authstack import AuthenticationStackPolicy

def groupfinder(userid):
    return ['group1', 'group2', 'group3']

auth_policy = AuthenticationStackPolicy(callback=groupfinder)
# Add an authentication policy with a one-hour timeout to control
# access to sensitive information.
auth_policy.add_policy(
    'sensitive',
    AuthTktAuthenticationPolicy('secret', cookie_name="sensitive", timeout=60 * 60, callback=groupfinder))
# Add a second authentication policy with a one-year timeout so
# we can identify the user.
auth_policy.add_policy(
    'identity',
    AuthTktAuthenticationPolicy('secret', cookie_name="identity", timeout=60 * 60 * 24 * 365, callback=groupfinder))
config.set_authentication_policy(auth_policy)

userid = request.authenticated_userid  # Calls groupfinder first time
effective_principals = request.effective_principals  # Calls groupfinder 2 times

Also, note that if you were to call effective_principals directly on the AuthenticationStackPolicy you would never end up calling it's authenticated_userid:

effective_principals = request.effective_principals

For example, would never call unauthenticated_userid or authenticated_userid on the AuthenticationStackPolicy, there is no guarantee that the callback passed to AuthenticationStackPolicy would ever be used before just getting the results from the underlying authentication policies. So you'd need to make sure that the callback passed to AuthenticationStackPolicy wasn't the one built to trust/not trust the result from unauthenticated_userid and trust blindly the sub-policies.

And since in the example there is no groupfinder passed to the AuthTkt implementation you are basically trusting whatever happens to be stored in the cookie, protected by a single secret.

Overall I don't think it is a good idea to add this to Pyramid core. There are significant pitfalls in the current implementation, and interoperability with other policies is meh at best (Mozilla's multiauth has similar pitfalls). I really like the fact that Pyramid has such a wide range and scope of add-ons that can be pulled from as necessary, and let the eco-system outside of core flourish.

For supporting auth_tkt or token/apikey there are different paths that are going to be followed. I would argue you'd want the authentication policy to be a factory that can instantiate the correct one depending on how the request is being made (for example, if Authorization header exists, do api key, if auth tkt cookie exists use authtkt based policy), or have the policy split at a different boundary.

For the example in the code, if you store a token server-side as I argue for in my pyramid_authsanity project you could also store the length of time since the user last authenticated, and use that to decide on what effective_principals to use, rather than the timeout set in a cookie (and it allows you to invalidate all other sessions/"sensitive" when a user changes their password for example).

[ergo]

You raised some very valid concerns here, thanks for that. Would following policy make more sense then?
It would pick existing ones based on what was present in request object itself. Maybe this would be a better candidate for inclusion, and if not - it could be added as an example to documentation.

@implementer(IAuthenticationPolicy)
class PyramidSelectorPolicy(object):

    def __init__(self, policy_selector=None, policies=None):
        """
        Policy factory - a callable that accepts argument ``request`` and
        decides which policy instance should be returned based on that.
        That name will be added to request object as an attribute ``matched_auth_policy``.
        The factory should always return a policy.

        Example usage::

            auth_tkt = AuthTktAuthenticationPolicy(...)
            auth_token_policy = AuthTokenAuthenticationPolicy(...)

            def policy_selector(request):
                # default policy
                policy = "auth_tkt"
                # return API token policy if header is present
                if request.headers.get("x-testscaffold-auth-token"):
                    policy = "auth_token_policy"
                log.info("Policy used: {}".format(policy))
                return policy

            auth_policy = PyramidSelectorPolicy(
                policy_selector=policy_selector,
                policies={
                    "auth_tkt": auth_tkt,
                    "auth_token_policy": auth_token_policy
                }
            )
            Configurator(settings=settings, authentication_policy=auth_policy,...)

        :param policy_factory:
        """
        self.policy_selector = policy_selector
        self.policies = policies

    def _get_policy(self, request, policy_key):
        if policy_key not in self.policies:
            raise ValueError("Policy {} is not found in PyramidSelectorPolicy".format(policy_key))
        request.matched_auth_policy = policy_key
        return self.policies[policy_key]

    def authenticated_userid(self, request):
        """ Return the authenticated :term:`userid` or ``None`` if
        no authenticated userid can be found. This method of the
        policy should ensure that a record exists in whatever
        persistent store is used related to the user (the user
        should not have been deleted); if a record associated with
        the current id does not exist in a persistent store, it
        should return ``None``.
        """
        policy = self._get_policy(request, self.policy_selector(request))
        return policy.authenticated_userid(request)

    def unauthenticated_userid(self, request):
        """ Return the *unauthenticated* userid.  This method
        performs the same duty as ``authenticated_userid`` but is
        permitted to return the userid based only on data present
        in the request; it needn't (and shouldn't) check any
        persistent store to ensure that the user record related to
        the request userid exists.

        This method is intended primarily a helper to assist the
        ``authenticated_userid`` method in pulling credentials out
        of the request data, abstracting away the specific headers,
        query strings, etc that are used to authenticate the request.
        """
        policy = self._get_policy(request, self.policy_selector(request))
        return policy.unauthenticated_userid(request)

    def effective_principals(self, request):
        """ Return a sequence representing the effective principals
        typically including the :term:`userid` and any groups belonged
        to by the current user, always including 'system' groups such
        as ``pyramid.security.Everyone`` and
        ``pyramid.security.Authenticated``.
        """
        policy = self._get_policy(request, self.policy_selector(request))
        return policy.effective_principals(request)

    def remember(self, request, userid, **kw):
        """ Return a set of headers suitable for 'remembering' the
        :term:`userid` named ``userid`` when set in a response.  An
        individual authentication policy and its consumers can
        decide on the composition and meaning of **kw.
        """
        policy = self._get_policy(request, self.policy_selector(request))
        return policy.remember(request, userid, **kw)

    def forget(self, request):
        """ Return a set of headers suitable for 'forgetting' the
        current user on subsequent requests.
        """
        policy = self._get_policy(request, self.policy_selector(request))
        return policy.forget(request)

[digitalresistor]

I wouldn't be opposed to adding something to core, but I want to see the API that shakes out of #3422 (comment) first before even considering adding something new to core. Especially since we are attempting to make the authentication/authorization story much better overall (and thus hopefully simplifying how to do this sort of stuff)

[ergo]

Sure, makes sense to not double the work.
What do you think about the approach I took here, as suggested this just picks one policy based on request. Single request sees/interacts with one policy object at same time.

[digitalresistor]

@ergo I like it much better than the previous solution. I also believe it to be more correct.

[mmerickel]

I'd really like to see an impl of this idea usable with the ISecurityPolicy from #3422. It's an open question how that would look. Since the changes relax the contract I think it's actually a little harder to define a stackable authentication policy.

[ergo]

I have a toy app with following policy and it switches between policies without issues.

@implementer(ISecurityPolicy)
class PyramidSelectorPolicy(object):
    def __init__(self, policy_selector=None, policies=None):
        """
        Policy factory - a callable that accepts argument ``request`` and
        decides which policy instance should be returned based on that.
        That name will be added to request object as an attribute ``matched_security_policy``.
        The factory should always return a policy.

        Example usage::

            auth_tkt = AuthTktAuthenticationPolicy(...)
            auth_token_policy = AuthTokenAuthenticationPolicy(...)

            def policy_selector(request):
                # default policy
                policy = "auth_tkt"
                # return API token policy if header is present
                if request.headers.get("x-auth-token"):
                    policy = "auth_token_policy"
                log.info("Policy used: {}".format(policy))
                return policy

            security_policy = PyramidSelectorPolicy(
                policy_selector=policy_selector,
                policies={
                    "auth_tkt": auth_tkt,
                    "auth_token_policy": auth_token_policy
                }
            )
            Configurator(settings=settings, security_policy=security_policy,...)

        :param policy_factory:
        """
        self.policy_selector = policy_selector
        self.policies = policies

    def _get_policy(self, request, policy_key):
        if policy_key not in self.policies:
            raise ValueError(
                "Policy {} is not found in PyramidSelectorPolicy".format(policy_key)
            )
        request.matched_security_policy = policy_key
        return self.policies[policy_key]

    def identify(self, request):
        """ Return an object identifying a trusted and verified user.  This
        object may be anything, but should implement a ``__str__`` method that
        outputs a corresponding :term:`userid`.

        """
        policy = self._get_policy(request, self.policy_selector(request))
        return policy.identify(request)

    def permits(self, request, context, identity, permission):
        """ Return an instance of :class:`pyramid.security.Allowed` if a user
        of the given identity is allowed the ``permission`` in the current
        ``context``, else return an instance of
        :class:`pyramid.security.Denied`.

        """
        policy = self._get_policy(request, self.policy_selector(request))
        return policy.permits(request, context, identity, permission)

    def remember(self, request, userid, **kw):
        """ Return a set of headers suitable for 'remembering' the
        :term:`userid` named ``userid`` when set in a response.  An
        individual authentication policy and its consumers can
        decide on the composition and meaning of ``**kw``.

        """
        policy_key = kw.get("policy")
        policy = self._get_policy(request, policy_key)
        return policy.remember(request, userid, **kw)

    def forget(self, request, **kw):
        """ Return a set of headers suitable for 'forgetting' the
        current user on subsequent requests, you may pass `policy`
        name to specify which identity should be forgotten, otherwise
        currently matched identity will be removed if possible.

        """
        policy_key = kw.get("policy") or getattr(
            request, "matched_security_policy", None
        )
        if policy_key is not None:
            policy = self._get_policy(request, policy_key)
            return policy.forget(request)

[ergo]

Example usage in a view:

    remember(request, "aaaaaa", policy="policy_a_name")
    remember(request, "bbbbb", policy="policy_b_name")
    # assumes **kw support in forget()
    forget(request) # forget currently matched identity
    forget(request, policy="policy_a_name")
    forget(request, policy="policy_b_name")

So far the one issue I've noticed is that currently pyramid's forget() unlike remember() does not accept any keywords, thus there is no way to specify which identities should be forgotten.

Adding a **kw argument to forget() API would be backwards compatible change, alternative solutions if policy cannot be passed:

• remove only currently matched identity (potential security issue)
• traverse registered policies and remove from all (potentially expensive operation?)
• add something that would grab current policy from registry and call forget on it (hacky and deviates from documented and standardized APIs of the framework)

Bonus question - would it make sense to have __all__ policy name to signify that all policies should be used by remember/forget?

[ergo]

Hi, any thoughts on this?

[digitalresistor]

I haven't had a chance to take a look at the latest updates to the security policy stuff, but this multiple stack switching looks okay to me.

[luhn]

IMO, I prefer the authstack implementation of just looping through the policy stack and returning the result of the first hit. A policy_selector seems like an unnecessary knob.

Overall, I'm in favor of adding a stacked auth policy to the core.

[ergo]

@luhn there may be cases where you want to sign out a single device/identity, or looking up identity via first hit could be expensive compared to others, there should be a way to customize the behavior.

[luhn]

I'm not convinced the value of the knob is worth the increased barrier to entry. Every user would need to implement it, no matter how simple their needs. To make matters worse, in order to properly do that they'd need to understand the implementation details of the policies they're using, to determine which one is "active."

Users with simple needs shouldn't have to do more work to satisfy the users with complicated needs.

[ergo]

You can ship default implementation of policy_selector that loops and make it replacable for people that need it - and very often you need it if you start stacking in the first place and/or you have any serious traffic to handle. I undestand your argument but "simple needs" should not prevent users with complicated needs to get things working and this is why most users pick pyramid in the first place, you start simple and grow, heck if you start simple you most likely don't need multiple policies in the first place.

[ergo]

@luhn I don't think you need to know which policy is currently active for request - it will be available as request.matched_security_policy.

[luhn]

I was referring strictly to policy_selector. To write that function, you need to know the implementation details of the child policies.

Can you give an example of how a policy selector could be more efficient than looping through well-implemented child policies? The more I think about it, assuming the child identity methods return as quick as they possibly can, a selector and a loop end up being equivalent.

[ergo]

@luhn A policy could check database for example (like api key policy - looking up header names/whatever) this is common in multi-tenant environments, a framework should not guess what their users might want to do.

The more I think about it, assuming the child identity methods return as quick as they possibly can, a selector and a loop end up being equivalen.

Then a default selector that loops would resolve the problem you raise here - correct?

[luhn]

I still don't see the need, but as long as there's a sensible default I won't make too much fuss.

I'm interested to hear what everybody else thinks.

[ergo]

Closing this since ISecurityPolicy makes this very easy to implement.