Add full support for deploying OpenCloud under a subpath - #3172
Open
kost wants to merge 2 commits into
Open
Conversation
Config fields for exactly this (WEB_HTTP_ROOT, GRAPH_HTTP_ROOT,
WEBDAV_HTTP_ROOT, IDP_URI_BASE_PATH, FRONTEND_HTTP_PREFIX,
PROXY_HTTP_ROOT, OCM_HTTP_PREFIX, ...) already existed on nearly every
service, and most services' own routers already gated correctly on
them. But the proxy -- the single front door all traffic passes
through -- never consulted PROXY_HTTP_ROOT when matching a request
against a policy route, so a prefixed request never matched and fell
through to the web app's catch-all; web's HTML server hardcoded
<base href="/"/>; frontend/ocm declared a prefix but reva's rhttp had
no way to apply it; and ocm/sse/userlog/activitylog had no root-gated
mux at all. Nothing derived any of this from OC_URL, so even where the
mechanism worked, a user had to discover and set 10+ separate env vars
by hand.
- pkg/shared.SubPath: extracts and cleans a URL's path component. Every
service's EnsureDefaults derives its own root/prefix field from
OC_URL via this helper.
- proxy: Router.Route matches policy routes against a root-stripped
copy of the request URL; the real, forwarded request is untouched.
New Route.StripRoot escape hatch for the handful of backends that
hardcode an unprefixed path regardless: lico's RFC 8615 well-known
endpoint, and the idp identifier/welcome/goodbye/consent/
chooseaccount pages and static assets (see below).
- web: <base href> now uses the service's own already-correctly-gated
root instead of a hardcoded "/".
- frontend/ocm: wire the existing, previously-unused HTTP.Prefix into
reva's rhttp config (a single top-level "prefix" key consumed once by
rhttp's own dispatch -- not into each sub-handler's own registration
prefix, which stays unprefixed; wrapping it twice broke dispatch to
every reva-based endpoint entirely, caught by live deployment testing,
see below). Needs a companion change to opencloud-eu/reva (rhttp has
no concept of a server-wide prefix at all); the vendored copy is
hand-patched to match until that lands and a real version bump can be
made.
- ocm, sse, userlog, activitylog: new Root-gated mux wrapping, mirroring
the pattern already used by web/graph.
- idp: two Go-side bugs in Index() -- a hardcoded /signin/v1
react-router basename that's supposed to include the deployment root,
and a bytes.Replace chaining bug that silently discarded earlier
template substitutions -- plus two bugs in the identifier's own React
source (services/idp/src): config.json and the theme logo fetched
from a hardcoded absolute path, when neither is one of the identifier
app's own bundled assets and both need the true deployment root
derived independently of the identifier app's own nested URL.
- idp: the vendored lico library also registers /welcome, /goodbye,
/consent, and /chooseaccount as routes served by its own built-in
identifier webapp, whose index.html is only ever read from disk when
IdentifierClientDisabled is false -- which it never is here, since
OpenCloud always runs its own, correctly-templated identifier app
instead. Previously only /signin/v1/identifier was overridden this
way; hitting any of the other four directly (a bookmarked URL, or a
SAML logout redirect) served a literally empty 200 response. Fixed by
registering all five through the same override, with a matching
StripRoot regex extension so they still resolve correctly under a
subpath. (This is the same fix as the independent
fix-idp-static-routes-empty-response branch, which fixes it for
root-of-domain deployments too and doesn't depend on any of the rest
of this patch; it's folded in here as well only because this
branch's StripRoot regex extension needs it to exist. If that PR
merges first, rebase this one to drop the now-duplicate commit.)
Testing: go build ./... and go test ./services/... ./pkg/... clean
across the whole repo, aside from one pre-existing, environment-only
failure (services/search/pkg/opensearch needs a live OpenSearch
instance, unrelated to this change). New unit tests for the router, web
asset server, subpath helper, the idp Index() template substitution
regression, and the Root/Prefix derivation regression described below.
Verified end to end against a real, unmodified passthrough nginx
reverse proxy and a real browser: built opencloud-eu/web from source
(not the pinned release) and the idp identifier app, built the full Go
binary, and deployed it live under /test/opencloud -- a separate
container/port/data volumes from any other deployment. This caught two
real bugs no unit test had surfaced:
1. Every reva-based endpoint (archiver, ocdav, ocs, datagateway,
appprovider, sciencemesh, ocmd) doubled the deployment prefix,
because each sub-handler's own registration prefix was wrapped with
the deployment prefix a second time on top of what rhttp's dispatch
already strips once -- breaking dispatch to all of them under a
subpath. Fixed by reverting those wraps; the single top-level
"prefix" key is the only place it needs to be wired in.
2. The OC_URL-derived subpath was silently dropped for 9 of 15
services: every EnsureDefaults derivation checked
"cfg.HTTP.Root == \"\"", but DefaultConfig gives most services a
non-empty baseline of their own ("/" for proxy/web/webdav/settings/
webfinger/sse/userlog/activitylog, or a fixed namespace like
"/graph"/"/ocs"/"/graph/v1.0"/"/thumbnails" for graph/ocs/
invitations/thumbnails), so the check never fired and every request
fell through to the web app's catch-all regardless of OC_URL. Fixed
by prepending the derived subpath onto whatever Root/Prefix already
resolved to, guarded against double-prepending since EnsureDefaults
runs more than once per process.
After both fixes: config.json, capabilities, .well-known, the
identifier login page and its static assets, WebDAV, Graph, Archiver,
and Settings all correctly reach their real handler under
/test/opencloud instead of the web app's catch-all, and a full
interactive login flow (landing page through OIDC callback into the
authenticated file browser) works with a clean DevTools console.
Known limitation, out of scope for this PR: some of the web frontend's
own empty-state illustrations and a couple of other images hardcoded an
absolute, domain-root image path rather than a deployment-relative one,
so they 404'd under a subpath. That's a separate, already-prepared fix
in opencloud-eu/web (not this repo).
Migrating from manual per-service env vars: if anyone was already
working around this by hand-setting PROXY_HTTP_ROOT/WEB_HTTP_ROOT/
IDP_URI_BASE_PATH/etc. individually, those explicit values are still
respected (this only fills in defaults otherwise) -- but they can now
be dropped in favor of just OC_URL.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
| Duplication | 14 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codacy flagged services/idp/src/components/ResponsiveScreen.jsx's logo <img> (added by this PR) for missing width/height and for using a raw <img> element. Both come from .codacy.yml's idp exclude entries being wrong -- and have been since before this PR, unrelated to any of its changes: 'idp/src/**', 'idp/scripts/**', and 'idp/ui_config/**' don't match anything, since those directories actually live under services/idp/. Fixed the paths so the identifier app's source (a semi-vendored Kopano/lico-derived CRA app with different conventions than the rest of this Go-centric repo) is excluded as originally intended. Also added an explicit height attribute to the flagged <img> matching its existing CSS default (services/idp/src/app.css's .oc-logo rule), which resolves that specific complaint regardless of the exclude fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Config fields for exactly this (WEB_HTTP_ROOT, GRAPH_HTTP_ROOT, WEBDAV_HTTP_ROOT, IDP_URI_BASE_PATH, FRONTEND_HTTP_PREFIX, PROXY_HTTP_ROOT, OCM_HTTP_PREFIX, ...) already existed on nearly every service, and most services' own routers already gated correctly on them. But the proxy - the single front door all traffic passes through - never consulted PROXY_HTTP_ROOT when matching a request against a policy route, so a prefixed request never matched and fell through to the web app's catch-all; web's HTML server hardcoded ; frontend/ocm declared a prefix but reva's rhttp had no way to apply it; and ocm/sse/userlog/activitylog had no root-gated mux at all. Nothing derived any of this from OC_URL, so even where the mechanism worked, a user had to discover and set 10+ separate env vars by hand.
Testing: go build ./... and go test ./services/... ./pkg/... clean across the whole repo, aside from one pre-existing, environment-only failure (services/search/pkg/opensearch needs a live OpenSearch instance, unrelated to this change). New unit tests for the router, web asset server, subpath helper, the idp Index() template substitution regression, and the Root/Prefix derivation regression described below.
Verified end to end against a real, unmodified passthrough nginx reverse proxy: built opencloud-eu/web from source (not the pinned release) and the idp identifier app, built the full Go binary, and deployed it live under /test/opencloud - a separate container/port/data volumes from any other deployment.
Known limitation, out of scope for this PR: depends on reva PR and opencloud-eu/web PR which are submitted.
Migrating from manual per-service env vars: if anyone was already working around this by hand-setting PROXY_HTTP_ROOT/WEB_HTTP_ROOT/ IDP_URI_BASE_PATH/etc. individually, those explicit values are still respected (this only fills in defaults otherwise) - but they can now be dropped in favor of just OC_URL.
Types of changes