Skip to content

acl: reconciliation reports access reductions but never expansions #108

Description

@drernie

Summary

analyze_user_downgrades compares each user's effective access before and after a
diff and reports only reductions. There is no counterpart for growth, so a change
that broadens access for everyone is invisible at the default verbosity.

Detail

_analyze_user_downgrade computes lost_permissions one-directionally
(quiltx/acl.py:1386-1391): a bucket is listed only when the after rank is
strictly lower than the before rank. Nothing computes the inverse, and
UserDowngrade.is_downgrade() (:199) gates on lost_permissions or admin_lost or undetermined, so a pure expansion produces no finding at all.

The non-verbose diff does not fill the gap either. print_diff emits one line per
changed role (quiltx/acl.py:1720-1723):

~ role public
~ role restricted

That is the whole report. Which policy arrived, and what it grants, appears only
under --verbose, which prints the composed policies: list per role.

Impact

This became materially more likely with config.default_policy (#105). Adding one
line to a policy composes it into every managed role, including deliberately
narrow service or bot roles, so a one-line diff can widen access across the whole
stack and the default output says only ~ role X per role. Reproduced during review:
a restricted role scoped to bot-bucket silently gained read on open-bucket,
with no downgrade finding (correctly — access grew) and no expansion finding either.

The asymmetry predates #105; that feature only made it easy to trigger. A tool with
delete semantics that reconciles permissions should be as legible about widening as
about narrowing, because over-granting is the failure mode nobody gets a warning
about.

Proposed fix

Two independent parts, either useful alone:

  1. Name the composed policy in the non-verbose role line, so ~ role restricted
    becomes something that says what changed and which buckets it adds.
  2. Add an expansion counterpart to the downgrade analysis — gained_permissions /
    admin_gained alongside the existing fields, reported in its own block rather
    than mixed into !! DOWNGRADE.

(2) is the larger change and needs a decision about whether growth is loud by
default or behind a flag; (1) is small and independently worth doing.

Notes

Found by a design-level review of #104/#105/#96/#106 (PR #107) and deliberately left
out of that PR's scope. Independent of the other follow-ups.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions