IdentityPython / oidc-op

An implementation of an OIDC Provider (OP)
Apache License 2.0
65 stars 27 forks source link

Session dump inconsistent state on some conditions #36

Closed peppelinux closed 3 years ago

peppelinux commented 3 years ago

@develop branch

If a first session have been done, the next one fails at verify_user view. the failure is cause by the fact that the flushed session database doesn't have a 'db' element in it

the patchy workaroud is this, in verify_user view:

    session_manager = ec.endpoint_context.session_manager
    dump = session_manager.dump()
    if not dump.get('db'):
        dump['db'] = {}
        session_manager.load(dump)

GHere the traceback of the error

Traceback (most recent call last):
  File "/path/to/DEV/IdentityPython/OIDC/env/lib/python3.8/site-packages/django/core/handlers/exception.py", line 47, in inner
    response = get_response(request)
  File "/path/to/DEV/IdentityPython/OIDC/env/lib/python3.8/site-packages/django/core/handlers/base.py", line 179, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/path/to/DEV/IdentityPython/OIDC/env/lib/python3.8/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "/path/to/DEV/IdentityPython/OIDC/django-oidc-op/example/oidc_provider/views.py", line 303, in verify_user
    _session_id = session_manager.create_session(
  File "/path/to/DEV/IdentityPython/OIDC/env/lib/python3.8/site-packages/oidcop/session/manager.py", line 189, in create_session
    _usi = self.get([user_id])
  File "/path/to/DEV/IdentityPython/OIDC/env/lib/python3.8/site-packages/oidcop/session/database.py", line 99, in get
    raise InconsistentDatabase('Missing session db')
oidcop.session.database.InconsistentDatabase: Missing session db

here the commented workaround in the example project https://github.com/peppelinux/django-oidc-op/commit/e97963c02afd4dee3eddcd8f8bee06e0b0dec3ff

peppelinux commented 3 years ago

this problem is still present

rohe commented 3 years ago

Where in django-oidc-op do you call the dump, load, flush methods the oidc-op provides ?

peppelinux commented 3 years ago

load in the endpoint views, example here BUT they're going to be refactored as ClassViews with method decorators https://github.com/peppelinux/django-oidc-op/blob/426003dc8c963b944b5e95431094335d8f9e9465/oidc_provider/views.py#L393

note that on each HTTP request I call the flush to avoid any exceptions that made db inconsistent (it will be in django sman decorator)

then I load the dump if client or http requests matches some already in my storage

I put also a consistency check (nott mandatory but usefull during DEV)

then I always flush the memory before doing a response, here: https://github.com/peppelinux/django-oidc-op/blob/426003dc8c963b944b5e95431094335d8f9e9465/oidc_provider/views.py#L102

I don't want to waste your time untill I made a refactor, just keep this issue open untill I have a closer look to that. I should finalize this during this weekend

peppelinux commented 3 years ago

but ... overall ... the flush always delete the db key element. And probably we could think that at least the structure, the schema, of the sman MUST be untouched, to avoid this kind of inconsistence.

even if it will cleaned of all its keys, it should rebuild them before returning the dictionary

This problably exceed my implementation in a more general approach to be handled in oidcmsg or in oidcop session_manager

peppelinux commented 3 years ago

Yes, it was definitely a bug that I patch here: https://github.com/IdentityPython/oidc-op/pull/59

with that PR key and salt always are present, even after flushes, and also now we have an additional feature (already present but unusable) to define key and salt in general configuration (good for distribuited workers).

ops ... test 50 failed ... probably I'll fix it this evening

peppelinux commented 3 years ago

Closed by https://github.com/IdentityPython/oidc-op/pull/59