Summary
The two fatal users: resolution cases are tested at the library level only. Nothing
asserts the contract that actually matters to an operator: the command exits non-zero
and writes nothing.
Detail
Both fatal paths raise ValueError out of compute_diff and are covered by
pytest.raises around that call:
test_user_block_rejects_key_matching_two_emails (tests/test_acl.py:4462) — a key
that names no account and is the email of two.
test_user_block_rejects_two_keys_addressing_one_account (:4349) — two keys
resolving to one account.
Neither goes through acl_tool.main, so the exit code is untested, and neither asserts
that no mutation was attempted.
The behaviour is correct today. _run calls compute_diff at
quiltx/tools/catalog/acl.py:177 and apply_acl at :203, with only
parse_acl_config and fetch_current_state in between — both read-only. Verified by
hand with --yes, a brand-new bucket to register and --create-and-email-users, all
primed to mutate:
Error: Configured user 'shared@example.com' is ambiguous: it is the email of 'alice', 'bob'; key this entry by the server username of the account you mean.
exit code: 1
mutations attempted: []
Impact
Fail-closed is the whole point of making these cases fatal, and it is the property
most likely to be broken by accident later — moving the resolution later in _run,
or wrapping compute_diff in a try that logs and continues, would both leave the
library tests green while turning an abort into a partial apply against real ACLs.
Worth noting the abort is whole-run, not per-entry: one ambiguous key also blocks
bucket registration, policy creation and role reconciliation. That is the safe
direction, but it is a property worth pinning rather than rediscovering.
Proposed fix
A CLI-level test per fatal case using the existing harness
(_install_acl_tool_stack, acl_tool.main([...]), capsys) asserting:
- return code is 1,
- the error names the conflicting usernames on stderr,
- no admin mutation was called — stub
buckets.add, policies.create_managed,
roles.create_managed, users.set_role, users.create etc. to record calls and
assert the recorder is empty.
The fixture wants a diff that would mutate (a new bucket, a role to create) so the
test fails if the abort ever moves after apply_acl.
Notes
Gap identified while verifying PR #107; the behaviour it describes is already correct,
so this is purely about locking it down.
Summary
The two fatal
users:resolution cases are tested at the library level only. Nothingasserts the contract that actually matters to an operator: the command exits non-zero
and writes nothing.
Detail
Both fatal paths raise
ValueErrorout ofcompute_diffand are covered bypytest.raisesaround that call:test_user_block_rejects_key_matching_two_emails(tests/test_acl.py:4462) — a keythat names no account and is the email of two.
test_user_block_rejects_two_keys_addressing_one_account(:4349) — two keysresolving to one account.
Neither goes through
acl_tool.main, so the exit code is untested, and neither assertsthat no mutation was attempted.
The behaviour is correct today.
_runcallscompute_diffatquiltx/tools/catalog/acl.py:177andapply_aclat:203, with onlyparse_acl_configandfetch_current_statein between — both read-only. Verified byhand with
--yes, a brand-new bucket to register and--create-and-email-users, allprimed to mutate:
Impact
Fail-closed is the whole point of making these cases fatal, and it is the property
most likely to be broken by accident later — moving the resolution later in
_run,or wrapping
compute_diffin atrythat logs and continues, would both leave thelibrary tests green while turning an abort into a partial apply against real ACLs.
Worth noting the abort is whole-run, not per-entry: one ambiguous key also blocks
bucket registration, policy creation and role reconciliation. That is the safe
direction, but it is a property worth pinning rather than rediscovering.
Proposed fix
A CLI-level test per fatal case using the existing harness
(
_install_acl_tool_stack,acl_tool.main([...]),capsys) asserting:buckets.add,policies.create_managed,roles.create_managed,users.set_role,users.createetc. to record calls andassert the recorder is empty.
The fixture wants a diff that would mutate (a new bucket, a role to create) so the
test fails if the abort ever moves after
apply_acl.Notes
Gap identified while verifying PR #107; the behaviour it describes is already correct,
so this is purely about locking it down.