Fix Exception FabAirflowSecurityManagerOverride

[maiconkkl]

related: #36432
The exception raised Your CUSTOM_SECURITY_MANAGER must extend FabAirflowSecurityManagerOverride, not FAB's own security manager.
My English is not good, so I ask you to be patient.
The error happens when extending the FabAirflow SecurityManagerOverride class to create custom authentication

[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: dev@airflow.apache.org
  Slack: https://s.apache.org/airflow-slack

[dirrao]

LGTM

[potiuk]

I think the intention here was a bit different and I'd wait for @vincbeck to come back from holidays to merge this one. There were a few changes to the structure of the Auth Manager class and @vincbeck has PRs with doc in progress to describe them better - so I'd keep it around till he is back.

[maiconkkl]

I think the intention here was a bit different and I'd wait for @vincbeck to come back from holidays to merge this one. There were a few changes to the structure of the Auth Manager class and @vincbeck has PRs with doc in progress to describe them better - so I'd keep it around till he is back.

I haven't seen these PRs, however they are intended for 2.8.* ?

I saw that there were some changes in main, but I believe it would be for a new version, maybe 2.9
Exemple #36232

[potiuk]

Not really. We are cherry-picking some changes from main to be released in 2.8.* - these are bug-fixes and doc updates mostly - so.I'd say this (and doc changes) should go to 2.8.1. it is about more complicated because in main we already moved FAB to separate provider, which will make cherry-picking a bit more complex - that's why as well I think we should do any fixes here while @vincbeck is around to make sure we all make sure we are doing the right things.

[DjVinnii]

I'm also running into the related issue, so a fix will be great as it is now blocking us to upgrade to Airflow 2.8. To me the proposed fix should do the trick as the raised error indicates the check is inverted.

[potiuk]

I'm also running into the related issue, so a fix will be great as it is now blocking us to upgrade to Airflow 2.8. To me the proposed fix should do the trick as the raised error indicates the check is inverted.

It will stil have to be released in 2.8.1 and it will take some time. So feel free to apply the patch for now in your version - this is the fastest way you can get it fixed for you now.

[potiuk]

This is a beauty of Open-Source software. You can ALWAYS patch things while waiting for a more complete ifx. And rushing fixes without having a full knowledge is often bad idea. Go slow to move fast.

[vincbeck]

I am against this change.

What is the value of SECURITY_MANAGER_CLASS in your scenario? There is something weird because this check was already there before we migrated the code to fab provider, so it should not be a breaking change. We want to make sure whatever class you defined in SECURITY_MANAGER_CLASS inherits from AirflowSecurityManager. Again, this check was there before so there must be something off somewhere.

[maiconkkl]

I am against this change.

What is the value of SECURITY_MANAGER_CLASS in your scenario? There is something weird because this check was already there before we migrated the code to fab provider, so it should not be a breaking change. We want to make sure whatever class you defined in SECURITY_MANAGER_CLASS inherits from AirflowSecurityManager. Again, this check was there before so there must be something off somewhere.

Hello vincbeck, how are you? Happy New Year!!!

When trying to extend AirflowSecurityManager I receive a DeprecationWarning with the following message
"Call to deprecated class AirflowSecurityManager. (If you want to override the security manager, you should inherit from airflow.auth.managers.fab.security_manager.override.FabAirflowSecurityManagerOverride instead)"

And when trying to extend FabAirflowSecurityManagerOverride I receive an Exception with the following message
"Your CUSTOM_SECURITY_MANAGER must extend FabAirflowSecurityManagerOverride, not FAB's own security manager."

Interpreting what the warnings say, I understand that the AirflowSecurityManager class is becoming obsolete and needs to extend the FabAirflowSecurityManagerOverride class and my change proposes exactly that

class Cav4Authorizer(FabAirflowSecurityManagerOverride):
    def auth_user_db(self, username: str, password: str):
        """
        Authenticate user, auth db style.

        :param username:
            The username or registered email address
        :param password:
            The password, will be tested against hashed password on db
        """
        if username is None or username == "":
            return None

[vincbeck]

Happy new year too @maiconkkl. Thanks for all the details and indeed there is a bug. This is related to #36343 as well. I just figured out the bug, let me create a PR for it. I'll explain the details there

[vincbeck]

See #36538

[vincbeck]

@maiconkkl if you could pull the changes of this PR locally and do some testing on your end, that would be very appreciated :)

[maiconkkl]

@maiconkkl if you could pull the changes of this PR locally and do some testing on your end, that would be very appreciated :)

I already tested it and it works well for me

[vincbeck]

@maiconkkl if you could pull the changes of this PR locally and do some testing on your end, that would be very appreciated :)

I already tested it and it works well for me

Thank you!