Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Ignore LDAP search referrals #1602

Merged
merged 2 commits into from
Apr 21, 2021

Conversation

fredthomsen
Copy link
Contributor

@fredthomsen fredthomsen commented Apr 7, 2021

Description

LDAP referrals are completely broken anyway, and we've already set
ldap.OPT_REFERRALS to 0, so there is no reason to care about these.

Thus the presence of referrals alone should not cause authentication of
a user to fail due to multiple results being returned.

Fixes issue #1581.

If this change is not desired, and rather a documentation update to
indicate that setting up LDAP against Microsoft AD requires a
organizational unit to be specified in the LDAP search base, and I
happy to make that update. I can add the necessary tests once I get
feedback if we want to move forward with this.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Is CRUD MVC related.
  • Is Auth, RBAC security related.
  • Changes the security db schema.
  • Introduces new feature
  • Removes existing feature

@fredthomsen fredthomsen changed the title Ignore LDAP search referrals Fix: Ignore LDAP search referrals Apr 7, 2021
@fredthomsen fredthomsen changed the title Fix: Ignore LDAP search referrals fix: Ignore LDAP search referrals Apr 7, 2021
@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #1602 (0ea1ea8) into master (27b15e5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1602   +/-   ##
=======================================
  Coverage   75.44%   75.45%           
=======================================
  Files          54       54           
  Lines        7869     7870    +1     
=======================================
+ Hits         5937     5938    +1     
  Misses       1932     1932           
Flag Coverage Δ
python 75.45% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
flask_appbuilder/security/manager.py 72.08% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27b15e5...0ea1ea8. Read the comment docs.

@fredthomsen fredthomsen force-pushed the ignoreLdapReferrals branch 3 times, most recently from 9a13ec5 to d3a166d Compare April 7, 2021 15:43
Copy link
Contributor

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think we should add some unit tests for this, but possibly that is overkill.

BTW, for unit tests you will need to add refs to our LDAP test structure, similar to how dalibo does it, but do note that they use a different LDAP test library.

@fredthomsen fredthomsen force-pushed the ignoreLdapReferrals branch from d3a166d to 673ec96 Compare April 13, 2021 02:44
@fredthomsen
Copy link
Contributor Author

fredthomsen commented Apr 13, 2021

I also think we should add some unit tests for this, but possibly that is overkill.

BTW, for unit tests you will need to add refs to our LDAP test structure, similar to how dalibo does it, but do note that they use a different LDAP test library.

I tend to agree that unit tests are likely overkill here, and using mockldap would be a no go, and I'd have to mock the search_s method directly. Ok regardless, I went ahead and made one.

@fredthomsen fredthomsen force-pushed the ignoreLdapReferrals branch from 673ec96 to 477efa6 Compare April 13, 2021 03:40
LDAP referrals are completely broken anyway, and we've already
set ldap.OPT_REFERRALS to 0, so there is no reason to care about
these.

Thus the presence of referrals alone should not cause
authentication of a user to fail due to multiple results being
returned, as they can be filtered out.

This fixes issue dpgaspar#1581.
@thesuperzapper
Copy link
Contributor

@dpgaspar this looks good, can you test this, and then put out a patch release ASAP, as this was a breaking change for some users.

The airflow community decided to push the default version of Flask-AppBuilder to 3.2.X, but this has caused some issues for people who are not setting AUTH_LDAP_SEARCH.

/cc @potiuk

Copy link
Owner

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dpgaspar dpgaspar merged commit efc6944 into dpgaspar:master Apr 21, 2021
@dpgaspar
Copy link
Owner

Just released 3.2.3rc1

@potiuk
Copy link
Contributor

potiuk commented Apr 21, 2021

Can we expect an officiall pypi release soon? I am preparing new constraint files for Airflow (to fix other problems) and I would love to include that one

@dpgaspar
Copy link
Owner

yes probably this week

@potiuk
Copy link
Contributor

potiuk commented Apr 21, 2021

I see. I push the new constraints in ~ 20 minutes. so i will limit it to 3.1.1 for now (but won't add install_requires limits). Thanks for acting quickly !

This was referenced Apr 22, 2021
potiuk added a commit to apache/airflow that referenced this pull request Apr 28, 2021
There was a bug in FAB 3.2.0 - 3.2.2 related to multiple results
returned by LDAP search when authenticating. This has been fixed
in 3.2.3 (dpgaspar/Flask-AppBuilder#1602)

Since FAB is an integral Part of the UI and 3.2.* line has
useful features areound role mappig, we bump the "golden"
version of the Airflow 2.0.2 to include that version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants