oic.utils.SessionDB is not conducive to a shared DB with multiple SessionDB instances

[gobengo]

oic.utils.SessionDB is not conducive to a shared DB with multiple SessionDB instances

[gobengo]

An initial implementation is something like

"""
A better version of oic.utils.sdb:SessionDB.

The real one is not designed to afford for sessions shared between multiple
processes/hosts, because it uses an in-memory cache of .uid2sid to find sessions
for a given user.
"""

from oic.utils.sdb import SessionDB as OICSessionDB

from collections import Mapping
import itertools
import logging

LOG = logging.getLogger(__name__)


def uid_from_session(session):
    return session['authn_event'].uid


class SessionStorageUid2Sid(Mapping):
    """
    Implement object required by oic.oic.provider:Provider.uid2sid,
    except it's not just a dict. It's a Mapping that reads from a Mapping
    of Session IDs (sids) to Sessions (as created by
    oic.oic.provider:Provider.create_authz_session)
    https://github.com/rohe/pyoidc/issues/146
    """

    def __init__(self, sessions_by_id, uid_from_session=uid_from_session):
        self._sessions_by_id = sessions_by_id
        self._uid_from_session = uid_from_session

    def __getitem__(self, uid):
        session_items_for_uid = itertools.ifilter(
            lambda i: uid == self._uid_from_session(i[1]),
            self._sessions_by_id.items())
        # Would be neat if oic.utils.sdb explicitly cast to list() bcuz lazy
        return list(sid for (sid, session) in session_items_for_uid)

    def __iter__(self):
        sessions_sorted_by_uuid = sorted(self._sessions_by_id.values(),
                                         key=self._uid_from_session)
        for uid, user_sessions in itertools.groupby(sessions_sorted_by_uuid,
                                                    self._uid_from_session):
            yield uid

    def __len__(self):
        return len(iter(self))


def create_uid2sid_from_db(sessions_by_id):
    """
    :param db: `db` like that passed to SessionDB constructor
    :returns: Object that reads from db to provide a usable uid2sid map,
      which currently means:
      try:
          self.uid2sid[uid].append(sid)
      except KeyError:
          self.uid2sid[uid] = [sid]
      self.uid2sid.items()
      self.uid2sid.values()
      self.uid2sid[sid] = sid
    """
    LOG.debug('create_uid2sid_from_db %s', sessions_by_id)
    return SessionStorageUid2Sid(sessions_by_id)


class SessionDB(OICSessionDB):

    def __init__(self, *args, **kwargs):
        """
        :param uid2sid: Mapping of UID to session IDs
        """
        uid2sid = kwargs.pop('uid2sid', None)
        super(SessionDB, self).__init__(*args, **kwargs)
        # https://github.com/rohe/pyoidc/pull/147
        kwarg_db = kwargs.get('db')
        if kwarg_db is not None:
            self._db = kwarg_db
        self.uid2sid = uid2sid or create_uid2sid_from_db(self._db)

    def __delitem__(self, sid):
        # Exactly like super(), except don't re-assing .uid2sid
        del self._db[sid]

    def do_sub(self, sid, client_salt, sector_id="", subject_type="public"):
        """
        Construct a sub (subject identifier) and add it to the sdb[sid]

        This is exactly like super(), except it doesn't try to write
        to .uid2sid.
        (see https://github.com/Livefyre/lfoidc/pull/62/files#r43441487)

        :param sid: Session identifier
        :param sector_id: Possible sector identifier
        :param subject_type: 'public'/'pairwise'
        :param client_salt: client specific salt - used in pairwise
        :return:
        """
        uid = self._db[sid]["authn_event"].uid
        user_salt = self._db[sid]["authn_event"].salt

        if subject_type == "public":
            sub = hashlib.sha256("{}{}".format(uid, user_salt).encode("utf-8")).hexdigest()
        else:
            sub = pairwise_id(uid, sector_id, "{}{}".format(client_salt, user_salt))

        logger.debug("uid2sid: %s" % self.uid2sid)
        self.update(sid, 'sub', sub)

        return sub

I don't mind subclassing that much, except for having to override all of do_sub just to prevent the self.uid2sid[uid].append(sid) mutation. += [sid] would give the uid2sid object a chance to interpret as __setitem__ (I have proposed this as #148)

[rohe]

24 nov 2015 kl. 02:22 skrev Benjamin Goering [email protected]:

An initial implementation is something like

"""
A better version of oic.utils.sdb:SessionDB.

The real one is not designed to afford for sessions shared between multiple
processes/hosts, because it uses an in-memory cache of .uid2sid to find sessions
for a given user.
”""

The code looks ok to me.
Could you provide a couple of tests that covers the new code ?

— Roland

”Everybody should be quiet near a little stream and listen.”
From ’Open House for Butterflies’ by Ruth Krauss

[bjmc]

👍 Has this been merged yet? Or is subclassing still needed currently?

[bjmc]

@gobengo Did you want to add tests for this and see if we can get it merged?

[bjmc]

Has there been any progress on this issue?

[gobengo]

No progress.

I no longer work for the same organization using pyoidc, so don't have access to the tests I wrote back then. :/

Maybe best to close for now until someone else can spend time with it.

[rohe]

OK, I'll close it.

[bjmc]

Can I ask that we keep this open? If the only thing lacking are tests, I'll write some myself. Being able to run multiple instances is critical for production use in a high availability system.

[rohe]

Sure

[tpazderka]

OK, I had a look, and the easiest solution seems to be storing a reverse mappings ("uid" and "sub") in the same db object.

Other solution would be to create an ABC class that would force users to implement such reverse lookups based on their database implementation.

@schlenk I sort of like the second solution better, what about you?

[schlenk]

I think you are right, that an ABC would be nicer. Especially as the mapping between uid<-> sub is kind of temporary, while the client registration is more permanent. So it would not be total strange to have different storage strategies there.

[tpazderka]

Closed via #660