Skip to content

✨(oidc) add support for generic OIDC IdPs and OIDC clients as users - #633

Open
piptouque wants to merge 6 commits into
openfun:mainfrom
piptouque:feat_generic_oidc_client_credentials
Open

✨(oidc) add support for generic OIDC IdPs and OIDC clients as users #633
piptouque wants to merge 6 commits into
openfun:mainfrom
piptouque:feat_generic_oidc_client_credentials

Conversation

@piptouque

Copy link
Copy Markdown
Contributor

Purpose

This PR contains multiple changes regarding OIDC

Fixes

Changes

Support for generic OIDC IdPs

More specifically, support for IdPs that return a token that is not a JWT.

The ID token and access token are different and have different purpose.
The ID token is always a JWT and contains user claims, but the access token may not be.
With this change, we get the user claims using the access token,
using the /userinfo OIDC endpoint, allowing us to support providers that return opaque access tokens.

Support OIDC clients as 'users'

Client applications, as authenticated with the 'client_credentials' flow, do not have a dedicated user.
As such, we can't get ID tokens for them, always opaque access tokens.
Instead, we authenticate them using their client_id.

But first, we need to check whether the access token we got was from a real Oauth2 user or a OIDC client application.
To do that, we query the /introspection endpoint of our IdP.
This requires that Ralph be registered as another client app to our IdP, and to have configured its client ID and secret.

Proposal

  • Fixed type of IDToken not following specs on aud (may be a list), exp and iat (may be floats)
  • Added query to /introspection endpoint when receiving an OIDC access token to determine if it comes from a ODIC client application or a user
  • Added query to /userinfo endpoint when receiving an OIDC access token if that token represents a user
  • Updated technical documentation
  • Updated CHANGELOG.md

piptouque added 3 commits June 15, 2026 11:23
The 'aud' claim may be a list:
https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.3

The 'exp' and 'iat' claims
are `NumericDate` and
may be floats:
https://www.rfc-editor.org/rfc/rfc7519#section-2
The server should ignore any
OAuth 2 scope that it does not
know, instead of returning an error.
The ID token and access token are different and
have different purpose.
The ID token is always a JWT and contains user claims,
but the access token may not be.
With this change, we get the user claims using the access token,
using the /userinfo OIDC enpoint,
allowing us to support providers that return opaque access tokens.

@MYilFun00 MYilFun00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this OIDC refactor — the move to introspection + /userinfo, Client Credentials support, and the scopes fix are solid improvements. However I found several blocking issues during local review:

Blocking

1. src/ralph/api/auth/oidc.pyget_token_info return type hint is wrong

Current implementation:

def get_token_info(...) -> UserInfo:
    ...
    return TokenInfo.model_validate(token_info)

The function returns TokenInfo, not UserInfo.

Suggested fix: change the annotation to -> TokenInfo.

2. tests/fixtures/auth.py + get_user_info_data — JWT /userinfo handling

Current mock:

return (200, {"Content-Type": "application/jwt"}, json.dumps(encoded_user_info))

Current production (get_user_info_data):

content_type = response.headers["Content-Type"]
return (response.json(), content_type.lower() == "application/jwt")

A real /userinfo endpoint with Content-Type: application/jwt returns the raw JWT in the response body. The current code calls response.json() unconditionally — this fails on a real IdP (JSONDecodeError). The mock passes by accident because json.dumps(jwt) makes response.json() return a Python string.

Both fixes are required together (tested locally):

Suggested fix (mock):

return (200, {"Content-Type": "application/jwt"}, encoded_user_info)

Suggested fix (production — get_user_info_data):

content_type = response.headers["Content-Type"]
media_type = content_type.split(";", 1)[0].strip().lower()
is_jwt = media_type == "application/jwt"
body = response.text if is_jwt else response.json()
return (body, is_jwt)

Local test results (current vs fix):

