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 2021/01/27 05:22:15 UTC
[GitHub] [airflow] jhtimmins opened a new pull request #13923: Don't add Website.can_read access to default roles.
jhtimmins opened a new pull request #13923:
URL: https://github.com/apache/airflow/pull/13923
While fixing #13856, I made the mistake of adding Website.can_read to all roles, including default roles. This gives public, and thus anonymous, users access to the homepage, which shows an empty list of DAGs.
This fixes that bug by only adding Website.can_read to custom roles.
related: #13856
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13923: Don't add Website.can_read access to default roles.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13923:
URL: https://github.com/apache/airflow/pull/13923#discussion_r565259174
##########
File path: airflow/www/security.py
##########
@@ -441,14 +441,15 @@ def _merge_perm(self, permission_name, view_menu_name):
def add_homepage_access_to_custom_roles(self):
"""
- Add Website.can_read access to all roles.
+ Add Website.can_read access to all custom roles.
:return: None.
"""
website_permission = self.add_permission_view_menu(
permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE
)
- for role in self.get_all_roles():
+ custom_roles = [role for role in self.get_all_roles() if role.name not in EXISTING_ROLES]
+ for role in custom_roles:
Review comment:
Can you add a test to prevent regression -- that checks that only custom roles have `Website.can_read`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #13923: Don't add Website.can_read access to default roles.
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #13923:
URL: https://github.com/apache/airflow/pull/13923
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13923: Don't add Website.can_read access to default roles.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13923:
URL: https://github.com/apache/airflow/pull/13923#discussion_r565559460
##########
File path: airflow/www/security.py
##########
@@ -441,14 +441,15 @@ def _merge_perm(self, permission_name, view_menu_name):
def add_homepage_access_to_custom_roles(self):
"""
- Add Website.can_read access to all roles.
+ Add Website.can_read access to all custom roles.
:return: None.
"""
website_permission = self.add_permission_view_menu(
permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE
)
- for role in self.get_all_roles():
+ custom_roles = [role for role in self.get_all_roles() if role.name not in EXISTING_ROLES]
+ for role in custom_roles:
Review comment:
I think we should have a test somewhere that verifies the exact permissions the public role is supposed to have
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Brett37490 commented on pull request #13923: Don't add Website.can_read access to default roles.
Posted by GitBox <gi...@apache.org>.
Brett37490 commented on pull request #13923:
URL: https://github.com/apache/airflow/pull/13923#issuecomment-769500709
he doesn't get paid til now
On Thu, 28 Jan 2021, 05:54 James Timmins, <no...@github.com> wrote:
> *@jhtimmins* commented on this pull request.
> ------------------------------
>
> In airflow/www/security.py
> <https://github.com/apache/airflow/pull/13923#discussion_r565554381>:
>
> > + custom_roles = [role for role in self.get_all_roles() if role.name not in EXISTING_ROLES]
> + for role in custom_roles:
>
> @kaxil <https://github.com/kaxil> All roles except for Public will get
> Website.can_read. This is just only added to custom roles explicitly,
> since the default ones already have it. So mostly I just wanted to not give
> Public that access.
>
> Still think it's worth adding a test to make sure that isn't available to
> Public?
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/airflow/pull/13923#discussion_r565554381>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ARWZRE7PJO3YXYPJBA4PVQTS4BOOFANCNFSM4WUUHAWA>
> .
>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13923: Don't add Website.can_read access to default roles.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13923:
URL: https://github.com/apache/airflow/pull/13923#discussion_r566345217
##########
File path: airflow/www/security.py
##########
@@ -441,14 +441,15 @@ def _merge_perm(self, permission_name, view_menu_name):
def add_homepage_access_to_custom_roles(self):
"""
- Add Website.can_read access to all roles.
+ Add Website.can_read access to all custom roles.
:return: None.
"""
website_permission = self.add_permission_view_menu(
permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE
)
- for role in self.get_all_roles():
+ custom_roles = [role for role in self.get_all_roles() if role.name not in EXISTING_ROLES]
+ for role in custom_roles:
Review comment:
@jhtimmins Can you make sure to add test (if it does not exist) in a follow up PR or the other PR that you have open
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13923: Don't add Website.can_read access to default roles.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13923:
URL: https://github.com/apache/airflow/pull/13923#discussion_r565559551
##########
File path: airflow/www/security.py
##########
@@ -441,14 +441,15 @@ def _merge_perm(self, permission_name, view_menu_name):
def add_homepage_access_to_custom_roles(self):
"""
- Add Website.can_read access to all roles.
+ Add Website.can_read access to all custom roles.
:return: None.
"""
website_permission = self.add_permission_view_menu(
permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE
)
- for role in self.get_all_roles():
+ custom_roles = [role for role in self.get_all_roles() if role.name not in EXISTING_ROLES]
+ for role in custom_roles:
Review comment:
If we already have it, then not needed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #13923: Don't add Website.can_read access to default roles.
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #13923:
URL: https://github.com/apache/airflow/pull/13923#discussion_r565554381
##########
File path: airflow/www/security.py
##########
@@ -441,14 +441,15 @@ def _merge_perm(self, permission_name, view_menu_name):
def add_homepage_access_to_custom_roles(self):
"""
- Add Website.can_read access to all roles.
+ Add Website.can_read access to all custom roles.
:return: None.
"""
website_permission = self.add_permission_view_menu(
permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE
)
- for role in self.get_all_roles():
+ custom_roles = [role for role in self.get_all_roles() if role.name not in EXISTING_ROLES]
+ for role in custom_roles:
Review comment:
@kaxil All roles except for Public will get `Website.can_read`. This is just only added to custom roles explicitly, since the default ones already have it. So mostly I just wanted to not give Public that access.
Still think it's worth adding a test to make sure that isn't available to Public?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org