Require logging in again when OIDC tokens can't be refreshed

[marius-mather]

We want to ensure users have a current, valid access token from the OIDC provider, so when attempting to refresh (due to access token expiry), log the user out and redirect to OIDC login. This is gated behind an oidc_require_refresh config flag.

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[mvdbeek]

@uwwint do you want to take a look at this ?

[nuwang]

Looks good to me except for one minor issue.

[uwwint]

@uwwint do you want to take a look at this ?

I was involved in this one @mvdbeek ;)

[mvdbeek]

Code Review — Require logging in again when OIDC tokens can't be refreshed

Code Structure (type annotations & imports)

lib/galaxy/authnz/managers.py — typing gaps on new/touched signatures:

• _reauth_required_from_refresh_exception(exc) — add exc: BaseException) -> bool
• refresh_expiring_oidc_tokens_for_provider(self, trans, auth) — annotate trans and auth: model.UserAuthnzToken
• refresh_expiring_oidc_tokens(self, trans, user=None) — annotate trans, user: Optional[model.User] = None

lib/galaxy/webapps/base/webapp.py — handle_user_reauthentication missing -> None; trailing bare return is redundant.

lib/galaxy/config/init.py, test/unit/authnz/test_authnz.py — clean. Minor: the relative import from ..webapps.test_webapp_base reaches across test packages; consider moving CORSParsingMockConfig/StubGalaxyWebTransaction to a shared test utility.

Dependency Injection

No new DI violations. Both trans.app.config.oidc_require_refresh and self.app.authnz_manager accesses are consistent with the existing pattern in legacy GalaxyWebTransaction / AuthnzManager request-path code. No new Depends(), no new app.* property, no internal construction of collaborators. handle_user_reauthentication is a cohesive sibling of handle_user_logout.

Tests (mocking / patches)

Four/five new tests. The first two exercise real manager logic with a mocked backend; the last three stub the manager wholesale, which weakens them.

Concerns:

1. Tests 3–5 replace AuthnzManager with MagicMock() — they effectively verify "webapp forwards the stubbed return value." Prefer the real AuthnzManager with a fake backend registered in config.
2. patch.object(manager, "_get_authnz_backend", ...) patches a private method. Introduce a thin backend-resolver seam, or register a fake in oidc_backends_config so the real _get_authnz_backend finds it.
3. patch("galaxy.webapps.base.webapp.url_for", ...) couples to an import detail — assert response.send_redirect was called with a location containing /authnz/oidc/login instead.
4. Prefer state over interaction verification — drop backend.refresh.assert_called_once_with(trans, auth); the returned value already proves the call happened. Tests 4/5 model the right style (assert response.status, error_message).

Top recommendations

1. Add annotations to the three signatures in managers.py and -> None on handle_user_reauthentication.
2. Stop mocking AuthnzManager in the webapp tests; use the real manager with a fake backend.
3. Remove the assert_called_once_with interaction checks in favor of state assertions.

Overall: small, focused change that follows surrounding patterns. Fixes are minor — primarily typing polish and test quality improvements.

[mvdbeek]

The package unit test failures are real though.

[marius-mather]

Thanks @mvdbeek - test failures should be fixed now

[mvdbeek]

This can only be enabled if all OIDC backends support refresh, right ? I think that's an important gotcha and maybe reason enough to push this into the backend instead ?

[nuwang]

@mvdbeek Can you clarify what you meant by push this into the backend? But in any case, @marius-mather, perhaps a clarifying note indicating that all OIDC backends will need to support refresh would be useful?

[mvdbeek]

Push that setting into the OIDC backend config

[marius-mather]

@mvdbeek the refresh config has been pushed down to the individual provider level now

[nuwang]

Looks great to me @marius-mather!