Skip to content

feat(config): expose openid-connect via OWNCLOUD_OPENID_CONNECT#519

Merged
phil-davis merged 1 commit into
masterfrom
feat/openid-connect-env-var
Jul 9, 2026
Merged

feat(config): expose openid-connect via OWNCLOUD_OPENID_CONNECT#519
phil-davis merged 1 commit into
masterfrom
feat/openid-connect-env-var

Conversation

@DeepDiver1975

@DeepDiver1975 DeepDiver1975 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds an environment variable for the openidconnect app's openid-connect config key, which had no env-var mapping — OIDC could only be configured by mounting a custom config.php or via occ. (The pre-existing OWNCLOUD_ENABLE_OIDC_REWRITE_URL is an unrelated Apache rewrite toggle, not the app config.)

Requested in #492 — reopened by @DeepDiver1975 confirming the mapping is still needed.

Details

  • New var OWNCLOUD_OPENID_CONNECT — a JSON-encoded object decoded into $config['openid-connect']:
    OWNCLOUD_OPENID_CONNECT='{"provider-url":"https://idp.example.net","client-id":"...","client-secret":"...","loginButtonName":"OpenID Connect"}'
    
  • openid-connect is a deeply nested associative array (scopes, provider-params, auto-provision.update, ...) that the flat env-var idioms cannot represent. Decoding to an array here is required, not just convenient: Client::getOpenIdConfig() (lib/Client.php) reads the system-config value via getSystemValue('openid-connect', null) and consumes it directly with array access (['auto-provision'], ['mode'], ...) — the app only json_decodes the app-value (DB) path, so a raw JSON string in system config would break it. We reuse the json_decode + is_array guard pattern already established for OWNCLOUD_LOG_CONDITIONS / OWNCLOUD_USER_BACKENDS. Invalid JSON is ignored, leaving the key unset.
  • Shape validated against the owncloud/openidconnect app source (lib/Client.php, app README) and the OIDC admin docs.
  • v24.04 only. The recent env-var additions (OWNCLOUD_USER_BACKENDS Missing envvar for 'user_backends' #490, OWNCLOUD_LOGIN_POLICY_GROUP_FORBID_MAP Missing envvar for loginPolicy.groupLoginPolicy.forbidMap #493, OWNCLOUD_CUSTOMGROUPS_* Missing envvars for customgroups.xxx configurations #491) all landed in v24.04 only; v20.04/v22.04 retain their older vars but no longer receive new ones, and CI builds only 22.04/24.04.

Note: the JSON payload contains client-secret. It is handled the same way as other secrets already exposed here (LDAP, S3 keys), so no new exposure class — but treat the env var as sensitive and source it from a secrets store where possible.

Verification

  • php -l passes.
  • Behavioral check via php -r dumping $CONFIG['openid-connect']: a nested payload (scopes list, provider-params, auto-provision.update) round-trips intact; key is absent when the var is unset; invalid JSON safely falls back to unset.

Closes #492

🤖 Generated with Claude Code

The openidconnect app's 'openid-connect' config key had no corresponding
env var, so OIDC could not be configured through the image's env-based
config mechanism (only the unrelated OWNCLOUD_ENABLE_OIDC_REWRITE_URL
Apache toggle existed).

Add OWNCLOUD_OPENID_CONNECT, a JSON-encoded object decoded into
$config['openid-connect']. The key is a deeply nested associative array
(provider-url, client-id/secret, scopes, provider-params, auto-provision,
...) that the flat env-var idioms cannot represent, so it reuses the
json_decode pattern already used for OWNCLOUD_LOG_CONDITIONS and
OWNCLOUD_USER_BACKENDS. Invalid JSON is ignored, leaving the key unset.
Added to v24.04 only, matching the convention that v20.04/v22.04 are frozen.

Closes #492

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
@DeepDiver1975

Copy link
Copy Markdown
Contributor Author

Code Review

Overview

Adds OWNCLOUD_OPENID_CONNECT, a JSON-encoded env var decoded into $config['openid-connect'] (system config) for the openidconnect app. Closes #492. Touches three files: CHANGELOG.md, ENVIRONMENT.md, and the v24.04 config template. +21/-0.

Correctness — verified against the app

I traced the consuming code in owncloud/openidconnect:

  • Client::getOpenIdConfig() (lib/Client.php:103) reads the app-value first, and falls back to getSystemValue('openid-connect', null) — returning it directly as the config used with array access (['auto-provision'], ['mode'], ...).
  • That means the system-config value must be a PHP array, which is exactly what this PR writes (json_decode(..., true)). A raw JSON string in system config would break the app's array access — so decoding to an array here is not merely convenient, it is required. Correct.
  • The app's own README confirms the shape: appid openidconnect, configkey openid-connect, value a JSON-String; the documented example matches the PR's example almost verbatim.

One nit for the PR description (not the code): "The app itself json_decodes this value" is only true for the DB/app-value path; the system-value path this env var populates is consumed as a raw array. The code does the right thing regardless — worth tightening the wording.

Conventions — clean

  • The block is a faithful copy of the established OWNCLOUD_LOG_CONDITIONS / OWNCLOUD_USER_BACKENDS idiom (getenv() != ''json_decode(_, true)is_array guard → assign). Invalid JSON safely leaves the key unset. Consistent with neighbors.
  • v24.04-only is correct. I checked master: the three most recent additions (USER_BACKENDS Missing envvar for 'user_backends' #490, FORBID_MAP Missing envvar for loginPolicy.groupLoginPolicy.forbidMap #493, CUSTOMGROUPS Missing envvars for customgroups.xxx configurations #491) all landed in v24.04 only, not v22.04v22.04 retains older vars but no longer receives new ones. So skipping v20.04/v22.04 matches current practice. (Minor: the PR body calls v22.04 "frozen"; more precisely it's "no longer receiving new env vars" — it still contains older ones.)
  • CHANGELOG entry (## 2026-07-09, * Added, issue link) matches the existing format exactly.
  • ENVIRONMENT.md entry is alphabetically placed (between OBJECTSTORE_* and OPENSSL_CONFIG) and documents the nested structure with a doc link. Good.

Security

  • client-secret lands in an env var and is decoded into config. This is consistent with how other secrets (LDAP, S3 keys) are already handled here, so no new exposure class — but the doc could note that the JSON contains a secret and should be sourced from a secrets store / *_FILE pattern if one exists. Not blocking.
  • No injection surface: value is JSON-parsed, never eval'd or string-interpolated into PHP.

Test coverage

  • The repo has no unit/behavioral harness for the template (CI main.yml only builds the image + a trivial php -r "echo 'OK'" smoke test), so the manual php -r round-trip verification in the PR body is the appropriate level of proof here. Matches how sibling env vars were merged.

Verdict

Correct, convention-aligned, and verified against the consuming app's actual read path. Approve. Only optional polish: tighten the two wording points above (app decode path; "frozen" v22.04) and optionally note the secret-handling caveat in the docs.

@phil-davis phil-davis merged commit bc8c992 into master Jul 9, 2026
6 checks passed
@phil-davis phil-davis deleted the feat/openid-connect-env-var branch July 9, 2026 15:21
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.

Missing envvar for openid-connect

2 participants