Skip to content

Conversation

@simonLeary42
Copy link
Collaborator

@simonLeary42 simonLeary42 commented Aug 27, 2025

  • optimize IDNumber selection
    • IDNumInUse is extremely wasteful: it queries LDAP for the entire users, pi_groups, and groups subtrees (all attributes) every single iteration while it increments IDNumbers until it finds a free one
    • now, getNext{PIGID,ORGGID,UIDGID}Number query LDAP one time for just the uidNumber and gidNumber attributes, and use that data for all iterations
  • remove site vars
    • the wasteful behavior of IDNumInUse created the need for the site vars, they are no longer needed
    • in lieu of site vars, base offsets for various IDNumbers are now part of the deployment config:
      [ldap]
      offset_UIDGID = 1000000
      offset_PIGID = 2000000
      offset_ORGGID = 3000000
    • table removal, new options are documented in the README
  • avoid conflicts
    • IDNumInUse did not consider custom mapped IDs or IDNumbers outside the users, pi_groups, and groups subtrees, this is fixed
  • rewrite custom mapping logic
    • to enable getNext{PIGID,ORGGID,UIDGID}Number to avoid giving out custom mapped IDs, the CSV parsing logic from getUnassignedID has been moved to its own function getCustomIDMappings
    • the custom mappings have been convered into an associative array
    • the remaining logic from getUnassignedID has been removed and re-implemented in getNextUIDGIDNumber
    • added warning to error log when a custom mapped ID has already been taken by another user
    • added warning to error log when a custom mapping file is ignored due to file extension != ".csv"
    • require exact match, no more matching with $netid = strtok($uid, "_")
  • refactor
    • to make it clear that there is an expectation of equal UID and GID for a user getNextUIDNumber is now getNextUIDGIDNumber
      • the uid gid equality issue is a topic for further discussion
    • getNextPiGIDNumber -> getNextPIGIDNumber (consistent capitalization)
  • add tests
    • user with a custom mapping gets the expected custom IDNumber
    • custom mapped IDNumbers will not be granted to any other user
    • to make these tests work, I added a new config option ldap.custom_user_mappings_dir, set $_SERVER["HTTP_HOST"] to "phpunit" during unit tests, created a config override called phpunit, created a user mapping directory test/custom_user_mapping/, and changed ldap.custom_user_mappings_dir to test/custom_user_mapping in the phpunit config override.

supercedes #248, #253

@simonLeary42 simonLeary42 marked this pull request as draft August 27, 2025 04:43
@simonLeary42 simonLeary42 marked this pull request as draft August 27, 2025 04:43
@simonLeary42 simonLeary42 marked this pull request as ready for review August 27, 2025 13:00

This comment was marked as outdated.

@simonLeary42 simonLeary42 requested review from Copilot and removed request for Copilot August 27, 2025 13:04
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the UID/GID allocation system to eliminate conflicts by checking IDs across the entire LDAP tree instead of specific OUs, and removes the SQL-based site variables system in favor of configuration-based offset values.

  • Replaces SQL sitevars table with configuration-based offset values for different ID ranges
  • Optimizes LDAP queries to check for ID conflicts tree-wide instead of per-OU
  • Removes iterative ID checking in favor of single LDAP query with array operations

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tools/docker-dev/sql/bootstrap.sql Removes sitevars table creation and related indexes
resources/lib/UnitySQL.php Removes getSiteVar and updateSiteVar methods and table constant
resources/lib/UnityLDAP.php Refactors ID allocation with tree-wide conflict checking and configuration offsets
resources/lib/UnityGroup.php Updates PI group creation to use new LDAP method signature
resources/init.php Adds new offset parameters to UnityLDAP constructor
defaults/config.ini.default Adds offset configuration values for different ID ranges
README.md Documents migration steps from version 1.2 to 1.3

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@simonLeary42 simonLeary42 changed the title fix uid conflict 2 optimize IDNumber selection, avoid conflicts Aug 27, 2025
@simonLeary42 simonLeary42 changed the title optimize IDNumber selection, avoid conflicts optimize IDNumber selection, avoid conflicts, remove site vars Aug 27, 2025
@simonLeary42 simonLeary42 changed the title optimize IDNumber selection, avoid conflicts, remove site vars optimize IDNumber selection, avoid conflicts, remove site vars, rewrite custom mapping Aug 27, 2025
@simonLeary42 simonLeary42 marked this pull request as draft August 27, 2025 14:37
@simonLeary42 simonLeary42 force-pushed the fix-uid-conflict2 branch 12 times, most recently from 9ed28b1 to afcd23f Compare August 27, 2025 19:21
@simonLeary42 simonLeary42 changed the title optimize IDNumber selection, avoid conflicts, remove site vars, rewrite custom mapping optimize IDNumber selection, avoid conflicts, remove site vars, rewrite custom mapping, add tests Aug 27, 2025
@simonLeary42 simonLeary42 force-pushed the fix-uid-conflict2 branch 2 times, most recently from ccadb28 to 7d7653c Compare August 27, 2025 21:17
@simonLeary42 simonLeary42 marked this pull request as ready for review August 27, 2025 21:31
@simonLeary42 simonLeary42 requested a review from bryank-cs August 27, 2025 21:31
@simonLeary42
Copy link
Collaborator Author

closes #121

Copy link
Contributor

@bryank-cs bryank-cs left a comment

Choose a reason for hiding this comment

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

Couple of optional changes, otherwise looks ok.

@simonLeary42 simonLeary42 force-pushed the fix-uid-conflict2 branch 2 times, most recently from 01c80f8 to d2d38fc Compare September 12, 2025 12:48
@simonLeary42 simonLeary42 merged commit 3c3ebbc into main Sep 12, 2025
2 checks passed
@simonLeary42 simonLeary42 deleted the fix-uid-conflict2 branch September 12, 2025 13:36
@simonLeary42 simonLeary42 mentioned this pull request Sep 11, 2025
@simonLeary42
Copy link
Collaborator Author

f9828e6

@simonLeary42
Copy link
Collaborator Author

b6dd299

@simonLeary42
Copy link
Collaborator Author

#307

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.

3 participants