vincbeck commented on PR #74371:
URL: https://github.com/apache/airflow/pull/74371#issuecomment-6269431522
The length of the PR makes it hard to review it by myself, so I ran my agent
on it and here is what I got:
```
Must fix
1. New non-team grants stay active after switching to team mode.
- create-team only narrows ReadOnly and Admin (commands.py
_update_read_only_permission_resources / _update_admin_permission_resources).
- It never narrows the 25 new permissions, which are attached to plain
role policies (Allow-Viewer, …).
- Because the resource server is now forced to AFFIRMATIVE, those grants
take effect on their own:
- AdminViewRead gives every Viewer access to AdminView, which is
supposed to be SuperAdmin-only in multi-team mode. is_authorized_view checks
the unsuffixed AdminView resource.
- ConnectionWrite, PoolWrite and VariableWrite let any Op write
team-less connections, pools and variables. In team mode only SuperAdmin should
be able to.
2. The decision-strategy change is client-wide and silent.
_ensure_resource_server_decision_strategy switches existing clients, team or
not, to AFFIRMATIVE and prints nothing on a real run. Custom permissions that
used UNANIMOUS to deny access stop working. This needs a printed message plus a
changelog and docs note.
Should fix
3. The docs are out of date. permissions.rst still lists the old four
permissions. The 0.9.0 changelog note says re-running create-permissions
updates ReadOnly and Admin, but now it creates new permissions instead.
4. The _validate_permission_creation_plan(teams) check in create-all builds
both sides from the same constants, so it can never fail. Only the create-team
call, which uses live scopes, checks anything real.
5. A real run of _attach_default_role_permissions makes about 120 attach
calls of roughly 6 HTTP requests each, so around 700 requests. Attaching all of
a permission's policies in one update would cut that sharply.
Nits:
- Two back-to-back if args.dry_run / if not args.dry_run blocks in
create_all_command can be merged.
- _get_permissions_to_create is called only for its validation and its
return value is thrown away.
- Each dry-run attach call now downloads the full authz export.
- The PR body says Ruff and the unit tests haven't been re-run since the
split.
```
What do you think?
--
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]
