Juan Hernandez has posted comments on this change.
Change subject: [WIP] Introduce new directory interface
......................................................................
Patch Set 1: (7 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/directory/DirectoryManager.java
Line 55: /**
Line 56: * Loads the directory implementations that are available in the
class path
Line 57: * using the service loader mechanism.
Line 58: */
Line 59: private void loadImplementations() {
No this needs to go in the class path, together with the .jar file that
provides the implementation. If you want to enable/disable a provider then you
just need to add/remove the .jar file from the class path.
Line 60: ServiceLoader<DirectorySpi> loader =
Line 61: ServiceLoader.load(DirectorySpi.class);
Line 62: for (DirectorySpi implementation : loader) {
Line 63: implementations.add(implementation);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/users/DirectoryEntryStatus.java
Line 10: * check performed by the engine.
Line 11: */
Line 12: public enum DirectoryEntryStatus implements Identifiable {
Line 13: INACTIVE(0),
Line 14: ACTIVE(1);
I will change this.
Line 15:
Line 16: private int value;
Line 17:
Line 18: private static Map<Integer, DirectoryEntryStatus> mappings = new
HashMap<Integer, DirectoryEntryStatus>();
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/users/Directory.java
Line 26: * @param name the name of the user
Line 27: * @return the user corresponding to the given name or
Line 28: * <code>null</code> if no such user can be found
Line 29: */
Line 30: DirectoryUser findUserByName(String name);
We don't do searches by random attributes in the engine, only by name and by
id. The rest of the searches are done using the query mechanism, and that
already supports all kinds of attributes.
The search by name is currently used only in one place: when authentication
fails we use this search to find the entry for the user (assuming it is unique)
and check for password expiration. Actually the code that does this doesn't
currently work, so I am in favor of removing it, and this method.
Line 31:
Line 32: /**
Line 33: * Retrieves an user from the directory given its identifier. Note
that this
Line 34: * identifier is the one assigned by the engine to the user, not
the one
Line 50: * with no particular order, note that the returned set may
contain less
Line 51: * elements than the given set of identifiers and that it may be
in a
Line 52: * different order
Line 53: */
Line 54: Set<DirectoryUser> findUsersById(Set<Guid> ids);
The UUIDs are the standard identifiers uses all over the engine. As I explain
in the comment these ids aren't the ones used by the directory, but the ones
used by the engine (by the directory implementation in this case). It is up to
the directory implementation to use directly the identifiers provided by the
directory or to use a mapping, so using UUIDs here doesn't dictate in any way
which identifiers should be used internally by the directory.
All the LDAP providers that we currently support (AD, IPA, RHDS, ITDS and
OpenLDAP) use 128 bit internal unique identifiers, so in these cases we can
just use them directly.
For other kind of directories we may be able or not to squeeze their internal
identifiers into the 128 bits of the UUID, but even if we can't the directory
implementation can always use its own mapping table.
Strings, specially if they have a common prefix like "uuid:", are expensive and
make bad indexes.
Line 55:
Line 56: /**
Line 57: * Retrieves a group from the directory given its identifier. Note
that this
Line 58: * identifier is the one assigned by the engine to the group, not
the one
Line 72: * @param query the query
Line 73: * @return a list containing the users that match the given query
Line 74: */
Line 75: Set<DirectoryUser> findUsersByQuery(String query);
Line 76:
What do you propose for that query interface?
Line 77: /**
Line 78: * Search the directory looking for groups that match the given
search
Line 79: * query.
Line 80: *
Line 80: *
Line 81: * @param query the query
Line 82: * @return a list containing the groups that match the given query
Line 83: */
Line 84: Set<DirectoryGroup> findGroupsByQuery(String query);
The query language used by the engine is not an implementation detail, it is
already part of the interface of the engine, even part of the contract between
the human users and the engine, as they can type queries in the GUI.
What we shouldn't have here is anything that is specific of the directory
implementation, for example, we shouldn't have here LDAP specific queries.
It is the responsibility of the directory implementation to translate the
engine queries into whatever the underlying directory supports.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/users/DirectoryUser.java
Line 15: private String firstName;
Line 16: private String lastName;
Line 17: private String title;
Line 18: private String email;
Line 19: private String department;
Hash maps are expensive data structures, in this case it would require approx
160 additional bytes per directory entry.
Line 20:
Line 21: // Flag indicating if this user has the administrator role:
Line 22: private boolean isAdmin = false;
Line 23:
--
To view, visit http://gerrit.ovirt.org/15596
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If84a0c9d6553d81cdbbe224972696f169cca90d4
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
