Skip to content

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

Open
piptouque wants to merge 15 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 15 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

@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
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.

@piptouque
piptouque force-pushed the feat_generic_oidc_client_credentials branch 6 times, most recently from 6027e9a to 75c2834 Compare September 4, 2026 14:35
@piptouque

Copy link
Copy Markdown
Contributor Author

Thanks for the review, I think that's all for the fixes.

piptouque added 3 commits September 7, 2026 13:42
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.
@piptouque
piptouque force-pushed the feat_generic_oidc_client_credentials branch from 75c2834 to 2faa723 Compare September 7, 2026 11:42
@piptouque
piptouque requested a review from MYilFun00 September 7, 2026 11:43

@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 @piptouque. I re-tested the current head after a local rebase onto main (53cc58c3). Lint is green and the 22 CI checks match that — but CI does not cover the upgrade path of an existing OIDC deployment, nor the Content-Type error branch.

The mechanical points from last round are done (print, ruff/black, CHANGELOG ≤ 80, cache_clear, rebase). The runtime bug from last round is not fixed, and the new flow introduces a regression on current main.

Please do not update-branch / merge until the points below are addressed. Suggested fixes below are the ones I validated locally: existing JWT deployments keep working, /userinfo + client-credentials still work, AUTH_CACHE_TTL stays 3600 for Basic auth.


🔴 1. HTTPException(error=...) — still raises TypeError (same as last review)

src/ralph/api/auth/oidc.py still passes error="invalid_request". FastAPI's signature is (status_code, detail=None, headers=None).

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

An IdP returning text/html or application/jwt+json still gets a 500 + stacktrace instead of the intended 400. Still no test on that branch, which is why CI stays green.

Suggested fix:

        if not is_jwt and not is_json:
            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"},
            )
        body = response.text if is_jwt else response.json()
        return (body, is_jwt)
    except HTTPException:
        raise
    except requests.exceptions.RequestException as exc:
        ...

Drop error=, split detail (clears E501), and re-raise HTTPException so it is not swallowed.

Test (the only branch of this function currently unexercised):

@pytest.mark.anyio
@responses.activate
async def test_api_auth_oidc_userinfo_invalid_content_type():
    get_user_info_data.cache_clear()
    responses.add(
        responses.GET,
        "https://idp.example/userinfo",
        body="not json",
        status=200,
        headers={"Content-Type": "text/html"},
    )
    with pytest.raises(HTTPException) as exc_info:
        get_user_info_data("https://idp.example/userinfo", "token")
    assert exc_info.value.status_code == 400
    assert "text/html" in exc_info.value.detail

🔴 2. Existing OIDC deployments 500 after upgrade

On current main, get_oidc_user decodes the JWT locally via JWKS. This PR always calls /introspection with:

RUNSERVER_AUTH_OIDC_CLIENT_ID     = None   # default
RUNSERVER_AUTH_OIDC_CLIENT_SECRET = None
AttributeError: 'NoneType' object has no attribute 'encode'

Every authenticated request dies. Tests always monkeypatch CLIENT_ID/CLIENT_SECRET, so they never see this.

introspection_endpoint is also optional in OIDC Discovery. A generic IdP without it raises KeyError → 500. That contradicts the PR title.

Suggested behaviour (opt-in, no break):

def _can_introspect(provider_config: dict) -> bool:
    """True when Ralph has client credentials and the IdP exposes introspect."""
    return bool(
        settings.RUNSERVER_AUTH_OIDC_CLIENT_ID
        and settings.RUNSERVER_AUTH_OIDC_CLIENT_SECRET
        and provider_config.get("introspection_endpoint")
    )


def get_oidc_user(...):
    ...
    access_token = auth_header.split(" ")[-1]
    provider_config = discover_provider(settings.RUNSERVER_AUTH_OIDC_ISSUER_URI)

    if not _can_introspect(provider_config):
        # Existing deployments: decode the JWT locally, no client credentials.
        return _user_from_jwt(access_token, provider_config)

    token_info = get_token_info(
        provider_config["introspection_endpoint"],
        token=access_token,
        client_id=settings.RUNSERVER_AUTH_OIDC_CLIENT_ID,
        client_secret=settings.RUNSERVER_AUTH_OIDC_CLIENT_SECRET,
    )
    ...
  • Client id and secret and introspection_endpoint set → new flow (introspect, then /userinfo for users / client_id for apps).
  • Otherwise → keep today's local JWT validation.

