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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
