shahar1 commented on code in PR #76702:
URL: https://github.com/apache/airflow/pull/76702#discussion_r4563979435
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -787,6 +796,7 @@ def create_team_command(args):
_attach_team_permissions(client, client_uuid, team, _dry_run=args.dry_run)
_attach_team_menu_permissions(client, client_uuid, team,
_dry_run=args.dry_run)
_attach_superadmin_permissions(client, client_uuid, team,
_dry_run=args.dry_run)
+ _attach_readonly_permissions(client, client_uuid, team,
_dry_run=args.dry_run)
Review Comment:
**major** — On a client provisioned with `create-all --teams ...`, the
global `ReadOnly` permission does not exist: the teams branch of
`_get_permissions_to_create` creates `ReadOnly-<team>` only. This call then
reaches `_attach_policy_to_scope_permission(permission_name="ReadOnly", ...)`,
which raises `ValueError("Permission 'ReadOnly' not found.")`, so `create-team`
stops here.
`test_create_team_command` mocks `_attach_policy_to_scope_permission`, so CI
does not see it. A dedicated permission created in the teams branch (see the
next comment) would avoid depending on `ReadOnly` at all.
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -1022,6 +1032,52 @@ def _attach_superadmin_permissions(
)
+def _attach_readonly_permissions(
+ client: KeycloakAdmin, client_uuid: str, team: str, *, _dry_run: bool =
False
+) -> None:
+ team_scoped_resources = [f"{resource}:{team}" for resource in
sorted(TEAM_SCOPED_RESOURCE_NAMES)]
+ _attach_policy_to_scope_permission(
+ client,
+ client_uuid,
+ permission_name="ReadOnly",
+ policy_name=_role_policy_name(READ_ONLY_ROLE_NAME),
+ scope_names=["GET", "LIST", "MENU"],
+ resource_names=team_scoped_resources,
+ decision_strategy="AFFIRMATIVE",
+ _dry_run=_dry_run,
+ )
+ _attach_policy_to_scope_permission(
+ client,
+ client_uuid,
+ permission_name="ViewAccess",
+ policy_name=_role_policy_name(READ_ONLY_ROLE_NAME),
+ scope_names=["GET"],
+ resource_names=[KeycloakResource.VIEW.value],
+ decision_strategy="AFFIRMATIVE",
+ _dry_run=_dry_run,
+ )
+ _attach_policy_to_scope_permission(
+ client,
+ client_uuid,
+ permission_name="AdminViewAccess",
+ policy_name=_role_policy_name(READ_ONLY_ROLE_NAME),
Review Comment:
**major** — `AdminView#GET` is not read-only. In multi-team mode
`is_authorized_view` checks `REPARSE_ALL` against `AdminView`
(`ADMIN_ACCESS_VIEW_NAMES`), and `requires_access_dag_from_file_token` uses
`REPARSE_ALL` to authorize reparsing a file with no registered Dag. With this
attachment a `GlobalReadOnly` user can queue reparse requests.
The docs also say `AdminView` is granted "to ``SuperAdmin`` only"
(`docs/auth-manager/manage/permissions.rst`). Either drop this attachment, or
split reparse from the read views before granting it.
*Flagged by the adversarial reviewer (copilot); cross-checked.*
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -258,14 +261,14 @@ def _ensure_multi_team_enabled(*, teams: list[str],
command_name: str) -> None:
def _ensure_default_role_policies(client: KeycloakAdmin, client_uuid: str, *,
_dry_run: bool = False) -> None:
- for role_name in (*TEAM_ROLE_NAMES, SUPER_ADMIN_ROLE_NAME):
+ for role_name in (*TEAM_ROLE_NAMES, READ_ONLY_ROLE_NAME,
SUPER_ADMIN_ROLE_NAME):
Review Comment:
**minor** — `_ensure_role_policy` looks the role up with `_get_role_id` and
never creates it, so `create-permissions` / `create-all` now fail with
`KeycloakGetError` on any client, including non-team ones, until a
`GlobalReadOnly` role is added in Keycloak. Existing multi-team clients also
need `create-permissions --teams` re-run before `create-team`, otherwise it
fails with `Policy 'Allow-GlobalReadOnly' not found.`
`providers/AGENTS.md`:
> Keep `provider.yaml` metadata, docs, and tests in sync.
> When a provider change needs a user-visible note (typically a breaking
change or important behavior change that warrants explanation), update the
provider's `docs/changelog.rst` directly in the same PR, just below the
`Changelog` header
Please add the upgrade note to `providers/keycloak/docs/changelog.rst` and
describe the role in `docs/auth-manager/manage/permissions.rst`, next to
`SuperAdmin`.
*Flagged by both the primary and adversarial reviewers.*
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -324,12 +327,18 @@ def _update_read_only_permission_resources(
return
permission_id = match["id"]
- scopes = client.get_client_authz_scopes(client_uuid)
- resources = client.get_client_authz_resources(client_uuid)
- scope_ids = [s["id"] for s in scopes if s["name"] in ["GET", "MENU",
"LIST"]]
- resource_names = TEAM_SCOPED_RESOURCE_NAMES | GLOBAL_SCOPED_RESOURCE_NAMES
- resource_ids = [r["_id"] for r in resources if r["name"] in resource_names]
+ scope_ids = [
+ s["id"] for s in client.get_client_authz_scopes(client_uuid) if
s["name"] in ["GET", "MENU", "LIST"]
+ ]
+ resource_ids = [
+ r["_id"]
+ for r in client.get_client_authz_resources(client_uuid)
+ if r["name"] in TEAM_SCOPED_RESOURCE_NAMES |
GLOBAL_SCOPED_RESOURCE_NAMES
Review Comment:
**major** — This filter matches exact names (`Dag`, `Connection`, ...), so
the `Dag:<team>` resources that `_attach_readonly_permissions` just put on
`ReadOnly` are removed again by this update. After `create-team`,
`GlobalReadOnly` has no `GET` on any team resource. The earlier thread on this
was resolved as fixed, but this filter and
`test_update_read_only_permission_resources_excludes_team_resources` still
exclude them.
Simply adding the team resources here would be unsafe: on clients that were
first set up without `--teams`, `ReadOnly` also carries the plain
`Allow-Viewer` / `Allow-User` / `Allow-Op` / `Allow-Admin` role policies under
`AFFIRMATIVE`, so every Viewer would get read access to every team.
`_attach_policy_to_scope_permission` also replaces the resource list on each
call, so a second team would overwrite the first.
Suggestion: a separate permission (for example `GlobalReadOnly`) that holds
only `Allow-GlobalReadOnly`, with its resources rebuilt across all teams the
way `_update_admin_permission_resources` does for `Admin`.
##########
providers/keycloak/tests/unit/keycloak/auth_manager/cli/test_commands.py:
##########
@@ -757,6 +760,32 @@ def test_create_team_command(
decision_strategy="AFFIRMATIVE",
_dry_run=False,
)
+ mock_attach_policy.assert_any_call(
Review Comment:
**minor** — These assertions check that the mocked attach was called, not
what `ReadOnly` ends up containing once `create-team` finishes, so they pass
while the two problems in `commands.py` (missing permission, resources removed
again) go unnoticed.
`AGENTS.md`, Testing Standards:
> Every changed or added behaviour must have a test; every test must fail
without the PR's change.
A test that runs `create-team` with only the Keycloak client mocked and
asserts the final `update_client_authz_scope_permission` payload for the
read-only permission would cover it.
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -1022,6 +1032,52 @@ def _attach_superadmin_permissions(
)
+def _attach_readonly_permissions(
+ client: KeycloakAdmin, client_uuid: str, team: str, *, _dry_run: bool =
False
+) -> None:
+ team_scoped_resources = [f"{resource}:{team}" for resource in
sorted(TEAM_SCOPED_RESOURCE_NAMES)]
Review Comment:
**minor** — The description says the role "should remain equivalent to the
'Viewer' role for each team". Team Viewer gets `GET`/`LIST` on `Dag:<team>` and
`Team:<team>` and the `TEAM_MENU_ITEMS` menu entries. This grants `GET` on
`Connection`, `Pool` and `Variable` of every team, every `MenuItem`, and
`AdminView`. Is the wider scope intended? If so, the description and docs
should say so; if not, `team_readable_resources` and `TEAM_MENU_ITEMS` are the
Viewer-equivalent sets.
--
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]
