potiuk commented on PR #70906:
URL: https://github.com/apache/airflow/pull/70906#issuecomment-5153615905
Good diagnosis. Four alternative role policies attached to one permission
with `UNANIMOUS` meant a member had to satisfy Viewer *and* User *and* Op *and*
Admin simultaneously to read their team's Dags — which nobody is, so no
non-admin could read anything.
The fix matches what the file already does elsewhere rather than inventing a
policy: `AFFIRMATIVE` is used in six other places for this same
multi-alternative-role shape, including the global `ReadOnly` a few hundred
lines up, which is the direct analogue of the team `ReadOnly` you are fixing.
So this reads as a case missed when team support was added. The single-policy
permissions (`Admin-{team}`, the super-admin `Admin`) correctly keep the
`UNANIMOUS` default, which is harmless with one policy.
One question: does the same apply to the global `Admin` permission?
```python
for role_name in ("Admin", SUPER_ADMIN_ROLE_NAME):
_attach_policy_to_scope_permission(
client,
client_uuid,
permission_name="Admin",
policy_name=_role_policy_name(role_name),
scope_names=_get_extended_resource_methods() + ["LIST"],
resource_names=[],
_dry_run=_dry_run,
)
```
That is two alternative policies on one permission with no
`decision_strategy`, so it also gets `UNANIMOUS`. By the same reasoning as this
PR, an Admin who is not also a super-admin would fail it. I have not traced how
those two policies evaluate for a given user, so I may be wrong about whether
it bites in practice — but it is the identical shape, and your own argument
applies to it. Worth either folding into this PR or splitting out, whichever
you prefer.
On the compatibility note: since existing Keycloak permissions are not
updated by this, operators need to re-run `create-team` or change the strategy
by hand. That is an action for deployments rather than just a code fix, so it
should land in the provider changelog and not only in the PR description —
otherwise anyone upgrading will still have the broken permission and no obvious
reason why.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
