vandonr-amz commented on code in PR #33213:
URL: https://github.com/apache/airflow/pull/33213#discussion_r1289054657
##########
airflow/auth/managers/base_auth_manager.py:
##########
@@ -54,6 +58,67 @@ def get_user_id(self) -> str:
def is_logged_in(self) -> bool:
"""Return whether the user is logged in."""
+ @abstractmethod
+ def is_authorized(
+ self,
+ action: ResourceAction,
+ resource_type: str,
+ resource_details: ResourceDetails | None = None,
+ user: BaseUser | None = None,
+ ) -> bool:
+ """
+ Return whether the user is authorized to perform a given action.
+
+ .. code-block:: python
+
+ # Check whether the logged-in user has permission to read the DAG
"my_dag_id"
+ get_auth_manager().is_authorized(
+ Action.GET,
+ Resource.DAG,
+ ResourceDetails(
+ id="my_dag_id",
+ ),
+ )
+
+ :param action: the action to perform
+ :param resource_type: the type of resource the user attempts to
perform the action on
+ :param resource_details: optional details about the resource itself
+ :param user: the user to perform the action on. If not provided (or
None), it uses the current user
+ """
+
+ def is_all_authorized(
+ self,
+ actions: Sequence[tuple[ResourceAction, str, ResourceDetails | None]],
Review Comment:
I'm thinking that defining a small data-only class for this rather than
using a tuple would be nice.
Using https://docs.python.org/3/library/typing.html#typing.NamedTuple for
instance.
##########
airflow/auth/managers/base_auth_manager.py:
##########
@@ -54,6 +58,67 @@ def get_user_id(self) -> str:
def is_logged_in(self) -> bool:
"""Return whether the user is logged in."""
+ @abstractmethod
+ def is_authorized(
+ self,
+ action: ResourceAction,
+ resource_type: str,
Review Comment:
no defined `ResourceType` ? I feel like it should be a finite and known list
so we should be able to make it an enum, don't your think ?
Even in the example code below it looks like it's coming from an enum ?
##########
airflow/auth/managers/fab/fab_auth_manager.py:
##########
@@ -81,3 +148,35 @@ def get_url_user_profile(self) -> str | None:
if not self.security_manager.user_view:
return None
return url_for(f"{self.security_manager.user_view.endpoint}.userinfo")
+
+ @staticmethod
+ def _get_fab_actions(action: ResourceAction) -> list[str]:
+ """
+ Convert the action to a list of FAB actions.
+
+ :param action: the action to convert
+
+ :meta private:
+ """
+ if action not in _MAP_ACTION_NAME_TO_FAB_ACTION_NAME:
+ raise AirflowException(f"Unknown action: {action}")
+ return _MAP_ACTION_NAME_TO_FAB_ACTION_NAME[action]
+
+ @staticmethod
+ def _resource_name_for_dag(dag_id: str) -> str:
+ """
+ Returns the FAB resource name for a DAG id.
+
+ Note that since a sub-DAG should follow the permission of its parent
DAG, you should pass
+ ``DagModel.root_dag_id`` to this function, for a subdag. A normal dag
should pass the
+ ``DagModel.dag_id``.
Review Comment:
maybe it'd make sense to extract the root dag ID inside this function rather
than outside.
That way this warning wouldn't be necessary.
And since the root dag ID is not needed for anything else, it'd make the
code cleaner.
##########
airflow/auth/managers/models/base_user.py:
##########
@@ -37,6 +38,11 @@ def is_active(self) -> bool:
def is_anonymous(self) -> bool:
...
+ @property
+ @abstractmethod
+ def perms(self) -> Any:
Review Comment:
can't we be more specific ? It seems we use that in a `x in user.perms`
expression, so can't we type this as `list | set` maybe ?
##########
airflow/auth/managers/fab/fab_auth_manager.py:
##########
@@ -17,13 +17,37 @@
# under the License.
from __future__ import annotations
+import itertools
+
from flask import url_for
from flask_login import current_user
from airflow import AirflowException
from airflow.auth.managers.base_auth_manager import BaseAuthManager
from airflow.auth.managers.fab.models import User
from airflow.auth.managers.fab.security_manager.override import
FabAirflowSecurityManagerOverride
+from airflow.auth.managers.models.base_user import BaseUser
+from airflow.auth.managers.models.resource_action import ResourceAction
+from airflow.auth.managers.models.resource_details import ResourceDetails
+from airflow.security.permissions import (
+ ACTION_CAN_ACCESS_MENU,
+ ACTION_CAN_CREATE,
+ ACTION_CAN_DELETE,
+ ACTION_CAN_EDIT,
+ ACTION_CAN_READ,
+ RESOURCE_DAG,
+ RESOURCE_DAG_PREFIX,
+)
+
+_MAP_ACTION_NAME_TO_FAB_ACTION_NAME = {
+ ResourceAction.POST: [ACTION_CAN_CREATE],
+ # ACTION_CAN_READ and ACTION_CAN_ACCESS_MENU are merged into because they
are very similar.
+ # We can assume that if a user has permissions to read variables, they
also have permissions to access
+ # the menu "Variables".
+ ResourceAction.GET: [ACTION_CAN_READ, ACTION_CAN_ACCESS_MENU],
Review Comment:
we are introducing quite some complexity just to handle this thing. WDYT
about retiring ACTION_CAN_ACCESS_MENU altogether and checking only
ACTION_CAN_READ ?
Right now the code is "giving" read access to someone who only had
`ACCESS_MENU`. If we only check `READ`, we "give" menu access to those who only
had read access, which probably less bad if it happens.
--
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]
