Skip to content

Let Dag viewers read the audit log of the Dags they can see #72247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rjgoyln wants to merge 1 commit into from
+64 −6
Closed

Let Dag viewers read the audit log of the Dags they can see #72247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions providers/fab/docs/auth-manager/access-control.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,10 @@ Endpoint
/assets GET Assets.can_read Viewer
/assets/{uri} GET Assets.can_read Viewer
/assets/events GET Assets.can_read Viewer
/eventLogs GET Audit Logs.can_read Admin
All Audit Logs.can_read (for rows not tied to a Dag)
/eventLogs/{event_log_id} GET Audit Logs.can_read Admin
All Audit Logs.can_read (for rows not tied to a Dag)
/eventLogs GET Dags.can_read, Audit Logs.can_read (rows tied to a Dag) Viewer
/eventLogs GET All Audit Logs.can_read (rows not tied to a Dag) Admin
/eventLogs/{event_log_id} GET Dags.can_read, Audit Logs.can_read (rows tied to a Dag) Viewer
/eventLogs/{event_log_id} GET All Audit Logs.can_read (rows not tied to a Dag) Admin
/importErrors GET ImportError.can_read Viewer
/importErrors/{import_error_id} GET ImportError.can_read Viewer
/health GET None Public
Expand Down
10 changes: 10 additions & 0 deletions providers/fab/docs/changelog.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,16 @@
Changelog
---------

.. warning::
On Airflow 3.4.0 and newer the ``Audit Logs.can_read`` and ``Audit Logs.menu_access``
permissions move from the ``Admin`` role to the ``Viewer`` role, so ``Viewer``, ``User`` and
``Op`` now reach the audit log of the Dags they may read. The audit rows that are not tied to
a Dag -- Connection, Variable and Pool operations -- stay behind ``All Audit Logs.can_read``,
which only ``Admin`` holds. On an older Airflow those rows are not gated separately, so both
permissions stay with ``Admin`` there. Upgrading syncs the new permissions onto the built-in
roles; a deployment that wants the previous behaviour has to revoke them from the ``Viewer``
role explicitly.

3.8.1
.....

Expand Down
21 changes: 19 additions & 2 deletions providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
from werkzeug.security import check_password_hash, generate_password_hash

from airflow.providers.common.compat.sdk import conf
from airflow.providers.common.compat.security.access_view import AUDIT_LOGS_ALL_ACCESS_VIEW
from airflow.providers.fab.auth_manager.models import (
Action,
Group,
Expand Down Expand Up @@ -158,6 +159,16 @@ def _iter_dags() -> Iterable[DAG | SerializedDAG]:
# purging the old sessions by using `airflow db clean` command.
MAX_NUM_DATABASE_USER_SESSIONS = 50000

# Airflow 3.4.0 moved the audit rows that carry no Dag -- Connection, Variable and Pool
# operations -- behind ``AccessView.AUDIT_LOGS_ALL``, which leaves ``Audit Logs.can_read``
# covering only the rows the event log endpoints already narrow to the caller's readable Dags.
# An older core returns those Dag-less rows to anyone holding ``Audit Logs.can_read``, so there
# the permission has to stay admin-only.
_DAG_AUDIT_LOG_PERMISSIONS = [
(permissions.ACTION_CAN_READ, permissions.RESOURCE_AUDIT_LOG),
(permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_AUDIT_LOG),
]


class FabException(Exception):
"""Custom exception for FAB security manager."""
Expand Down Expand Up @@ -269,6 +280,7 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2):
(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG_WARNING),
(permissions.ACTION_CAN_READ, RESOURCE_ASSET),
(permissions.ACTION_CAN_READ, RESOURCE_ASSET_ALIAS),
(permissions.ACTION_CAN_READ, permissions.RESOURCE_AUDIT_LOG),
(permissions.ACTION_CAN_READ, permissions.RESOURCE_BACKFILL),
(permissions.ACTION_CAN_READ, permissions.RESOURCE_CLUSTER_ACTIVITY),
(permissions.ACTION_CAN_READ, permissions.RESOURCE_POOL),
Expand All @@ -289,6 +301,7 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2):
(permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DAG_DEPENDENCIES),
(permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DAG_RUN),
(permissions.ACTION_CAN_ACCESS_MENU, RESOURCE_ASSET),
(permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_AUDIT_LOG),
(permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_CLUSTER_ACTIVITY),
(permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DOCS),
(permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DOCS_MENU),
Expand Down Expand Up @@ -352,8 +365,6 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2):

