[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #28781: Fix UIAlert should_show when AUTH_ROLE_PUBLIC set

[GitBox <gi...@apache.org>]

pierrejeambrun commented on code in PR #28781:
URL: https://github.com/apache/airflow/pull/28781#discussion_r1064049902


##########
airflow/www/utils.py:
##########
@@ -821,9 +821,24 @@ def __init__(
         self.message = Markup(message) if html else message
 
     def should_show(self, securitymanager) -> bool:
-        """Determine if the user should see the message based on their role membership"""
+        """

Review Comment:
   Can we take the opportunity to type annotate the `securitymanager` parameter



##########
airflow/www/utils.py:
##########
@@ -821,9 +821,24 @@ def __init__(
         self.message = Markup(message) if html else message
 
     def should_show(self, securitymanager) -> bool:
-        """Determine if the user should see the message based on their role membership"""
+        """
+        Determine if the user should see the message based on their role membership.
+        If the user is anonymous and AUTH_ROLE_PUBLIC is set in webserver_config.py,
+        provide that user with the AUTH_ROLE_PUBLIC role.
+        """
         if self.roles:
-            user_roles = {r.name for r in securitymanager.current_user.roles}
+            current_user = securitymanager.current_user
+            user_roles = set()
+
+            if current_user and hasattr(current_user, "roles") and len(current_user.roles) > 0:

Review Comment:
   I think the security manager loads only `User` objects, cf `BaseSecurityManager`, so I don't think we need the extra check `hasattr(current_user, "roles")`, same for the check on the length of the 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