Repository navigation
Expose typed endpoint permissions and detect permission drift - #999
Conversation
990a59f to
5770830
Compare
|
I'm not really sure that we should bundle permissions. One thing is API schema, it's supposed to be backwards compatible. But permissions may change (become more gradual). That information can be easily extracted in runtime by requesting the OpenAPI schema via |
|
@mdevils I'm open to it, but my concern would be
perhaps caching the OpenAPI locally could work but it would need some baked in fallbacks. |
|
@sdairs from what I see in the code, permissions are not used anywhere right now. My understanding is that they will be used in permission negotiation process, which of course requires a network connection. Is there any other use-case? |
|
@mdevils that's a good point. It will exclusively be used for cloud stuff, and so the offline support side of things is probably not that important. That said, the CLI still needs to work offline. Even though you won't be able to actually do any cloud operations, we can't have failures if people do want to explore cloud commands offline, so there needs to be something. Primary use case for the permissions is agent permission negotiation. I also don't think that we necessarily need to go the dynamic route from the get-go. None of this is a single-way door, we can always go down a dynamic route in the future if we see the permissions are changing to a degree that the CLI is becoming out of sync. We have a similar thing anyway with commands being changed in the Open API that are then hard-coded in the CLI, and fields of requests changing in the Open API, so we have this problem outside of just permissions already. To date it hasn't been a problem, we move fast to react to change in the OpenAPI. I am not entirely sure what the second question means - a user/agent doesn't need to know what OpenAPI operations are performed behind the scenes. |
OK, fair enough, we can start this way.
Operations are exactly the list you've generated in this PR: I.e.: pub const ACTIVE_BALANCES_GET: OperationMetadata = OperationMetadata {
operation_id: "activeBalancesGet",
rust_method: "active_balances_get",
method: "GET",
path: "/v1/organizations/{organizationId}/activeBalances",
required_permissions: &["control-plane:organization:view-billing"],
};I see that you map If so, it would be useful to have some kind of flag:
And then it shows what permission it needs to run it. That way agent can figure out for what commands what permissions are needed. |
5770830 to
10dbbbc
Compare
…1000) Cloud command help currently does not advertise the permissions needed by its API calls. Declare each command's operations beside its domain definitions, then derive help from the HTTP library's operation metadata so permission strings have one source. - Cover all 175 executable Cloud paths, including the runnable `org prometheus` parent. Pure grouping commands and local commands receive no endpoint-permission lines. - Combine permission IDs across multiple calls, deduplicate them, and retain flag/workflow conditions for selectors, read-before-write, setup, diagnostics, polling, cleanup and rollback. - Use one decorated clap factory for parsing and help. Reject missing, duplicate, stale or unknown declarations and conditional flags that do not exist; non-OpenAPI commands require an explicit explanation. - Keep permission information in the existing eight-line agent context. OAuth and SQL authorization behavior are unchanged. Closes #998 after prerequisite #999 lands. ### Stack and merge order This is the second PR in GitHub stack #1001, targeting #999 (`codex/998-permission-metadata`). PR #999 now targets `main`; both permission commits have been replayed onto the merged prerequisite stack, including #993. Merge #999 before this PR. ### Validation - `cargo fmt --all --check` - `cargo clippy -p clickhousectl -- -D warnings` - `cargo test -p clickhousectl` - `cargo check -p clickhousectl --no-default-features` - `cargo clippy -p clickhousectl --all-targets --no-default-features -- -D warnings` - `python3 -m unittest discover -s scripts/tests -p 'test_*.py'` (96 tests; both path classifiers include the new source file) - Whole-tree declaration and help-structure checks; negative tests for undeclared/stale/invalid declarations; conditional permission union and exact flag validation. - Real-binary help without credentials/network calls, multiple-permission endpoint coverage, and observed HTTP calls for force deletion with managed-key cleanup and query-endpoint read-before-write. - Independent audit of declarations against the current handlers and shared helpers. The enforced structural guarantee is declaration coverage. The audit and focused request tests verify current call paths; this does not introduce arbitrary Rust call-graph analysis or a new runtime authorization gate. ### Rebase verification Rebased onto merged `main` without conflicts; `git range-diff` confirms both permission patches are unchanged. Formatting, all eight permission-unit tests, three permission-help subprocess tests, the query-endpoint request test, and the inherited pagination shell-token test pass on the rebased top branch. CI and live Cloud checks must attest the new PR head.
The OpenAPI analyzer currently ignores operation security requirements, so published permission changes can leave the library and CLI with no usable permission information. Add an offline, typed operation catalog and drift checks that keep it aligned with the spec.
meta::operations, with exact operation ID, Rust method, HTTP method/path, and all required API-key permission IDs. Unknown operation lookup returnsNone; a known empty list means a valid API key with no extra named permissions.Refs #998. CLI consumption is the separate PR #1000 in the permissions series.
Stack and merge order
This is the first PR in GitHub stack #1001, targeting
main. PR #1000 builds on this branch. The previous prerequisite stack and external PR #993 have merged; both permission commits have now been replayed onto that mergedmain. Merge this PR before #1000.Validation
cargo fmt --all --checkcargo clippy -p clickhouse-cloud-api -p clickhouse-openapi-analyzer --all-targets -- -D warningscargo test -p clickhouse-cloud-api -p clickhouse-openapi-analyzerpython3 -m unittest discover -s scripts/tests -p 'test_*.py'Live Cloud provisioning tests are not part of the local test run; this change adds static metadata and offline tooling, with no request-execution changes.
Rebase verification
Rebased onto merged
mainwithout conflicts;git range-diffconfirms both permission patches are unchanged. Formatting, all eight permission-unit tests, three permission-help subprocess tests, the query-endpoint request test, and the inherited pagination shell-token test pass on the rebased top branch. CI and live Cloud checks must attest the new PR head.