Update ldapjs to include fixes for syncing issues

[differsthecat]

Type of change

• Bug fix
• New feature development
• Tech debt (refactoring, code cleanup, dependency upgrades, etc)
• Build/deploy pipeline (DevOps)
• Other

Objective

Fix issue where testing and syncing with an ldap connection would sometimes spin forever. This is due to an outdated version of the ldapjs package that has had many updates since it was last updated in this project(in 2019). This is one of the issues I was seeing locally: ldapjs/node-ldapjs#602

The version of ldapjs that was being used was a fork of the original package, and the fix implemented in the forked version has long been merged back to the original package: ldapjs/node-ldapjs#497

Asana Issue
Fix has been confirmed to fix issue locally by QA

Code changes

• package.json & package-lock.json Update ldapjs to get several bug fixes for the search function used for to get users and groups.

Testing requirements

Test against various ldap services and make sure that syncing works as expected. This issue was noticed originally with Jumpcloud, but we should test others as well such as against a proper AD server. I did a smoke test of it locally and it worked fine but another set of eyes is always good.

Before you submit

• I have checked for linting errors (npm run lint) (required)
• I have added unit tests where it makes sense to do so (encouraged but not required)
• This change requires a documentation update (notify the documentation team)
• This change has particular deployment requirements (notify the DevOps team)

[eliykat]

I went through this exact process last year 😞

• upgraded here: Update ldapjs dependency #111
• we had enough customers experiencing syncing issues that we released a hotfix reverting the change here: Revert ldapjs update, use forked repo instead #133

I'll try to find some of the internal tickets from the time, but from memory the customers experienced the same symptoms that caused us to fork the repo in the first place, even though the fix had been merged into the official repo for some time. The upgrade had passed QA and we couldn't figure out how to reproduce it, so it got shelved as the fork appeared to be working fine.

It might be time to try again, but we should consider what we can do to avoid history repeating before we merge.

[cscharf]

It might be time to try again, but we should consider what we can do to avoid history repeating before we merge.

@differsthecat / @eliykat , perhaps we can re-engage those originally impacted customers, please work with @sevensixseven to see if we can get a preview test from the PR build to determine if it will work or break (if someone is willing).

[differsthecat]

@cscharf I'm a little concerned about giving a preview build from this PR to any clients at this point, it has a ton of changes such as the complete state service refactor and other tech debt items that I'm not sure have been regression tested yet. Also once they use this build, the state will be migrated so if it is broken, they won't be able to simply go back a version without having to re-enter their directory settings. I may be over worrying about that part though.

If we don't make this fix for the next release though, I'm worried that things will break regardless for a lot of people as both @Larry-Sussman and I saw syncing hang forever, sometimes for me but all of the time for him.

[differsthecat]

@cscharf Do you think we can create a build/hotfix off of the last release with just this package update? Then we don't have all of the other stuff that needs to be tested in play, just the package update

[cscharf]

Yes, we could do that. You should be able to just create another branch from the latest release tag and then merge this fix in... we can work with @bitwarden/dept-devops on getting a trial build of that done.

[differsthecat]

I have created the branch update-ldapjs-2.9.8 off of our last release and did some testing locally both against jumpcloud and AD, with and without filters (which is what appears to have broken clients in the past) and everything is working as expected. I'll rope in DevOps and see if we can get a build of it.

[differsthecat]

Holding off on merging this to see if we get any feedback from clients who saw errors when we updated this library last year

[addisonbeck]

Would we want to merge this before Friday when we cut rc branches?

[patrick-bitwarden]

@eliykat Robyn said she was planning on putting you in charge of merging this if everything looked good, since she's out this week. Did that conversation happen and, if so, does everything look good?

[eliykat]

As discussed with @patrick-bitwarden, we're waiting on feedback from customers who had trouble with this dependency upgrade last time. If we don't hear anything then I think we have to merge it anyway though, it sounds like it's definitely broken at the moment without the upgrade, compared to the risk of some issues later with the upgrade. And get QA to give it a real good going over.