# [START security_admin_perms]
ADMIN_PERMISSIONS = [
(permissions.ACTION_CAN_READ, permissions.RESOURCE_AUDIT_LOG),
(permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_AUDIT_LOG),
(permissions.ACTION_CAN_READ, permissions.RESOURCE_AUDIT_LOG_ALL),
(permissions.ACTION_CAN_READ, permissions.RESOURCE_IMPORT_ERROR_ALL),
(permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_RESCHEDULE),
Expand All @@ -367,6 +378,12 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2):
]
# [END security_admin_perms]

if AUDIT_LOGS_ALL_ACCESS_VIEW is None:
VIEWER_PERMISSIONS = [
permission for permission in VIEWER_PERMISSIONS if permission not in _DAG_AUDIT_LOG_PERMISSIONS
]
ADMIN_PERMISSIONS = ADMIN_PERMISSIONS + _DAG_AUDIT_LOG_PERMISSIONS

###########################################################################
# DEFAULT ROLE CONFIGURATIONS
###########################################################################
Expand Down
31 changes: 31 additions & 0 deletions providers/fab/tests/unit/fab/auth_manager/test_security.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,11 @@ class _TestBase(DeclarativeBase):
Mapped = Any # type: ignore[assignment,misc]

from airflow.api_fastapi.app import get_auth_manager
from airflow.api_fastapi.auth.managers.models.resource_details import DagAccessEntity, DagDetails
from airflow.models import DagModel
from airflow.models.dag import DAG
from airflow.models.dagbundle import DagBundleModel
from airflow.providers.common.compat.security.access_view import AUDIT_LOGS_ALL_ACCESS_VIEW
from airflow.providers.common.compat.sqlalchemy.orm import mapped_column
from airflow.providers.fab.auth_manager.fab_auth_manager import FabAuthManager
from airflow.providers.fab.auth_manager.models.anonymous_user import AnonymousUser
Expand Down Expand Up @@ -512,6 +514,11 @@ def test_get_user_roles_for_anonymous_user(app, security_manager):
(permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DOCS_MENU),
(permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_DOCS),
}
if AUDIT_LOGS_ALL_ACCESS_VIEW is not None:
viewer_role_perms |= {
(permissions.ACTION_CAN_READ, permissions.RESOURCE_AUDIT_LOG),
(permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_AUDIT_LOG),
}
app.config["AUTH_ROLE_PUBLIC"] = "Viewer"

with app.app_context():
Expand All @@ -523,6 +530,30 @@ def test_get_user_roles_for_anonymous_user(app, security_manager):
assert perms_views == viewer_role_perms


@pytest.mark.skipif(
AUDIT_LOGS_ALL_ACCESS_VIEW is None,
reason="Viewers are granted audit log access only on a core that gates the Dag-less rows separately",
)
@pytest.mark.parametrize(
("role_name", "expected_all_audit_logs"),
[("Viewer", False), ("User", False), ("Op", False), ("Admin", True)],
)
def test_default_roles_read_dag_audit_logs(app, role_name, expected_all_audit_logs):
"""Every default role reads the audit rows of a Dag it can see; only Admin reads the rows with no Dag."""
with app.app_context():
with create_user_scope(app, username="audit_log_user", role_name=role_name) as user:
assert get_auth_manager().is_authorized_dag(
method="GET",
access_entity=DagAccessEntity.AUDIT_LOG,
details=DagDetails(id="example_dag"),
user=user,
)
assert (
get_auth_manager().is_authorized_view(access_view=AUDIT_LOGS_ALL_ACCESS_VIEW, user=user)
is expected_all_audit_logs
)


def test_get_current_user_permissions(app):
action = "can_some_action"
resource = "SomeBaseView"
Expand Down