Yair Zaslavsky has posted comments on this change.
Change subject: [WIP] Add LDAP directory
......................................................................
Patch Set 3:
(5 comments)
Some first comments,
I have more questions about the TLS/SSL code -
not sure I fully understand it, can you please elaborate in email and/or at the
patches?
For example I did not fully understand why you use ASM.
In addition, how will multiple domains be supported? shall i place several conf
files at the auth.d directory?
....................................................
File
backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/ldap/LdapDirectory.java
Line 19: import org.ovirt.engine.core.common.utils.ExternalId;
Line 20: import org.slf4j.Logger;
Line 21: import org.slf4j.LoggerFactory;
Line 22:
Line 23: public class LdapDirectory implements Directory {
what about the rootDSE query that we use in the code? the scope for it is
object_scope - how are we going to handle it? what method are you going to
expose for it, and how is it going to interact with the code?
Line 24: private static final Logger log =
LoggerFactory.getLogger(LdapDirectory.class);
Line 25:
Line 26: /**
Line 27: * The name of the directory.
Line 70: controls.setSearchScope(SearchControls.SUBTREE_SCOPE);
Line 71:
Line 72: NamingEnumeration<SearchResult> results = null;
Line 73: try {
Line 74: results = ctx.search(base, "(uid=" + uid + ")",
controls);
arent thee queries should be based on provider specific attributes? uid is not
always valid, IIRC.
Line 75: if (results.hasMore()) {
Line 76: SearchResult result = results.next();
Line 77: return mapUser(result.getAttributes());
Line 78: }
Line 145: @Override
Line 146: public List<DirectoryUser> findUsers(List<ExternalId> ids) {
Line 147: List<DirectoryUser> result = new ArrayList<>(ids.size());
Line 148: for (ExternalId id : ids) {
Line 149: DirectoryUser user = findUser(id);
why not create an ldap query with OR on all the ids?
(|(uid=id1)(uid=id2)..... )
Line 150: if (user != null) {
Line 151: result.add(user);
Line 152: }
Line 153: }
Line 188: // Create the LDAP context:
Line 189: ctx = contextFactory.create();
Line 190:
Line 191: // Do the search:
Line 192: SearchControls controls = new SearchControls();
In general I assume this can be refactored as I see that it's repeating itself
in the code (creating search controls + setting the subtree scope + going over
the result)
Line 193: controls.setSearchScope(SearchControls.SUBTREE_SCOPE);
Line 194:
Line 195: NamingEnumeration<SearchResult> results = null;
Line 196: try {
Line 230: }
Line 231:
Line 232: private DirectoryUser mapUser(Attributes attr) throws
NamingException {
Line 233: // Get the basic attribures:
Line 234: String uid = (String) attr.get("uid").get();
always correct for all ldap providers we encountered so far?
Line 235: ExternalId id = new ExternalId(uid.getBytes());
Line 236:
Line 237: // Create the user with the basic attributes:
Line 238: DirectoryUser user = new DirectoryUser(this, id, uid);
--
To view, visit http://gerrit.ovirt.org/22386
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8d1883145a86a4f338cd17f2d36c150e67ab653
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
