Repository navigation
Conversation
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve this change. I found no blocking defect at 6508e3e1f822e24f91e8e6885046d7e260bc8562. I left one optional documentation correction inline. This is a COMMENT review because the authenticated account is the PR author.
A cluster can contain graphs with different access policies. Previously, deployment status and receipt lookup required read access to every graph. One graph without a policy could therefore block an authorized cluster administrator. This PR removes that extra requirement from the shared deployment check. The administrator still needs the current cluster config_manage permission. Changes to existing graph schemas still need the graph's schema_apply permission.
This fits a workload with several graphs, separate data readers, and operators who inspect deployment results or retry an unchanged configuration. It fixes the authority boundary at its source. It does not add a special bypass for one endpoint.
The important boundaries remain intact:
- The shared check loads the applied policies and checks cluster authority. For an existing cluster, candidate policies cannot authorize their own installation. Exact receipt lookup also checks the current caller before it returns a result.
- Schema preflight and schema recovery retain their graph permission checks. The engine also checks policy through the actor-aware schema methods. The rules for graph creation and storage-owner authority do not change.
- The HTTP boundary rejects graph-scoped credentials and checks cluster authority. Graph data routes retain their separate policy checks.
The tradeoff is deliberate metadata visibility. A cluster administrator can inspect graph identifiers, contract hashes, deployment results, and commit lineage without permission to read graph rows. The receipt types contain no graph rows or schema source. config_manage remains a strong administration permission. This PR does not introduce a weaker status-only role.
The efficiency gain is limited to removing unnecessary graph permission evaluations. Policy loading still reads and verifies the applied policy bundles within the existing bounds. Unchanged deployment attempts still inspect graph contracts for drift. This PR does not make deployment cost constant or remove its dependence on graph availability. I did not benchmark performance.
Long-term liability decreases. The production helper loses five lines overall and removes a boolean that every caller set to true. More importantly, it removes a relationship between unrelated permissions. It adds no public API, stored state, cache, or authority source. The larger line count comes mainly from one regression in the existing test owner. Five similar fixes should continue to use the existing cluster and graph checks, rather than add bypass flags or duplicate grants.
Validation:
- All 115 cluster library tests passed on the restored PR head. The 10 deployment tests passed before any temporary edits.
- With the old implementation restored, the new regression failed at status lookup with
graph_policy_requiredfor the unrelated sibling. - A temporary extension removed the owner group’s graph-read grant from an installed policy. Metadata access and unchanged convergence still passed. The explicit graph-read check returned
policy_denied, while the schema permission remained valid. - Local formatting, documentation links, documentation checks, and diff whitespace checks passed. I restored all temporary source changes.
- Workspace CI passed, including the new regression and recovery authorization tests. Server CI passed the deployment ownership and receipt-retry test. CI tested merge commit
9ee658c. Its file tree exactly matches this PR head. - I checked the callers, returned types, storage adapter, schema checks, and pinned Lance 11.0.0 source (
ab6b5bbe). Status reads use the cluster storage adapter. The graph contract checks use the existing read-only open path. This PR changes no Lance operation or publication rule.
Local execution used file storage. Cloud and HTTP results above are CI evidence, not local reproductions. Azure integration CI was still running at review time. The local linker reported an unwind-table size warning, but all tests completed successfully.
| `read` on unrelated graphs. Reading graph data still requires that graph's | ||
| applied read permission. New graphs need suitable declared policies too. |
There was a problem hiding this comment.
[P3] Qualify the read-permission statement for installed policies
This sentence describes a stronger rule than the server enforces. With an ordinary bearer token and no graph policy, handlers::authorize permits Read. See the existing fallback and its regression, which passed in this head's CI. Signed data credentials have a separate rule. Please say that graph reads retain their existing authorization rules and that an installed graph policy must permit the read. This avoids implying that a graph without a policy is unreadable. This is an optional wording correction, not a defect in the deployment check.
|
Closing as superseded by merged #878, shipped in 0.13. Deployment metadata authorization now uses cluster Evidence: replacement PR, current deployment regression. No remaining implementation from this draft needs a second merge. |
Summary
Extract the configuration-metadata authorization change already present in #878 for independent review and qualification, without its CLI, graph-deletion, or broader lifecycle changes.
config_managepolicy, rather than requiringreadon every graph in the inventory.schema_applychecks unchanged.Verification
GET /cluster/deploymentsreturns HTTP 403graph_policy_requiredfor a cluster administrator when one unrelated sibling lacks a graph policy. The ledger remains byte-for-byte unchanged.cargo fmt --all --check: passed.git diff --check: passed.This change does not deploy or replace any production server and does not add graph-data privileges or change storage formats.