Skip to content

3.5.0: extra_id_token_claims callable is now invoked in stateless mode, where there is no user_db - #73

Open
alisonnRB wants to merge 1 commit into
IdentityPython:masterfrom
alisonnRB:stateless-mode-to-extra-id-token
Open

alisonnRB wants to merge 1 commit into
IdentityPython:masterfrom
alisonnRB:stateless-mode-to-extra-id-token

Conversation

@alisonnRB

Copy link
Copy Markdown

Since 3.5.0, _do_code_exchange invokes the extra_id_token_claims callable unconditionally. In 3.4.2 it was only invoked in the non-stateless branch:

# 3.4.2 — provider.py:449-454
extra_id_token_claims = {}
if self.stateless:
    extra_id_token_claims.update(self.authz_state.get_extra_id_token_claims_for_code(code))
elif callable(extra_id_token_claims):
    extra_id_token_claims = extra_id_token_claims(user_id, client_id)
# 3.5.0 — provider.py:452-464
extra_id_token_claims = (
    extra_id_token_claims(user_id, authentication_request['client_id'])
    if callable(extra_id_token_claims)
    else {} if not extra_id_token_claims else extra_id_token_claims
)
extra_id_token_claims_in_code = (
    self.authz_state.get_extra_id_token_claims_for_code(token_request['code'])
    if self.stateless else {}
)
extra_id_token_claims.update(extra_id_token_claims_in_code)

3.5.0 also made user_id defined in the stateless branch (user_id = sub, provider.py:437-442). In 3.4.2 user_id was left undefined there — which shows that branch was never reachable with a callable in stateless mode.
The problem is that a callable typically consults the provider's userinfo database. That is what SATOSA's OpenIDConnectFrontend does:

# satosa/frontends/openid_connect.py
def _get_extra_id_token_claims(self, user_id, client_id):
    if "extra_id_token_claims" in self.config["provider"]:
        config = self.config["provider"]["extra_id_token_claims"].get(client_id, [])
        if type(config) is list and len(config) > 0:
            requested_claims = {k: None for k in config}
            return self.provider.userinfo.get_claims_for(user_id, requested_claims)
    return {}

And with a stateless storage SATOSA deletes the user from user_db as soon as authorize() returns, since the claims are already embedded in the authorization code:

self.user_db[internal_resp.subject_id] = dict(combine_claim_values(claims.items()))
auth_resp = self.provider.authorize(auth_req, internal_resp.subject_id,
                                    extra_id_token_claims=lambda user_id, client_id: ...)
if self.stateless:
    del self.user_db[internal_resp.subject_id]   # user_db is just a {} when stateless

So at the token endpoint the callable receives a user_id that is no longer in the (empty) user_db, and Userinfo.get_claims_for raises:

File ".../pyop/provider.py", line 453, in _do_code_exchange
    extra_id_token_claims(user_id, authentication_request['client_id'])
File ".../satosa/frontends/openid_connect.py", line 367, in <lambda>
    lambda user_id, client_id: self._get_extra_id_token_claims(user_id, client_id))
File ".../pyop/userinfo.py", line 34, in get_claims_for
    userinfo = self._db[user_id] if user_id else {}
KeyError: '2889b8d6-...'

On top of that, in stateless mode the callable's return value is discarded anyway — extra_id_token_claims.update(extra_id_token_claims_in_code) overwrites it with the claims stored in the code.
Worth noting that pyop already handles exactly this situation correctly a few lines below, for requested_claims:

# provider.py:467-471
if self.stateless:
    user_info = self.authz_state.get_user_info_for_code(token_request['code'])
    user_claims = self.userinfo.get_claims_for(None, requested_claims, user_info)
else:
    user_claims = self.userinfo.get_claims_for(user_id, requested_claims)

Suggested fix — keep the 3.5.0 improvement (a plain dict is now honored; in 3.4.2 it was silently dropped) but do not invoke the callable when stateless:

if callable(extra_id_token_claims):
    # In stateless mode there is no user_db for the callable to consult, and its
    # result would be overwritten by the claims stored in the code below.
    extra_id_token_claims = (
        {} if self.stateless
        else extra_id_token_claims(user_id, authentication_request['client_id'])
    )
elif not extra_id_token_claims:
    extra_id_token_claims = {}

if self.stateless:
    extra_id_token_claims.update(
        self.authz_state.get_extra_id_token_claims_for_code(token_request['code'])
    )

Alternatively, or in addition, Userinfo.get_claims_for could tolerate an unknown subject, since it is documented as a read-only wrapper over the database:

userinfo = self._db.get(user_id, {}) if user_id else {}

@vladimir-mencl-eresearch

Copy link
Copy Markdown
Collaborator

I ran into I believe the same issue in SATOSA (using OIDC frontend) and solved it in IdentityPython/SATOSA#506

This has been merged - but not been released yet.

Would it solve the issue you're running into?

@alisonnRB

Copy link
Copy Markdown
Author

I ran into I believe the same issue in SATOSA (using OIDC frontend) and solved it in IdentityPython/SATOSA#506

This has been merged - but not been released yet.

Would it solve the issue you're running into?

Yes, that would solve my issue! Thanks for pointing that out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants