Fix inheritance chain in security manager

[vandonr-amz]

ℹ️ the diff is big because I re-unified the various FAB security managers in one class. See #33901 (comment)

---

There is currently a weird situation with the security managers, where the AirflowSecurityManager, which is supposed to be the more generic construct, inherits from FabAirflowSecurityManagerOverride, supposed to be the more specific one. Because of this, FabAirflowSecurityManagerOverride doesn't bear well its name, because it cannot override anything from AirflowSecurityManager since it sits lower on the inheritance tree.

This was done initially to be able to extend functionalities of the Security Manager, while retaining an existing feature which was that users could plug their own security manager inheriting from AirflowSecurityManager (so we couldn't squeeze the FAB one in between because that'd be a breaking change).

The solution I used here is to have an empty shell stay where the AirflowSecurityManager was, just to maintain back-compatibility. That one inherits from FAB Security Manager, which can now sit below the actual AirflowSecurityManager.

This means that users can only override the FAB security manager and not other auth provider's SM but that's ok because this feature is pretty much rendered obsolete by AIP-56. See #33690 (comment)

Joining some class diagrams to help understanding.
Arrows read as "is inherited/implemented by".

current situation:

after:

I had to make the ApplessAirflowSecurityManager inherit from the "FAB" one instead of the generic airflow SM because it had some dependencies there. It's not ideal, and not what we want in a final state, but for now we just want a state that closer to it.
We'll sort that dependency later on.

[vincbeck]

The whole goal of this change was not to make FabAirflowSecurityManagerOverride inherit from AirflowSecurityManager?

Oh now I might understand why you made inherited Db and Oauth module from AirflowSecurityManager. I guess you wanted a more vertical hierarchy. The idea of modules I introduced in FabAirflowSecurityManagerOverride was to avoid to build a big monster class very hard to maintain, so I split it in multiple modules. To me, making these modules inherited from AirflowSecurityManager does not really make sense.

I would:

• Remove dependency between the modules and AirflowSecurityManager
• Add dependency between FabAirflowSecurityManagerOverride and AirflowSecurityManager

WDYT?

[vandonr-amz]

The idea of modules I introduced in FabAirflowSecurityManagerOverride was to avoid to build a big monster class very hard to maintain, so I split it in multiple modules.

BTW, I'm wondering if this isn't causing more harm than good.
A big file is not necessarily hard to maintain, and I'd argue it's actually harder to maintain code that's spread over 3 different files.
Inheritance also comes with extra features that we don't want here. For instance, defining the same method/property/field in 2 files would result in one overriding the other rather than an error.

[vincbeck]

The idea of modules I introduced in FabAirflowSecurityManagerOverride was to avoid to build a big monster class very hard to maintain, so I split it in multiple modules.

BTW, I'm wondering if this isn't causing more harm than good. A big file is not necessarily hard to maintain, and I'd argue it's actually harder to maintain code that's spread over 3 different files. Inheritance also comes with extra features that we don't want here. For instance, defining the same method/property/field in 2 files would result in one overriding the other rather than an error.

If you think this is better without these modules, let's go then

[vincbeck]

Since on the long term we will deprecate AirflowSecurityManager in favor of AirflowSecurityManagerV2 I am wondering if AirflowSecurityManagerV2 is a good name since this name will stay. Should we keep the name AirflowSecurityManager? It can confusing but we can assume this is only temporary? Or should we come up with another name? SimplifiedAirflowSecurityManager? I dont like it either xD.

[vandonr-amz]

temporary things still stay around for a pretty long time to give users time to migrate...
Idk, I'm open to suggestions, but we can always rename it once we remove the old one.

[potiuk]

I need to twist my head around it :). Will take a look tomorrow .

[vincbeck]

We should double check it resolves: #34107

[pankajastro]

@vandonr-amz @vincbeck are we considering removing EXISTING_ROLES from here a breaking change or I'm missing something here?

[vincbeck]

Why would that be a breaking change? In which case user code would use this constant? The only use case I can think of is, if the user provide its own security manager and inside this security manager, they use this constant. But if we want to avoid such things ... every change will be breaking change and doing any change in security managers will be almost impossible.

Or maybe you had something else in mind?

[pankajastro]

Why would that be a breaking change

yes, it breaking change because I import this constant from airflow.www.security import EXISTING_ROLES in my security manager and after this changes my code will break.

But if we want to avoid such things ... every change will be breaking change and doing

from airflow.www.security import * import path was very important for the implementing custom security manager and I feel it would be natural for many people to import EXISTING_ROLES if they need this rather than redefine so removing this suddenly is not good and may impact other users. I understand that maintaining backward compatibility for constant type this is painful but still, I would like to keep this maybe we can remove it when we decide to remove airflow.www.security.py

[vincbeck]

I see ... I'll try to think about a solution. We might have to keep whatever is static (constants, variables, functions) in these files then ... Which I really dont like ... But it seems we do not have much choice if we want to be backward compatible with users bringing their own security managers. I wish this feature did not exist xD.

But anyway, thanks for the feedback. I'll probably create a PR to fix that and will tag you on it.

[pankajastro]

Thanks @vincbeck