Remove call to map_kv2sid

[nsklikas]

State can't be assumed to be unique since the client chooses it, so we
can't use it as an identifier for getting the sid

In general I don't think don't see a use case for the map_kv2sid function.

[rohe]

True, but client_id+state should be unique.

[nsklikas]

Actually it is up to the client to decide whether the state will be unique (it should but there are no guarantees). In addition some clients may not even use state.

This is why I believe we should not rely on state for fetching the sid, since it is not within out control.

[peppelinux]

Interesting and generally I agree, state would not used as a search key because it's forged client side and it could produce a kind of escalation to get other sessions.

I found this weakness in my storage implementation in django-oidc-op, I'd like to go deeper.
@nsklikas can you refactor to get the unit tests working, then this PR will be merged in develop branch if @rohe agree.
Probably we'll have to investigate better this aspect as well, whatever wilt with this PR.

[peppelinux]

@nsklikas im my storage implementation (RDBMS ORM, whatever if MariaDB, sqlite, Postgres...) state is not unique but as you tell us it cannot be used as search key, because a state belonging to a session could collide to another.

Actually I get only the last from those are not expired.

Please corect me if you see something ambigous or wrong. We have the following flow in oidcendpoint, after a client registration have been done, and Authorization endpoint returns a HTML authentication form with a token and once the user has been authenticated, we have the first query in sso_db that checks if is there a sid linked to the state.

Probably we should link together client_id+state+sid or simply client_id+state and not state+sid (@rohe any clue on this?):

[2020-07-29 08:54:34,788] Known clients: ['1UUl6cwNigmj'] [(DEBUG) oidcendpoint.oidc.authorization.setup_auth:415]
[2020-07-29 08:54:35,039] SSODb set kbx7l8k28G6hA4YSSOAzwkYiHIDMwWgE - state: 3d7fca77424569736274644bf4df0d5050d27dbc3d40b54cfca204cb [(DEBUG) oidcendpoint.sso_db.set:16]
> /home/wert/DEV3/OIDC-Project/env/lib/python3.7/site-packages/oidcendpoint/sso_db.py(19)set()
-> _dic = self._db.get(key, {})
(Pdb) a
self = <oidcendpoint.sso_db.SSODb object at 0x7f0dc83bb310>
key = 'kbx7l8k28G6hA4YSSOAzwkYiHIDMwWgE'
sec_key = 'state'
value = '3d7fca77424569736274644bf4df0d5050d27dbc3d40b54cfca204cb'

It didn't find any, so set it:

[2020-07-29 08:57:15,600] <oidc_op.db_interfaces.OidcSsoDb object at 0x7f0dc83c2610> can't find any attribute with this name as attribute: kbx7l8k28G6hA4YSSOAzwkYiHIDMwWgE [(DEBUG) oidc_op.db_interfaces.get:254]
DEBUG:oidc_op.db_interfaces:<oidc_op.db_interfaces.OidcSsoDb object at 0x7f0dc83c2610> can't find any attribute with this name as attribute: kbx7l8k28G6hA4YSSOAzwkYiHIDMwWgE
[2020-07-29 08:57:15,774] SSODb set 3d7fca77424569736274644bf4df0d5050d27dbc3d40b54cfca204cb - uid: wert [(DEBUG) oidcendpoint.sso_db.set:16]
> /home/wert/DEV3/OIDC-Project/env/lib/python3.7/site-packages/oidcendpoint/sso_db.py(19)set()
-> _dic = self._db.get(key, {})

Then it get or set a sid, linked to that state:

[2020-07-29 08:57:57,325] SSODb set wert - sid: 3d7fca77424569736274644bf4df0d5050d27dbc3d40b54cfca204cb [(DEBUG) oidcendpoint.sso_db.set:16]
> /home/wert/DEV3/OIDC-Project/env/lib/python3.7/site-packages/oidcendpoint/sso_db.py(19)set()
-> _dic = self._db.get(key, {})
(Pdb) a
self = <oidcendpoint.sso_db.SSODb object at 0x7f0dc83bb310>
key = 'wert'
sec_key = 'sid'
value = '3d7fca77424569736274644bf4df0d5050d27dbc3d40b54cfca204cb'

So, by my point of view, the weakness is the in first query, when sso_db try to get the session quering the state value.
In a RP forges a state value that collides to another there will be a mess.

I put some kind of remediations, filtering only valid_until sessions, cleaning them on user logout and scheduling an expired sessions cleanup. All this belong to my data handler and oidc-op implementation.
What do you think about this, is there something that we could generlize for oidcendpoint?

[peppelinux]

Can't merge, tests still fails
https://travis-ci.com/github/IdentityPython/oidcendpoint/jobs/360432132#L437

[rohe]

I think we need to rethink the session handling. Would like to use this PR as input to that work.

[peppelinux]

A long Road ... Which Will be the concepts and principles to handle, ideally, a persistent session would be the question

[peppelinux]

ILGTM
we should keep an eye on the branch that Roland have created for the new session model

[peppelinux]

@rohe let's merge this before the new session storage, this latter would be merged on top. Do you think that there Will be conflicts?

if It Is so, would this be closed when new session storage Will be merged?

[rohe]

There will probably be conflicts but that can be dealt with then.