CX-182: Includes the posibility of auto generating IDs for new PosixAccounts #148
Conversation
WalkthroughAdds optional POSIX ID auto-calculation to the account creation schema, moves profile extraction into a method on Changes
Sequence DiagramsequenceDiagram
actor User
participant CreateAccount
participant Profile as userResourceType.extractProfile
participant LDAPClient as LDAP Client
participant LDAPServer as LDAP Server
User->>CreateAccount: Submit account creation (profile + calculatePosixIDs flag)
CreateAccount->>Profile: extractProfile(ctx, accountInfo)
alt calculatePosixIDs enabled
Profile->>LDAPClient: CalculateUIDAndGID(searchDomain, pageSize)
LDAPClient->>LDAPServer: Paged search (objectClass=posixAccount)
LDAPServer-->>LDAPClient: Return entries
LDAPClient->>LDAPClient: Compute max uidNumber/gidNumber → +1
LDAPClient-->>Profile: Return next UID and GID
Profile->>Profile: Inject UID/GID attributes
end
Profile-->>CreateAccount: Return DN and attributes
CreateAccount->>LDAPServer: Add entry
LDAPServer-->>CreateAccount: Success
CreateAccount-->>User: Account created
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
JavierCarnelli-ConductorOne
requested review from
a team,
FeliLucero1,
agustin-conductor,
btipling,
johnallers,
luisina-santos,
mateoHernandez123,
mateovespConductor and
sergiocorral-conductorone
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@pkg/connector/user.go`:
- Around line 571-576: In the loop over objectClass in pkg/connector/user.go
(the block that checks each oc and sets isPosixAccount), make the posixAccount
detection case-insensitive: when oc is asserted to string, compare it to
"posixAccount" using a case-insensitive comparison (e.g., strings.EqualFold or
by normalizing with strings.ToLower) before setting isPosixAccount so values
like "PosixAccount" or "POSIXACCOUNT" are recognized.
- Around line 559-564: The DN is built by concatenating rdnValue (and
unvalidated path/suffix), which allows DN injection or invalid DNs; escape the
RDN value with ldap3.EscapeDN() before using it in the concatenation in the DN
construction block (the variables rdnKey, rdnValue, path, suffix and the dn
assignment), and ensure path and suffix are validated/escaped (or normalized) as
well before joining so special characters in path/suffix cannot break the DN.
In `@pkg/ldap/client.go`:
- Around line 135-186: CalculateUIDAndGID currently does a max-scan and can
return duplicate uidNumber/gidNumber under concurrent provisioning; fix by
adding application-level synchronization or retry-on-conflict: either (A)
introduce a package-level mutex around the critical section in
CalculateUIDAndGID (acquire before listing entries and release after returning
new values) to serialize calls, or (B) implement optimistic retry by attempting
the create operation and, on LDAP uniqueness violation/error, re-running
CalculateUIDAndGID (or LdapSearch + compute) with exponential backoff a few
times before failing; ensure the chosen approach references CalculateUIDAndGID
and LdapSearch and documents that server-side uniqueness must be relied on if
you choose not to add locking/retries.
🧹 Nitpick comments (1)
pkg/ldap/client.go (1)
135-186: Avoid loading all entries/attributes into memory.Line 135-186: accumulating every entry and requesting
*attributes is heavy for large directories. You can compute max per page and only request uidNumber/gidNumber.♻️ Suggested refactor
- var totalEntries []*ldap.Entry - var page string + var page string + maxUID := 0 + maxGID := 0 for { userEntries, nextPage, err := c.LdapSearch( ctx, ldap.ScopeWholeSubtree, searchDomain, "(objectClass=posixAccount)", - []string{"*"}, + []string{"uidNumber", "gidNumber"}, page, pageSize, ) if err != nil { return "", "", fmt.Errorf("baton-ldap: failed to list users on CalculateUIDAndGID: %w", err) } - totalEntries = append(totalEntries, userEntries...) + for _, entry := range userEntries { + uVal := entry.GetAttributeValue("uidNumber") + if uVal != "" { + if i, err := strconv.Atoi(uVal); err == nil && maxUID < i { + maxUID = i + } + } + gVal := entry.GetAttributeValue("gidNumber") + if gVal != "" { + if i, err := strconv.Atoi(gVal); err == nil && maxGID < i { + maxGID = i + } + } + } if nextPage == "" { break } page = nextPage } - maxUID := 0 - maxGID := 0 - - for _, entry := range totalEntries { - // ... - }
| var dn string | ||
| if path != "" { | ||
| dn = strings.Join([]string{fmt.Sprintf("%s=%s", rdnKey, rdnValue), path, suffix}, ",") | ||
| } else { | ||
| dn = strings.Join([]string{fmt.Sprintf("%s=%s", rdnKey, rdnValue), suffix}, ",") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -20 pkg/connector/user.go | cat -nRepository: ConductorOne/baton-ldap
Length of output: 813
🏁 Script executed:
sed -n '550,575p' pkg/connector/user.go | cat -nRepository: ConductorOne/baton-ldap
Length of output: 970
🏁 Script executed:
rg -n "import" pkg/connector/user.go | head -20Repository: ConductorOne/baton-ldap
Length of output: 77
🏁 Script executed:
rg -n "EscapeDN" --type goRepository: ConductorOne/baton-ldap
Length of output: 275
🏁 Script executed:
rg -n "ldap" pkg/connector/user.go | head -20Repository: ConductorOne/baton-ldap
Length of output: 1435
🏁 Script executed:
sed -n '540,570p' pkg/connector/user.go | cat -nRepository: ConductorOne/baton-ldap
Length of output: 1080
🏁 Script executed:
sed -n '351,365p' vendor/github.com/go-ldap/ldap/v3/ldap.go | cat -nRepository: ConductorOne/baton-ldap
Length of output: 663
🏁 Script executed:
rg "EscapeAttribute|EscapeDN" vendor/github.com/go-ldap/ldap/v3/ -A 3Repository: ConductorOne/baton-ldap
Length of output: 659
Escape RDN value in DN construction to prevent DN injection and invalid DNs.
Lines 559–564: rdnValue is concatenated directly into the DN without escaping. Values containing commas, equals signs, or special characters will alter DN structure or produce invalid DNs. Use ldap3.EscapeDN() to escape the RDN value before concatenation. Also validate or escape path and suffix to prevent injection through those fields.
Suggested fix
+ rdnValueEsc := ldap3.EscapeDN(rdnValue)
+ rdn := fmt.Sprintf("%s=%s", rdnKey, rdnValueEsc)
if path != "" {
- dn = strings.Join([]string{fmt.Sprintf("%s=%s", rdnKey, rdnValue), path, suffix}, ",")
+ dn = strings.Join([]string{rdn, path, suffix}, ",")
} else {
- dn = strings.Join([]string{fmt.Sprintf("%s=%s", rdnKey, rdnValue), suffix}, ",")
+ dn = strings.Join([]string{rdn, suffix}, ",")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var dn string | |
| if path != "" { | |
| dn = strings.Join([]string{fmt.Sprintf("%s=%s", rdnKey, rdnValue), path, suffix}, ",") | |
| } else { | |
| dn = strings.Join([]string{fmt.Sprintf("%s=%s", rdnKey, rdnValue), suffix}, ",") | |
| } | |
| var dn string | |
| rdnValueEsc := ldap3.EscapeDN(rdnValue) | |
| rdn := fmt.Sprintf("%s=%s", rdnKey, rdnValueEsc) | |
| if path != "" { | |
| dn = strings.Join([]string{rdn, path, suffix}, ",") | |
| } else { | |
| dn = strings.Join([]string{rdn, suffix}, ",") | |
| } |
🤖 Prompt for AI Agents
In `@pkg/connector/user.go` around lines 559 - 564, The DN is built by
concatenating rdnValue (and unvalidated path/suffix), which allows DN injection
or invalid DNs; escape the RDN value with ldap3.EscapeDN() before using it in
the concatenation in the DN construction block (the variables rdnKey, rdnValue,
path, suffix and the dn assignment), and ensure path and suffix are
validated/escaped (or normalized) as well before joining so special characters
in path/suffix cannot break the DN.
| // CalculateUIDAndGID returns the next valid values for UIDNumber and GIDNumber. That's the maximum stored increased by one. | ||
| func (c *Client) CalculateUIDAndGID(ctx context.Context, searchDomain *ldap.DN, pageSize uint32) (string, string, error) { | ||
| var totalEntries []*ldap.Entry | ||
| var page string | ||
|
|
||
| for { | ||
| userEntries, nextPage, err := c.LdapSearch( | ||
| ctx, | ||
| ldap.ScopeWholeSubtree, | ||
| searchDomain, | ||
| "(objectClass=posixAccount)", | ||
| []string{"*"}, | ||
| page, | ||
| pageSize, | ||
| ) | ||
| if err != nil { | ||
| return "", "", fmt.Errorf("baton-ldap: failed to list users on CalculateUIDAndGID: %w", err) | ||
| } | ||
|
|
||
| totalEntries = append(totalEntries, userEntries...) | ||
|
|
||
| if nextPage == "" { | ||
| break | ||
| } | ||
| page = nextPage | ||
| } | ||
|
|
||
| maxUID := 0 | ||
| maxGID := 0 | ||
|
|
||
| for _, entry := range totalEntries { | ||
| uVal := entry.GetAttributeValue("uidNumber") | ||
| if uVal != "" { | ||
| if i, err := strconv.Atoi(uVal); err == nil { | ||
| if maxUID < i { | ||
| maxUID = i | ||
| } | ||
| } | ||
| } | ||
|
|
||
| gVal := entry.GetAttributeValue("gidNumber") | ||
| if gVal != "" { | ||
| if i, err := strconv.Atoi(gVal); err == nil { | ||
| if maxGID < i { | ||
| maxGID = i | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return strconv.Itoa(maxUID + 1), strconv.Itoa(maxGID + 1), nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the repository structure
git ls-files | grep -E "\.go$" | head -20Repository: ConductorOne/baton-ldap
Length of output: 575
🏁 Script executed:
# Find the file and examine the function context
cat -n pkg/ldap/client.go | head -50Repository: ConductorOne/baton-ldap
Length of output: 1365
🏁 Script executed:
# Search for LDAP schema or uniqueness constraints
rg -i "unique|constraint" --type go -A 3 -B 3Repository: ConductorOne/baton-ldap
Length of output: 50382
🏁 Script executed:
# Find all callers of CalculateUIDAndGID
rg "CalculateUIDAndGID" --type go -B 2 -A 2Repository: ConductorOne/baton-ldap
Length of output: 991
🏁 Script executed:
# Check the Client struct for synchronization primitives
rg -A 20 "type Client struct" pkg/ldap/client.goRepository: ConductorOne/baton-ldap
Length of output: 582
🏁 Script executed:
# Look for retry or conflict handling patterns related to UID/GID
rg -i "retry|conflict|unique" --type goRepository: ConductorOne/baton-ldap
Length of output: 50381
🏁 Script executed:
# Examine provisioning logic to understand concurrency model
rg -i "provision|create.*user|create.*account" --type go -A 5 -B 2 | head -100Repository: ConductorOne/baton-ldap
Length of output: 50379
CalculateUIDAndGID has a race condition for concurrent provisioning.
Two concurrent calls to CalculateUIDAndGID can return the same uidNumber/gidNumber because the function performs a max-scan without synchronization. Without LDAP schema uniqueness constraints or application-level locking/retry logic, this violates POSIX requirements. Add a mutex, implement retry-on-conflict behavior, or confirm that uniqueness is enforced at the LDAP server level.
🤖 Prompt for AI Agents
In `@pkg/ldap/client.go` around lines 135 - 186, CalculateUIDAndGID currently does
a max-scan and can return duplicate uidNumber/gidNumber under concurrent
provisioning; fix by adding application-level synchronization or
retry-on-conflict: either (A) introduce a package-level mutex around the
critical section in CalculateUIDAndGID (acquire before listing entries and
release after returning new values) to serialize calls, or (B) implement
optimistic retry by attempting the create operation and, on LDAP uniqueness
violation/error, re-running CalculateUIDAndGID (or LdapSearch + compute) with
exponential backoff a few times before failing; ensure the chosen approach
references CalculateUIDAndGID and LdapSearch and documents that server-side
uniqueness must be relied on if you choose not to add locking/retries.
There was a problem hiding this comment.
Makes sense to consider concurrency on this function @btipling ?
I understand the concern from CodeRabbit but since this is only called when provisioning a user, we wouldn't face a concurrent case, right?
Provisioning tasks are sequential or could be done simultaneously?
If it isn't a realistic case, I don't think we should dedicate time to support this
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/connector/helpers.go`:
- Around line 147-149: The keyEqualsTo function currently lowercases both
strings (keyEqualsTo) which triggers Staticcheck SA6005 and allocates
intermediate strings; replace the manual lowercasing with the idiomatic
strings.EqualFold check to perform a case-insensitive comparison without
allocations (update the keyEqualsTo implementation to call
strings.EqualFold(receivedKey, expectedKey) and remove the ToLower usage).
In `@pkg/connector/user.go`:
- Around line 591-605: The main loop over data appends user-supplied
uidNumber/gidNumber into attrs even when calculatePosixIDs is true, causing
duplicate attributes; update the loop in the block iterating over data (the for
k, v := range data loop that calls toAttr and appends to attrs) to also skip
"uidNumber" and "gidNumber" when calculatePosixIDs is enabled—either add those
keys to the current slices.Contains list conditionally or add an extra if check
(e.g., if calculatePosixIDs && (k == "uidNumber" || k == "gidNumber") { continue
}) so the auto-generated POSIX IDs are not duplicated.
♻️ Duplicate comments (1)
pkg/connector/user.go (1)
559-564: Escape RDN value in DN construction (DN injection risk).
rdnValueis concatenated directly into the DN. Values with commas/equals can break the DN or inject additional RDNs. Please escape the RDN value (and consider validatingpath/suffix).🔒 Suggested fix
- if path != "" { - dn = strings.Join([]string{fmt.Sprintf("%s=%s", rdnKey, rdnValue), path, suffix}, ",") - } else { - dn = strings.Join([]string{fmt.Sprintf("%s=%s", rdnKey, rdnValue), suffix}, ",") - } + rdnValueEsc := ldap3.EscapeDN(rdnValue) + rdn := fmt.Sprintf("%s=%s", rdnKey, rdnValueEsc) + if path != "" { + dn = strings.Join([]string{rdn, path, suffix}, ",") + } else { + dn = strings.Join([]string{rdn, suffix}, ",") + }
| for _, entry := range totalEntries { | ||
| uVal := entry.GetAttributeValue("uidNumber") | ||
| if uVal != "" { | ||
| if i, err := strconv.Atoi(uVal); err == nil { |
There was a problem hiding this comment.
this below can be just maxUID = max(maxUID, i)
There was a problem hiding this comment.
Thanks for the suggestion!
I agree it's the right way, so I've changed it in b49560c
pkg/ldap/client.go
Outdated
| gVal := entry.GetAttributeValue("gidNumber") | ||
| if gVal != "" { | ||
| if i, err := strconv.Atoi(gVal); err == nil { | ||
| if maxGID < i { |
There was a problem hiding this comment.
Thanks for the suggestion!
I've updated this in b49560c
| var totalEntries []*ldap.Entry | ||
| var page string | ||
|
|
||
| for { |
There was a problem hiding this comment.
can we add sort order to the search query by uidNumber ? that way we just need the first page and the top element?
There was a problem hiding this comment.
As I understand, for being able to do a sorted search on an LDAP server it must have a 'feature' enabled to allow Server-Side Sorting. Since it may not be enabled for all LDAP servers (or configurations), I don't think we could rely on that.
I did some extra research and I even found that it's not that recommended for larger sets of entries, since it turns to be quite expensive for the server to load them into memory and execute a sort before answering.
I think we could stick to this approach for now, and revisit this idea (or look for another one) in case we see that a performance improvement is needed.
Thanks for the suggestion tho!
It supports the autogeneration of unique and incremental IDs for provisioned PosixAccounts.
Currently, generating (if enabled) the uidNumber and gidNumber as mentioned on the ticket.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.