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/09/13 23:11:18 UTC
[GitHub] [airflow] jaketf opened a new pull request #18222: make current_user_has_permissions backwards compatible
jaketf opened a new pull request #18222:
URL: https://github.com/apache/airflow/pull/18222
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
make current_user_has_permissions backwards compatible
---
**^ Add meaningful description above**
IMHO Subclasses that already override `get_current_user_permissions` should not also have to override the newly added method `current_user_has_permissions` and this PR would fix that.
Currently `AiflowSecurityManager.current_user_has_permissions` sort of duplicates logic of `AiflowSecurityManager.get_current_user_permissions` but bakes in the assumption that all permissions come from roles.
As of https://github.com/apache/airflow/pull/17838 the new method is used when deciding to route a user to the no permissions or role page.
This causes a regression for existing subclasses of `AirflowSecurityManager` that override `get_current_user_permissions` directly and those permissions do not come from a role.
Such subclasses should still allow users with permissions to the airflow UI.
I'm not sure how best to add test coverage for something like this.
cc @jedcunningham @andrewgodwin
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] jaketf edited a comment on pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jaketf edited a comment on pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#issuecomment-919365903
> I mean, both Airflow and FAB overall assume permissions come from roles.
hmm, that's good to know. it feels misleading for that 403 page to say "user has no roles and/**or** permissions" if we really are only checking roles.
> seems reasonable to me to need to adjust a few new methods every so often
yeah it's certainly not the end of the world.
--
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 edited a comment on pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jedcunningham edited a comment on pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#issuecomment-919193243
I mean, both Airflow and FAB overall assume permissions come from roles. If someone is trying to subclass away roles, seems reasonable to me to need to adjust a few new methods every so often 🤷♂️. It was written as it is so it can short-circuit vs merging all permissions if a user has multiple roles.
@jhtimmins, what are your thoughts here?
--
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] jaketf commented on a change in pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jaketf commented on a change in pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#discussion_r708488295
##########
File path: airflow/www/security.py
##########
@@ -346,10 +346,7 @@ def get_current_user_permissions(self):
return perms
def current_user_has_permissions(self) -> bool:
- for role in self.get_user_roles():
- if role.permissions:
- return True
- return False
+ return len(self.get_current_user_permissions()) > 0
Review comment:
alternatively (pardon my ignorance) is there a way for us to cache the user permissions for a sessions so we don't have to make that query twice?
--
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] jaketf commented on a change in pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jaketf commented on a change in pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#discussion_r708483857
##########
File path: airflow/www/security.py
##########
@@ -346,10 +346,7 @@ def get_current_user_permissions(self):
return perms
def current_user_has_permissions(self) -> bool:
- for role in self.get_user_roles():
- if role.permissions:
- return True
- return False
+ return len(self.get_current_user_permissions()) > 0
Review comment:
@uranusjr thanks for that feedback on blowing up DB queries. Would this be an acceptable workaround?
Look for roles first (existing behavior), then look for permissions only if necessary?
```suggestion
for role in self.get_user_roles():
if role.permissions:
return True
return len(self.get_current_user_permissions()) > 0
```
--
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] jaketf commented on a change in pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jaketf commented on a change in pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#discussion_r708483857
##########
File path: airflow/www/security.py
##########
@@ -346,10 +346,7 @@ def get_current_user_permissions(self):
return perms
def current_user_has_permissions(self) -> bool:
- for role in self.get_user_roles():
- if role.permissions:
- return True
- return False
+ return len(self.get_current_user_permissions()) > 0
Review comment:
@uranusjr thanks for that feedback on blowing up DB queries. Would this be an acceptable workaround?
Look for roles first, then look for permissions only if necessary?
```suggestion
for role in self.get_user_roles():
if role.permissions:
return True
return len(self.get_current_user_permissions()) > 0
```
--
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 change in pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#discussion_r708574349
##########
File path: airflow/www/security.py
##########
@@ -346,10 +346,7 @@ def get_current_user_permissions(self):
return perms
def current_user_has_permissions(self) -> bool:
- for role in self.get_user_roles():
- if role.permissions:
- return True
- return False
+ return len(self.get_current_user_permissions()) > 0
Review comment:
Yeah, having an ABC could make this a lot cleaner 👍. Before you do the work though, it'd be worth pinging @jhtimmins directly to coordinate.
--
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] jaketf commented on a change in pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jaketf commented on a change in pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#discussion_r708488295
##########
File path: airflow/www/security.py
##########
@@ -346,10 +346,7 @@ def get_current_user_permissions(self):
return perms
def current_user_has_permissions(self) -> bool:
- for role in self.get_user_roles():
- if role.permissions:
- return True
- return False
+ return len(self.get_current_user_permissions()) > 0
Review comment:
alternatively (pardon my ignorance) is there a way for us to cache the user permissions for a sessions so we don't have to make those queries twice? It seems like immediately after this we'll have to get user permissions anyway.
--
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 pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#issuecomment-918828337
This blows up db query counts. Backward compatibility is valuable, but perferrably we should keep the nicer current behaviour by default and only fall back to compatibility behaviour if 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.
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 #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#issuecomment-919408132
> it feels misleading for that 403 page to say "user has no roles and/**or** permissions" if we really are only checking roles.
It's actually checking both given the assumption of user -> roles -> permissions, so it seems like decent language to me. However I'm certainly open to alternate wording here, maybe "user has no 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] jaketf closed pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jaketf closed pull request #18222:
URL: https://github.com/apache/airflow/pull/18222
--
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 #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#issuecomment-919193243
I mean, both Airflow's `get_current_user_permissions` and FAB overall assume permissions come from roles. If someone is trying to subclass away roles, seems reasonable to me to need to adjust a few new methods every so often 🤷♂️. It was written as it is so it can short-circuit vs merging all permissions if a user has multiple roles.
@jhtimmins, what are your thoughts here?
--
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] jaketf commented on a change in pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jaketf commented on a change in pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#discussion_r708557702
##########
File path: airflow/www/security.py
##########
@@ -346,10 +346,7 @@ def get_current_user_permissions(self):
return perms
def current_user_has_permissions(self) -> bool:
- for role in self.get_user_roles():
- if role.permissions:
- return True
- return False
+ return len(self.get_current_user_permissions()) > 0
Review comment:
good point, this logic resonates with me. It's smelly to make implementation choices that cause inefficiency for this default `AirflowSecurityManager` based on "what if people want to subclass this". The purpose of this class is to be the default `AirflowSecurityManager` so sub-classers have to assume the risk / responsibility that methods like this might crop up over time and need to be overridden.
If the goal is to improve the security manager subclassing experience it'd probably be better addressed by pulling out a `BaseAirlfowSecurityManager(abc.ABC)` that clearly spells out the contract of which abstract methods should be overridden to make this work correctly with the rest of airflow and making this AirflowSecurityManager a subclass of that.
that's not really the topic of this PR so I will close it and can open a new PR for implementing a `BaseAirlfowSecurityManager` if you think that's worthwhile.
--
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 change in pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#discussion_r708543688
##########
File path: airflow/www/security.py
##########
@@ -346,10 +346,7 @@ def get_current_user_permissions(self):
return perms
def current_user_has_permissions(self) -> bool:
- for role in self.get_user_roles():
- if role.permissions:
- return True
- return False
+ return len(self.get_current_user_permissions()) > 0
Review comment:
It seems like an odd fallback to me as it doesn't make sense in the context of `AirflowSecurityManager`.
I'm not sure where you draw that line. E.g. should `get_accessible_dags` also have a fallback to support subclasses trying to remove the need for roles? What if someone else tried to add another layer instead of removing one?
My thoughts are: If you are redefining where permissions come from (not roles) you should be prepared to implement all of the permissions based methods. I'm not saying changes to make it more subclass-friendly shouldn't happen, however if you need a comment explaining why a fallback is needed that could only happen with a subclass and removal of a core assumption, idk.
--
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] jaketf commented on pull request #18222: make current_user_has_permissions backwards compatible
Posted by GitBox <gi...@apache.org>.
jaketf commented on pull request #18222:
URL: https://github.com/apache/airflow/pull/18222#issuecomment-919365903
> I mean, both Airflow and FAB overall assume permissions come from roles.
hmm, that's good to know. it feels misleading for that 403 page to say "user has no roles and/**or** permissions" if we really are only checking 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