Skip to content.

plope

Personal tools
You are here: Home » Pyramid "Auth" API Design Postmortem
 
 

Pyramid "Auth" API Design Postmortem

A postmortem of a set of poor API design choices in Pyramid.

Here's a postmortem of poor API design in the Pyramid web framework. Unfortunately, when designing a framework, you get exactly one shot at creating "the right" API. In this case, we didn't. This design mistake isn't nearly fatal, it's just annoying. We won't be changing any Pyramid APIs, even though the design choices we discuss below are indeed poor, because the APIs involved are already in fairly wide use. But it's useful to record why these decisions were wrong for future reference and for the benefit of other framework developers.

Pyramid has the concept of a pluggable "authentication policy", which interrogates the request, checking and validating credentials. It has an API that looks like this:

  class IAuthenticationPolicy(Interface):
      """ An object representing a Pyramid authentication policy. """
      def authenticated_userid(request):
          """ Return the authenticated 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``."""

      def unauthenticated_userid(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 neednt (and
          shouldn't) check any persistent store to ensure that the user record
          related to the request userid exists."""

      def effective_principals(request):
          """ Return a sequence representing the effective principals
          including the userid and any groups belonged to by the current
          user, including 'system' groups such as Everyone and
          Authenticated. """

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

      def forget(request):
          """ Return a set of headers suitable for 'forgetting' the
          current user on subsequent requests. """

Pyramid also has the concept of a pluggable authorization policy. The responsibility of an authorization policy is to determine whether a user is permitted to perform a particular action. It depends on information retrieved from the authentication policy (namely, the "principals" associated with a user, as usually retrieved from effective_principals). Here's the API of an authorization policy:

  class IAuthorizationPolicy(Interface):
      """ An object representing a Pyramid authorization policy. """
      def permits(context, principals, permission):
          """ Return True if any of the principals is allowed the
          permission in the current context, else return False """

      def principals_allowed_by_permission(context, permission):
          """ Return a set of principal identifiers allowed by the permission """

Here are the mistakes in these APIs:

  • The authentication policy authenticated_userid API is too complex: in particular, the method is required to return None if the user does not exist in persistent storage. This means that the authentication policy is responsible not only for interrogating the request for credentials but also requires cooperation from code that inspects the user's database. This has the unfortunate real-world effect that using a "stock" authentication policy shipped with Pyramid requires that a "knob" function called a "groupfinder" be passed to the authentication policy at its construction time. A "groupfinder" knows about the user's particular user store and either returns a list of groups or the None value if the user doesn't exist in the database.
  • The authentication policy effective_principals API is too complex: particular, the method is required to return group information. Like, authenticated_userid, this also means that the authentication policy is responsible both for interrogating the request for credentials but also requires cooperation from code that inspects the user's database. If a "stock" authentication policy is used, the "groupfinder" detailed in the above bullet point is used for this purpose too.
  • The authorization policy deals in "principals" and the idea of principals is baked into the framework. While the idea of a "principal" (an abstraction representing either a user or a group) is simple enough to understand, the fact that it's baked into the framework rather than being an implementation detail of a particular authorization policy is incorrect.

Why, exactly, are these choices wrong?

  • The "groupfinder" is a "knob on a knob". A Pyramid authentication policy is already replaceable wholesale; you can write a custom authentication policy and use it as necessary. However, because writing a custom authentication policy no fun, the default authentication policies themselves have become miniframeworks by allowing (really, requiring) a user to pass a "groupfinder" function. This is a common source of confusion. It would be much better if there was only one "knob" for a user to turn: registering a custom authentication policy, rather than two (allowing a custom authentication policy or allowing them to use the groupfinder miniframework with stock authentication policies). But for it to be feasible for a nonexpert user to create an authentication policy, the contract of the policy itself needs to be simpler.
  • If the user wants to control the horizontal and vertical of authorization, they have to override both the authentication policy and the authorization policy because both rely on persistent storage local to the application itself. This is bad.

How would we fix it if we had it to do all over again?

We'd ditch the idea of an "authentication policy", and instead just have an "identity policy". The responsibilities of the identity policy would be to return the identity, remember the identity, and forget the identity. It would make no judgment about whether the userid related to the identity "exists" in the application, it just tells you who the request claims the user to be:

  class IIdentityPolicy(Interface):
      """ An object representing a Pyramid identity policy. """
      def identify(request):
          """ Return the claimed identity of the user associated request or
          ``None`` if no identity can be found associated with the request."""

      def remember(request, identity, **kw):
          """ Return a set of headers which can be used to remember
          ``identity`` for a subsequent request.  An individual identity
          policy and its consumers can decide on the composition and meaning of
          **kw."""

      def forget(request):
          """ Return a set of headers which can be used to 'forget' the current
          identity on subsequent requests."""

Users would rarely need to create a custom identity policy. The stock implementations would be used almost exclusively.

We'd simplify the interface of an "authorization policy". Make it just an instance that implements two methods: "authorized_userid" and "permits". "authorized_userid" would be passed the identity from the identity policy and will return a userid. "permits" would be passed the context, the identity, and a permission. The responsibility for determining if the identity "exists" is moved to the authorization policy, as well as determining group memberships and so forth:

  class IAuthorizationPolicy(Interface):
      """ An object representing a Pyramid authorization policy. """
      def permits(context, identity, permission):
          """ Return True if the userid is allowed the permission in the
          current context, else return False"""

      def authorized_userid(self, identity):
          """ Return the userid of the user identified by the identity 
          or 'None' if no user exists related to the identity """

The framework would no longer deal in "principals" at all. Users would influence authorization by supplying a custom authorization policy that might deal internally in "principals", but needn't do that if it's unnecessary (if there are no group memberships). We would move the ACL checking code to a library of some kind that would be used by authentication policy implementers as necessary, and we'd supply a default authorization policy that just didn't consider group memberships at all.

Here's a scifi example of how such a system might work:

  from zope.interface import Interface
  from zope.interface import implements

  from pyramid.location import lineage
  from pyramid import security
  from pyramid.decorator import reify

  from pyramid.interfaces import IIdentityPolicy
  from pyramid.interfaces import IAuthorizationPolicy

  class CookieIdentityPolicy(object):
      implements(IIdentityPolicy)
      def identify(self, request):
          return request.cookies.get('userid')

      def remember(self, request, identity, **kw):
          return [('Set-Cookie', 'userid=%s' % identity)]

      def forget(request):
          return [
              ('Set-Cookie',
               'userid=expired; Expires=Wed, 31-Dec-1971 01:01:01 GMT')
              ]

  class DictionaryAuthorizationPolicy(object):
      implements(IAuthorizationPolicy)
      def __init__(self, data):
          self.data = data
          self.acl_helper = ACLHelper()

      def permits(self, context, identity, permission):
          record =  self.data.get(identity)
          if record is not None:
              principals = record.get('principals', [])
              return self.acl_helper.permits(context, principals, permission)
          return False

      def authorized_userid(self, identity):
          if identity in self.data:
              return identity

  class ACLHelper(object):
      def permits(self, context, principals, permission):
          """ Return an instance of
          :class:`pyramid.security.ACLAllowed` instance if the policy
          permits access, return an instance of
          :class:`pyramid.security.ACLDenied` if not."""

          acl = '<No ACL found on any object in resource lineage>'

          for location in lineage(context):
              try:
                  acl = location.__acl__
              except AttributeError:
                  continue

              for ace in acl:
                  ace_action, ace_principal, ace_permissions = ace
                  if ace_principal in principals:
                      if not hasattr(ace_permissions, '__iter__'):
                          ace_permissions = [ace_permissions]
                      if permission in ace_permissions:
                          if ace_action == security.Allow:
                              return security.ACLAllowed(ace, acl, permission,
                                                         principals, location)
                          else:
                              return security.ACLDenied(ace, acl, permission,
                                                        principals, location)

          # default deny (if no ACL in lineage at all, or if none of the
          # principals were mentioned in any ACE we found)
          return security.ACLDenied(
              '<default deny>',
              acl,
              permission,
              principals,
              context)

  def emulate_router():
      from pyramid import testing
      ipol = CookieIdentityPolicy()
      apol = DictionaryAuthorizationPolicy(
          {'chris':{'principals':['chris', 'group:foo']}})
      request = testing.DummyRequest()
      context1 = testing.DummyModel()
      context1.__acl__ = [(security.Allow, 'group:foo', 'view')]
      context2 = testing.DummyModel()
      context2.__acl__ = [(security.Allow, 'others', 'edit')]
      request.cookies['userid'] = 'chris'
      identity = ipol.identify(request)
      permitted = apol.permits(context1, identity, 'view')
      assert(bool(permitted))
      permitted = apol.permits(context2, identity, 'edit')
      assert(not bool(permitted))

  def emulate_user():
      class User(object):
          pass
      class DummyDBConnection(object):
          def query(self, q):
              return User()
      from pyramid.registry import Registry
      ipol = CookieIdentityPolicy()
      apol = DictionaryAuthorizationPolicy(
          {'chris':{'principals':['chris', 'group:foo']}})

      # this would go in pyramid.security
      def authorized_userid(request):
          reg = request.registry
          ipol = reg.getUtility(IIdentityPolicy)
          apol = reg.getUtility(IAuthorizationPolicy)
          identity = ipol.identify(request)
          if identity is not None:
              userid = apol.authorized_userid(identity)
              if userid is not None:
                  return userid

      class RequestWithUser(object):
          registry = Registry()
          registry.settings = {}
          registry.settings['dbconn'] = DummyDBConnection()
          registry.registerUtility(ipol, IIdentityPolicy)
          registry.registerUtility(apol, IAuthorizationPolicy)
          cookies = {'userid':'chris'}
          @reify
          def user(self):
              userid = authorized_userid(self)
              if userid is not None:
                  return self.registry.settings['dbconn'].query({'user':userid})

      request = RequestWithUser()
      assert(request.user.__class__ is User)

  if __name__ == '__main__':
      emulate_router()
      emulate_user()

But, c'est la vie, that won't happen at this point. Sorry about the bad design.

Created by chrism
Last modified 2011-01-11 05:28 PM

Why specify persistence at all?

In your scifi specification, why does the ``authorized_userid`` method need to return None if no persistent user is found? And why does it return the userid rather than a user data object (presumably with name and email attributes)?

When using a signed cookie system such as mod_auth_tkt/pubcookie/etc. it is possible for an application to return the userid, and even some group tokens and user data, without reference to a persistent store - only the shared login server needs that.

I would have thought that the persistent storage was only an implementation detail, though clearly not all implementations would allow modification of user data or decorate a user object with extra attributes.

If one needs to differentiate between distributes (openid) userids and local userids, surely that would be better done by assigning local users a member role?

Laurence

1294791296

> In your scifi specification, why does the ``authorized_userid`` method need to return None if no persistent user is found?

To signal that no user exists by that name (the user was deleted from the database).

> And why does it return the userid rather than a user data object (presumably with name and email attributes)?

Because coming up with a generic "user" abstraction is not really possible.

> When using a signed cookie system such as mod_auth_tkt/pubcookie/etc. it is possible for an application to return the userid, and even some group tokens and user data, without reference to a persistent store - only the shared login server needs that.

Even if a user posseses such a token, he may have been, say, fired from his job and should no longer be allowed to access company resources. In many systems, we can't just let him in without checking for this.

> I would have thought that the persistent storage was only an implementation detail, though clearly not all implementations would allow modification of user data or decorate a user object with extra attributes.

It's easiest to just not deal in "user objects" at all.

> If one needs to differentiate between distributes (openid) userids and local userids, surely that would be better done by assigning local users a member role?

Sorry, no idea what that means.