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

passport-ldapauth does not allow caching of ldap responses by ldapauth-fork #96

Open
1 of 2 tasks
aneeshpu opened this issue Feb 6, 2020 · 3 comments
Open
1 of 2 tasks

Comments

@aneeshpu
Copy link

aneeshpu commented Feb 6, 2020

If you know how to fix the issue, make a pull request instead.

  • I have a question that is inappropriate for StackOverflow. (Please ask any appropriate questions, such as how to use the library, there).
  • I believe this is an issue in this library and not in the underlying libraries ldapjs or ldapauth-fork. (This library is a passport strategy and does not implement the LDAP communication)

Note: if the issue template is not used, the issue will be closed.

Problem Description

ldapauth-fork accepts a boolean flag called cache. passport-ldapauth does not allow this flag to be processed as it creates a new instance of LdapAuth every single time. This PR allows for caching of LDAP responses when options are passed as an object to Strategy's constructor. It creates a new instance when options is a function.

Steps to Reproduce

set cache: true in options. ldapauth-fork will still make a call to the LDAP server for the same username. Ideally it should cache for 5 minutes.

@aneeshpu
Copy link
Author

@vesse I could raise a PR for this. Any chance you might be able to provide some guidance on how to approach it? It looks like we need to reuse LdapAuth instances as opposed to creating new instances every time.

@vesse
Copy link
Owner

vesse commented Nov 16, 2020

The reason for recreating the LdapAuth instance was originally (years ago) because after some time of not using the connection it got disconnected and there was no option to reconnect automatically. Now some good people of Internet have added reconnection support to ldapauth-fork which could mean that it could be possible to reuse the instance.

Unfortunately I haven't been working for a project with LDAP authentication for years so I don't have possibilities to try this in real life situation, but I'd suggest enabling reconnection and then just see if reusing the connection works also after a long inactivity.

@aneeshpu
Copy link
Author

aneeshpu commented Dec 7, 2020

Sounds good. I will try that and if that works, will raise a PR with that change.

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

No branches or pull requests

2 participants