Content-Type Body Current code Fix (mock + prod)
application/json JSON dict ✅ dict ✅ dict
application/jwt raw JWT JSONDecodeError ✅ JWT string → jwt.decode OK
application/jwt; charset=utf-8 raw JWT JSONDecodeError ✅ JWT string
application/jwt+json raw JWT JSONDecodeError ❌ (non-standard type, expected)

⚠️ Applying the production fix without fixing the mock would still break tests: response.text on json.dumps(jwt) returns "eyJ..." (with JSON quotes) → jwt.decode fails. Both changes must land together.

3. src/ralph/api/auth/oidc.py L202 — f-string syntax breaks Python < 3.12

The multiline f-string in the Authorization header raises SyntaxError on Python 3.10/3.11. Project requires >=3.9, CI tests 3.9→3.12. Module import fails locally.

Suggested fix:

basic = encode_client_secret_basic_token(client_id=client_id, client_secret=client_secret)
headers={"Authorization": f"Basic {basic}"}

Non-blocking (nice to have / discuss before prod)

4. @lru_cache() keyed by access token — tested behaviour

Verified locally: @lru_cache() defaults to maxsize=128 (not unbounded). After 130 distinct tokens, currsize=128 (LRU eviction). Same token twice → cache hit (useful for repeated requests on one connection).

Remaining risks:

  • A revoked token can stay "valid" in cache until evicted (no TTL).
  • Up to 128 token introspection/userinfo results kept in memory.

Suggested fix (minimal): remove @lru_cache() from token-specific functions; keep it only on stable data:

@lru_cache(maxsize=1)   # discover_provider — OK (issuer config)
def discover_provider(...): ...

@lru_cache(maxsize=1)   # get_public_keys — OK (JWKS rotates rarely)
def get_public_keys(...): ...

# NO cache on get_token_info / get_user_info_data — tokens are short-lived
def get_token_info(...): ...
def get_user_info_data(...): ...

Suggested fix (if caching is desired): explicit bounded cache with TTL ≤ token lifetime:

@lru_cache(maxsize=128)  # document the choice
def get_token_info(...): ...
# + add cache_clear() in tests (already done for basic auth in fixtures)

Or a TTL cache (e.g. 60s) so revoked tokens are not served stale for long.


5. Double network call /introspect + /userinfo — flow analysis

Current flow (get_oidc_user):

  • User token (token_info.sub set): POST /introspectGET /userinfo → compare sub
  • Client credentials (sub absent): POST /introspect only ✅

So client-app auth is already optimized (1 call). User auth always costs 2 IdP round-trips vs 1 JWT decode on old main.

