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

Adding Mock LDAP Functionality for improved testing #103

Merged
merged 3 commits into from
Jan 25, 2022
Merged

Adding Mock LDAP Functionality for improved testing #103

merged 3 commits into from
Jan 25, 2022

Conversation

andrewjroth
Copy link
Contributor

Hello,

As described in #28, I've added an option LDAP_MOCK_DATA that, when not None (default), will implement a Mock connection, instead of a real connection. This "mocking" is described in detail in the Python ldap3 package page on Mocking.

Minimal changes were made in the package code to setup the connection using a different strategy. Due to a problem with how the connection is setup with a ServerPool (see cannatag/ldap3#1007) a fake Server object is also created and used for the connection, ignoring the established ServerPool object for the extension.

Additional changes were needed in order to setup unit tests, including dumping the test "directory" to a JSON file to be used to load entries into the mock server DIT. There is also a difference between the schema used by the test directory information and what is used and returned by ldap3 during mocking, so a slight modification to the test directory was made. For a cleaner or more long term solution, the structure of the test directory might be modified to match the ldap3 mock implementation or the schema on the mock ldap3 implementation could be better modeled. In theory, all tests could start using the ldap3 mock instead of the existing mock.

I originally intended to use the Flask app instance directory for the relative import of the mock data, but found that adding this relative import required too much additional overhead. I believe that the import is relative to the current working directory of the Python thread, but unsure if this is always the case. Users of flask-ldap3-login should specify LDAP_MOCK_DATA using an absolute path or may want to do some trial and error to determine how to load data.

Thank you for considering this PR!!! I'm open to further modifications for improvements on this feature.

@andrewjroth
Copy link
Contributor Author

Submitted cannatag/ldap3#1009 to be able to correctly address the Flask app instance directory relative import of the mock data, potentially for the future.

Copy link
Collaborator

@gmacon gmacon left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me, just one nit.

Thanks for implementing this. Did you have in mind a period of time you wanted to wait for ldap3 to take your PR before we merge this as-is?

flask_ldap3_login_tests/Directory.py Outdated Show resolved Hide resolved
@andrewjroth
Copy link
Contributor Author

andrewjroth commented Jan 11, 2022

There is no need to wait for the PR for ldap3. I have not submitted a PR to ldap3 that addresses cannatag/ldap3#1007 since it is too complex an issue for me to sort out. This PR works with what is currently published for ldap3. The PR I submitted for ldap3 (cannatag/ldap3#1009) would support Flask app instance directory relative import of mock data, which would require additional code if/when it is accepted. It would be a new future improvement.

@andrewjroth
Copy link
Contributor Author

Sorry to ask, but I'm not sure what the procedure is. With multiple approvals, should this PR be merged? Or tagged to be included in a future release? Not trying to rush, but just curious what the timeline or next steps are.

Thank you!

@gmacon gmacon merged commit 09f0ce6 into nickw444:master Jan 25, 2022
@nickw444
Copy link
Owner

nickw444 commented Jan 31, 2022

@gmacon does this one need a release published (backport to 0.x)? Or will it be bundled into v1?

@gmacon
Copy link
Collaborator

gmacon commented Feb 2, 2022

I think it's probably OK to leave this until v1.

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