Adding missing login provider related methods from Flask-Appbuilder

[aa3pankaj]

Login API of Flask-Appbuilder is breaking due to recent changes in airflow 2.2.0 related to SecurityManager
Code: https://github.com/dpgaspar/Flask-AppBuilder/blob/3a6b45b1c12a52a794de27910896cbae61270d6b/flask_appbuilder/security/schemas.py#L23

Above code is expecting security manager to have methods: api_login_allow_multiple_providers, auth_type_provider_name
But these methods are missing in Airflow's BaseSecurityManager class.

related: #16647

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

[potiuk]

Hmm @aa3pankaj - do you know in which version of FAB it appeared by heart? I beleive that might mean that we either merge that one (when succeeds and we discuss other similar issues) to 2.2.4 or limit 2.2.4 to the version of FAB that has a matching interface.

CC: @jedcunningham - I provisionally mark it as 2.2.4 so that we won't forget.

[potiuk]

cc: @jhtimmins

[potiuk]

Also I think there are (at least) two related issues #20449 and #20866 opened recently with this problem.

[potiuk]

Those two issues complain about missing attributes though - but maybe that means that we should fully reconcile the changes between FAB and AirlfowSecurityManager ?

• AttributeError: 'AirflowSecurityManager' object has no attribute 'get_user_datamodel'
• AttributeError: type object 'AirflowSecurityManager' has no attribute 'get_app'

[aa3pankaj]

@potiuk this appeared in Flask-AppBuilder 3.3.4 via dpgaspar/Flask-AppBuilder#1712
https://github.com/dpgaspar/Flask-AppBuilder/releases/tag/v3.3.4

[potiuk]

@potiuk this appeared in Flask-AppBuilder 3.3.4 via dpgaspar/Flask-AppBuilder#1712 https://github.com/dpgaspar/Flask-AppBuilder/releases/tag/v3.3.4

I looked briefly and seems there are (not many) more things missing there. Would it be possible that you look at the two implementattions and add the missing methods/attributes too in this PR @aa3pankaj ?

[potiuk]

Plus - I think few tests are failing now and they need to be fixed anyway

[aa3pankaj]

@potiuk sure, will check other missing attributes and tests.

[aa3pankaj]

@potiuk @jhtimmins what was the reason to move FAB's BaseSecurityManager to airflow?
Becz even if we sync methods and attributes between FAB and Airflow to fix these issues, we would be required to limit Flask-Appbuilder to current version i.e 3.4.4

[aa3pankaj]

looks like we have few changes in create_db, update_user_auth_stat compared to flask-appbuilder's base class, this could be the reason to move FAB's base class to airflow.

But, why don't we have AirflowBaseSecurityManager instead of moving FAB's base class to airflow?

something like:
from flask_appbuilder.security.manager import BaseSecurityManager
class AirflowBaseSecurityManager(BaseSecurityManager)

[potiuk]

@potiuk @jhtimmins what was the reason to move FAB's BaseSecurityManager to airflow? Becz even if we sync methods and attributes between FAB and Airflow to fix these issues, we would be required to limit Flask-Appbuilder to current version i.e 3.4.4

I believe, the main reasons are that the new UI will not need FAB but uses the security model. so the idea is that at some point in time (we do not know when) we might want to remove FAB as dependency. And yeah. I think it is a good point that we should immediately limit FAB to 3.4.4 and in the future deliberatly move to new versions with conscious mind. I think this is what you should really do in this PR so in setup.cfg set ==3.4.4 for FAB - precisely for the reasons you described.

[potiuk]

Is this one still needed @aa3pankaj @jhtimmins ?

[aa3pankaj]

@potiuk As per me, yes we would need this ... do you see any other possible fix for this issue?

[potiuk]

Just checking if there are other plans. I belive we want to finally remove FAB classes and need for FAB as dependency but I do not follow the detailed plans on it.

[jhtimmins]

@aa3pankaj @potiuk The reason for copying FAB permission classes directly into Airflow is that it allowed us to dramatically simplify the code and standardize naming around Airflow's conventions, which helps minimize bugs and makes it easier to modify. The change also allowed for faster queries.

This code change looks good. I agree that pinning FAB is the right move.

[jedcunningham]

@aa3pankaj, can you fix the conflict in setup.cfg? Also can you add a comment explaining why we are pinning FAB?

@potiuk, do you think we should accept a range here, with a strict upper bound instead? e.g from the current flask-appbuilder~=3.4, <4.0.0 to flask-appbuilder~=3.4, <=3.4.4 instead of pinning to only 3.4.4.

[potiuk]

@aa3pankaj, can you fix the conflict in setup.cfg? Also can you add a comment explaining why we are pinning FAB?

@potiuk, do you think we should accept a range here, with a strict upper bound instead? e.g from the current flask-appbuilder~=3.4, <4.0.0 to flask-appbuilder~=3.4, <=3.4.4 instead of pinning to only 3.4.4.

I think because of the way we vendor parts of FAB's code, we should basically every time manually review if there are any changes coming in the new FAB version (we already got examples of that in patchlevel and actually it is not FAB's fault - it's our approach to vendor in parts of the code).

Ths is one of the "justified exceptions" from no-upper bounding IMHO. And I think in this case we are pretty much tightly-coupled with the version of FAB.

I think ==3.4.4 is fine. And we should have have comment explaining it. "Every time you bump version of FAB here, make sure that you review this and that file and compare it with this and that files in FAB to see if they need syncronizing".

This will keep us in perfect sync with FAB code (until we remove it as dependency) and our users will avoid surprises.

[github-actions]

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

[boring-cyborg]

Awesome work, congrats on your first merged pull request!

[jedcunningham]

@aa3pankaj thanks! Congrats on your first commit 🎉