Skip to content

Conversation

flo-renaud
Copy link
Collaborator

Integration domain: users_dn is a full DN

The config parameter users_dn is documented as
"Optional full DN of LDAP tree where users are"
but the current implementation is not consistent with the description.

Make this parameter mandatory, and expect it to contain the full
DN of the users tree, for instance:
ou=users,dc=example,dc=com

With this new behavior, no need to build the user dn from
relative dn + suffix.

Integration domain: de-duplicate code between AD and LDAP

The classes are 95% identical.
The current differences so far:

  • the expected naming convention: users in AD are named
    cn=,$USER_BASE_DN
    while users in LDAP are named
    uid=,$USER_BASE_DN

  • the bind DN (client_id) used to connect to the remote AD/LDAP
    server. For AD the code is using client_id@domain but we can
    use a user DN instead.

Introduce a class variable _user_rdn_attr that stores the
RDN attribute used to build a user DN.

Integration domain: allow 255 chars for client_id

The client_id contains a bind DN and can be longer than 20 chars.

The config parameter users_dn is documented as
"Optional full DN of LDAP tree where users are"
but the current implementation is not consistent with the description.

Make this parameter mandatory, and expect it to contain the full
DN of the users tree, for instance:
ou=users,dc=example,dc=com

With this new behavior, no need to build the user dn from
relative dn + suffix.
The classes are 95% identical.
The current differences so far:
- the expected naming convention: users in AD are named
cn=<username>,$USER_BASE_DN
while users in LDAP are named
uid=<username>,$USER_BASE_DN

- the bind DN (client_id) used to connect to the remote AD/LDAP
server. For AD the code is using client_id@domain but we can
use a user DN instead.

Introduce a class variable _user_rdn_attr that stores the
RDN attribute used to build a user DN.
The client_id contains a bind DN and can be longer than 20 chars.
@f-trivino
Copy link
Collaborator

@flo-renaud thanks for your PR. This considerably improves the code. I've completed e2e tests for AD and LDAP use cases using the following Payloads:

LDAP:

{
  "name": "ldap.test",
  "description": "LDAP Integration Domain",
  "integration_domain_url": "ldap://rhds.ldap.test",
  "client_id": "cn=Directory Manager",
  "client_secret": "Secret123",
  "id_provider": "ldap",
  "user_extra_attrs": "mail:mail, sn:sn, givenname:givenname",
  "user_object_classes": "",
  "users_dn": "ou=people,dc=ldap,dc=test",
  "ldap_tls_cacert": "/etc/openldap/certs/cacert.pem"
}

AD:

{
  "name": "da.test",
  "description": "AD Integration Domain",
  "integration_domain_url": "ldap://ad.da.test",
  "client_id": "[email protected]",
  "client_secret": "Secret123",
  "id_provider": "ad",
  "user_extra_attrs": "mail:mail, sn:sn, givenname:givenname",
  "user_object_classes": "",
  "users_dn": "cn=Users,dc=ldap,dc=test",
  "ldap_tls_cacert": "/etc/openldap/certs/cacert.pem"
}

AD and LDAP writable interface is able to add users in the integration domain, accordingly.

I'm approving and merging.

Copy link
Collaborator

@f-trivino f-trivino left a comment

Choose a reason for hiding this comment

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

LGTM.

@f-trivino
Copy link
Collaborator

no makemigrations are needed for this change so I'm merging it.

@f-trivino f-trivino merged commit 2abe50c into freeipa:main Jun 15, 2023
@flo-renaud flo-renaud deleted the refactor branch June 15, 2023 14:47
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