SECURITY_MANAGER_CLASS should be a referrence to class, not a string

[Michalosu]

See this comment: #33586 (comment)

If SECURITY_MANAGER_CLASS is a string then you will get error like:

  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/www/extensions/init_appbuilder.py", line 661, in init_appbuilder
    if not issubclass(security_manager_class, AirflowSecurityManager):
TypeError: issubclass() arg 1 must be a class

@vincbeck fyi

[boring-cyborg]

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

• Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
• In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
• Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
• Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
• Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
• Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
  Apache Airflow is a community-driven project and together we are making it better 🚀.
  In case of doubts contact the developers at:
  Mailing List: [email protected]
  Slack: https://s.apache.org/airflow-slack

[vincbeck]

Thank you! Nice catch! Could you run static checks? It is currently failing

[potiuk]

Yeah. You can CLEARLY see that that part of Airflow is not getting the love it needs... Hopefully with the AIP-56 we can turn it around and make it more of "ours" so to speak.

[Michalosu]

It would be good to mentioned it in release note of 2.7.0 as this is kinda breaking change if you have own implementation of AirflowSecurityManager. Code has been change that you can't use FAB_SECURITY_MANAGER_CLASS but docs were still pointing to that and with the wrong example of value.

Good that I know what happened after upgrade to 2.7.0 and created this PR for others if someone else will see this problem - "Boy scout rule"

[Michalosu]

Thank you! Nice catch! Could you run static checks? It is currently failing

Executed pre-commit locally for that file and it is corrected, waiting for results from CI and looking forward for first contribution to airflow repository 🥇

[potiuk]

It would be good to mentioned it in release note of 2.7.0 as this is kinda breaking change if you have own implementation of AirflowSecurityManager. Code has been change that you can't use FAB_SECURITY_MANAGER_CLASS but docs were still pointing to that and with the wrong example of value.

Good that I know what happened after upgrade to 2.7.0 and created this PR for others if someone else will see this problem - "Boy scout rule"

I think It's been like that for a while already :)

[Michalosu]

To be honest, I don't see it directly in https://airflow.apache.org/docs/apache-airflow/stable/release_notes.html, "grepping" for FAB_SECURITY_MANAGER_CLASS and AirflowSecurityManager but perhaps I'm blind, never mind :)

[vincbeck]

Exactly, it has been like this for a while, docs were not updated ... for a while too :) No there is no breaking change, just nobody uses that feature I guess :)

[boring-cyborg]

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

[potiuk]

Exactly, it has been like this for a while, docs were not updated ... for a while too :) No there is no breaking change, just nobody uses that feature I guess :)

ALMOST nobody :) (except @Michalosu ) - or maybe people who used it just figured out how to do it. But yeah, this does not seem like something that is used often. This is also the reason why we want to move it out of Airflow/away from FAB so we can just expose stable API for anyone who would like to develop their custom auth.

[vincbeck]

This issue makes me think ... Should we keep this feature of letting user bringing their own security manager? This feature only made my design/implementation way more complex than without it. When auth managers will be available in Airflow, bringing his own security manager makes no sense to me. And more importantly, it will be impossible to make both world working (the set of auth managers + the set of custom security managers written by users)

[potiuk]

When auth managers will be available in Airflow, bringing his own security manager makes no sense to me.

100% . It should be a feature of FAB Auth Manager only.

[andrew-candela]

Just ran into this issue when trying to upgrade from 2.6 to 2.8. Thanks for updating the docs @Michalosu !