ERP5Security: make ERP5AccessTokenExtractionPlugin work with user ids

Make this plugin also an IAuthenticationPlugin which does all the job of
returning the user id.
It does not really make sense to delegate this to default authenticator.
A side effect is that token can still authenticate users with no
assignments, since tokens are scriptable, if this is a requirement, it
can be implemented in scripts.

Because this was broken, we took the liberty to introduce a breaking
change to fix naming, now type based scripts are getUserId.

product/ERP5Security/ERP5AccessTokenExtractionPlugin.py

@@ -59,35 +59,49 @@ class ERP5AccessTokenExtractionPlugin(BasePlugin):
   #ILoginPasswordHostExtractionPlugin#
   ####################################
   security.declarePrivate('extractCredentials')
-  @UnrestrictedMethod
   def extractCredentials(self, request):
-    """ Extract CookieHash credentials from the request header. """
+    """ Extract credentials from the request header. """
     creds = {}
-    # XXX Extract from HTTP Header, URL parameter are hardcoded.
-    # More flexible way would be to configure on the portal type level
-    token = request.getHeader("X-ACCESS-TOKEN", None)
-    if token is None:
-      token = request.form.get("access_token", None)
-    if token is not None:
+    # Extract token from HTTP Header
+    token = request.getHeader("X-ACCESS-TOKEN", request.form.get("access_token", None))
+    if token:
+      creds['erp5_access_token_id'] = token
+      creds['remote_host'] = request.get('REMOTE_HOST', '')
+      try:
+        creds['remote_address'] = request.getClientAddr()
+      except AttributeError:
+        creds['remote_address'] = request.get('REMOTE_ADDR', '')
+    return creds
+
+  #######################
+  #IAuthenticationPlugin#
+  #######################
+  security.declarePrivate('authenticateCredentials')
+  @UnrestrictedMethod
+  def authenticateCredentials(self, credentials):
+    """ Map credentials to a user ID. """
+    if 'erp5_access_token_id' in credentials:
+      erp5_access_token_id = credentials['erp5_access_token_id']
       token_document = self.getPortalObject().access_token_module.\
-                                        _getOb(token, None)
+                     _getOb(erp5_access_token_id, None)
       # Access Token should be validated
       # Check restricted access of URL
       # Extract login information
       if token_document is not None:
-        external_login = None
+        # Token API changed from returning a login to returning a user id.
+        # We detect if the old way of configuration is still in place and
+        # advise that configuration has to be updated in that case.
         method = token_document._getTypeBasedMethod('getExternalLogin')
+        assert method is None, "Please update and remove obsolete method %r" % method
+
+        user_id = None
+        method = token_document._getTypeBasedMethod('getUserId')
         if method is not None:
-          external_login = method()
-
-        if external_login is not None:
-          creds['external_login'] = external_login
-          creds['remote_host'] = request.get('REMOTE_HOST', '')
-          try:
-            creds['remote_address'] = request.getClientAddr()
-          except AttributeError:
-            creds['remote_address'] = request.get('REMOTE_ADDR', '')
-    return creds
+          user_id = method()
+
+        if user_id is not None:
+          return (user_id, 'token {erp5_access_token_id} for {user_id}'.format(**locals()))
+
 
 #Form for new plugin in ZMI
 manage_addERP5AccessTokenExtractionPluginForm = PageTemplateFile(
@@ -109,6 +123,7 @@ def addERP5AccessTokenExtractionPlugin(dispatcher, id, title=None, REQUEST=None)
 
 #List implementation of class
 classImplements(ERP5AccessTokenExtractionPlugin,
-                plugins.ILoginPasswordHostExtractionPlugin
-               )
+                plugins.ILoginPasswordHostExtractionPlugin,
+                plugins.IAuthenticationPlugin,
+                )
 InitializeClass(ERP5AccessTokenExtractionPlugin)

[Vincent Pelletier]

Picking a nit (getting rid of a try..except):

creds['remote_address'] = getattr(
  request,
  'getClientAddr',
  lambda: request.get('REMOTE_ADDR', ''),
)()

[Jérome Perrin]

This code is in other plugins as well, so we can change all places. I'm also wondering why getClientAddr would be not defined. It's defined in HTTPRequest (which inherits from BaseRequest) ... maybe FTP or webdav requests.

[Vincent Pelletier]

I'm also wondering why getClientAddr would be not defined.

Good point. And even for ftp & webdav, the notion of a client address is still applicable, so it should be present (EDIT: ...by which I mean, it can be monkey-patched and have a meaning if it's not actually present in Zope code). Maybe some Zope backward-compatibility ?

[Jérome Perrin]

I looked a bit more, even FTP uses HTTPRequest, BaseRequest seems to be never used directly. Also these tokens only really make sense for HTTP Requests, so I feel we can even drop the fallback and assume getClientAddr will always be defined here ( the request.form.get("access_token", None) above would anyway fail if this is not an HTTPRequest )

[Vincent Pelletier]

Isn't _getTypeBasedMethod expensive, especially when it does not find anything (iterate over all base types & classes, generate all possible names, getattr them on an acquisition context, going down in skins...) ?

Do we really want to check this before the new way ?

[Jérome Perrin]

That's true, it's a bit expensive.

Thinking twice, probably this was a bad idea. It would be the only place where the code dynamically try to detect if there is an obsolete customisation after an API change. If a project customized that, there should be something failing in the tests suite, or at least the authentication will not work not with this explicit message, but it will not work and it should not be too hard to find out customization has to be updated. So I'll amend the commit and drop this check.

[Vincent Pelletier]

**locals() ? Ew.

[Vincent Pelletier]

Also, would it make sense to put a relative url in there instead of a natural-language value ?

[Jérome Perrin]

**locals() ? Ew.

how about using https://github.com/asottile/future-fstrings ? ( it's a joke 😉 ). Seriously, for such a small string, yes it's maybe a bad idea if it's not your taste I can change.

Also, would it make sense to put a relative url in there instead of a natural-language value ?

erp5_access_token_id is the ID and by convention enforced by this plugin, all tokens are in access_token_module, so I was thinking a relative URL is not needed.

I did this because it might be useful for debugging sometimes, but honnestly, I'm not sure where it will appear. It appears in Base_viewSecurity, probably in error_log as well.

[Jérome Perrin]

ah but I see you mean just returning:

return (user_id, token_document.getRelativeUrl()) 

and this relative url would be enough for someone to understand that it's a token ?

That someone in the scenario I vaguely imagined was an admin debugging a problem or maybe auditing access.

[Vincent Pelletier]

Seriously, for such a small string, yes it's maybe a bad idea if it's not your taste I can change.

It's not about format, or about using keyword substitution, these I don't mind at all.

It's about using locals() for retrieving substitution values. Beyond niche I-really-have-only-printable-values-in-my-locals-and-I'm-rendering-a-template usage, I think it's not a reasonable pattern.

and this relative url would be enough for someone to understand that it's a token ?

Correct.