Skip to content

Fix inheritance chain in security manager - #33901

Merged
vincbeck merged 24 commits into
apache:mainfrom
aws-mwaa:vandonr/fab
Sep 11, 2023
Merged

vincbeck merged 24 commits into
apache:mainfrom
aws-mwaa:vandonr/fab

Conversation

@vandonr-amz

@vandonr-amz vandonr-amz commented Aug 29, 2023 •

Copy link
Copy Markdown
Contributor

ℹ️ 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:
image

after:
image

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.

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-56 Extensible user management area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants