refactor: AUTH_LDAP/AUTH_OAUTH + implement role mapping

[thesuperzapper]

This PR is a significant refactor for AUTH_LDAP and AUTH_OAUTH.

Aside adding significant unit testing, and resolving many issues, it implements a feature with AUTH_ROLES_MAPPING to allow mapping LDAP/OAUTH groups to FAB Roles.

For more information, please see the updated docs/security.rst

Resolves:

• Mapping External roles to FAB roles #518
• Map LDAP groups to FAB roles #956
• AUTH_LDAP_APPEND_DOMAIN doesnt' work with Active Directory #1123
• Airflow LDAP authentication with RBAC features apache/airflow#8179
• New Feature: Decide Superset Role Automatically based on certain LDAP field (like department) apache/superset#3419

Superseeds these PRs:

• Fix indirect bind when using append/format #770

[thesuperzapper]

@dpgaspar can you please review this?

[thesuperzapper]

I have accidentally introduce a behaviour change for LDAP servers which bind without a search account, I will make some changes tomorrow. (+ I need to fix that failing test)

[dpgaspar]

Did a first pass, but I do not feel comfortable on merging this without more tests, that would cover the new config keys and feature

[dpgaspar]

@thesuperzapper, I expect to release FAB major 3 soon, would love to get this one in.

[thesuperzapper]

@dpgaspar yeah, I will pick this up again, got a bit busy over the last few weeks. What is your timeline for 3?

[jedcunningham]

@thesuperzapper, gentle bump. Also happy to help get some test coverage on this. FAB 3 has already been released, so we missed that boat.

[thesuperzapper]

@jedcunningham, would welcome some help on the testing side.

I can pretty easily address the requested changes above.
However there remains the question raised about allowing multiple role mappings for a single CN in: AUTH_ROLES_MAPPING.

[jedcunningham]

@thesuperzapper, I've actually got pretty close to complete coverage on the ldapauth side of things at this point and have addressed some of the concerns raised above. I don't have an easy way to test the oauth side out though. Let me get my stuff through my $works legal and I'll get my changes out.

[jedcunningham]

@thesuperzapper, sorry for the delay...

https://github.com/jedcunningham/Flask-AppBuilder/commits/role_binding

I had to refactor some of the ldap portion to get it matching the existing behavior, which probably undid some of the fixes for #770 and #1123. It might be worth handling those separately though, as at least for me, it's easier to reason about and review changes that aren't all mixed together 🤷‍♂️.

I'm also sending you a DM on the Airflow slack. It might be easier (and less noisy for others) to coordinate there.

[accorvin]

@thesuperzapper we’d love to make use of this functionality in our project. Is there anything we can do to help get this ready for approval faster?

[thesuperzapper]

@accorvin we are currently working on it, so expect an update in a week or so.

[Karthik-13]

@thesuperzapper we require this functionality in our project as well. Any idea on when this will be released or how we can help speed this up?

[thesuperzapper]

@dpgaspar yes, please

[sandhuja]

Any updates?

[thesuperzapper]

@dpgaspar FYI, the only failing check is a code coverage one (it dropped below 72.75%), but I am not sure its correctly detecting the coverage.

Regardless, I would love to hear your thoughts on the changes.

[thesuperzapper]

@dpgaspar do you know when you will get a chance to review this?

This PR is pretty important for Airflow 2.0, given we now exclusively use FAB.

[dpgaspar]

@thesuperzapper awesome work, this looks pretty good! going to take it for a test run today, if all good I think it should be ready to merge

Note: not a blocker but would be great if you could add some missing type annotations

Released this out of this PR: https://pypi.org/project/Flask-AppBuilder/3.2.0rc1/ for easier testing

[dpgaspar]

After testing found the email field user registration problem

[dpgaspar]

we should add f"{username}@email.notfound" or similar, if an LDAP server does not have their email fields populated, it's impossible to register multiple users this way since it hits the email DB unique constraint

[dpgaspar]

@thesuperzapper ^^

[thesuperzapper]

done @dpgaspar

[thesuperzapper]

@dpgaspar should be ready to review again

[jonathonbattista]

Any update on this?

[thesuperzapper]

@dpgaspar anything more you want to change before we merge?

[dpgaspar]

LGTM

[msardana94]

@dpgaspar Do you know when will this feature be released? This PR is super helpful and will make ldap based rbac so much better 🥇

[dpgaspar]

@msardana94 Planning to release next week

[Asturias-sam]

@dpgaspar @thesuperzapper Is multiple role supported for AUTH_USER_REGISTRATION_ROLE i.e can assign role A and B both
After this We can have multiple role using LDAP group

[Asturias-sam]

And one more thing

AUTH_ROLES_MAPPING = {
   "cn=fab_users,ou=groups,dc=example,dc=com &&(cn=fab_users_2,ou=groups,dc=example,dc=com)": ["User"],
   "cn=fab_admins,ou=groups,dc=example,dc=com": ["Admin"],
}

Is OR AND condition supported ?

[basanthcse]

@dpgaspar @thesuperzapper
Airflow Version 1.10.12
Flask-AppBuilder 2.3.4
We have implemented LDAP authentication with RBAC in Airflow user authentication. I have one situation where every user is able to login as Admin, if i mention AUTH_USER_REGISTRATION_ROLE = "Admin" in webserver_config.py file. I have two ldap group (airflow_admin and airflow_dev groups), i need airflow_admin user to login as ADMIN role and airflow_dev group users as OP role. How can I dynamically assign the AUTH_USER_REGISTRATION_ROLE based on the user's LDAP role?

is this feature will be include in latest version of flask-appbuilder and do we have to use airflow 2.0.0 ?

[basanthcse]

And one more thing

AUTH_ROLES_MAPPING = {
   "cn=fab_users,ou=groups,dc=example,dc=com &&(cn=fab_users_2,ou=groups,dc=example,dc=com)": ["User"],
   "cn=fab_admins,ou=groups,dc=example,dc=com": ["Admin"],
}

Is OR AND condition supported ?

#is AUTH_ROLES_MAPPING working for you to authenticate different users from LDAP groups ?
which version are you using for airflow and flask-appbuilder ?

[thesuperzapper]

@Asturias-sam currently, we do not support OR/AND semantics in AUTH_ROLES_MAPPING. Note, the keys are not LDAP search terms, but the literal UID's of groups found in the user's AUTH_LDAP_GROUP_FIELD after a lookup.

But most people can achieve the same outcome by creating nested groups in your LDAP/AD itself, however, I can see a need for this in OAUTH (where making groups may be harder).

We could allow python functions as the keys of AUTH_ROLES_MAPPING, something like:

def is_fab_user(user_role_keys: Set[str]) -> bool:
  is_users_1 = "cn=fab_users_1,ou=groups,dc=example,dc=com" in user_role_keys
  is_users_2 = "cn=fab_users_2,ou=groups,dc=example,dc=com" in user_role_keys
  return is_users_1 and is_users_2

AUTH_ROLES_MAPPING = {
    is_fab_user: ["User"],
    "cn=Admin,ou=groups,dc=example,dc=com": ["Admin"],
}

@Asturias-sam if you want this feature, please raise and issue, and tag me in it (probably link to this comment also)