[matthewberry]

Stacked on #163, which hides logs from the default listing. This PR makes them inaccessible to anyone but operators. The review on #163 deferred this.

Change

"Operator" means a logged-in member of the reporting group, checked with the existing require_reporting_group.

• GET /v1/jobs/{id}/artifacts?include=logs: 403 problem for anyone else.
• GET /v1/jobs/{id}/artifacts/{name} for a top-level log or marker name (LOG_ARTIFACTS): 404 for anyone else, the same answer as a missing file, so it does not confirm the log exists.
  • The path is normalized first, so ./log cannot reach what log is refused.
  • A result in a subfolder that happens to be named log (e.g. run1/log) is unaffected, matching fix: hide the job template's log and marker files from the v1 artifact listing #163's top-level-only rule.
• With no reporting group configured, nobody counts as an operator. That mirrors require_reporting_group, which refuses rather than falling open.
• MCP list_job_artifacts loses include_logs. The MCP server calls the API without the operator's cookie, so the option could never succeed.
• README updated.
• The operator check can no longer stall the backend (from review, 7eca1eb):
  • validate_auth_cookie skips the userinfo lookup when there is no cookie;
  • userinfo() times out after 5 seconds;
  • require_reporting_group runs the lookup in a thread pool, which also fixes /internal/reports/usage.

Deploy note

After deploy, nobody can read logs through the API. Operators can still read them in MinIO directly. Three things stand in the way, and all three are already true for /internal/reports/usage:

• auth.reportingGroup is not set in any values file.
• auth.userInfoUrl is the chart default http://oauth2.proxy.local/oauth2/userinfo, which does not resolve from the prod backend pod, so every lookup fails.
• oauth2-proxy sets its cookie for .platform.moleculemaker.org, while the API is served at mmli.fastapi.mmli2.ncsa.illinois.edu, so browsers don't send the cookie to the API.

Making operator access work means serving the API under *.platform.moleculemaker.org (or changing the cookie domain), pointing userInfoUrl at the in-cluster oauth2-proxy, making sure Keycloak puts groups in the token, and then setting the group. That is out of scope here.

Callers checked

• Within this repo, the only caller asking for logs was the MCP option removed here.
• I searched the local checkouts of the frontends for /artifacts and include=logs and found none. The reviewer then searched the default branch of all 15 frontend repos and found no callers either.

Tests

New cases in TestArtifacts:

• anonymous and non-member listing with ?include=logs gets 403;
• downloading log, ./log and error.log as a non-operator gets 404;
• an operator downloads a log;
• run1/log stays downloadable;
• an unset group refuses even a logged-in user;
• an anonymous log request makes no userinfo call, the lookup has a timeout, and it does not run on the event loop.

Nine of these failed before the changes they cover. Full suite: 636 passed.

[matthewberry]

Looks right. One finding, inline on _is_operator, that matters once reportingGroup is set. Nothing blocking today.

Every way to read a log through the API is covered. I enumerated the paths that serve files from a job's out/:

• The two v1 routes: both are gated. The download check normalizes the path first; .. and a leading / were already rejected, and since object names are case-sensitive, LOG is a different object rather than a way around the check. A non-operator gets the same 404 as for a missing file.
• Legacy files.py routes: inputs/ gives presigned URLs for /in/ only, errors/ reads {job_id}/errors.txt outside out/, and results/ returns per-tool results. The rest are uploads, the chemscraper export, and genome-viewer data.
• Presigned URLs: the only listing is get_file_urls, called once, for /in/.
• MCP: it only lists artifacts, and now never with logs.
• Direct MinIO access: per #164's allowlist, anonymous access covers */in/* and the ez-specificity complexes only, not out/log.

The deploy note is accurate: nothing in chart/ or app/cfg sets reportingGroup or REPORTING_GROUP.

Frontend callers, checked on main: the PR body notes the frontend search covered local checkouts only. I fetched all 15 frontend repos and searched each default branch for /artifacts, include=logs, include_logs and includeLogs. Nothing matched, so nothing in a frontend loses access.

Minor

• Listing with ?include=logs returns 403, while downloading returns 404. That's deliberate and fine; the 403 doesn't reveal whether any log exists.
• This is stacked on #163, so retarget it to main after #163 merges, unless GitHub does it when #163's branch is deleted.

Not run locally; CI pytest and docker were still pending when I reviewed.

[matthewberry]

Once reportingGroup is set, this lets anyone make the backend stall on an untimed outbound call. I found this by reading; I haven't run it.

• This awaits require_reporting_group, which calls validate_auth_cookie → userinfo(). That's a synchronous requests.get(app_config['auth']['userInfoUrl'], ...) with no timeout, so inside this async route it blocks the event loop, and with it every other request, until the call returns.
• userinfo() runs even when there's no cookie (token=None), so an anonymous request still makes the call.
• So anyone can trigger it with GET /v1/jobs/{any id}/artifacts/log or ?include=logs.
• /internal/reports/usage already has the same problem. This adds two more public routes that trigger it.
• It's dormant today, because the unset-group check in require_reporting_group raises before any network call.

Cheapest fix: return False without the lookup when there's no auth cookie, and give the requests.get in userinfo() a timeout. Running the lookup in a threadpool (run_in_threadpool) would also stop it blocking the event loop.

[matthewberry]

Confirmed by reading userinfo_service.py: the requests.get has no timeout, and the lookup ran even with no cookie. I did not reproduce the stall itself. Fixed in 7eca1eb:

• validate_auth_cookie returns None without a lookup when there is no cookie, so anonymous hits on a log URL make no outbound call. That lookup would have failed and returned None anyway, so the reports and molli callers see no change.
• userinfo() now has a 5-second timeout; a timeout counts as not logged in.
• require_reporting_group runs the lookup with run_in_threadpool, which also fixes /internal/reports/usage.

Three new tests: an anonymous log request makes no requests.get, the lookup passes a timeout, and the lookup does not run on the event loop. All three failed before the change. Full suite: 636 passed.

Separately, userinfo() logs the access token at debug level on failure (line 61). I left that alone here; it is worth its own change.

[bodom0015]

Based on past experience, we may need to be careful with this... One reason that I never enabled the userinfo checks in the backend (even tho some of the code is already there) is that I was afraid we may accidentally create a situation where we DDoS ourselves 😅

As I understand it, the change proposed in this PR is only for the reporting / logs endpoints, which are not typically used in the frontends. If that is true, then all is well here 👍

Downside: But I would advise caution against using this pattern on the more frequently used job submission/status endpoints that the frontend applications use. Supporting such a case would require much testing and thinking thru the full chain of requests. Every time we decode the cookie we follow the chain: mmli-backend -> OAuth2 Proxy -> Redis. Ultimately Redis is pretty good at caching (probably better than etcd was with my last not-so-great experience), so maybe this case would have less performance concerns than I'm thinking it might.

Upside: if a timeout simply means "not logged in", then an end user's path thru using the app should not be affected much (or at all) by a failure in the login system if it were to get overwhelmed. Some frontend users may see issues logging in, but right now that is an edge case anyways since so few people sign up and log in (as there's no real benefit or incentive for them to do so)

[matthewberry]

@bodom0015 , I'd be happy to replace the authorized group here if you'd like to suggest an alernative.

[bodom0015]

LGTM 👍 minor concerns about possible future usage of this pattern, but nothing that should prevent merging the current PR 🙏 we may want to deploy this one and test it

Especially if we're assuming that the unused userinfo code that I had previously written would work out of the box 😅

[bodom0015]

@bodom0015 , I'd be happy to replace the authorized group here if you'd like to suggest an alernative.

I may not be understanding the question - are you asking if it's ok that we reuse the mmli-reporting group for this purpose? If so, I think that is ok 👍 logs could be considered a report of sorts for a particular job, so I think it makes sense that these groups are shared. We could have an explicit group for logs, but currently the pool of users in both groups would be identical.. so I'm happy either way :)