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