vincbeck commented on code in PR #61256:
URL: https://github.com/apache/airflow/pull/61256#discussion_r2746760273
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##########
@@ -358,26 +397,51 @@ def _is_authorized(
elif method == "GET":
method = "LIST"
- resp = self.http_session.post(
- self._get_token_url(server_url, realm),
- data=self._get_payload(client_id,
f"{resource_type.value}#{method}", context_attributes),
- headers=self._get_headers(user.access_token),
- timeout=5,
- )
+ if (
Review Comment:
Can you explain what you are doing here. I am having hard time understanding
it. Why LIST is special? What's the user story?
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -46,6 +47,27 @@
)
log = logging.getLogger(__name__)
+TEAM_SCOPED_RESOURCE_NAMES = {
+ KeycloakResource.DAG.value,
+ KeycloakResource.ASSET.value,
Review Comment:
`ASSET_ALIAS` should be there too. Actually I think it is a miss on my side,
it should be added to the `USER` role as well.
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##########
@@ -208,29 +224,41 @@ def is_authorized_dag(
details: DagDetails | None = None,
) -> bool:
dag_id = details.id if details else None
+ team_name = details.team_name if details else None
access_entity_str = access_entity.value if access_entity else None
return self._is_authorized(
method=method,
resource_type=KeycloakResource.DAG,
user=user,
resource_id=dag_id,
+ team_name=team_name,
attributes={"dag_entity": access_entity_str},
)
def is_authorized_backfill(
self, *, method: ResourceMethod, user: KeycloakAuthManagerUser,
details: BackfillDetails | None = None
) -> bool:
backfill_id = str(details.id) if details else None
+ team_name = getattr(details, "team_name", None) if details else None
return self._is_authorized(
- method=method, resource_type=KeycloakResource.BACKFILL, user=user,
resource_id=backfill_id
+ method=method,
+ resource_type=KeycloakResource.BACKFILL,
+ user=user,
+ resource_id=backfill_id,
+ team_name=team_name,
)
def is_authorized_asset(
self, *, method: ResourceMethod, user: KeycloakAuthManagerUser,
details: AssetDetails | None = None
) -> bool:
asset_id = details.id if details else None
+ team_name = getattr(details, "team_name", None) if details else None
return self._is_authorized(
- method=method, resource_type=KeycloakResource.ASSET, user=user,
resource_id=asset_id
+ method=method,
+ resource_type=KeycloakResource.ASSET,
+ user=user,
+ resource_id=asset_id,
+ team_name=team_name,
)
def is_authorized_asset_alias(
Review Comment:
You do not want to teammify `is_authorized_asset_alias`?
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##########
@@ -181,10 +195,7 @@ def is_authorized_configuration(
) -> bool:
config_section = details.section if details else None
return self._is_authorized(
- method=method,
Review Comment:
You do not want to use the `team_name` here?
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -145,13 +180,57 @@ def _get_client_uuid(args):
return matches[0]["id"]
+def _create_group_membership_mapper(
+ client: KeycloakAdmin, client_uuid: str, *, _dry_run: bool = False
+) -> None:
+ if not conf.getboolean("core", "multi_team"):
Review Comment:
Instead of doing it here in private method, in each command accepting
`teams` as parameter, I would this check as first thing and return an error if
the user passes a list of team and the env is not configured as multi-team
##########
providers/keycloak/docs/auth-manager/manage/permissions.rst:
##########
@@ -73,20 +80,45 @@ This command will create resources for certain types of
permissions.
.. code-block:: bash
- airflow keycloak-auth-manager create-resources
+ airflow keycloak-auth-manager create-resources --teams team-a,team-b
Finally, with the command below, we create the permissions using the
previously created scopes and resources.
.. code-block:: bash
- airflow keycloak-auth-manager create-permissions
+ airflow keycloak-auth-manager create-permissions --teams team-a,team-b
This will create
-* read-only permissions
-* admin permissions
-* user permissions
-* operations permissions
+* read-only permissions (per-team when ``--teams`` is provided)
+* admin permissions (global)
+* user permissions (per-team when ``--teams`` is provided)
+* operations permissions (per-team when ``--teams`` is provided)
+
+Managing teams with Keycloak
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+When using team-scoped resources, you can create Keycloak groups that
represent teams and attach them to the
+team-specific permissions. The CLI provides helpers for this flow:
+
+.. code-block:: bash
+
+ airflow keycloak-auth-manager create-team team-a
+ airflow keycloak-auth-manager add-user-to-team user-a team-a
+
+These commands create a Keycloak group named ``<name>``, set up team-scoped
resources and permissions,
Review Comment:
```suggestion
These commands create a Keycloak group named ``team-a``, set up team-scoped
resources and permissions,
```
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##########
@@ -73,6 +73,20 @@
log = logging.getLogger(__name__)
RESOURCE_ID_ATTRIBUTE_NAME = "resource_id"
+TEAM_SCOPED_RESOURCES = frozenset(
+ {
+ KeycloakResource.DAG,
+ KeycloakResource.ASSET,
Review Comment:
Add `ASSET_ALIAS` as well
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/cli/commands.py:
##########
@@ -175,6 +254,7 @@ def _create_scopes(client: KeycloakAdmin, client_uuid: str,
*, _dry_run: bool =
def _get_resources_to_create(
client: KeycloakAdmin,
client_uuid: str,
+ teams: list[str] | None = None,
Review Comment:
I do not think these method can receive `None`? They always receive the
value from `_parse_teams` which returns a list. That's true for all methods
```suggestion
teams: list[str],
```
--
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]
