PoC: enforce upstream IdP session_expiry ceiling#117
Conversation
…laim extraction Add a login-time lockout guard to complete_interactive_login: when the upstream IdP asserts a session_expiry ceiling already in the past at login (compared against the ID token iat with the same 30s leeway as read-time enforcement), raise the new flow-agnostic SessionExpiredError instead of persisting an already-expired session. A missing claim stays a no-op, preserving existing behavior. Generalize extract_session_expiry into extract_epoch_claim(claims, name), reused for both session_expiry and iat, and rename the ceiling predicates to is_session_ceiling_reached (read-time) and is_session_ceiling_in_past (login). Document the login rejection in the README, RetrievingData guide, and the ipsie-webapp example.
…m boundary The signature-verified, Auth0-issued ID token has already been validated upstream (the platform refuses to emit a malformed session_expiry), so the SDK reads it like every other operational claim instead of running a bespoke validator. Removes State.extract_epoch_claim and reads session_expiry/iat with a plain .get() at both extraction sites; the None guards in the ceiling comparison functions still deliver the absent/null "no ceiling" safe default. Production-reachable inputs (absent/null, clean integer) are unchanged; only unreachable malformed values change behavior, now failing closed rather than being silently accepted.
| user_claims = UserClaims.parse_obj(user_info) | ||
| # authlib populates `userinfo` from parsed ID token claims, so the | ||
| # IPSIE session_expiry claim may surface here. | ||
| session_expires_at = user_info.get("session_expiry") |
There was a problem hiding this comment.
I want to make sure we are reading session_expiry from a trusted source here. In the id_token branch below we verify the signature and run the normalized issuer check before we trust any claim. On this userinfo branch we read session_expiry and iat straight off the dict without either of those checks. Since session_expiry is the thing that caps the session, trusting it from an unverified source feels risky. The comment says authlib fills userinfo from the parsed ID token, so does authlib actually verify that token's signature for us in every flow we support ? If we are not sure, I would rather pull session_expiry from the verified claims path so both branches are equally safe.
|
|
||
| # Refuse to persist a session whose ceiling is already in the past. | ||
| if State.is_session_ceiling_in_past(session_expires_at, issued_at): | ||
| raise SessionExpiredError() |
There was a problem hiding this comment.
When we raise here we skip the transaction cleanup that happens later in the method, so the transaction record is left behind. The auth code has already been spent at fetch_token, so the user cannot reuse this transaction anyway, a retry will start a brand new one. So the old record just sits there until it expires. Is leaving it intentional, or should we delete the transaction before raising so we do not pile up dead records ? The test comment says we leave it so the user can retry, but a retry does not reuse it, so that reasoning does not quite hold.
|
|
||
| # Once the session ceiling has passed, fail instead of serving or refreshing a token. | ||
| internal = (state_data_dict or {}).get("internal") or {} | ||
| if State.is_session_ceiling_reached(internal.get("session_expires_at")): |
There was a problem hiding this comment.
This is the same ceiling check plus session delete that _is_session_expired_by_ceiling already does. The only difference is that here we raise instead of returning None. Could we have the helper do the check and delete, and just decide here whether to raise or return ? That way the leeway and delete behavior live in one place and cannot drift apart later.
| # (it doesn't round-trip the upstream IdP), so the value from the | ||
| # refreshed ID token must NOT overwrite or erase the original | ||
| # ceiling — doing so would let the session outlive its bound. | ||
| internal = dict(state_data_dict.get("internal") or {}) |
There was a problem hiding this comment.
Just so I understand this line: the return below already spreads state_data_dict, which carries internal along with session_expires_at, and the refresh response has no path that writes into internal. So is this copy actually changing anything, or is it just a defensive copy ? The comment makes it sound like without this line the ceiling would get erased on refresh, but I do not think the old code ever touched internal. If it is defensive only, can we soften the comment so it does not imply a bug that was not there ?
| if session_expires_at is None: | ||
| return False | ||
| reference = issued_at if issued_at else int(time.time()) | ||
| return session_expires_at <= (reference + cls.SESSION_EXPIRY_LEEWAY_SECONDS) |
There was a problem hiding this comment.
Small thing: this uses if issued_at, so an iat of 0 would fall through to wall clock now. In practice iat is never 0, so this is not a real bug, but if issued_at is not None would read more clearly and match how we guard the ceiling itself.
| # ============================================================================= | ||
|
|
||
|
|
||
| def test_is_session_ceiling_reached_none_never_expires(): |
There was a problem hiding this comment.
Nice coverage on the ceiling math and the read and login paths. Two gaps I noticed. First, nothing tests that a refresh keeps the original ceiling, which is the whole reason for the internal copy in helpers.py, so a regression there would slip through. Could we add a test that runs update_state_data with a refresh response and checks session_expires_at is still set ? Second, all the login tests go through the id_token branch. The userinfo branch that reads session_expiry is never exercised, and that is the branch I asked about on the security side. A test for it would help.
2 participants
Adds enforcement of the IPSIE
session_expiryclaim for enterprise connections.When the connection is configured to honor an upstream IdP session ceiling, Auth0
includes a
session_expiryclaim (absolute Unix seconds) in the ID token. The SDKreads it at login, stores it with the session, and enforces it on every session read.
Changes
Enforcement
session_expiryfrom userinfo / verified ID-token claims atcomplete_interactive_loginand stamps it onto the internal session state.get_user()andget_session()returnNoneonce the ceiling is reached(silent — behaves like no session, so existing redirect-to-login fires).
get_access_token()raisesAccessTokenErrorwith codesession_expired(loud — checked before serving cache or refreshing; refresh is never attempted).
Claims access
session_expiryis surfaced onUserClaims, so it can be read viaget_user()without triggering enforcement.New / changed types & errors
UserClaims.session_expiry: Optional[int]InternalStateData.session_expires_at: Optional[int]AccessTokenErrorCode.SESSION_EXPIRED = "session_expired"