uranusjr commented on a change in pull request #19121:
URL: https://github.com/apache/airflow/pull/19121#discussion_r736114083
##########
File path: airflow/models/dagbag.py
##########
@@ -630,25 +631,24 @@ def _serialize_dag_capturing_errors(dag, session):
@provide_session
def _sync_perm_for_dag(self, dag, session: Optional[Session] = None):
"""Sync DAG specific permissions, if necessary"""
- from flask_appbuilder.security.sqla import models as sqla_models
-
from airflow.security.permissions import DAG_ACTIONS,
resource_name_for_dag
+ from airflow.www.fab_security.sqla.models import Action, Permission
Review comment:
I think these should be imported at the top of the module together with
`Resource`?
##########
File path: airflow/www/views.py
##########
@@ -4530,29 +4530,49 @@ def _node_dict(node_id, label, node_class):
}
-class CustomPermissionModelView(PermissionModelView):
- """Customize permission names for FAB's builtin PermissionModelView."""
+class ActionModelView(PermissionModelView):
+ """Customize permission names for FAB's builtin PermissionModelView.."""
Review comment:
```suggestion
"""Customize permission names for FAB's builtin PermissionModelView."""
```
##########
File path: tests/test_utils/permissions.py
##########
@@ -16,34 +16,21 @@
# under the License.
-from flask_appbuilder.security.sqla import models as sqla_models
-
from airflow.security.permissions import RESOURCE_DAG_PREFIX
from airflow.utils.session import create_session
+from airflow.www.fab_security.sqla.models import Permission, Resource,
assoc_permission_role
def delete_dag_specific_permissions():
with create_session() as session:
- dag_vms = (
- session.query(sqla_models.ViewMenu)
- .filter(sqla_models.ViewMenu.name.like(f"{RESOURCE_DAG_PREFIX}%"))
- .all()
- )
+ dag_vms =
session.query(Resource).filter(Resource.name.like(f"{RESOURCE_DAG_PREFIX}%")).all()
Review comment:
Probably want to also change the variable name as well (same for
`vm_ids`, `dag_pvms` etc. below)
##########
File path: airflow/www/fab_security/sqla/models.py
##########
@@ -0,0 +1,201 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# This product contains a modified portion of 'Flask App Builder' developed by
Daniel Vaz Gaspar.
+# (https://github.com/dpgaspar/Flask-AppBuilder).
+# Copyright 2013, Daniel Vaz Gaspar
+
+import datetime
+
+from flask import g
+from flask_appbuilder._compat import as_unicode
+from flask_appbuilder.models.sqla import Model
+from sqlalchemy import (
+ Boolean,
+ Column,
+ DateTime,
+ ForeignKey,
+ Integer,
+ Sequence,
+ String,
+ Table,
+ UniqueConstraint,
+)
+from sqlalchemy.ext.declarative import declared_attr
+from sqlalchemy.orm import backref, relationship
+
+_dont_audit = False
+
+
+class Action(Model):
+ """Represents permission actions such as `can_read`."""
+
+ __tablename__ = "ab_permission"
Review comment:
I feel we should add some notes (probably in the module-wide docstring
at the top of the file) that table names are inherited from F.A.B. for
compatibility.
##########
File path: tests/utils/test_db.py
##########
@@ -57,7 +57,7 @@ def
test_database_schema_and_sqlalchemy_model_are_in_sync(self):
lambda t: (t[0] == 'remove_table' and t[1].name ==
'ab_permission_view_role'),
lambda t: (t[0] == 'remove_table' and t[1].name == 'ab_user_role'),
lambda t: (t[0] == 'remove_table' and t[1].name == 'ab_user'),
- lambda t: (t[0] == 'remove_table' and t[1].name == 'ab_view_menu'),
+ lambda t: (t[0] == 'remove_table' and t[1].name == 'ab_resource'),
Review comment:
I think this shouldn't change because we did not change the table name?
It might be better to import the models and access `Resource.__tablename__`
etc. instead to avoid this. Not sure.
##########
File path: airflow/security/permissions.py
##########
@@ -35,8 +35,10 @@
RESOURCE_MY_PASSWORD = "My Password"
RESOURCE_MY_PROFILE = "My Profile"
RESOURCE_PASSWORD = "Passwords"
-RESOURCE_PERMISSION = "Permissions"
-RESOURCE_PERMISSION_VIEW = "Permission Views" # Refers to a Perm <-> View
mapping, not an MVC View.
+RESOURCE_PERMISSION = "Permissions" # TODO: Convert to Actions
+RESOURCE_PERMISSION_VIEW = (
+ "Permission Views" # Refers to a Perm <-> View mapping, not an MVC View.
# TODO: Convert to Perms
+)
Review comment:
Should the name (not value) of these resources be changed in this PR?
--
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]
