ucaeon commented on code in PR #72820:
URL: https://github.com/apache/airflow/pull/72820#discussion_r3985420885
##########
providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py:
##########
@@ -1241,11 +1241,112 @@ def sync_roles(self) -> None:
# Sync the default roles (Admin, Viewer, User, Op, public) with
related permissions
self.bulk_sync_roles(self.ROLE_CONFIGS)
+ self.create_roles_from_config()
+
self.add_homepage_access_to_custom_roles()
# init existing roles, the rest role could be created through UI.
self.update_admin_permission()
self.clean_perms()
+ def _get_custom_roles_config(self) -> dict[str, list[tuple[str, str]]]:
+ config = conf.getjson("fab", "custom_roles", fallback={})
+ if not isinstance(config, dict):
+ raise AirflowConfigException("[fab] custom_roles must be a JSON
object")
+
+ roles: dict[str, list[tuple[str, str]]] = {}
+ for name, items in config.items():
+ if not isinstance(name, str) or not name.strip() or len(name) > 64:
+ raise AirflowConfigException("[fab] custom_roles role names
must contain 1 to 64 characters")
+ if not isinstance(items, list):
+ raise AirflowConfigException(f"[fab] custom_roles[{name!r}]
must be a list")
+ perms: set[tuple[str, str]] = set()
+ for index, item in enumerate(items):
+ location = f"[fab] custom_roles[{name!r}][{index}]"
+ if not isinstance(item, dict) or item.keys() != {"action",
"resource"}:
+ raise AirflowConfigException(f"{location} must contain
only 'action' and 'resource'")
+ for key, limit in (("action", 100), ("resource", 250)):
+ value = item[key]
+ if not isinstance(value, str) or not value.strip() or
len(value) > limit:
+ raise AirflowConfigException(
+ f"{location}.{key} must be a non-empty string of
at most {limit} characters"
+ )
+ perms.add((item["action"], item["resource"]))
+ roles[name] = sorted(perms)
+ return roles
+
+ def create_roles_from_config(self) -> None:
+ """Create missing configured roles, preserving permissions on existing
roles."""
+ roles = self._get_custom_roles_config()
+ for name, perms in roles.items():
+ if name in EXISTING_ROLES:
+ log.warning("Skipping built-in role '%s' in [fab]
custom_roles", name)
+ continue
+ self._create_role_from_config(name, perms)
+
+ def _create_role_from_config(self, name: str, perms: list[tuple[str,
str]]) -> None:
+ # FAB's public creation helpers commit individually; a configured role
must
+ # become visible only after all its declared permissions have been
attached.
+ for attempt in range(3):
+ conflict: tuple[str, ...] | None = None
+ try:
+ if self.find_role(name) is not None:
+ return
+ role = self.role_model()
+ role.name = name
+ role.permissions = []
+ self.session.add(role)
+ self.session.flush()
+
+ for action_name, resource_name in perms:
+ perm = self.get_permission(action_name, resource_name)
+ if perm is None:
+ action = self.get_action(action_name)
+ if action is None:
+ conflict = ("action", action_name)
+ action = self.action_model()
Review Comment:
Thanks for pointing this out!
I've updated the implementation in `1e3179cf1d` to validate `action` and
`resource` names against existing FAB database entries before creating any
configured roles.
Unknown names now raise a configuration error instead of creating new
actions or resources. The names are validated independently, so a new
permission combination is still allowed when both names already exist.
I've also updated the documentation and tests. The targeted tests passed on
SQLite, PostgreSQL, and MySQL.
Thanks again for the helpful feedback!☺️
--
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]
