Codacy flagged the colon-path middleware comment claiming both `$`
and `!` need percent-encoding for chi's tree match, while the
implementation only calls r.URL.RawPath = r.URL.EscapedPath() which
does not encode either character per the suggestion's reading.
In practice EscapedPath does encode `!` as `%21` (only `?` is the
hardcoded escape, `!` is escaped because Go's net/url treats it as
needing encoding outside specific contexts). It leaves `$` literal,
which chi handles fine. chi.URLParam returns the encoded segment
verbatim, and downstream OpenCloud handlers (parseIDParam,
GetDriveAndItemIDParam) already PathUnescape before parsing IDs, so
the round-trip works end-to-end. The acceptance tests on this
branch already exercise this with real `$`/`!`-containing IDs.
Add a focused unit test that mounts the middleware behind chi,
sends a colon URL, and asserts the actual contract:
- driveID (only `$`): chi.URLParam returns it literal
- itemID (with `!`): PathUnescape(chi.URLParam(...)) == original
Update the misleading comment so future readers (and reviewers) see
what the encoding actually does and which downstream contract it
relies on.
Three more Copilot review nits on the colon-syntax middleware:
- CS3 Stat returning UNAUTHENTICATED now surfaces as HTTP 401 (was 500
via the default-case fallback). Distinct sentinel + handler branch.
- Item-anchored form (/drives/{driveID}/items/{itemID}:/...) now
validates that driveID's storage/space prefix matches the itemID's,
short-circuiting to 400 InvalidRequest on mismatch instead of doing
a CS3 Stat that would only fail at the handler layer.
- ParseID failures on the anchor id now surface as 400 (was 404). In
practice this branch is defensive — storagespace.ParseID is lenient
and only errors on empty input, which the regex already filters out
— but the right semantic for malformed client input is 400, not 404.
Tests: added two new cases (UNAUTHENTICATED → 401, drive/item id
mismatch → 400). The "malformed drive id" case Copilot suggested
isn't reachable in practice given ParseID's leniency, so it's not
covered.
Two follow-up Copilot review nits on the colon-syntax middleware:
- Don't blank r.URL.RawPath after the rewrite. Graph.ServeHTTP sets
RawPath = EscapedPath() as a workaround for chi's parameter-binding
quirks with `$`/`!` in IDs (see go-chi/chi#641). Clearing RawPath
for rewritten requests negates that workaround. Re-establish the
same invariant after the rewrite.
- Tests previously used log.NewLogger(), which mutates global zerolog
state. Switch to log.NopLogger() — order-independent, no global
side effects.
- Drop double-decoding of URL path components. r.URL.Path is already
decoded by net/http; calling url.PathUnescape again would let crafted
inputs like "%252F" become "/", changing path semantics. Path and
anchor-id strings now go straight to utils.MakeRelativePath /
storagespace.ParseID.
- Distinguish operational errors from "not found". Gateway selection
failure, RPC transport errors, and unexpected CS3 status codes now
surface as 500 (don't mask outages); only NOT_FOUND and PERMISSION_DENIED
collapse to 404 (no existence disclosure). Implemented via a sentinel
errPathNotFound + a 500 fall-through.
- Refactor the two regex-handling branches in rewriteColonPath to share a
resolution path via a small colonMatch struct + matchInto/extract helpers.
Removes the SonarCloud duplication finding without changing behavior.
- Update the docstring on ResolveGraphPath to reflect that the rewrite
preserves the requested API version (/{version}/...) rather than
hard-coding /v1beta1/...
- Test cleanup: register t.Cleanup to remove the per-subtest selector
entry from pool's global selectors map after the subtest, and update
the "unexpected status" case to expect 500 (matches the new error
semantics).
Adds a chi middleware that detects MS Graph colon-syntax URLs and rewrites
them to the canonical /items/{itemID}/... form before chi performs route
matching. Existing handlers, routes, and GetDriveAndItemIDParam stay
unchanged.
Two URL shapes are recognized at both /v1.0 and /v1beta1:
/drives/{driveID}/root:/<path>[:/<suffix>][:]
/drives/{driveID}/items/{itemID}:/<relativePath>[:/<suffix>][:]
Path resolution runs as the request user via CS3 Stat. Both NOT_FOUND and
PERMISSION_DENIED collapse to a 404 response so existence isn't disclosed
to unauthorized callers. URLs without colon syntax fast-path through with
a single substring check. The original URL is stashed in request context
under OriginalPathContextKey for downstream tracing/logging.
The middleware is registered as a top-level mux.Use so it runs before any
route matching: chi middleware on a sub-router runs after the prefix is
matched but cannot redirect to a different leaf route. Top-level
middleware lets URL rewriting actually re-route the request.
Tests cover regex matching across versions, all rewrite variants
(root/items anchored, with/without suffix, with/without trailing colon,
deep paths), NOT_FOUND -> 404, PERMISSION_DENIED -> 404 (security: no
existence disclosure), and original-URL preservation in request context.
Allow a "permission denied error" from reva to bubble up to the client.
Reva was fixed to return "permission denied" only when the space to be
delete can actually be listed by the user. Other wise it will return
"not found". See reva commit 1bf72cb76394671f373e87f15f23f978cf41ab08.
So when a user with the 'can manage' role tries to purge an already
disabled space it will now get "Forbidden" status instead of a "Not
found".
Also fixes the expected status codes in the tests.
GetRoleDefinition/s does now handle l10n correctly. Previsouly it just
returned the non-localized string. What made things worse was that
ListPermissions() mutated global list of available roles and replaced
some strings with translated values depending on the `accept-language`
header. Which resulted in GetRoleDefinition returning results in mixed
localization depending on who/what called ListPermissions before.
Fixes: #2800
The Libre Graph spec declares webUrl on driveItem but
cs3ResourceToDriveItem never set it. Build the /f/<resource-id>
permalink off the configured public base URL (same source drives.go
reads for drive.webUrl) and parse it once at service construction
so callers can use it as a plain field.
This gets us rid of quite a bit of special casing for space permission.
Also provides us with "real" permission IDs instead of those faked
"u:<userid>" ones.
Instead of doing a lexical sort we sort the drive/space in a more
"natural" order so that e.g. "Space 2" is sorted before "Space 10".
Related: https://github.com/opencloud-eu/web/issues/2430
