Skip to content

[API] Add rbac.restrict_all_users_mutations to reserve -u for admins - #10623

Open
SeungjinYang wants to merge 2 commits into
masterfrom
restrict-all-users-mutations
Open

[API] Add rbac.restrict_all_users_mutations to reserve -u for admins#10623
SeungjinYang wants to merge 2 commits into
masterfrom
restrict-all-users-mutations

Conversation

@SeungjinYang

@SeungjinYang SeungjinYang commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Adds an opt-in API server config, rbac.restrict_all_users_mutations (default false), that reserves the --all-users/-u flag on mutating commands for admins: sky down -u, sky stop -u, sky autostop -u, sky cancel -u, sky jobs cancel -u. Without it, any user can tear down or cancel a teammate's work in a single command.
  • Read-only uses of -u (sky status, sky queue, sky jobs queue) are unaffected — this restricts what a user may do, not what they may see.
  • Admins are exempt, as is a server with no per-user identity (local/single-user, or auth terminated upstream). That exemption reuses role_filter.request_owner_scope, which the rest of RBAC already treats as "unrestricted caller".
rbac:
  restrict_all_users_mutations: true

Enforcement is split, by necessity

Command Where it's enforced
sky cancel -u, sky jobs cancel -u Serverall_users is in the request body, so POST /cancel and POST /jobs/cancel reject with 403. Holds for SDK callers.
sky down/stop/autostop -u Client — these expand -u into one request per cluster in _down_or_stop_clusters, so the server never receives the flag and cannot reject it.

For those three the check is therefore a guardrail, not a boundary: a caller driving the Python SDK directly is not stopped by it. The CLI drives it off a per-caller flag surfaced on GET /api/health (already resolved server-side, so the client never has to reason about roles; an older server omits the field and the client defaults to false). This is called out in the docstring and in the config docs, which point at workspaces as the hard isolation mechanism.

Making down/stop/autostop server-enforced needs a per-resource ownership check rather than a flag check — deliberately out of scope here, and a reasonable follow-up.

Implementation notes

  • sky/server/requests/role_filter.py — the two 403 gates are Depends() shims, matching the existing force_caller_scope_cancel_body; the decision needs the caller's role, which only the dispatch context has.
  • No dashboard change: it has no all-users mutation affordance, it acts per-resource.

Test plan

Automated (55 pass):

pytest tests/unit_tests/test_sky/server/test_role_filter.py \
       tests/unit_tests/test_sky/users/test_rbac.py \
       tests/unit_tests/test_sky/test_cli_restrict_all_users.py

covering: restricted user denied, admin allowed, flag off by default, no-auth server allowed, -a/--all never gated, and the CLI gate firing for all five commands.

Also ran a FastAPI TestClient against both endpoints to confirm the request body still parses as JSON through the new Depends, and that restricted → 403, admin → 200, config-off → 200.

Manual:

  1. Set rbac.restrict_all_users_mutations: true in ~/.sky/config.yaml; sky api stop && sky api start.
  2. As a non-admin, confirm each of sky down -u, sky stop -u, sky autostop -u, sky cancel <cluster> -u, sky jobs cancel -u is refused with a message naming the config key.
  3. Confirm sky status -u, sky queue <cluster> -u, sky jobs queue -u and every -a/--all variant still work.
  4. Repeat step 2 as an admin; all five are allowed.
  5. Unset the key, restart, confirm every -u works for everyone (default behavior unchanged).

🤖 Generated with Claude Code


Devin Review

`--all-users`/`-u` on a mutating command fans the operation out over every
user's resources, so one user can tear down or cancel a teammate's work in a
single command. Add an opt-in API server config that reserves the flag for
admins on `sky down/stop/autostop/cancel` and `sky jobs cancel`. Read-only
uses of `-u` (`sky status`, `sky queue`, `sky jobs queue`) are unaffected.

Defaults to false, so an upgrade changes nothing. Admins are exempt, as is a
server with no per-user identity -- that exemption reuses
`role_filter.request_owner_scope`, which the rest of RBAC already treats as
"unrestricted caller".

Enforcement is split, by necessity:
  - `sky cancel -u` / `sky jobs cancel -u` transmit `all_users` in the body,
    so the API server rejects them with a 403. This holds for SDK callers.
  - `sky down/stop/autostop -u` expand the flag into one request per cluster
    in the client, so the server never sees it. For those three the check is
    client-side, driven by a per-caller flag surfaced on `GET /api/health`.
    Documented as a guardrail rather than a boundary; workspaces remain the
    hard isolation mechanism.

Tested:
  - `pytest tests/unit_tests/test_sky/server/test_role_filter.py
     tests/unit_tests/test_sky/users/test_rbac.py
     tests/unit_tests/test_sky/test_cli_restrict_all_users.py` (55 pass),
    covering: restricted user denied, admin allowed, flag off by default,
    no-auth server allowed, `-a/--all` never gated, and the CLI gate firing
    for all five commands.
  - FastAPI TestClient run against both endpoints confirming the body still
    parses as JSON through the new `Depends` and that restricted -> 403,
    admin -> 200, config-off -> 200.
  - Manual: set `rbac.restrict_all_users_mutations: true` in
    `~/.sky/config.yaml`, restart the API server (`sky api stop; sky api
    start`), then as a non-admin confirm `sky down -u`, `sky stop -u`,
    `sky autostop -u`, `sky cancel <cluster> -u` and `sky jobs cancel -u` are
    all refused naming the config key, while `sky status -u`, `sky queue -u`,
    `sky jobs queue -u` and every `-a/--all` variant still work; then repeat
    as an admin and confirm all five are allowed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

- config.rst referenced `:ref:` on a label that does not exist
  (`config-yaml-workspaces`), and the doc build treats warnings as errors.
  Use `<workspaces>`, the label the same file already links to at line 1723.

- Adding a field to `ApiServerInfo` pushed its body from 9 lines to over
  pylint's `docstring-min-length=10`, so `missing-class-docstring` began
  firing on a class that had never needed a docstring. Give it one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant