[tests-only][full-ci]test(cli): add clean-orphaned-grants CLI test scenarios

[anon-pradip]

Description

Add acceptance tests proving the clean-orphaned-grants CLI command detects and removes orphaned share grants.

New test scenarios (tests/acceptance/features/cliCommands/cleanOrphanedGrants.feature) 4 scenarios covering clean-orphaned-grants under different modes:

Scenario	Unique Assertion
non-dry-run mode	"mode: dry run disabled: grants may be changed"
space-id filter	"scope: limiting scan to space" + "1 target space(s)"
force flag	"flags: --force active"
on a space with shares	"== Primary scan =="

All scenarios follow: create project space → upload → share → stop oCIS → orphan grant → start oCIS → run CLI → assert "Orphans: 1 candidate(s), 1 removed".

New step: theShareGrantsForSpaceHaveBeenOrphaned

Orphan creation flow

Test creates orphaned grants by surgically modifying oCIS metadata on disk, so clean-orphaned-grants has real orphans to find:

1. Stop oCIS - server must be down before modifying blob files, otherwise writes may be lost or corrupted by the running process.

2. Get space opaque ID - space ID from Graph API is storageId$opaqueId; split on $, take second part (the actual space UUID). E.g. cbc25bff-...$2b255be2-... → 2b255be2-....

3. Walk blob files - iterate share-manager metadata blobs at:
   ocis-data/storage/metadata/spaces/js/{jsoncs3|oncs3}-share-manager-metadata/blobs/
   Each blob is stored by full UUID, split into 4 hex directory levels + filename.
   Full UUID: 6c627973-37c8-4132-9d2b-a552d315efcd
   ├─┬─┬─┬─┴──────────────────────────┘
   Blob path: blobs/6c/62/79/73/-37c8-4132-9d2b-a552d315efcd
   The first 8 hex chars (6c627973) become 4 directory levels (6c/62/79/73/). The remaining UUID portion (including first hyphen) is the filename.

4. Match grant by space_id - each blob contains JSON like:

{
  "Shares": {
    "cbc25bff-...:2b255be2-...:c8102449-...": {
      "resource_id": {
        "space_id": "2b255be2-..."
      }
    }
  }
}

If resource_id.space_id matches the target space UUID, change it to 00000000-0000-0000-0000-000000000000 (dead UUID - no space exists with this ID).

5. Write blob back - JSON-encode the modified blob and write to disk.

6. Assert - Assert::assertTrue($found) ensures at least one grant was actually modified.

7. Start oCIS - server restarts with the orphaned grant in metadata. After restart, the grant's space_id points to a non-existent space → clean-orphaned-grants reports it as orphan.

Only the matching grant entry is altered (surgical). No blobs deleted, no unrelated grants touched.

Related Issue

• Fixes [QA] Add CLI test to clean orphaned grants #11989

Motivation and Context

How Has This Been Tested?

• test environment:
• locally
• ci

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[kw-security]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[DeepDiver1975]

Reviewed as tests-only — looks good overall. The 4 scenarios build real ocis shares clean-orphaned-grants invocations (correct --service-account-id/secret, --space-id, --force, --dry-run=false flags) through CliHelper::runCommand, and every asserted output substring maps to an actual print statement in ocis/pkg/command/shares.go (pre-flight mode/scope/force lines, == Primary scan ==, Summary:, Orphans:/Reverse orphans: counts). The flag-specific assertions (scope: limiting scan to space, 1 target space(s), flags: --force active) keep the scenarios genuinely distinct and exercise different branches. No hard sleeps, no cross-scenario ordering deps, and the assertion steps (the command output should contain, the command should be successful) are the existing harness steps that decode the JSON response and assert on real stdout — not no-ops. CI is fully green, including the cliCommands acceptance job.

One observation (non-blocking): all scenarios assert Orphans: 0 candidate(s) / Reverse orphans: 0 candidate(s) and none set up a genuinely orphaned grant, so the count is effectively pinned at 0 in every path (incl. the "space with shares" case, where a legitimately-shared file correctly yields a non-orphan grant). These tests confirm the command runs, accepts its flags, scopes correctly, and reports cleanly on a healthy system, but they do not exercise the detection/removal path (count going N→0). A follow-up that orphans a grant and asserts it gets cleaned would strengthen coverage — but that requires corrupting share-manager state, so the current scope is reasonable for acceptance tests.

mergeable=MERGEABLE; the BLOCKED merge state is just the required-review gate.

🤖 Generated with Claude Code

[PrajwolAmatya]

Currently we are only checking the command output. We should also create orphaned grants and run the cli command to check if it removes the grants.

[PrajwolAmatya]

LGTM.
Please rebase the PR.

[dj4oC]

Test-only PR adding 4 acceptance scenarios for the clean-orphaned-grants CLI command. Re-review at head 5e262af — the new commit implements real orphan creation (the theShareGrantsForSpaceHaveBeenOrphaned step), which closes the gap flagged in the earlier pass and matches @PrajwolAmatya's request: the scenarios now assert Orphans: 1 candidate(s), 1 removed against a genuinely orphaned grant rather than a pinned 0. Reviewed as tests-only (coverage lens); the orphan-creation approach is sound and the assertions map to real command output.

Findings

1. (flakiness — please address) Hardcoded decomposedfs path couples the test to the storage backend. getOcisDataPath().'/storage/metadata/spaces/js/…share-manager-metadata/blobs' only exists when metadata runs on local decomposedfs. oCIS CI runs a storage matrix; if any job places metadata on posix/s3, the dir is absent, $found stays false, and Assert::assertTrue($found) hard-fails rather than skips. Recommend gating these scenarios to the decomposed metadata backend (tag/skip) so they don't flake as the matrix evolves.

2. (correctness — worth confirming) explode('$', spaceId)[1] assumes a two-segment id. If getSpaceIdByName ever returns a storageId$opaqueId!item form, parts[1] keeps the !… suffix and won't === the grant's bare resource_id.space_id, so nothing matches and the step fails silently via the assert. Splitting on ! as well would harden it.

3. (over-count risk) The step orphans every matching grant and sets found=true per file. Fine today because Background creates exactly one share, but if the same space_id is persisted in more than one blob (e.g. a created/received index), multiple entries get zeroed and the 1 candidate(s) assertion breaks. Worth confirming the grant lives in exactly one blob. (The second dir name oncs3-share-manager-metadata also looks like a typo for jsoncs3-…; harmless via the is_dir guard, but it means only one dir is ever really walked.)

4. (completeness) Over-deletion is untested. All scenarios prove 1 orphan → removed; none assert that a legitimate, non-orphaned grant survives the run. A command that over-deletes would still pass. Adding a second live share and asserting it remains (with Orphans: 1 unchanged) would close this.

5. (minor) Scenario 4 ("on a space with shares") is the same invocation as scenario 1 (non-dry-run) with only a different asserted line — good candidate to repurpose as the over-deletion/survival case in #4.

JSON round-trip (json_decode(...,true) → json_encode) is low risk here — jsoncs3 stores JSON and the ids are strings; only theoretical exposure is numeric-field coercion.

Verdict

Commenting. The tests are correct and a genuine improvement for the default decomposed backend, so this isn't blocking — but the storage-backend coupling (#1) is the one I'd resolve before merge to avoid matrix flakiness. Please also action the pending rebase requested in-thread.

🤖 Automated review by Claude Code (test-coverage)

---

Generated by Claude Code

[anon-pradip]

1. (flakiness — please address) Hardcoded decomposedfs path couples the test to the storage backend. getOcisDataPath().'/storage/metadata/spaces/js/…share-manager-metadata/blobs' only exists when metadata runs on local decomposedfs. oCIS CI runs a storage matrix; if any job places metadata on posix/s3, the dir is absent, $found stays false, and Assert::assertTrue($found) hard-fails rather than skips. Recommend gating these scenarios to the decomposed metadata backend (tag/skip) so they don't flake as the matrix evolves.

Thanks for raising this. I investigated the claim and found the premise incorrect: metadata storage only supports the ocis (decomposedfs) driver. The Drivers struct in services/storage-system/pkg/config/config.go:74 has a single field OCIS OCISDriver with no posix or S3 option.

Both STORAGE_USERS_DRIVER=ocis and STORAGE_USERS_DRIVER=s3ng use decomposedfs for metadata internally (both set metadata_backend: "messagepack" in services/storage-users/pkg/revaconfig/drivers.go:161,275), so the path storage/metadata/spaces/js/...share-manager-metadata/blobs exists regardless of which user storage driver is selected.

The real risk is Docker container separation. In Docker mode (make test-ocis-feature-*-storage), the PHP test runs in the acceptance-tests container while oCIS data lives in the ocis-server container, with no shared volume for the metadata directory. But cliCommands runs in a dedicated cli-tests job on ubuntu-latest (non-Docker) in GitHub Actions (.github/workflows/acceptance-tests.yml:282), so this does not apply today.

No fix needed. If these tests are ever added to the Docker matrix, the filesystem manipulation in theShareGrantsForSpaceHaveBeenOrphaned would need to be replaced with a server-side CLI approach via the wrapper.

[anon-pradip]

2. (correctness — worth confirming) explode('$', spaceId)[1] assumes a two-segment id. If getSpaceIdByName ever returns a storageId$opaqueId!item form, parts[1] keeps the !… suffix and won't === the grant's bare resource_id.space_id, so nothing matches and the step fails silently via the assert. Splitting on ! as well would harden it.

The claim is theoretically valid but does not trigger in practice.

FormatResourceID in vendor/github.com/owncloud/reva/v2/pkg/storagespace/storagespace.go:74 produces <storageId>$<spaceId> when OpaqueId is empty, and <storageId>$<spaceId>!<opaqueId> when present. For decomposedfs (the only backend that writes to jsoncs3-share-manager-metadata blobs), root nodes always have SpaceId == OpaqueId (vendor/github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/spaces.go:110), so the Graph API clears OpaqueId and produces a clean format without !. explode('$', ...)[1] yields the bare spaceId matching resource_id.space_id.

The ! suffix would only appear if a backend like owncloudsql returned SpaceId != OpaqueId (vendor/github.com/owncloud/reva/v2/pkg/storage/fs/owncloudsql/spaces.go:138), but owncloudsql uses a MySQL-backed share manager, not the jsoncs3 blob files the test walks. The mismatch path and the blob path never intersect.