That is how existing Keycloak-style deployments survive, and how opaque-token / client-credentials support stays available once operators register Ralph as a client.

Test for the upgrade path (no client id/secret, JWT bearer token, IdP without introspection_endpoint):

async def test_api_auth_oidc_legacy_jwt_without_client_credentials(client, monkeypatch):
    configure_env_for_mock_oidc_auth(monkeypatch, with_oidc_client=False)
    oidc_token = mock_oidc_jwt_user(scopes=["all", "profile/read"])
    response = await client.get("/whoami", headers={"Authorization": f"Bearer {oidc_token}"})
    assert response.status_code == 200
    assert response.json()["agent"]["openid"] == "https://iss.example.com/123|oidc"

🔴 3. {iss}/{sub}{iss}/user/{sub} breaks stored authority

On main the agent is f"{iss}/{sub}". This PR writes f"{iss}/user/{sub}".

That value is copied onto every statement as authority, and mine=true / LRS_RESTRICT_BY_AUTHORITY filter on it. After upgrade, previously stored statements no longer match the same user.

Suggested fix — keep {iss}/{sub} for end-users; /application/ only for the new client-credentials path:

    if token_info.sub:
        user_info = get_user_info(provider_config, access_token=access_token)
        if user_info.sub != token_info.sub:
            ...
            raise _unauthorized()
        return AuthenticatedUser(
            agent={"openid": f"{token_info.iss}/{user_info.sub}"},
            scopes=get_user_scopes(user_info.scope),
            target=user_info.target,
        )

    return AuthenticatedUser(
        agent={"openid": f"{token_info.iss}/application/{token_info.client_id}"},
        scopes=get_user_scopes(token_info.scope),
        target=token_info.target,
    )

Also update the whoami assertion (and tests/api/test_statements_get.py) from {iss}/user/{sub} back to {iss}/{sub}.


🔴 4. Helm: RALPH_AUTH_OIDC_CACHE_TTL is gated on basic auth

Current:

{{- if .Values.lrs.auth.basic.enabled }}
  RALPH_AUTH_CACHE_MAX_SIZE: {{ .Values.lrs.auth.basic.cacheMaxSize | quote }}
  RALPH_AUTH_CACHE_TTL: {{ .Values.lrs.auth.basic.cacheTTL | quote }}
  RALPH_AUTH_OIDC_CACHE_TTL: {{ .Values.lrs.auth.oidc.cacheTTL | quote }}
{{- end }}
{{- if .Values.lrs.auth.oidc.enabled }}
  RALPH_RUNSERVER_AUTH_OIDC_AUDIENCE: ...
  RALPH_RUNSERVER_AUTH_OIDC_ISSUER_URI: ...
{{- end }}

