Merge pull request #21 from peppelinux/master

lukewarm refactor of client_authn.py

Author: rohe

src/oidcendpoint/client_authn.py

@@ -136,17 +136,13 @@ def verify(self, request, **kwargs):
             logger.info("%s" % sanitize(err))
             raise AuthnFailure("Could not verify client_assertion.")
 
-        try:
-            logger.debug("authntoken: %s" % sanitize(ca_jwt.to_dict()))
-        except AttributeError:
-            logger.debug("authntoken: %s" % sanitize(ca_jwt))
+        authtoken = sanitize(ca_jwt)
+        if hasattr(ca_jwt, 'to_dict') and callable(ca_jwt, 'to_dict'):
+            authtoken = sanitize(ca_jwt.to_dict())
+        logger.debug("authntoken: {}".format(authtoken))
 
         request[verified_claim_name("client_assertion")] = ca_jwt
-
-        try:
-            client_id = kwargs["client_id"]
-        except KeyError:
-            client_id = ca_jwt["iss"]
+        client_id = kwargs.get("client_id") or ca_jwt["iss"]
 
         # I should be among the audience
         # could be either my issuer id or the token endpoint
@@ -244,51 +240,45 @@ def verify_client(
         else:
             raise UnknownOrNoAuthnMethod(authorization_info)
 
-    try:
-        client_id = auth_info["client_id"]
-    except KeyError:
-        try:
-            _token = auth_info["token"]
-        except KeyError:
+    client_id = auth_info.get("client_id")
+    _token = auth_info.get("token")
+
+    if client_id:
+
+        if not client_id in endpoint_context.cdb:
+            raise ValueError("Unknown Client ID")
+
+        _cinfo = endpoint_context.cdb[client_id]
+        if isinstance(_cinfo, str):
+            if not _cinfo in endpoint_context.cdb:
+                raise ValueError("Unknown Client ID")
+
+        if not valid_client_info(_cinfo):
+            logger.warning("Client registration has timed out")
+            raise ValueError("Not valid client")
+
+        # store what authn method was used
+        if auth_info.get("method"):
+            if endpoint_context.cdb[client_id].get("auth_method") and \
+               request.__class__.__name__ in endpoint_context.cdb[client_id]["auth_method"]:
+                endpoint_context.cdb[client_id]["auth_method"][
+                    request.__class__.__name__
+                ] = auth_info["method"]
+            else:
+                endpoint_context.cdb[client_id]["auth_method"] = {
+                    request.__class__.__name__: auth_info["method"]
+                    }
+
+    elif not client_id and get_client_id_from_token:
+        if not _token:
             logger.warning("No token")
-        else:
-            if get_client_id_from_token:
-                try:
-                    _id = get_client_id_from_token(endpoint_context, _token, request)
-                except KeyError:
-                    raise ValueError("Unknown token")
-
-                if _id:
-                    auth_info["client_id"] = _id
-    else:
+            raise ValueError("No token")
+
         try:
-            _cinfo = endpoint_context.cdb[client_id]
+            # get_client_id_from_token is a callback... Do not abuse for code readability.
+            auth_info["client_id"] = get_client_id_from_token(endpoint_context,
+                                                              _token, request)
         except KeyError:
-            raise ValueError("Unknown Client ID")
-        else:
-            if isinstance(_cinfo, str):
-                try:
-                    _cinfo = endpoint_context.cdb[_cinfo]
-                except KeyError:
-                    raise ValueError("Unknown Client ID")
-
-            try:
-                valid_client_info(_cinfo)
-            except KeyError:
-                logger.warning("Client registration has timed out")
-                raise ValueError("Not valid client")
-            else:
-                # store what authn method was used
-                try:
-                    endpoint_context.cdb[client_id]["auth_method"][
-                        request.__class__.__name__
-                    ] = auth_info["method"]
-                except KeyError:
-                    try:
-                        endpoint_context.cdb[client_id]["auth_method"] = {
-                            request.__class__.__name__: auth_info["method"]
-                        }
-                    except KeyError:
-                        pass
+            raise ValueError("Unknown token")
 
     return auth_info

src/oidcendpoint/in_memory_db.py

@@ -2,17 +2,19 @@ class InMemoryDataBase(object):
     def __init__(self):
         self.db = {}
 
+    def __contains__(self, key):
+        if self.db.get(key):
+            return 1
+
     def set(self, key, value):
         self.db[key] = value
 
     def get(self, key):
-        try:
-            return self.db[key]
-        except KeyError:
-            return None
+        return self.db.get(key, None)
 
     def delete(self, key):
-        del self.db[key]
+        if self.db.get(key):
+            del self.db[key]
 
     def keys(self):
         return self.db.keys()

src/oidcendpoint/oidc/authorization.py

@@ -38,7 +38,7 @@
 from oidcendpoint.user_authn.authn_context import pick_auth
 from oidcendpoint.user_info import SCOPE2CLAIMS
 
-LOGGER = logging.getLogger(__name__)
+logger = logging.getLogger(__name__)
 
 FORM_POST = """<html>
   <head>
@@ -71,7 +71,7 @@ def inputs(form_args):
 
 def max_age(request):
     cn = verified_claim_name("request")
-    return request.get(cn, {}).get("max_age") or request.get("max_age", 0) 
+    return request.get(cn, {}).get("max_age") or request.get("max_age", 0)
 
 
 def re_authenticate(request, authn):
@@ -108,7 +108,7 @@ def verify_uri(endpoint_context, request, uri_type, client_id=None):
     _cid = request.get("client_id", client_id)
 
     if not _cid:
-        LOGGER.error("No client id found")
+        logger.error("No client id found")
         raise UnknownClient("No client_id provided")
 
     _redirect_uri = unquote(request[uri_type])
@@ -185,23 +185,26 @@ def get_uri(endpoint_context, request, uri_type):
     :param uri_type: 'redirect_uri' or 'post_logout_redirect_uri'
     :return: redirect_uri
     """
+    uri = ""
+
     if uri_type in request:
         verify_uri(endpoint_context, request, uri_type)
         uri = request[uri_type]
     else:
-        try:
-            _specs = endpoint_context.cdb[str(request["client_id"])][
-                "{}s".format(uri_type)
-            ]
-        except KeyError:
-            raise ParameterError("Missing {} and none registered".format(uri_type))
-        else:
+
+        uris = "{}s".format(uri_type)
+        client_id = str(request["client_id"])
+        if client_id in endpoint_context.cdb:
+            _specs = endpoint_context.cdb[client_id].get(uris)
+            if not _specs:
+                 raise ParameterError("Missing {} and none registered".format(uri_type))
+
             if len(_specs) > 1:
                 raise ParameterError(
                     "Missing {} and more than one registered".format(uri_type)
                 )
-            else:
-                uri = join_query(*_specs[0])
+
+            uri = join_query(*_specs[0])
 
     return uri
 
@@ -267,15 +270,15 @@ def create_authn_response(endpoint, request, sid):
         if "token" in rtype:
             _dic = _context.sdb.upgrade_to_token(issue_refresh=False, key=sid)
 
-            LOGGER.debug("_dic: %s" % sanitize(_dic))
+            logger.debug("_dic: %s" % sanitize(_dic))
             for key, val in _dic.items():
                 if key in aresp.parameters() and val is not None:
                     aresp[key] = val
 
             handled_response_type.append("token")
 
         _access_token = aresp.get("access_token", None)
-        
+
         if "id_token" in request["response_type"]:
             kwargs = {}
             if {"code", "id_token", "token"}.issubset(rtype):
@@ -291,7 +294,7 @@ def create_authn_response(endpoint, request, sid):
             try:
                 id_token = _context.idtoken.make(request, _sinfo, **kwargs)
             except (JWEException, NoSuitableSigningKeys) as err:
-                LOGGER.warning(str(err))
+                logger.warning(str(err))
                 resp = AuthorizationErrorResponse(
                     error="invalid_request",
                     error_description="Could not sign/encrypt id_token",
@@ -374,7 +377,7 @@ def _post_parse_request(self, request, client_id, endpoint_context, **kwargs):
         :return:
         """
         if not request:
-            LOGGER.debug("No AuthzRequest")
+            logger.debug("No AuthzRequest")
             return AuthorizationErrorResponse(
                 error="invalid_request", error_description="Can not parse AuthzRequest"
             )
@@ -383,7 +386,7 @@ def _post_parse_request(self, request, client_id, endpoint_context, **kwargs):
 
         _cinfo = endpoint_context.cdb.get(client_id)
         if not _cinfo:
-            LOGGER.error(
+            logger.error(
                 "Client ID ({}) not in client database".format(request["client_id"])
             )
             return AuthorizationErrorResponse(
@@ -414,7 +417,7 @@ def pick_authn_method(self, request, redirect_uri, acr=None, **kwargs):
         auth_id = kwargs.get("auth_method_id")
         if auth_id:
             return self.endpoint_context.authn_broker[auth_id]
-        
+
         if acr:
             res = self.endpoint_context.authn_broker.pick(acr)
         else:
@@ -462,7 +465,7 @@ def setup_auth(self, request, redirect_uri,
             identity = None
             _ts = 0
         except ToOld:
-            LOGGER.info("Too old authentication")
+            logger.info("Too old authentication")
             identity = None
             _ts = 0
         else:
@@ -474,22 +477,16 @@ def setup_auth(self, request, redirect_uri,
                 else:
                     identity = json.loads(as_unicode(_id))
 
-                    try:
-                        session = self.endpoint_context.sdb[identity["sid"]]
-                    except KeyError:
+                    session = self.endpoint_context.sdb[identity.get("sid")]
+                    if not session or "revoked" in session:
                         identity = None
-                    else:
-                        if session is None:
-                            identity = None
-                        elif "revoked" in session:
-                            identity = None
 
         authn_args = authn_args_gather(request, authn_class_ref,
                                        cinfo, **kwargs)
 
         # To authenticate or Not
         if identity is None:  # No!
-            LOGGER.info("No active authentication")
+            logger.info("No active authentication")
             if "prompt" in request and "none" in request["prompt"]:
                 # Need to authenticate but not allowed
                 return {
@@ -500,7 +497,7 @@ def setup_auth(self, request, redirect_uri,
             else:
                 return {"function": authn, "args": authn_args}
         else:
-            LOGGER.info("Active authentication")
+            logger.info("Active authentication")
             if re_authenticate(request, authn):
                 # demand re-authentication
                 return {"function": authn, "args": authn_args}
@@ -516,7 +513,7 @@ def setup_auth(self, request, redirect_uri,
                         sids[-1]
                     ).uid
                     ):
-                        LOGGER.debug("Wanted to be someone else!")
+                        logger.debug("Wanted to be someone else!")
                         if "prompt" in request and "none" in request["prompt"]:
                             # Need to authenticate but not allowed
                             return {
@@ -610,7 +607,7 @@ def post_authentication(self, user, request, sid, **kwargs):
                     response_info, "server_error", "{}".format(err.args)
                 )
 
-        LOGGER.debug("response type: %s" % request["response_type"])
+        logger.debug("response type: %s" % request["response_type"])
 
         if self.endpoint_context.sdb.is_session_revoked(sid):
             return self.error_response(
@@ -694,7 +691,7 @@ def authz_part2(self, user, authn_event, request, **kwargs):
 
                 opbs = session_cookie[ec.cookie_name["session_management"]]
 
-                LOGGER.debug("compute_session_state: client_id=%s, origin=%s, opbs=%s, salt=%s",
+                logger.debug("compute_session_state: client_id=%s, origin=%s, opbs=%s, salt=%s",
                              request["client_id"], resp_info["return_uri"], opbs.value, salt)
 
                 _session_state = compute_session_state(
@@ -753,8 +750,8 @@ def process_request(self, request_info=None, **kwargs):
 
         _function = info.get("function")
         if not _function:
-            LOGGER.debug("- authenticated -")
-            LOGGER.debug("AREQ keys: %s" % request_info.keys())
+            logger.debug("- authenticated -")
+            logger.debug("AREQ keys: %s" % request_info.keys())
             res = self.authz_part2(
                 info["user"], info["authn_event"],
                 request_info, cookie=cookie
@@ -768,5 +765,5 @@ def process_request(self, request_info=None, **kwargs):
                 "return_uri": request_info["redirect_uri"],
             }
         except Exception as err:
-            LOGGER.exception(err)
+            logger.exception(err)
             return {"http_response": "Internal error: {}".format(err)}

src/oidcendpoint/session.py

@@ -13,7 +13,7 @@
 from oidcendpoint import token_handler
 from oidcendpoint.authn_event import AuthnEvent
 from oidcendpoint.in_memory_db import InMemoryDataBase
-from oidcendpoint.sso_db import SSODb
+from oidcendpoint.sso_db import (SSODb, KEY_FORMAT)
 from oidcendpoint.token_handler import AccessCodeUsed
 from oidcendpoint.token_handler import ExpiredToken
 from oidcendpoint.token_handler import UnknownToken
@@ -119,7 +119,7 @@ def dict_match(a, b):
 
 class SessionDB(object):
     def __init__(self, db, handler, sso_db, userinfo=None, sub_func=None):
-        # db must implement the InMemoryStateDataBase interface
+        # db must implement the InMemoryDataBase interface
         self._db = db
         self.handler = handler
         self.sso_db = sso_db
@@ -224,13 +224,14 @@ def update_by_token(self, token, **kwargs):
         return self.update(_sid, **kwargs)
 
     def map_kv2sid(self, key, value, sid):
-        self._db.set("__{}__{}__".format(key, value), sid)
+        """ KEY_FORMAT = "__{}__{}" """
+        self._db.set(KEY_FORMAT.format(key, value), sid)
 
     def delete_kv2sid(self, key, value):
-        self._db.delete("__{}__{}__".format(key, value))
+        self._db.delete(KEY_FORMAT.format(key, value))
 
     def get_sid_by_kv(self, key, value):
-        return self._db.get("__{}__{}__".format(key, value))
+        return self._db.get(KEY_FORMAT.format(key, value))
 
     def get_token(self, sid):
         _sess_info = self[sid]
@@ -241,7 +242,8 @@ def get_token(self, sid):
             return _sess_info["access_token"]
 
     def do_sub(
-        self, sid, uid, client_salt, sector_id="", subject_type="public", user_salt=""
+        self, sid, uid, client_salt, sector_id="",
+        subject_type="public", user_salt=""
     ):
         """
         Create and store a subject identifier