fix(credentials): deny non-admin patches that would widen an auto-enabled destination

An auto_enable destination with no explicit grants falls back to proxy-wide
visibility, so a team-admin emptying the last team grant turned a
team-scoped destination into one receiving every tenant's traces. The
decider now refuses a non-admin patch when the post-merge access would
have no grant left on an auto_enable destination; revoking with a
surviving org or global grant, revoking on manual destinations, and
proxy-admin edits are unchanged. The deny reason is safe to surface
because the path is only reachable by a caller whose own teams hold every
grant being removed, so the destination is already visible to them
This commit is contained in:
Yucheng Zhu 2026-07-24 17:59:51 -07:00
parent 3fe6b46f39
commit 3d6951476b
2 changed files with 69 additions and 1 deletions

View file

@ -70,7 +70,8 @@ def decide_credential_patch(
modify any immutable ``credential_info`` field, and (c) limits its
``access`` change to appending team_ids the caller is team-admin of to
``access.teams`` (no removals, no foreign ids, no ``global``/``orgs``
edits).
edits), and (d) does not remove the last explicit grant from an
``auto_enable`` destination, whose empty-grant fallback is proxy-wide.
"""
if is_proxy_admin:
return Allow()
@ -129,4 +130,14 @@ def decide_credential_patch(
from_user_input=True,
)
auto_enable = existing_info.auto_enable if existing_info is not None else False
had_explicit_grants = bool(existing_global or existing_teams or existing_orgs)
grants_after_merge = bool(existing_global or existing_orgs or patch_teams)
if auto_enable and had_explicit_grants and not grants_after_merge:
return Deny(
"removing the last grant would make this auto-enabled destination "
"proxy-wide; only the proxy admin can remove it",
from_user_input=True,
)
return Allow()

View file

@ -293,6 +293,63 @@ class TestTeamAdminRevoke:
assert "team-B" not in d.reason
class TestAutoEnableWideningGuard:
"""Emptying the last grant of an auto_enable destination falls through to
the documented empty-grants-means-proxy-wide fallback, so a narrowing edit
by a team-admin would widen the destination to every tenant. The decider
must refuse to cross that boundary for non-admins."""
def test_emptying_sole_grant_on_auto_enable_destination_is_denied(self):
existing = {
**_EXISTING_INFO,
"auto_enable": True,
"access": {"global": False, "teams": ["team-T"], "orgs": []},
}
d = _decision(
existing_info=existing,
patch_info={"access": {"teams": []}},
)
assert isinstance(d, Deny)
assert "proxy-wide" in d.reason
def test_emptying_teams_with_surviving_org_grant_is_allowed(self):
existing = {
**_EXISTING_INFO,
"auto_enable": True,
"access": {"global": False, "teams": ["team-T"], "orgs": ["org-1"]},
}
d = _decision(
existing_info=existing,
patch_info={"access": {"teams": []}},
)
assert isinstance(d, Allow)
def test_emptying_sole_grant_without_auto_enable_is_allowed(self):
existing = {
**_EXISTING_INFO,
"auto_enable": False,
"access": {"global": False, "teams": ["team-T"], "orgs": []},
}
d = _decision(
existing_info=existing,
patch_info={"access": {"teams": []}},
)
assert isinstance(d, Allow)
def test_proxy_admin_may_empty_sole_grant_on_auto_enable_destination(self):
existing = {
**_EXISTING_INFO,
"auto_enable": True,
"access": {"global": False, "teams": ["team-T"], "orgs": []},
}
d = _decision(
is_proxy_admin=True,
existing_info=existing,
patch_info={"access": {"teams": []}},
)
assert isinstance(d, Allow)
class TestTeamAdminMultipleTeams:
def test_can_add_multiple_own_team_ids(self):
d = _decision(