Rendered: default (basic on, oidc off) sets an unused OIDC TTL; oidc on, basic off (this PR's target) omits the variable.

Suggested fix in src/helm/ralph/templates/cm_lrs.yaml:

{{- if .Values.lrs.auth.basic.enabled }}
  RALPH_AUTH_CACHE_MAX_SIZE: {{ .Values.lrs.auth.basic.cacheMaxSize | quote }}
  RALPH_AUTH_CACHE_TTL: {{ .Values.lrs.auth.basic.cacheTTL | quote }}
{{- end }}
{{- if .Values.lrs.auth.oidc.enabled }}
  RALPH_AUTH_OIDC_CACHE_TTL: {{ .Values.lrs.auth.oidc.cacheTTL | quote }}
  RALPH_RUNSERVER_AUTH_OIDC_AUDIENCE: {{ .Values.lrs.authOIDCAudience | quote }}
  RALPH_RUNSERVER_AUTH_OIDC_ISSUER_URI: {{ .Values.lrs.authOIDCIssuerURI | quote }}
{{- end }}

🟠 5. AUTH_CACHE_TTL 3600 → 36000 (not 60, not 3600)

You did split AUTH_OIDC_CACHE_TTL = 60 — thank you, that was the request.

But Basic auth now defaults to 10 hours (36000), while values.yaml still has cacheTTL: 3600. Revoking a user in auth.json can take 10 h to take effect.

Suggested fix in src/ralph/conf.py — restore the Basic default, keep the OIDC split:

    AUTH_CACHE_MAX_SIZE: int = 100
    AUTH_CACHE_TTL: int = 3600
    AUTH_OIDC_CACHE_TTL: int = 60

Drop the CHANGELOG line « changed default TTL of cache to 60 seconds » (wrong for Basic, trailing space).


🟡 6. The headline feature has no test

tests/api/auth/test_oidc.py still has the same 7 functions. The agent={"openid": ".../application/{client_id}"} path (token with no sub) is never exercised.

Suggested test:

@pytest.mark.anyio
@responses.activate
async def test_api_auth_oidc_client_credentials_application_agent(client, monkeypatch):
    """A token without `sub` is treated as a client application."""
    configure_env_for_mock_oidc_auth(monkeypatch)
    oidc_token = mock_oidc_user(sub=None, scopes=["all", "profile/read"])
    response = await client.get(
        "/whoami",
        headers={"Authorization": f"Bearer {oidc_token}"},
    )
    assert response.status_code == 200
    assert response.json()["agent"] == {
        "openid": "https://iss.example.com/application/my-other-client-id",
        "objectType": "Agent",
    }

(_mock_oidc_token_info must omit sub when it is None.)

CHANGELOG: move the two Fix … lines into the existing ### Fixed section (they currently sit under a second ### Changed):

### Fixed
- Fix type of OIDC ID tokens (`aud` may be a list, `exp`/`iat` may be floats)
- Ignore OIDC scopes unrelated to Ralph instead of returning an error

### Changed
- OIDC: query `/userinfo` for access tokens that are not JWTs
- OIDC: introspect tokens to support client-credentials apps as users

Happy to re-review once those are in.

piptouque added 3 commits September 8, 2026 16:23
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 pushed a commit to piptouque/ralph that referenced this pull request Sep 8, 2026
openfun#633 (review)
Added some tests for `/userinfo` endpoint.
@piptouque
piptouque force-pushed the feat_generic_oidc_client_credentials branch from 2faa723 to 411d8aa Compare September 8, 2026 14:23
piptouque added 4 commits September 8, 2026 18:35
Add `cacheTTL` value for `lrs.auth.oidc`.
openfun#633 (review)
Added some tests for `/userinfo` endpoint.
Now these function expect a complete `auth_header`,
starting with `Basic` or `Bearer` depending on the token type.
That test was missing `@responses.activate`,
which meant that the reported error was not the right one.
It also made it leek out in tests that were run afterwards.
@piptouque
piptouque force-pushed the feat_generic_oidc_client_credentials branch 2 times, most recently from 4a26ee6 to 7e537a6 Compare September 8, 2026 17:12
@piptouque
piptouque force-pushed the feat_generic_oidc_client_credentials branch 3 times, most recently from 985fda8 to c5945b8 Compare September 9, 2026 10:23
piptouque added 2 commits September 9, 2026 12:37
Specifically refactor the auth guards in OIDC fixtures
in order to reuse them.
These should cover the basic handling to our IdP's
`/userinfo` and `/introspection` endpoints.
@piptouque
piptouque force-pushed the feat_generic_oidc_client_credentials branch from c5945b8 to c060cc9 Compare September 9, 2026 10:37
piptouque added 3 commits September 9, 2026 12:44
If `CLIENT_ID`, `CLIENT_SECRET` are not set, or that the IdP did not advertise
an `/introspection` endpoint,
we assume that the access token we were given is a JWT ID Token and
can be decoded to get `UserInfo`.
We now check the `/whoami` endpoint with:
- an OIDC user authenticated via `/userinfo` IdP endpoint
- an OIDC user authenticated directly by ID token
- an OIDC client
Using `ruff` and `black`.
@piptouque
piptouque force-pushed the feat_generic_oidc_client_credentials branch from c060cc9 to 07fecef Compare September 9, 2026 10:44
piptouque pushed a commit to piptouque/ralph that referenced this pull request Sep 9, 2026
commit 07fecef
Author: piptouque <pierre.thiel@touque.fr>
Date:   Wed Sep 9 12:22:53 2026 +0200

    🚨(tests,oidc) run linter and formatter

    Using `ruff` and `black`.

commit c77498f
Author: piptouque <pierre.thiel@touque.fr>
Date:   Tue Sep 8 18:31:41 2026 +0200

    ✅(tests,oidc) update the OIDC `/whoami` validity test

    We now check the `/whoami` endpoint with:
    - an OIDC user authenticated via `/userinfo` IdP endpoint
    - an OIDC user authenticated directly by ID token
    - an OIDC client

commit 0644d33
Author: piptouque <pierre.thiel@touque.fr>
Date:   Tue Sep 8 18:24:53 2026 +0200

    🐛(oidc) add back support for ID tokens as access token

    If `CLIENT_ID`, `CLIENT_SECRET` are not set, or that the IdP
    did not advertise
    an `/introspection` endpoint,
    we assume that the access token we were given is a JWT ID Token and
    can be decoded to get `UserInfo`.

commit 5ea3419
Author: piptouque <pierre.thiel@touque.fr>
Date:   Tue Sep 8 16:18:59 2026 +0200

    ✅(tests,oidc) add user_info and intropection tests

    These should cover the basic handling to our IdP's
    `/userinfo` and `/introspection` endpoints.

commit e249ff5
Author: piptouque <pierre.thiel@touque.fr>
Date:   Tue Sep 8 16:18:31 2026 +0200

    ♻️(tests,oidc) refactor fixtures

    Specifically refactor the auth guards in OIDC fixtures
    in order to reuse them.

commit 715f6a0
Author: piptouque <pierre.thiel@touque.fr>
Date:   Sun Sep 6 13:07:37 2026 +0200

    🐛(tests) fix missing responses in oidc test

    That test was missing `@responses.activate`,
    which meant that the reported error was not the right one.
    It also made it leek out in tests that were run afterwards.

commit 6e4e6c7
Author: piptouque <pierre.thiel@touque.fr>
Date:   Sat Sep 5 18:31:29 2026 +0200

    ♻️(oidc) refactor OIDC auth args

    Now these function expect a complete `auth_header`,
    starting with `Basic` or `Bearer` depending on the token type.

commit 9ef1d9d
Author: piptouque <pierre.thiel@touque.fr>
Date:   Tue Sep 8 13:28:14 2026 +0200

    🐛(oidc) fix issues with PR from review

    openfun#633 (review)
    Added some tests for `/userinfo` endpoint.

commit e75790a
Author: piptouque <pierre.thiel@touque.fr>
Date:   Fri Sep 4 16:33:26 2026 +0200

    🔧(auth) update helm chart

    Add `cacheTTL` value for `lrs.auth.oidc`.

commit aa2e7cb
Author: piptouque <pierre.thiel@touque.fr>
Date:   Fri Sep 4 16:31:28 2026 +0200

    🔧(auth) change auth caching values

    Now using one minute for credentials
    and responses from/to IdP.

commit 0e084c0
Author: piptouque <pierre.thiel@touque.fr>
Date:   Tue Aug 4 15:13:00 2026 +0200

    ✨(oidc) improve caching of IdP responses

    Using a Time-To-Live (TTL) to invalidate the responses of IdP.

commit 8ff7aa1
Author: piptouque <pierre.thiel@touque.fr>
Date:   Tue Feb 17 17:27:53 2026 +0100

    ✨(oidc) add support for client apps 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 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.

commit 325ffcc
Author: piptouque <pierre.thiel@touque.fr>
Date:   Tue Feb 17 15:04:13 2026 +0100

    ✨(oidc) get user claims from /userinfo endpoint

    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.

commit 7a66dae
Author: piptouque <pierre.thiel@touque.fr>
Date:   Wed Feb 18 19:23:55 2026 +0100

    🐛(oidc) fix ignore unrelated scopes

    The server should ignore any
    OAuth 2 scope that it does not
    know, instead of returning an error.

commit ad42220
Author: piptouque <pierre.thiel@touque.fr>
Date:   Wed Feb 18 18:47:43 2026 +0100

    🐛(oidc) fix types of id token

    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
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