You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/06/01 07:27:57 UTC
[GitHub] [airflow] jhtimmins opened a new pull request, #24065: Rename Permissions to Permission Pairs.
jhtimmins opened a new pull request, #24065:
URL: https://github.com/apache/airflow/pull/24065
This is a fix for #23926.
The issue is that the following code
```
if self.appbuilder.app.config.get("FAB_ADD_SECURITY_PERMISSION_VIEW", True):
self.appbuilder.add_view(
self.actionmodelview,
"Actions",
icon="fa-lock",
label=_("Actions"),
category="Security",
)
```
https://github.com/apache/airflow/blob/main/airflow/www/fab_security/manager.py#L769-L775
causes the following actions
```
{manager.py:545} INFO - Removed Permission View: menu_access on Permissions
{manager.py:509} INFO - Created Permission View: menu access on Permissions
```
The lines
```
if self.appbuilder.app.config.get("FAB_ADD_SECURITY_PERMISSION_VIEWS_VIEW", True):
self.appbuilder.add_view(
self.permissionmodelview,
"Permissions",
icon="fa-link",
label=_("Permissions"),
category="Security",
)
```
https://github.com/apache/airflow/blob/main/airflow/www/fab_security/manager.py#L785-L792
cause
```
{manager.py:509} INFO - Created Permission View: menu access on Permissions
{manager.py:571} INFO - Added Permission menu access on Permissions to role Admin
```
This is because in code block 1, `ActionModelView.class_permission_name = "Permissions"`, and the view name used in code block 2 is `"Permissions",`. This causes a conflict where permissions get removed from and then added to `Permissions` every time the app worker loads.
As a solution, I've renamed the view name in the second code block to `Permission Pairs`, which removes the conflict between the two code blocks.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #24065:
URL: https://github.com/apache/airflow/pull/24065#issuecomment-1144206431
Let's rebase on main as a first attempt to fix that.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r887321891
##########
airflow/www/fab_security/manager.py:
##########
@@ -196,11 +194,11 @@ def oauth_tokengetter(token=None):
userinfoeditview = UserInfoEditView
""" Override if you want your own User information edit view """
- rolemodelview = RoleModelView
- actionmodelview = PermissionModelView
+ rolemodelview = CustomRoleModelView
+ actionmodelview = ActionModelView
Review Comment:
@jedcunningham Yeah, I removed that though to minimize the chance of adding unexpected bugs.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on PR #24065:
URL: https://github.com/apache/airflow/pull/24065#issuecomment-1144205670
@jedcunningham does it matter that the static checks are failing bc of isort issues with `/home/runner/actions-runner/_work/airflow/airflow/docker_tests/test_prod_image.py` and similar `_work` folders?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r886892604
##########
airflow/www/fab_security/manager.py:
##########
@@ -196,11 +194,11 @@ def oauth_tokengetter(token=None):
userinfoeditview = UserInfoEditView
""" Override if you want your own User information edit view """
- rolemodelview = RoleModelView
- actionmodelview = PermissionModelView
+ rolemodelview = CustomRoleModelView
+ actionmodelview = ActionModelView
Review Comment:
Is this stuff fixing something else?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r887319391
##########
airflow/www/fab_security/manager.py:
##########
@@ -787,7 +785,7 @@ def register_views(self):
if self.appbuilder.app.config.get("FAB_ADD_SECURITY_PERMISSION_VIEWS_VIEW", True):
self.appbuilder.add_view(
self.permissionmodelview,
- "Permissions",
+ "Permission Pairs",
Review Comment:
@ashb That's correct.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins merged pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jhtimmins merged PR #24065:
URL: https://github.com/apache/airflow/pull/24065
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r887589684
##########
airflow/www/fab_security/manager.py:
##########
@@ -787,7 +785,7 @@ def register_views(self):
if self.appbuilder.app.config.get("FAB_ADD_SECURITY_PERMISSION_VIEWS_VIEW", True):
self.appbuilder.add_view(
self.permissionmodelview,
- "Permissions",
+ "Permission Pairs",
Review Comment:
I don’t have a better suggestion 😞
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r887329549
##########
airflow/www/fab_security/manager.py:
##########
@@ -787,7 +785,7 @@ def register_views(self):
if self.appbuilder.app.config.get("FAB_ADD_SECURITY_PERMISSION_VIEWS_VIEW", True):
self.appbuilder.add_view(
self.permissionmodelview,
- "Permissions",
+ "Permission Pairs",
Review Comment:
It doesn't get shown to the user, the `label` 2 lines down is what is shown 👍
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r887343420
##########
airflow/www/fab_security/manager.py:
##########
@@ -787,7 +785,7 @@ def register_views(self):
if self.appbuilder.app.config.get("FAB_ADD_SECURITY_PERMISSION_VIEWS_VIEW", True):
self.appbuilder.add_view(
self.permissionmodelview,
- "Permissions",
+ "Permission Pairs",
Review Comment:
Ah, yes, you are totally right! Thanks 🍺
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r887321891
##########
airflow/www/fab_security/manager.py:
##########
@@ -196,11 +194,11 @@ def oauth_tokengetter(token=None):
userinfoeditview = UserInfoEditView
""" Override if you want your own User information edit view """
- rolemodelview = RoleModelView
- actionmodelview = PermissionModelView
+ rolemodelview = CustomRoleModelView
+ actionmodelview = ActionModelView
Review Comment:
Yeah, I removed that though to minimize the chance of adding unexpected bugs.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r887332639
##########
airflow/www/fab_security/manager.py:
##########
@@ -787,7 +785,7 @@ def register_views(self):
if self.appbuilder.app.config.get("FAB_ADD_SECURITY_PERMISSION_VIEWS_VIEW", True):
self.appbuilder.add_view(
self.permissionmodelview,
- "Permissions",
+ "Permission Pairs",
Review Comment:
@jedcunningham I believe that the label is what's shown in the dropdown, and "permission pairs" is what shows up in the admin view of permissions.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r886908785
##########
airflow/www/fab_security/manager.py:
##########
@@ -787,7 +785,7 @@ def register_views(self):
if self.appbuilder.app.config.get("FAB_ADD_SECURITY_PERMISSION_VIEWS_VIEW", True):
self.appbuilder.add_view(
self.permissionmodelview,
- "Permissions",
+ "Permission Pairs",
Review Comment:
Does this ever get shown anywhere to a user? Perhaps in the CRUD screens to manage permissions?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24065:
URL: https://github.com/apache/airflow/pull/24065#issuecomment-1144175129
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r887360898
##########
airflow/www/fab_security/manager.py:
##########
@@ -787,7 +785,7 @@ def register_views(self):
if self.appbuilder.app.config.get("FAB_ADD_SECURITY_PERMISSION_VIEWS_VIEW", True):
self.appbuilder.add_view(
self.permissionmodelview,
- "Permissions",
+ "Permission Pairs",
Review Comment:
I wonder if there’s a better name than _Permission Pairs_ (which doesn’t explain much on its own).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zachliu commented on pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
zachliu commented on PR #24065:
URL: https://github.com/apache/airflow/pull/24065#issuecomment-1150420776
> This is because in code block 1, `ActionModelView.class_permission_name = "Permissions"`
i don't see this in code block 1 :thinking: can you elaborate? thanks :pray:
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on PR #24065:
URL: https://github.com/apache/airflow/pull/24065#issuecomment-1154801838
@zachliu sorry for the confusion, that comment is out of date.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r887322487
##########
airflow/www/fab_security/manager.py:
##########
@@ -196,11 +194,11 @@ def oauth_tokengetter(token=None):
userinfoeditview = UserInfoEditView
""" Override if you want your own User information edit view """
- rolemodelview = RoleModelView
- actionmodelview = PermissionModelView
+ rolemodelview = CustomRoleModelView
+ actionmodelview = ActionModelView
Review Comment:
Thank you. A separate PR would be great as we can leave that for 2.4 and cherry-pick this one.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a diff in pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on code in PR #24065:
URL: https://github.com/apache/airflow/pull/24065#discussion_r887388409
##########
airflow/www/fab_security/manager.py:
##########
@@ -787,7 +785,7 @@ def register_views(self):
if self.appbuilder.app.config.get("FAB_ADD_SECURITY_PERMISSION_VIEWS_VIEW", True):
self.appbuilder.add_view(
self.permissionmodelview,
- "Permissions",
+ "Permission Pairs",
Review Comment:
Do you have an alternative suggestion? This is unlikely to be seen by users (since it's automatically added to admin roles).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #24065: Rename Permissions to Permission Pairs.
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #24065:
URL: https://github.com/apache/airflow/pull/24065#issuecomment-1144207368
Actually, nevermind, don't worry about it for now, also failing on main.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org