stegololz commented on code in PR #74371:
URL: https://github.com/apache/airflow/pull/74371#discussion_r4246477931
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -263,53 +263,81 @@ def _ensure_default_role_policies(client: KeycloakAdmin,
client_uuid: str, *, _d
_ensure_role_policy(client, client_uuid, role_name, _dry_run=_dry_run)
-def _attach_default_role_permissions(
- client: KeycloakAdmin, client_uuid: str, *, _dry_run: bool = False
-) -> None:
- for role_name in TEAM_ROLE_NAMES:
- _attach_policy_to_scope_permission(
- client,
- client_uuid,
- permission_name="ReadOnly",
- policy_name=_role_policy_name(role_name),
- scope_names=["GET", "MENU", "LIST"],
- resource_names=[],
- decision_strategy="AFFIRMATIVE",
- _dry_run=_dry_run,
+def _get_default_permission_configs() -> list[dict]:
+ """Build resource-scoped permissions for non-team installations."""
+ read_roles = [*TEAM_ROLE_NAMES, SUPER_ADMIN_ROLE_NAME]
+ user_roles = ["User", "Op", "Admin", SUPER_ADMIN_ROLE_NAME]
+ op_roles = ["Op", "Admin", SUPER_ADMIN_ROLE_NAME]
+ admin_roles = ["Admin", SUPER_ADMIN_ROLE_NAME]
+
+ user_resources = {
+ KeycloakResource.DAG,
+ KeycloakResource.ASSET,
+ }
+ op_resources = {
+ KeycloakResource.CONNECTION,
+ KeycloakResource.POOL,
+ KeycloakResource.VARIABLE,
+ KeycloakResource.BACKFILL,
+ }
+
+ permission_configs = []
+
+ for resource in KeycloakResource:
+ if resource in user_resources:
+ write_roles = user_roles
+ elif resource in op_resources:
+ write_roles = op_roles
+ else:
+ write_roles = admin_roles
+
+ permission_configs.extend(
+ [
+ {
+ "name": f"{resource.value}Read",
+ "type": "scope-based",
+ "resources": [resource.value],
+ "scope_names": ["GET", "LIST"],
+ "role_names": read_roles,
+ },
+ {
+ "name": f"{resource.value}Write",
+ "type": "scope-based",
+ "resources": [resource.value],
+ "scope_names": ["POST", "PUT", "DELETE"],
+ "role_names": write_roles,
+ },
+ ]
)
- _attach_policy_to_resource_permission(
- client,
- client_uuid,
- permission_name="User",
- policy_name=_role_policy_name("User"),
- resource_names=[KeycloakResource.DAG.value,
KeycloakResource.ASSET.value],
- _dry_run=_dry_run,
+ permission_configs.append(
+ {
+ "name": "MenuAccess",
+ "type": "scope-based",
+ "resources": [item.value for item in MenuItem],
+ "scope_names": ["MENU"],
+ "role_names": read_roles,
+ }
)
- _attach_policy_to_resource_permission(
- client,
- client_uuid,
- permission_name="Op",
- policy_name=_role_policy_name("Op"),
- resource_names=[
- KeycloakResource.CONNECTION.value,
- KeycloakResource.POOL.value,
- KeycloakResource.VARIABLE.value,
- KeycloakResource.BACKFILL.value,
- ],
- _dry_run=_dry_run,
- )
- 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=[],
- decision_strategy="AFFIRMATIVE",
- _dry_run=_dry_run,
- )
+
+ return permission_configs
+
+
+def _attach_default_role_permissions(
+ client: KeycloakAdmin, client_uuid: str, *, _dry_run: bool = False
+) -> None:
+ for permission in _get_default_permission_configs():
+ for role_name in permission["role_names"]:
+ _attach_policy_to_scope_permission(
+ client,
+ client_uuid,
+ permission_name=permission["name"],
+ policy_name=_role_policy_name(role_name),
+ scope_names=permission["scope_names"],
+ resource_names=permission["resources"],
+ decision_strategy="AFFIRMATIVE",
Review Comment:
Every permission is now `AFFIRMATIVE` internally, but the CLI never sets the
**resource server's** decision strategy, so realms run on Keycloak's default
`UNANIMOUS`. Suggestion: have `create-all` / `create-permissions` (and
`create-team`) PUT `decisionStrategy: AFFIRMATIVE` on `authz/resource-server`
explicitly.
- **Free on fresh realms:** with these per-resource permissions, the results
are identical under both strategies.
- **Fixes upgraded realms with no cleanup:** `main` + this PR under
`AFFIRMATIVE` gives the same results as a fresh realm.
- **Multi-team only works under `AFFIRMATIVE`.** Under `UNANIMOUS`, every
`<Res>:<team>` request is denied for every user, SuperAdmin included. The
global `Admin` (SuperAdmin-only) and the overlapping `User-/Op-/Admin-<team>`
permissions block each other. Today the two modes need opposite server
strategies on one realm, which is weird.
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -263,53 +263,81 @@ def _ensure_default_role_policies(client: KeycloakAdmin,
client_uuid: str, *, _d
_ensure_role_policy(client, client_uuid, role_name, _dry_run=_dry_run)
-def _attach_default_role_permissions(
- client: KeycloakAdmin, client_uuid: str, *, _dry_run: bool = False
-) -> None:
- for role_name in TEAM_ROLE_NAMES:
- _attach_policy_to_scope_permission(
- client,
- client_uuid,
- permission_name="ReadOnly",
- policy_name=_role_policy_name(role_name),
- scope_names=["GET", "MENU", "LIST"],
- resource_names=[],
- decision_strategy="AFFIRMATIVE",
- _dry_run=_dry_run,
+def _get_default_permission_configs() -> list[dict]:
+ """Build resource-scoped permissions for non-team installations."""
+ read_roles = [*TEAM_ROLE_NAMES, SUPER_ADMIN_ROLE_NAME]
+ user_roles = ["User", "Op", "Admin", SUPER_ADMIN_ROLE_NAME]
+ op_roles = ["Op", "Admin", SUPER_ADMIN_ROLE_NAME]
+ admin_roles = ["Admin", SUPER_ADMIN_ROLE_NAME]
+
+ user_resources = {
+ KeycloakResource.DAG,
+ KeycloakResource.ASSET,
+ }
+ op_resources = {
+ KeycloakResource.CONNECTION,
+ KeycloakResource.POOL,
+ KeycloakResource.VARIABLE,
+ KeycloakResource.BACKFILL,
+ }
+
+ permission_configs = []
+
+ for resource in KeycloakResource:
Review Comment:
Nit: this also creates `MenuRead` / `MenuWrite` on the `Menu` resource. The
auth manager never checks that resource (menus use `<MenuItem>#MENU`), so these
are unused. Suggest skipping `KeycloakResource.MENU` here.
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -263,53 +263,81 @@ def _ensure_default_role_policies(client: KeycloakAdmin,
client_uuid: str, *, _d
_ensure_role_policy(client, client_uuid, role_name, _dry_run=_dry_run)
-def _attach_default_role_permissions(
- client: KeycloakAdmin, client_uuid: str, *, _dry_run: bool = False
-) -> None:
- for role_name in TEAM_ROLE_NAMES:
- _attach_policy_to_scope_permission(
- client,
- client_uuid,
- permission_name="ReadOnly",
- policy_name=_role_policy_name(role_name),
- scope_names=["GET", "MENU", "LIST"],
- resource_names=[],
- decision_strategy="AFFIRMATIVE",
- _dry_run=_dry_run,
+def _get_default_permission_configs() -> list[dict]:
+ """Build resource-scoped permissions for non-team installations."""
+ read_roles = [*TEAM_ROLE_NAMES, SUPER_ADMIN_ROLE_NAME]
+ user_roles = ["User", "Op", "Admin", SUPER_ADMIN_ROLE_NAME]
+ op_roles = ["Op", "Admin", SUPER_ADMIN_ROLE_NAME]
+ admin_roles = ["Admin", SUPER_ADMIN_ROLE_NAME]
+
+ user_resources = {
+ KeycloakResource.DAG,
+ KeycloakResource.ASSET,
+ }
+ op_resources = {
+ KeycloakResource.CONNECTION,
+ KeycloakResource.POOL,
+ KeycloakResource.VARIABLE,
+ KeycloakResource.BACKFILL,
+ }
+
+ permission_configs = []
+
+ for resource in KeycloakResource:
+ if resource in user_resources:
+ write_roles = user_roles
+ elif resource in op_resources:
+ write_roles = op_roles
+ else:
+ write_roles = admin_roles
+
+ permission_configs.extend(
+ [
+ {
+ "name": f"{resource.value}Read",
+ "type": "scope-based",
+ "resources": [resource.value],
+ "scope_names": ["GET", "LIST"],
+ "role_names": read_roles,
+ },
+ {
+ "name": f"{resource.value}Write",
+ "type": "scope-based",
+ "resources": [resource.value],
+ "scope_names": ["POST", "PUT", "DELETE"],
+ "role_names": write_roles,
+ },
+ ]
)
- _attach_policy_to_resource_permission(
- client,
- client_uuid,
- permission_name="User",
- policy_name=_role_policy_name("User"),
- resource_names=[KeycloakResource.DAG.value,
KeycloakResource.ASSET.value],
- _dry_run=_dry_run,
+ permission_configs.append(
+ {
+ "name": "MenuAccess",
Review Comment:
This gives a name collision on a multiteam mode as it also creates a
permission called `MenuAccess` (SuperAdmin-only).
##########
providers/keycloak/tests/unit/keycloak/auth_manager/cli/test_commands.py:
##########
@@ -55,6 +58,116 @@
)
[email protected](
+ ("permission_name", "resources", "scopes", "roles"),
+ [
+ (
+ "PoolRead",
+ ["Pool"],
+ ["GET", "LIST"],
+ ["Viewer", "User", "Op", "Admin", "SuperAdmin"],
+ ),
+ (
+ "PoolWrite",
+ ["Pool"],
+ ["POST", "PUT", "DELETE"],
+ ["Op", "Admin", "SuperAdmin"],
+ ),
+ (
+ "DagWrite",
+ ["Dag"],
+ ["POST", "PUT", "DELETE"],
+ ["User", "Op", "Admin", "SuperAdmin"],
+ ),
+ (
+ "ConfigurationWrite",
+ ["Configuration"],
+ ["POST", "PUT", "DELETE"],
+ ["Admin", "SuperAdmin"],
+ ),
+ (
+ "MenuAccess",
+ [item.value for item in MenuItem],
+ ["MENU"],
+ ["Viewer", "User", "Op", "Admin", "SuperAdmin"],
+ ),
+ ],
+)
+def test_get_default_permission_configs(permission_name, resources, scopes,
roles):
Review Comment:
+1 to @SameerMesiah97. These tests mostly restate the config. A parametrized
**role × (resource, scope) → allowed / denied** table, with negative cases,
would test what we actually care about.
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -575,6 +577,24 @@ def _get_permissions_to_create(
all_scopes = client.get_client_authz_scopes(client_uuid)
all_resources = client.get_client_authz_resources(client_uuid)
+ if not teams:
+ available_resources = {resource["name"] for resource in all_resources}
+ available_scopes = {scope["name"] for scope in all_scopes}
+
+ required_resources = {name for permission in perm_configs for name in
permission["resources"]}
+ required_scopes = {name for permission in perm_configs for name in
permission["scope_names"]}
+
+ missing_resources = required_resources - available_resources
+ missing_scopes = required_scopes - available_scopes
+
+ if missing_resources or missing_scopes:
Review Comment:
Good addition. A few suggestions to make it more robust:
- **Validate before writing anything.** `create_permissions_command` calls
`_ensure_default_role_policies` before this check runs, so a failed run is
partly applied. Could the check be pulled into a small helper (e.g.
`_validate_required_authz_objects(client, client_uuid, perm_configs)`) and
called at the top of `create_permissions_command` / `create_all_command`,
before any write?
- **Apply it to team mode too.** The check is under `if not teams:`, so team
mode still silently creates permissions without the resources that are missing.
The same helper could run on the team `perm_configs`.
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -605,6 +625,18 @@ def _get_permissions_to_create(
def _preview_permissions(client: KeycloakAdmin, client_uuid: str, teams:
list[str]):
"""Preview permissions that would be created."""
+ if not teams:
Review Comment:
The non-team preview returns before `_get_permissions_to_create`, so dry-run
never evaluates the new missing-resources check:
- On an **empty realm**, `--dry-run` lists all 27 permissions and exits 0,
while the real run exits 1.
- On an **already-provisioned realm**, it still lists 27 "to be created" and
104 "Would attach" lines when nothing would change.
Ideally dry-run would show the difference against what's already in Keycloak.
--
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]
