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

Filter out deleted AD users unless otherwise instructed #548

Merged

Conversation

addisonbeck
Copy link
Contributor

@addisonbeck addisonbeck commented Aug 9, 2024

Tracking

https://bitwarden.atlassian.net/browse/PM-10380

Type of change

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

Objective

Directory Connector is deleting existing users even without the "Overwrite existing users" flag set. This happens for Active Directory configurations that have the Active Directory "Recycle Bin" enabled. This is a very specific and IdP configuration related bug, and so has probably flown under the radar for some time.

Code changes

This issue is caused because Directory Connector always checks for deleted users for Active Directory configurations, even with a live sync without the overwrite flag set. Adding a condition to the getUsers query for LDAP that returns early before searching for deleted users if the the sync is non-test and the overwrite flag is not set allows the users to come through in reports of a test sync, but stops them from being overwritten when a live sync is preformed. This mirrors the way disabled/deleted users work for other directory types.

I've also added some inline documentation to this section of the code, since it required a lot of research to figure out exactly what was even happening here.

Screenshots

For a recreation of the issue see Jira. The below recording shows the behavior post-fix.

Screen.Recording.2024-08-09.at.2.10.31.PM.mov

@addisonbeck addisonbeck requested a review from a team as a code owner August 9, 2024 18:18
@addisonbeck addisonbeck requested a review from r-tome August 9, 2024 18:18
Copy link
Contributor

github-actions bot commented Aug 9, 2024

Logo
Checkmarx One – Scan Summary & Detailsbecf8983-4d25-43bf-8780-9b1fa87aa0db

No New Or Fixed Issues Found

@addisonbeck addisonbeck merged commit decada8 into main Aug 12, 2024
16 of 17 checks passed
@addisonbeck addisonbeck deleted the ac/addison/pm-10380/users-removed-without-permission branch August 12, 2024 15:04
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.

2 participants