Drop LDAP email attribute#9361
Open
gmazoyer wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
type/documentation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 7 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. These changes are low-risk: they add a startup warning for inert LDAP config on community deployments, remove an unused LDAP email attribute, and make ExternalIdentity.email optional with backward-compatible handling, all covered by unit tests and isolated from core business logic.
Re-trigger cubic
gmazoyer
force-pushed
the
gma-20260527-ifc2605
branch
from
bc5bdf3 to
470ab75
Compare
Contributor
|
Hey @gmazoyer, I'm wondering if this is actually needed if you check this validation: https://github.com/opsmill/infrahub/blob/develop/backend/infrahub/server.py#L54 The idea would be that if you try to enable LDAP in the community version the server would fail to start. |
The community account schema has no email field, and the LDAP path explicitly diverges from SSO's silent-takeover semantics so the email fallback in account-name resolution is never reached. Collecting an LDAP email attribute we never use only adds operator surface to a config block that is already easy to misread. Email stays available on the shared external-identity model for the SSO providers that do use it; the LDAP runtime omits it. `_pick_account_name` gains an explicit branch that raises when the fallback is needed but the identity provider did not supply an email, instead of dereferencing a missing value.
gmazoyer
force-pushed
the
gma-20260527-ifc2605
branch
from
470ab75 to
209361f
Compare
gmazoyer
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Two community-side LDAP loose ends. First, LDAPSettings carries an attribute_email field that doesn't pay rent: the community account schema has no email field, and the LDAP runtime never reaches the place where ExternalIdentity.email matters (SSO uses it as a fallback name on display-name collision, but the LDAP path refuses same-name collisions outright before that fallback would fire). Operators get a knob they don't need on an already-busy config block. Second, the SSO model requires email to be a non-empty string, which means LDAP either fabricates a value or carries one through purely to keep the schema happy. Making email optional removes the fabrication.
What changed
LDAPSettings.attribute_emailis removed.ExternalIdentity.emailbecomesstr | None, defaulting toNone. OIDC and OAuth2 callers still pass it; the LDAP runtime can now omit it.Summary by cubic
Warns at startup when LDAP is configured on community deployments (it’s inert there), and removes the unused LDAP email attribute. Makes
ExternalIdentity.emailoptional and adds a clear error when a name collision needs an email but the provider didn’t supply one; updates docs and the sample config.New Features
Migration
LDAPSettings.attribute_emailand anyINFRAHUB_LDAP_ATTRIBUTE_EMAILenv var from configs.NoneforExternalIdentity.emailin any custom code; OIDC/OAuth2 still provide it.Written for commit 209361f. Summary will update on new commits. Review in cubic