LdapData helper #55
LdapData helper #55
Conversation
# Conflicts: # README.md # composer.json # project/.env.example # src/StarterKitServiceProvider.php # tests/Feature/InstallStarterKitTest.php # tests/TestCase.php
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new LDAP helper library for retrieving Cornell University LDAP data, providing a standardized way to query and parse LDAP information with caching capabilities.
Key Changes
- Adds complete LDAP data retrieval system with
LdapSearchandLdapDataclasses - Implements service provider for configuration management and installation
- Updates installation process to include LDAP configuration options
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Ldap/LdapSearch.php | Core LDAP search functionality with caching and connection handling |
| src/Ldap/LdapData.php | Immutable data object for normalized LDAP user information |
| src/Ldap/LdapDataServiceProvider.php | Service provider for LDAP configuration |
| config/ldap.php | LDAP configuration file with Cornell-specific defaults |
| tests/Unit/Ldap/LdapDataTest.php | Unit tests for LDAP data parsing and normalization |
| src/StarterKitServiceProvider.php | Updates installation process to include LDAP configuration |
| composer.json | Adds ext-ldap dependency and registers new service provider |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
| 'previousemplids' => $data['cornelledupreviousemplids'] ?? null, | ||
| ]; |
There was a problem hiding this comment.
Do we really need to return all these data from LDAP? Most of the time we just need to get a name and netid. I think the BindID must have special permissions to return some of these data (like emplid) and usually we do not have and do not need those permissions.
There was a problem hiding this comment.
Do we need to return all the data from LDAP: No and most queries won't, which is fine and the code requires only 'uid' to be populated in the results. (And even that can be made optional.)
The attribute list in here was meant as a starter list, merging what is in DailyCheck, IT Gov, and EMN phone, which happened to be three place I was working at the time I was putting this together. Looking at the LDAP names in Confluence, I see that indeed emplid is not public, so you are right about special permission on that.
| 'emplid' => $data['cornelleduemplid'] ?? '', | ||
| 'firstname' => $firstName ?? '', | ||
| 'lastname' => $lastName ?? '', | ||
| 'name' => trim("$firstName $lastName"), |
There was a problem hiding this comment.
I think full name must include middle name or at least it should be an option for both options. Also name formatting is usually application task: some applications need to display last name fist, some - first name first
There was a problem hiding this comment.
The array that is being populated is only there to match the LDAP::data() function that this is intended to replace.
This $ldapData = [... chunk could be removed entirely if we don't want to use the library for supporting existing applications.
|
|
||
| campusPhone: $data['cornelleducampusphone'] ?? null, | ||
| deptName: $data['cornelledudeptname1'] ?? null, | ||
| workingTitle: $data['cornelleduwrkngtitle1'] ?? null, |
There was a problem hiding this comment.
if an application needs department name and working title, then these data should be coming from Workday not from LDAP
There was a problem hiding this comment.
I think you are bringing up a good question about the source of the data. I can see that the attribute documentation indicates the those fields come from "HR" (i.e., Workday), but I don't know much else.
Do we have reason to be concerned about some of these pass-through fields? Looking at the sources for the attributes, lots of them come from Workday and Peoplesoft.
| public function email(): string | ||
| { | ||
| return $this->email ?: $this->principalName; | ||
| } |
There was a problem hiding this comment.
how can alias email exists without primary email?
There was a problem hiding this comment.
Oh! The doc comment is backwards 🤦🏻♂️
I think this confusion (mine, yours) highlights that we should be very clear about the function names. Maybe:
- emailAlias() : hopefully obvious, and should be null if there isn't one
- mail(): which is simply the direct value from LDAP (alias if it exists, or [email protected], if user has email)
- principalEmail(): just making this name up... but meant to be the
edupersonprincipalnameattribute which looks like [email protected] for Ithaca people
inaydich
left a comment
inaydich
left a comment
There was a problem hiding this comment.
I looked through the LDAP PR. I think it is too complicated and we do not need most of that. At the same time it, I think it is missing some functionality. We usually use LDAP to look up the person by full or partial netid or name. I suggest to discuss the requirements before writing the code.
I like the suggestion to review the requirements together before proceeding. I have two current use case that are applicable (one in WA Reviews, another that came up for EMN Phone), but you've got a lot more experience with Cornell LDAP use cases than I do. I'm not sure on timing for when to have the discussion, but maybe the FCS project will have a task that needs LDAP and we can catch it there. |
A helper for retrieving Cornell University LDAP data. Based on the legacy App\Helpers\LDAP::data() class that exists on various Cornell Laravel sites.
This PR does the following
To Review:
Notes
The testing fixtures are real data from LDAP that I scrubbed of personal identifiers. I felt that this rudimentary testing was necessary to assure that we don't break how we retrieve this data that users will see as very personal.