Propagate tag constraints from tagged groups to associated roles - #601
Propagate tag constraints from tagged groups to associated roles#601barborico wants to merge 24 commits into
Conversation
Gates whether a tag's constraints reach associated roles. Separate from `enabled` so that turning off propagation does not turn off the tag.
Propagation feeds a role's member-side constraints only: a member association reads the same key on the associated group, an owner association reads the owner-side counterpart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Returns every constraint in force with its coalesced value and the tags contributing it, so the UI panel and enforcement share one source of truth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
include_provenance=True (effective_constraints' only behavior beyond
earlier callers of constraint_sources) was untested: all existing
tests reached the role via _propagated_sources, whose origin is
hardcoded, so _own_tag_sources' "direct" vs "app" branch (driven by
OktaGroupTagMap.active_app_tag_mapping) never ran.
Add two tests that exercise effective_constraints directly against a
group's own tags: one tag applied straight to a group ("direct"), one
inherited from the group's App via AppTagMap ("app"). Both need a new
_load_group_with_provenance helper, since the existing _load_role
loader doesn't eager-load active_app_tag_mapping -- confirmed by a
scratch test that calling effective_constraints without that eager
load raises on the lazy="raise_on_sql" relationship. No production
code changed; this is coverage-only.
Deletes the hand-rolled associated-group traversals in both constraint classes. The helper covers both the member and owner association directions, and both are now gated by Tag.propagate_to_roles. Adds blocking_source() and constraint_source_clause() to api/models/tag.py so the coalesced boolean checks can still name the offending group in error messages, since coalescing on its own would discard that detail.
test_owner_association_blocks_self_add_to_role proves the owner-association direction for CheckForSelfAdd, but no equivalent existed for CheckForReason; the only test touching that path (test_require_reason_modify_group_users) is confounded because its role fixture is both a member of a require_member_reason group and an owner of a require_owner_reason group, so either source alone passes even if the owner-association branch regressed. Add a test that isolates a role owning a require_owner_reason group with no member association, and checks both that a reasonless add is rejected and a reasoned add succeeds.
Closes the control gap: time-limit constraints now reach roles, in both association directions, at grant time.
No behaviour change for the swapped call sites -- these targets are never
roles, so effective_constraint/effective_ended_at reduce to the same
own-tags-only computation coalesce_constraints/coalesce_ended_at already
did. Consolidating them means a future constraint gets propagation without
a new traversal.
Two of the brief's call sites are deliberately left on the old helpers,
because the swap is not neutral there:
- create_group_request.py:116 has no group instance to pass -- the
GroupRequest targets a group that does not exist yet, so only a bare
Tag list and a hardcoded group_is_managed=True are available.
- role_requests.py's owned_groups_no_self_member (DISALLOW_SELF_ADD_
MEMBERSHIP_CONSTRAINT_KEY) iterates groups owned by the assignee with no
type filter, so the target can be a RoleGroup a user owns. That key
propagates onto roles, and effective_constraint would then read
active_role_associated_group_{member,owner}_mappings, which this query
does not eager-load -- raising InvalidRequestError for a role owner
browsing "requests I can resolve". No existing test exercises this path.
The sibling owned_groups_no_self_owner call (DISALLOW_SELF_ADD_OWNERSHIP_
CONSTRAINT_KEY) was swapped since owner-side keys never propagate onto a
role regardless of type, so it is unconditionally safe.
See .superpowers/sdd/task-6-report.md for the full per-site analysis.
The assignee-filter branch of the role-requests list was left on the old
coalesce_constraints helper because owned_groups can contain a RoleGroup
(a role owner's own role), and effective_constraint reads
active_role_associated_group_{member,owner}_mappings for propagation,
which the query didn't eager-load; swapping without the loads would
raise InvalidRequestError for role owners.
This filter decides which pending role requests an assignee may act on,
so leaving it on the direct-tags-only helper let it drift from
enforcement (which already reads propagated constraints), offering an
assignee requests their approval would then be rejected for. Add the
selectin_polymorphic + selectinload stack check_for_self_add.py already
uses for the same relationships, then swap to effective_constraint.
Add a regression test proving a role that inherits
disallow_self_add_membership by being a member of a tagged group (not
tagged directly) is filtered out of the assignee's resolvable list too.
Verified it fails against the reverted coalesce_constraints behaviour.
One bulk update per association direction, computed once -- this operation commits, so it must not be called per-role. tests/test_time_limit_constraint.py's existing counts encoded the old (incomplete) behavior, where a role's own pre-existing members missed out on a tag applied to a group the role is associated with until a later, looser tag happened to touch the role directly. Updated those expectations to reflect the new, correct capping.
Caps grants that predate the feature. Separate from the migration so the mass mutation of ended_at happens on the operator's schedule.
Same helper the enforcement path uses, so the UI cannot disagree with what is actually enforced.
…tail
The prior test for effective_constraints on GET /api/groups/{id} fetched a
role, which only reaches tags via _propagated_sources; that path never
reads OktaGroupTagMap.active_app_tag_mapping, so the eager-load landmine
the task brief warned about was never actually exercised. Add two tests
that fetch a group carrying its own direct or app-inherited tag map, which
walk _own_tag_sources and do touch active_app_tag_mapping; confirmed
load-bearing by temporarily removing the eager load and observing both
fail with the expected raise_on_sql InvalidRequestError.
Shows every constraint in force and where each comes from, including constraints reaching the group by association rather than by tag.
Guards the Grid item wrapper in Read.tsx so groups with no constraints don't pay for empty accordion padding, guards the optional `sources` field against a real crash, switches to the generated API types instead of hand-rolled duplicates, drops an unnecessary `any` cast now that all three GroupDetail variants share the same field type, rounds the seconds-to-days conversion, and gives unrecognized constraint origins a neutral label instead of falsely asserting "direct". Also populates source_group_name with the app's name for origin: "app" in _own_tag_sources, since the eager-loaded AppTagMap.active_app was already available but unused, and the panel had nothing to show after "via app" for any app-inherited constraint.
The view states plainly whether a tag's constraints reach roles; the edit form gates it, with a tooltip distinguishing it from disabling the tag.
Read.tsx built the propagation sentence from its own literal strings so it could bold "do"/"do not" inline, while propagationNote.ts exported a separate plain-string helper that only its own test used; nothing enforced that the two stayed in sync. propagationNote.ts now exposes propagationParts(), and both propagationNote() (a join over the parts) and the new PropagationNoteView component render from it, so drift is no longer possible by construction. PropagationNoteView.test.tsx mounts the real component and asserts its DOM text against propagationNote()'s output. Also trims the unused render/screen imports and renames the test file (previously CreateUpdate.test.tsx, despite testing propagationNote.ts) to PropagationNoteView.test.tsx.
`GET /api/role-requests?assignee_user_id=...` 500'd for Access admins whenever any pending request targeted a `RoleGroup`. The admin branch evaluates `effective_constraint(DISALLOW_SELF_ADD_MEMBERSHIP, rr.requested_group)`, and that key is in `OWNER_SIDE_COUNTERPART`, so for a role target the helper reads `active_role_associated_group_*_mappings` -- `lazy="raise_on_sql"`, which the query did not load. It fired regardless of whether the group carried any tags, since propagation is consulted unconditionally. A role reaches `requested_group` in production via `ModifyGroupType` converting a group that already has a pending `RoleRequest` (unlike `DeleteGroup`, it does not reject pending requests). Pre-branch this call site used `coalesce_constraints` over already-loaded state, so this was a regression. Add the propagation loader stack -- matching the sibling `owned_groups_no_self_member` site -- to both the owner-side and member-side queries. The owner-side one cannot crash today (its key is not in `OWNER_SIDE_COUNTERPART`), but loading it too removes the coupling that made the query safe only because of which constraint key it happened to pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six related backend fixes from the whole-branch review.
`_own_tag_sources` read `.active_app.name` unguarded. `AppTagMap.active_app`
filters `App.deleted_at`, so it is None for a soft-deleted app. Not reachable
through `group_tag_map_options()` today -- it uses `joinedload`, and
`active_app` is `innerjoin=True`, so a deleted app collapses the whole chain
and the source degrades to "direct" -- but every other `active_*` read here is
treated as nullable, and a switch to `selectinload` would make it a 500. The
new test loads it that way to pin the behaviour.
The `value is None ? source.value : coalesce(...)` fold was duplicated in
`effective_constraint` and `effective_constraints`; extract `_fold`. Kept
seeded on `is None`, not truthiness -- a first source contributing `False` or
a `0`-second limit is a real contribution.
`ModifyGroupsTimeLimit`'s retroactive block capped role members without
checking the role is managed, disagreeing with the other two enforcement
points (`effective_ended_at` returns early for an unmanaged group; the
`cap-role-memberships` sweep filters `RoleGroup.is_managed`). Join through and
filter.
`get_tag`'s `propagated_to_groups` was gated only on `propagate_to_roles`, so
a *disabled* tag listed roles that receive nothing -- `_propagated_sources`
short-circuits on `tag.enabled` -- and the UI then claimed those constraints do
apply. It also listed unmanaged roles, which are exempt from enforcement. Gate
on `enabled` and filter `RoleGroup.is_managed`; display now matches
enforcement.
Rename `EffectiveConstraintSourceDetail.source_group_{id,name}` to
`source_{id,name}`: for an "app" origin those fields hold an App's name with a
None id, so the old names were simply wrong. Brand-new public API in an
already-breaking 2.0, so renaming now beats committing to it.
`TagPropagationTargetDetail` keeps its `source_group_*` names, which are
accurate -- its sources really are groups.
Type `EffectiveConstraintDetail.value` as `int | bool` instead of `Any`: `Any`
emits an empty JSON schema, which the client generator renders as `void`.
Also adds the coverage the review called out: POST /api/tags round-tripping an
explicit `propagate_to_roles` (both values) and defaulting when absent; a role
that is both a member and an owner of one tagged group coalescing to the
minimum across both counterpart directions, parametrized so each direction
takes a turn being the shorter one; and a negative test that the CLI sweep
leaves `is_owner=True` grants alone, since owning a role confers nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Frontend follow-through on the review, plus the generated client. `EffectiveConstraints` rounded every time limit to the nearest day, so the one-hour limits the branch's own tests use rendered as "0 days" -- read as no access at all. Render sub-day limits as "<1 day", and pluralize, so a one-day limit no longer says "1 days". With `EffectiveConstraintDetail.value` now typed `int | bool` rather than `Any`, the generator emits `number | boolean` instead of `void`, so the `Number()` cast at the arithmetic site is gone; a `typeof` narrow does the job and `tsc` is clean. `propagate_to_roles` is optional in the generated type and defaults to `true` on the server, so `Boolean(undefined)` encoded the opposite of the default. Use `?? true` on both the tag read page and the create/update form -- the form one mattered most, since opening and saving an older tag would have silently turned propagation off. Renames the `EffectiveConstraintSourceDetail` fields to `source_id` / `source_name` at their frontend consumers, and retitles a `groupsWithTagRows` test that claimed one row per source group while asserting one row total with a chip per source group. Regenerates `src/api/apiSchemas.ts` from the spec. `apiComponents.ts` is deliberately left alone: regenerating reformats it by ~66 lines, but every one of those is prettier collapsing a multi-line object literal and dropping its trailing comma -- verified character-identical once whitespace is stripped -- and that drift is pre-existing on `main`. Documents the seventh tag knob and the propagation rule in `.claude/CLAUDE.md`, which the file itself asks contributors to keep current. The maintenance watermark is deliberately not bumped: this branch isn't merged, so claiming reconciliation against `fb260a2` would be false. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- origin becomes a StrEnum: same JSON, but the OpenAPI schema now carries the value set and the generated client gets a literal union, not string - an app-origin source now carries the app's id, not just its name - blocking_source folds into constraint_source_clause, which now names every blocking source rather than the first: these constraints coalesce with OR, so each one blocks independently and fixing one would not unblock - public helpers document their args, returns, and the eager-load failure Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rewrites the comments and docstrings on this branch that narrated the change rather than the code: the propagate_to_roles column, the self-add check, and three test docstrings. The narrative belongs in commit messages. Adds the convention to CLAUDE.md, along with the expectation that public functions document args, returns, and raises -- pointing at api/plugins/app_group_lifecycle.py as the reference style. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- name why ModifyGroupUsers loads both association directions: they are raise_on_sql and effective_ended_at reads them even for a non-role group - note in both time-limit tests which row moved buckets and why, since the changed counts otherwise look arbitrary - fold propagationNote.ts into PropagationNoteView; the copy now lives in one place and the test asserts it through rendered DOM Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
824b455 to
59b1ca1
Compare
- encodeURIComponent the tag name in the Effective constraints source link. Tag names enforce no character pattern, so a name containing a slash produced an extra path segment that no route matched. - the schema comment claimed source_id is None for an app origin; it carries the app's id. - constraint_source_clause documented a fallback on a missing name, but it falls back on no truthy source. - skip a soft-deleted source group in _propagated_sources rather than dereferencing None, matching how the module reads every other active_*. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| that parses these audit logs (e.g. a SIEM or a Panther detection schema) also needs to be | ||
| updated. Those schemas typically live in the operator's own private repo, outside Access. | ||
|
|
||
| ## Comments and docstrings describe the present |
There was a problem hiding this comment.
Thanks for adding this!
I kept having to tell Claude to do this on my PRs 😂
| tags=[tag_map.active_tag for tag_map in group.active_group_tags], | ||
| initial_ended_at=self.groups_added_ended_at, | ||
| group_is_managed=group.is_managed, | ||
| membership_ended_at = effective_ended_at( |
There was a problem hiding this comment.
This caps the new RoleGroupMap, but nothing caps the role's existing user memberships. ModifyGroupsTimeLimit only runs when tags change (modify_group_tags.py:96, modify_app_tags.py:117, modify_group_type.py:287, tags.py:173), never when an association is created. Attaching a role that has 50 indefinite members to a SOX group leaves all 50 uncapped, and renewing the role's access to the group restores every derived grant without re-reviewing role membership, which is the audit gap this PR exists to close. This recurs on every new association, it is not only a historical-grant problem, and cap-role-memberships is not on a cron, so the design's "enforcement is complete on day one" holds only for users added after the association exists. Note ModifyGroupsTimeLimit.execute() commits, so a fix has to call it once after this operation's own commit with the full groups_to_add set, not per group.
| if group.is_managed and effective_constraint(key, group) is True: | ||
| clause = constraint_source_clause(key, group) | ||
| return False, f"Reason for adding owners to {group.name} group is required {clause}" | ||
| if len(self.members_to_add) > 0: |
There was a problem hiding this comment.
invalid_reason at line 34 uses reason.strip() == "", while the router writes body.created_reason or "" at api/routers/groups.py:707, which is truthiness-based. A whitespace-only reason is therefore rejected when this constraint applies and stored verbatim when it does not, so "provided" is judged by two different standards depending on the toggle. It puts blank-looking reasons into the audit trail.
eguerrant
left a comment
eguerrant
left a comment
There was a problem hiding this comment.
Created a thread to discuss internally but also found some other things that needed changing. Inlined what I could but some files aren't included in the PR so I'm copy-pasting some issues below with file names and line numbers
Not anchorable inline (files absent from the diff, post as top-level review comments)
src/helpers.tsx:73
getActiveTagsFromGroups is the shared entry point for every group-based constraint helper, minTagTimeGroups:118, requiredReasonGroups:128, and ownerCantAddSelfGroups:140, and none of them filter on propagate_to_roles. The UI still mirrors the old unconditional propagation rules in both directions.
src/pages/groups/AddUsers.tsx:135
requiredReasonGroups over the role's associated groups does not consult propagate_to_roles, so with the toggle off this dialog still marks the reason field required at line 292 while CheckForReason no longer requires it. Harmless since the value is stored either way but should be updated.
src/pages/groups/AddUsers.tsx:139
Only possibly an issue (discussion in internal thread)
Also in ownerCantAddSelfGroups, with the toggle off the form still blocks a role owner from self-adding while the API now allows it. That makes the toggle-off self-add bypass API-reachable rather than UI-reachable. I surfaced this issue in the thread but ideally if we decide to keep the current behavior (I don't think we should) the UI should match.
src/pages/groups/AddUsers.tsx:104
timeLimit reads only the group's own tags. The role block at 119 to 142 reassigns reason and disallow_owner_add but not timeLimit, so there is no client-side mirror for the newly propagating time limits at all. Adding a user to a role that is a member of a time-limited group offers the full unrestricted range at 147 to 167, the user picks a year, and the backend silently caps it with no error and no indication. This is the PR's headline feature being invisible exactly where the choice is made.
src/pages/groups/BulkRenewal.tsx:243
Same split on the renewal path. setRequiredReason at 338 to 351 does traverse role associations and does apply the owner-side counterpart via requiredReasonGroups(roleGroupOwnerGroups, true), so it mirrors today's backend rules correctly but is blind to propagate_to_roles. timeLimit at 243 to 253 uses only ownedGroups and memberGroups, so a renewed role membership shows no propagated cap and gets silently shortened.
Suggested fix for the frontend group
The pattern across both dialogs is consistent, the frontend already mirrors the four constraints that propagate today and has no mirror for the two that this PR adds, and it cannot see the toggle in either case. Rather than threading propagate_to_roles through getActiveTagsFromGroups and adding a role traversal for minTagTime, drive both dialogs off the API's effective_constraints. That is the "display and enforcement read the same code" property the design claims, currently achieved only for the read-only panel. EffectiveConstraints.tsx surfaces the value correctly, but it lives on the group read page and is collapsed by default, not in the dialog where the duration is chosen.
src/pages/groups/AddRoles.tsx is fine and needs no change, its minTagTime/requiredReason/ownerCantAddSelf calls at 120 to 125 read the target group's own tags, which is the role-to-group direction that correctly stays ungated.
3 participants
Summary
Adds
Tag.propagate_to_roles, a single switch governing whether a tag's constraints reach roles associated with a tagged group. Propagation is computed at read time. Additionally, applicable constraints are made more visible in the UI.Urgency: one week; we want to put this in place and then initiate a one-time audit in time to show off mid-September
Expected review effort: HIGH — breaking change, security-adjacent, 23 commits
Motivation
Today four of the six tag constraints already reach member roles — unconditionally, invisibly, and with no way to opt out. The two time-limit constraints do not reach them at all. A role granting access to a compliance-scoped group means that, while the suitability of the group for the role is reviewed periodically, the suitability of the role for an individual may not be.
After this change, all six constraints propagate under one operator-visible switch, and the UI shows which constraints are in force on a group and where each one comes from.
Description & Screenshots of Changes
The propagation rule. A role confers access upon its members, so propagation always feeds the role's member-side constraints. What differs is which key is read on the associated group:
Nothing propagates to a role's own owner side — owning a role does not confer the role's grants.
OWNER_SIDE_COUNTERPARTinapi/models/tag.pyencodes both the mapping and, via its key set, which constraints propagate at all.Computed, not materialized. An earlier draft materialized derived
OktaGroupTagMaprows. That was rejected:RoleGroupMapis time-bounded and bulk-ended by operations that bypassModifyRoleGroups, so a materialized cache would need a trigger-completeness invariant and a repair cron — and under a single gate, a missed trigger stops being "a time limit lands late" and becomes "a compliance control is silently off." A computed set cannot drift.Enforcement.
effective_constraintreplaces every hand-rolled tag traversal. The associated-group loops inCheckForSelfAdd/CheckForReasonare deleted; the helper covers both directions, including the owner loops those blocks contained, which an earlier plan would have dropped. Error messages keep naming the group that imposed the restriction, viaconstraint_source_clause.Time limits are the one category that mutates state rather than vetoing, so they land twice: at grant time in
ModifyGroupUsers, and retroactively inModifyGroupsTimeLimitwhen a time-limited tag arrives on a group roles are already associated with. A separatecap-role-membershipsCLI sweep covers grants predating the deploy.UI. A new Effective constraints panel on group and role pages lists every constraint in force with its source (e.g.
SOX, via membership in App-Foo-Admin). Tag chips are deliberately unchanged — chip styling already carries fill-vs-outline for direct-vs-app and grey for disabled, and the question users actually have is about constraints, not tags. The tag page gains a plain-language note on whether that tag reaches roles and a yes/no control to set it.Validation of Changes
make testgreen at every commit; 916 backend / 51 frontend at head, ruff + ty clean.App-Foo-Admin, withRole-Financea member of that group andRole-Auditoran owner:Role-Finance→ member time limit 90d viamember_associationRole-Auditor→ member time limit 30d viaowner_association— i.e. the group's owner limit, read onto the role's member side. This is the counterpart-key rule working end to end.Quarterly Renewaltag with propagation off still applied its own constraints while SOX propagated — confirming the gate suppresses propagation, not direct application.active_app_tag_mappingeager load were each verified by removing the code under test and confirming the test fails.Guidance for Reviewers
Read commit-by-commit. Commits map one-to-one onto logical units and are individually green. Within this PR the steps depend on the one before (helpers → consumers → API → UI), so they are not further decomposable into parallel PRs.
Three places most worth your judgement:
api/models/tag.py— the propagation rule. Everything else calls into it. Worth checking the owner-side axis swap and thatconstraint_key in OWNER_SIDE_COUNTERPARTis the right test for "does this key propagate."The breaking change (
3e52907). Deleting the associated-group loops meanseffective_constraintis now the sole enforcement path for self-add and reason on associated roles. The full suite passed with zero assertion edits, which was the parity bar. Note this also removed a latentUnboundLocalError— the old owner loop interpolatedmember_group.name, so an ownership-only role crashed instead of returning a clean rejection.Eager loading.
lazy="raise_on_sql"makes a missing loader a 500 on a live endpoint rather than a test failure, and one such bug was found and fixed during review (d2ecb00):effective_constraintsraises only for groups that actually carry a tag, so an untagged smoke test proves nothing. Worth confirming no call path intoeffective_constraintlacks its loaders.Rollout note: the advertised breaking change (a tag can now stop propagating self-add/reason) is opt-in and defaults to today's behaviour. The change every operator gets on deploy with no action is the opposite — time limits reaching roles for the first time, so role memberships begin getting shortened immediately. Worth calling out in release notes above the switch itself.
cap-role-membershipsis not wired to any cron; historical grants stay uncapped until it is run.Known follow-ups, deliberately out of scope:
ModifyGroupTypedoesn't reject pendingRoleRequests on conversion the wayDeleteGroupdoes (this is the root cause that made the eager-load bug reachable); sweep-capped rows get noended_actor_id/created_reason; theCLAUDE.mdmaintenance watermark should be refreshed once this merges.🤖 Generated with Claude Code