feat: path-scoped repository visibility (Phase 1) for #18#25
Conversation
|
hello bro @beardthelion kindly rebase to main and please fix conflicts |
Implements per-path read visibility for repositories, addressing Gitlawb#18. A repo's private status becomes a property of a path subtree rather than a whole-repo boolean, enforced on the git read path via the node's existing DID identity and RFC 9421 HTTP signatures. Phase 1 scope (whole-repo enforcement, data model for the rest): - visibility_rules data model (path_glob, mode, reader_dids) + DB access - visibility_check pure decision function (owner-always-allow, most- specific rule wins, reader-DID allow-list, is_public fallback) - enforcement on git_info_refs and git_upload_pack: unauthorized reads return 404 byte-identical to a missing repo (no existence leak) - owner-only management API: PUT/DELETE/GET /api/v1/repos/{o}/{r}/visibility - gl visibility set/remove/list CLI mode A (hide) is whole-repo ("/") only by construction: hiding a subtree's existence would rewrite ancestor tree hashes and diverge history. Subtrees use mode B (content withheld, SHAs intact); B-subtree clone filtering is deferred to a later phase and the CLI/API say so. 260 tests (69 node + 191 gl); fmt + clippy clean.
5f14098 to
b5fc7dd
Compare
|
Done, rebased onto current main and pushed. The only real conflict was in server.rs: the visibility route now goes through the new add_auth_layers helper alongside protect (so it gets HTTP Signature + UCAN chain validation, with UCAN passthrough when no X-Ucan header is sent, matching the owner-direct flow). The visibility_rules table auto-merged into the v1 migration list next to protected_branches. 281 tests pass (86 gitlawb-node + 195 gl), fmt and clippy clean. Shows mergeable now. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds path-scoped repository visibility: core decision logic and tests, DB migration and CRUD, owner-only REST endpoints (PUT/DELETE/GET), enforcement in git read handlers, server routing, and a CLI with signed requests and tests. ChangesRepository Visibility Rules
Sequence DiagramsequenceDiagram
participant CLI
participant NodeClient
participant Server
participant Db
participant GitHandler
CLI->>NodeClient: cmd_list / cmd_set / cmd_remove
NodeClient->>Server: PUT/DELETE/GET /api/v1/repos/{owner}/{repo}/visibility (signed for GET)
Server->>Db: set_visibility_rule / remove_visibility_rule / list_visibility_rules
Db-->>Server: OK / rules
Server->>GitHandler: git-upload-pack/info-refs request (client)
GitHandler->>Db: list_visibility_rules
GitHandler->>Server: visibility_check decision (owner/caller, path)
Server-->>GitHandler: Allow or RepoNotFound (Deny)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/api/visibility.rs`:
- Around line 56-75: Validate req.path_glob before calling
state.db.set_visibility_rule: ensure it's non-empty, begins with '/' and parses
as a valid glob pattern (use glob::Pattern::new or equivalent) and return
AppError::BadRequest on failure; keep the existing VisibilityMode::A check but
perform the path_glob format validation first (check req.path_glob, parse it,
then proceed to the mode == VisibilityMode::A subtree check and finally call
set_visibility_rule).
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 612-624: Migration v1 currently contains creation of the
visibility_rules table and idx_visibility_rules_repo index which will be skipped
on nodes that already applied v1; extract those DDL statements out of migration
v1 and place them into a new sequential migration (e.g., migration v2) so they
are applied on upgrade, referencing the visibility_rules table name and
idx_visibility_rules_repo index in the new migration; ensure the migration
registry/versioning code recognizes and runs the new migration during startup
and add a small migration test that simulates upgrading from v1 to v2 to verify
the table and index get created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d09eabfb-ba85-4679-a379-23f2633c15e9
📒 Files selected for processing (10)
crates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/api/visibility.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/visibility.rscrates/gl/src/http.rscrates/gl/src/main.rscrates/gl/src/visibility.rs
- migrations: move visibility_rules DDL out of the v1 migration into a new v3 (CodeRabbit, critical). Appending to v1 meant nodes that had already recorded v1 would skip the statements forever, so the table would be missing after upgrade. v3 matches the convention Gitlawb#23 set with v2 (agent_profiles) and the documented 'never append to v1' rule. - api: validate path_glob on set (must start with '/', no trailing '/', only a trailing '/**' wildcard, '/' for whole-repo not '/**'). Empty or slash-less globs would otherwise silently misconfigure access. Added unit tests for the accepted/rejected forms. 281 -> 283 tests pass (88 node + 195 gl); fmt + clippy clean.
|
Thanks @coderabbitai, both addressed in 5a59064:
283 tests pass (88 gitlawb-node + 195 gl), fmt and clippy clean. |
|
Tip For best results, initiate chat on the files or code changes.
281 → 283 tests with the new validation coverage is a healthy uptick. Happy with the state of this. 🐇✨ |
Implements path-scoped repository visibility (Phase 1), the feature proposed in #18: a repo's "private" status becomes a property of a path subtree rather than a whole-repo boolean, enforced on the git read path using the node's existing DID identity and RFC 9421 HTTP signatures.
This supersedes the demo in #24 (which deliberately enforced only the existing
is_publicboolean to make the design concrete). This PR is the real, mergeable Phase 1.What this does
visibility_rulesdata model (path_glob,mode,reader_dids) with DB access methods, modeled onprotected_branches.visibility_checkpure decision function: owner is always allowed, most-specific matching rule wins, reader-DID allow-list, falls back to the legacyis_publicflag. Exhaustively unit-tested (truth table).git_info_refsandgit_upload_pack: an unauthorized read returns404byte-identical to a missing repo, so a private repo's existence does not leak.PUT/DELETE/GET /api/v1/repos/{owner}/{repo}/visibilitybehindrequire_signature.gl visibility set/remove/list.Phasing and a deliberate constraint
mode A(hide existence) is whole-repo (/) only by construction: hiding a subtree's existence would rewrite every ancestor tree hash and diverge history (the Merkle DAG hashes child names+hashes). Subtrees therefore usemode B(content withheld, SHAs intact). The data model carriesmodefrom day one so later phases need no migration.Two pieces are intentionally not in this PR and are flagged as such in the CLI/API output:
git upload-pack, which won't impose server-side authz; this needs a cached filtered view). Whole-repo rules are fully enforced today.Tests
260 tests pass (69
gitlawb-node+ 191gl);cargo fmt --checkandcargo clippy --all-targetsclean.Closes nothing on its own; advances #18.
Summary by CodeRabbit
New Features
Tests