Suggested optimizations (pick one):

  • A (simplest): if introspection already returns scope + target + sub, skip /userinfo when all Ralph claims are present (config flag RUNSERVER_AUTH_OIDC_SKIP_USERINFO_IF_COMPLETE=true).
  • B (safe default): keep 2 calls but cache both per token with short TTL (see #4).
  • C (document only): accept 2 calls as OIDC-correct, document IdP latency impact in oidc.md.

The sub cross-check between introspection and userinfo is valuable — do not remove without replacement.


6. Content-Type: application/jwt detection — tested and verified

Prefer parsing the media type over startswith (avoids false positive on application/jwt+json):

Content-Type == "application/jwt" startswith("application/jwt") split(";")[0] parse
application/jwt
application/jwt; charset=utf-8 ❌ fails ✅ (works) ✅ (works)
application/jwt+json ⚠️ false positive ✅ correctly rejects

The recommended parse + response.text fix is verified locally against simulated IdP responses. See blocking issue #2 above for the complete production code — it is the same fix, and must be paired with the mock correction.

Do not use startswith("application/jwt") alone — it would treat application/jwt+json as JWT.


7. conf.py formatting

Current:

BASE_SETTINGS_CONFIG = SettingsConfigDict(
    case_sensitive=True, env_nested_delimiter="__", env_prefix="RALPH_", extra="ignore"
    , secrets_dir=os.environ.get("RALPH_SECRETS_DIR")
)

Suggested fix:

BASE_SETTINGS_CONFIG = SettingsConfigDict(
    case_sensitive=True,
    env_nested_delimiter="__",
    env_prefix="RALPH_",
    extra="ignore",
    secrets_dir=os.environ.get("RALPH_SECRETS_DIR"),
)

8. Long parametrize line in test_oidc.py

Line 23 is a single 109-char entry (readable but dense). Split for lint/readability:

@pytest.mark.parametrize(
    "runserver_auth_backends,userinfo_response_type",
    [
        ([AuthBackend.BASIC, AuthBackend.OIDC], "jwt"),
        ([AuthBackend.OIDC], "plain"),
        ([AuthBackend.OIDC], "jwt"),
    ],
)

Once the blocking issues above are addressed, please rebase on current main (includes CI fixes + #630/#632) before re-requesting review.

@MYilFun00 MYilFun00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Request changes

Thanks for this OIDC refactor — the move to token introspection combined with /userinfo, the addition of Client Credentials support, and the scope handling improvements are all valuable changes.

However, during my local review, I identified several blocking issues, along with a few points worth discussing before production deployment.

🔴 Blocking

1. src/ralph/api/auth/oidc.py — Incorrect return type annotation for get_token_info

Current implementation:

def get_token_info(...) -> UserInfo:
    ...
    return TokenInfo.model_validate(token_info)

The function returns a TokenInfo instance, not a UserInfo instance.

Suggested fix:

Update the return type annotation to:

def get_token_info(...) -> TokenInfo:

2. tests/fixtures/auth.py — JWT /userinfo mock does not reflect the real endpoint behavior

Current implementation:

return (
    200,
    {"Content-Type": "application/jwt"},
    json.dumps(encoded_user_info),
)

A real /userinfo endpoint returning Content-Type: application/jwt sends the raw JWT in the response body, not a JSON-encoded string.

The current mock causes the tests to pass accidentally because response.json() parses "eyJ..." into a Python string.

Suggested fix (test fixture):

Return encoded_user_info directly, without json.dumps().

Suggested fix (production code – get_user_info_data):

When the response content type is application/jwt, use response.text instead of response.json().


3. src/ralph/api/auth/oidc.py (L202) — Multiline f-string is incompatible with Python < 3.12

The multiline f-string used to build the Authorization header raises a SyntaxError on Python 3.9–3.11.

Since the project supports Python >= 3.9 and the CI pipeline runs against Python 3.9 through 3.12, the module cannot be imported on supported versions.

Suggested fix:

basic = encode_client_secret_basic_token(
    client_id=client_id,
    client_secret=client_secret,
)

headers = {"Authorization": f"Basic {basic}"}


🟡 Non-blocking / Discussion points

4. @lru_cache() on access tokens — verified behavior

Verified locally:

  • @lru_cache() defaults to maxsize=128 (it is not unbounded).

  • After 130 distinct access tokens, currsize=128 and LRU eviction occurs.

  • Reusing the same token correctly results in a cache hit.

While the implementation works, a few concerns remain:

  • A revoked token may continue to be considered valid until it is evicted (no TTL).

  • Up to 128 introspection/userinfo results may remain cached in memory.

Suggested minimal fix:

Keep caching only for stable provider metadata:

@lru_cache(maxsize=1)
def discover_provider(...):
...

@lru_cache(maxsize=1)
def get_public_keys(...):
...

No cache for token-specific data

def get_token_info(...):
...

def get_user_info_data(...):
...

If caching is intentional:

Use an explicit bounded cache and document the choice:

@lru_cache(maxsize=128)
def get_token_info(...):
...

or use a short TTL cache (e.g. 60 seconds) to reduce the risk of serving stale results for revoked tokens.


5. Double network call (/introspect + /userinfo) — flow analysis

Current get_oidc_user() flow:

User access token (token_info.sub present)

POST /introspect
GET /userinfo
Compare sub

Client Credentials (sub absent)

POST /introspect only

The Client Credentials flow is already optimized (single network call).

However, user authentication always requires two IdP round trips, whereas the previous implementation only decoded the JWT locally.

The sub consistency check between introspection and /userinfo is valuable and should not be removed without an equivalent safeguard.

Possible improvements:

  • Option A (simplest): If introspection already provides all required Ralph claims (scope, target, sub, etc.), skip /userinfo (possibly behind a configuration flag such as RUNSERVER_AUTH_OIDC_SKIP_USERINFO_IF_COMPLETE=true).

  • Option B: Keep both requests, but cache them per access token with a short TTL (see point #4).

  • Option C: Keep the current behavior and document the additional IdP latency in oidc.md.


6. Content-Type: application/jwt detection

Verified locally:

Content-Type == "application/jwt" startswith("application/jwt") Media type parsing
application/jwt
application/jwt; charset=utf-8
application/jwt+json

Using startswith() may incorrectly match unexpected media types.

Suggested fix:

Parse the media type before comparing:

media_type = content_type.split(";", 1)[0].strip().lower()

is_jwt = media_type == "application/jwt"

if is_jwt:
body = response.text
else:
body = response.json()

return (body, is_jwt)

Verified locally:

  • A raw JWT body cannot be parsed with json.loads().

  • The current mock using json.dumps(jwt) unintentionally hides this issue.


7. conf.py formatting

Current:

BASE_SETTINGS_CONFIG = SettingsConfigDict(
case_sensitive=True, env_nested_delimiter="", env_prefix="RALPH_", extra="ignore"
, secrets_dir=os.environ.get("RALPH_SECRETS_DIR")
)

Suggested fix:

BASE_SETTINGS_CONFIG = SettingsConfigDict(
case_sensitive=True,
env_nested_delimiter="
",
env_prefix="RALPH_",
extra="ignore",
secrets_dir=os.environ.get("RALPH_SECRETS_DIR"),
)

8. test_oidc.py — Long parametrize entry

One of the parametrized entries exceeds the project's preferred line length.

For readability, consider formatting it as:

@pytest.mark.parametrize(
"runserver_auth_backends,userinfo_response_type",
[
([AuthBackend.BASIC, AuthBackend.OIDC], "jwt"),
([AuthBackend.OIDC], "plain"),
([AuthBackend.OIDC], "jwt"),
],
)

Once the blocking issues above have been addressed, please rebase this branch onto the current main (which already includes the CI fixes as well as PRs #630 and #632) before requesting another review.

piptouque added 3 commits August 4, 2026 14:40
Client applications, as authenticated with
the 'client_credentials' flow, do not have a dedicated user.
As such, we can't get ID tokens for them, always
opaque access tokens.
Instead, we authenticate them using their client_id.

But first, we need to check whether
the access token we got was from a real oauth2 user
or a client app.
To do that, we query the /introspection endpoint of our IdP.
This requires that Ralph
be registered s another client app to our IdP,
and to have configured its client ID and secret.
Using a Time-To-Live (TTL) to invalidate the responses of IdP.
Now using one minute for credentials
and responses from/to IdP.
@piptouque
piptouque force-pushed the feat_generic_oidc_client_credentials branch from 51a49e9 to 33bb61a Compare August 4, 2026 13:29
@piptouque

Copy link
Copy Markdown
Contributor Author

Thanks for the review.
Fixed most issues with the first commit, and added caching using TTL as suggested in the next commit.
Last commit is for setting the AUTH_CACHING_TTL to 60 (previously 3600) because I think this setting should be shared with Basic auth, and it seems that you find a shorter TTL preferable.

@MYilFun00

MYilFun00 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Thanks @piptouque. I re-tested the branch locally at 33bb61a.

The three blocking points are resolved:

  • Point 1get_token_info is now annotated -> TokenInfo
  • Point 2 — the mock returns the raw JWT (encoded_user_info, no json.dumps), and
    get_user_info_data parses the media type then picks response.text vs
    response.json()
  • Point 3 — the multiline f-string is gone; oidc.py, basic.py and conf.py all
    parse under Python 3.10 ✅

And the caching rework is exactly what I was hoping for: @lru_cache(maxsize=1)
on discover_provider / get_public_keys, TTLCache with a Lock on the two
token-keyed functions. That closes both point 4 and point 5 (option B).

I verified point 2 and point 6 end to end against simulated IdP responses:

  OK       application/json                 is_jwt=False  body={'sub': 'u1'}
  OK       application/jwt                  is_jwt=True   body=eyJhbGciOiJIUzI1NiJ9...
  OK       application/jwt; charset=utf-8   is_jwt=True   body=eyJhbGciOiJIUzI1NiJ9...

The three valid cases behave correctly. The two remaining cases do not — see
below.


🔴 1. HTTPException(error=...) raises TypeError at runtime

src/ralph/api/auth/oidc.py:150

raise HTTPException(
    status_code=status.HTTP_400_BAD_REQUEST,
    error="invalid_request",          # <- not a valid parameter
    detail=f"Invalid Media type in header: {media_type}, ...",
    headers={"WWW-Authenticate": "Bearer"},
)

FastAPI's signature is HTTPException(status_code, detail=None, headers=None).
There is no error parameter, so this line raises:

TypeError: HTTPException.__init__() got an unexpected keyword argument 'error'

Continuing the run above with the media types that are supposed to hit this branch:

  TypeError  application/jwt+json    HTTPException.__init__() got an unexpected keyword argument 'error'
  TypeError  text/html               HTTPException.__init__() got an unexpected keyword argument 'error'

So an IdP answering with an unexpected Content-Type gets a 500 with a stack
trace instead of the intended 400. Nothing currently covers this path, which is
why the tests stay green.

Suggested fix:

raise HTTPException(
    status_code=status.HTTP_400_BAD_REQUEST,
    detail=(
        f"Invalid media type in header: {media_type}, "
        "expected application/jwt or application/json"
    ),
    headers={"WWW-Authenticate": "Bearer"},
)

Splitting the string also clears the E501 on line 153. A test with an
unexpected Content-Type would be worth adding — it is the only branch of this
function that is currently unexercised.


🔴 2. ci/circleci: lint-git — leftover debug print

src/ralph/api/auth/oidc.py:145

content_type = response.headers["Content-Type"]
print(content_type)

The lint-git job explicitly rejects this:

- run:
    name: enforce absence of print statements in code
    command: |
      ! git diff origin/main..HEAD -- . ':(exclude).circleci' | grep "print("

Reproduced locally — this single line is the only reason lint-git is red;
gitlint itself passes cleanly on all six commit messages. Removing the print
fixes the job. It also writes the content type to stdout on every authenticated
request, which you probably don't want in production logs.


🔴 3. ci/circleci: lint — 6 ruff errors + 1 black

src/ralph/api/auth/oidc.py:145:9   T201  `print` found                     (same line as above)
src/ralph/api/auth/oidc.py:153:89  E501  Line too long (115 > 88)          (fixed by the split above)
src/ralph/api/auth/oidc.py:222:34  F541  f-string without any placeholders
src/ralph/conf.py:3:1              I001  Import block is un-sorted
tests/conftest.py:3:1              I001  Import block is un-sorted
tests/helpers.py:3:1               I001  Import block is un-sorted

F541 is on the new introspection header:

"Authorization": f"Basic "
+ encode_client_secret_basic_token(client_id=client_id, client_secret=client_secret)

f"Basic " has no placeholder. Assigning first reads better and drops the
concatenation:

basic_token = encode_client_secret_basic_token(
    client_id=client_id, client_secret=client_secret
)
...
headers={"Authorization": f"Basic {basic_token}"},

The three I001 are auto-fixable with ruff check --fix.

black --check flags one file:

-from tests.fixtures.auth import AUDIENCE, ISSUER_URI,CLIENT_ID,CLIENT_SECRET
+from tests.fixtures.auth import AUDIENCE, ISSUER_URI, CLIENT_ID, CLIENT_SECRET

🔴 4. ci/circleci: lint-changelog — two lines over 80 characters

The job asserts that no CHANGELOG.md line exceeds 80 characters:

  90 chars | - OIDC: Add query to `/userinfo` endpoint when receiving a token to support more OIDC IdPs
  95 chars | - OIDC: Add token introspection to support querying from OIDC clients (Client Credentials flow)

There is also a trailing space on - Auth: changed default TTL of cache to 60 seconds .

Suggested rewrap:

- Auth: changed default TTL of cache to 60 seconds
- OIDC: Add query to `/userinfo` endpoint when receiving a token,
  to support more OIDC IdPs
- OIDC: Add token introspection to support querying from OIDC
  clients (Client Credentials flow)

🟠 5. Worth discussing: AUTH_CACHE_TTL 3600 → 60 is shared with Basic auth

This is the one I'd like your opinion on before it lands.

You're right that the setting is shared — that's precisely the problem.
src/ralph/api/auth/basic.py:102 uses the same AUTH_CACHE_TTL to cache
get_basic_auth_user, and what that cache is protecting is a bcrypt
verification. Measured locally:

bcrypt.checkpw average: 294.5 ms

TTL 3600s (current main) ->  1 bcrypt recomputation/hour/user =   295 ms/h
TTL   60s (this PR)      -> 60 bcrypt recomputations/hour/user = 17672 ms/h

So the change makes every Basic-auth user pay a ~300 ms penalty once a minute
instead of once an hour, a 60× increase in authentication CPU cost. bcrypt is
deliberately slow, so this is not a micro-optimisation.

The reason a short TTL is desirable for OIDC does not apply to Basic auth: OIDC
access tokens are revocable at the IdP, so serving a stale introspection result
matters. Basic credentials live in a local file and change only when an operator
edits it.

Suggested fix — split the two:

# conf.py
AUTH_CACHE_TTL: int = 3600            # basic auth, unchanged
AUTH_OIDC_CACHE_TTL: int = 60         # IdP responses, short by design
# oidc.py
@cached(
    cache=TTLCache(
        maxsize=settings.AUTH_CACHE_MAX_SIZE, ttl=settings.AUTH_OIDC_CACHE_TTL
    ),
    lock=Lock(),
)

That keeps the security property you want for tokens without regressing Basic
auth. If you'd rather keep a single setting, that's a defensible call, but it
should be called out in the CHANGELOG as a performance-affecting change to Basic
auth rather than described only as an auth caching tweak.

Related: src/helm/ralph/values.yaml still ships cacheTTL: 3600, so the chart
default and the code default now disagree. Whichever way you go, they should be
aligned.


🟡 6. Minor points

Test cache isolation. tests/api/auth/test_oidc.py:156 clears
discover_provider, but nothing clears the two new TTLCaches. cachetools
exposes cache_clear() on both (verified locally), so adding
get_token_info.cache_clear() and get_user_info_data.cache_clear() next to
the existing call would prevent cross-test bleed as the suite grows.

Commit message typo. Two commits use (oicd) instead of (oidc):

🐛(oicd) fix ignore unrelated scopes
🐛(oicd) fix types of id token

gitlint accepts any scope so this doesn't fail CI, but since you'll be rebasing
anyway it's a cheap fix.


Rebase and conflict

The branch is still based on 5e1558d. CHANGELOG.md conflicts with main:

CONFLICT (content): Merge conflict in CHANGELOG.md

check-changelog and the four test-python jobs should clear on their own after
the rebase: your diff touches nothing under tests/backends/, and #634 (already
on main) is what fixed the mongo tests and the check-changelog job.

To summarise what is actually left: remove the print, fix the HTTPException
call, run ruff check --fix plus black, rewrap the CHANGELOG lines, and rebase.
The TTL question in point 5 is the only one that needs a decision rather than a
mechanical fix.

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