Fix security manager inheritance in fab provider

[vincbeck]

Related to #36343 and #36437.

In fab provider, the security manager inheritance check is wrong. We should check that the security manager provided by the user inherits from FabAirflowSecurityManagerOverride period. This change is also backward compatible with historical users of this feature because historically we were forcing the class defined in SECURITY_MANAGER_CLASS to inherit from AirflowSecurityManager. And this class inherits from FabAirflowSecurityManagerOverride.

---

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

[vincbeck]

cc @maiconkkl

[hussein-awala]

Is it possible to add a test for this check?

[maiconkkl]

cc @maiconkkl

LGTM

[vincbeck]

Is it possible to add a test for this check?

Good point. Let me do it

[maiconkkl]

I have a doubt!
Isn't it worth leaving this part of code to keep a compatibility warning?

if issubclass(sm_from_config, AirflowSecurityManagerV2):
	warnings.warn(
		"Please make your custom security manager inherit from "
		"FabAirflowSecurityManagerOverride instead of AirflowSecurityManagerV2.",
		AirflowProviderDeprecationWarning,
	)

This way, whoever uses the class "AirflowSecurityManagerV2" will understand that it is being depreciated and which one they should use.

It can even be used as a test case too.

@vincbeck

[vincbeck]

I have a doubt! Isn't it worth leaving this part of code to keep a compatibility warning?

if issubclass(sm_from_config, AirflowSecurityManagerV2):
	warnings.warn(
		"Please make your custom security manager inherit from "
		"FabAirflowSecurityManagerOverride instead of AirflowSecurityManagerV2.",
		AirflowProviderDeprecationWarning,
	)

This way, whoever uses the class "AirflowSecurityManagerV2" will understand that it is being depreciated and which one they should use.

It can even be used as a test case too.

@vincbeck

No, this change has been added very recently and is not right. SECURITY_MANAGER_CLASS is a feature of fab provider, as such, if you want to use it, you should inherit from FabAirflowSecurityManagerOverride. Plus, FabAirflowSecurityManagerOverride is a subclass of AirflowSecurityManagerV2, thus by default the deprecation warning is thrown. Plus, AirflowSecurityManagerV2 is not deprecated at all :)

[potiuk]

Yeah. I knew that it can be don "properly".

[vincbeck]

The failures are not related to this PR, fix: #36558

[nicolamarangoni]

Hello, does this mean that we need to wait for 2.8.1 to test the setup with SSO over OAUTH and Azure?

[vincbeck]

Hello, does this mean that we need to wait for 2.8.1 to test the setup with SSO over OAUTH and Azure?

If you make your custom security manager inherit from FabAirflowSecurityManagerOverride, you can do it today if I am not mistaken

[nicolamarangoni]

This code trigger exactly the error Your CUSTOM_SECURITY_MANAGER must extend FabAirflowSecurityManagerOverride, not FAB's own security manager.

"""Default configuration for the Airflow webserver"""
import os

from flask_appbuilder.security.manager import AUTH_OAUTH
from airflow.auth.managers.fab.security_manager.override import (
    FabAirflowSecurityManagerOverride,
)


class AzureCustomSecurity(FabAirflowSecurityManagerOverride):
    """Custom security class"""


MANDANTID = os.environ.get("MANDANTID")
CLIENTID = os.environ.get("CLIENTID")
CLIENTSECRET = os.environ.get("CLIENTSECRET")

AUTH_TYPE = AUTH_OAUTH
AUTH_ROLES_SYNC_AT_LOGIN = True
AUTH_USER_REGISTRATION = True
AUTH_USER_REGISTRATION_ROLE = "Public"
AUTH_ROLES_MAPPING = {
    "airflow_admin": ["Admin"],
    "airflow_user": ["User"],
    "airflow_viewer": ["Viewer"],
}

OAUTH_PROVIDERS = [
    {
        "name": "azure",
        "icon": "fa-windows",
        "token_key": "access_token",
        "remote_app": {
            "client_secret": CLIENTSECRET,
            "client_id": CLIENTID,
            "api_base_url": f"https://login.microsoftonline.com/{MANDANTID}/",
            "client_kwargs": {
                "scope": "openid",
                "resource": CLIENTID,
            },
            "request_token_url": None,
            "access_token_url": f"https://login.microsoftonline.com/{MANDANTID}/oauth2/v2.0/token/",
            "authorize_url": f"https://login.microsoftonline.com/{MANDANTID}/oauth2/v2.0/authorize/",
            "jwks_uri": "https://login.microsoftonline.com/common/discovery/v2.0/keys/",
        },
    }
]

SECURITY_MANAGER_CLASS = AzureCustomSecurity  # pylint: disable=invalid-name

[vincbeck]

My bad, can you try by inheriting from AirflowSecurityManager instead of FabAirflowSecurityManagerOverride . Basically class AzureCustomSecurity(AirflowSecurityManager): instead of class AzureCustomSecurity(FabAirflowSecurityManagerOverride):. You should get another deprecation warning but at least it should be working.

[nicolamarangoni]

With class AzureCustomSecurity(AirflowSecurityManager) I gor this error:
[2024-01-11T15:08:20.052+0000] {views.py:671} ERROR - Error returning OAuth user info: 'oid'
Then I changed "scope": "openid", with "scope": "openid email profile", and everything looks fine now.

Many thanks!

[randymelo]

https://gist.github.com/wallyhall/915fedb4dfc766b61f442a32c95e1c29