Juan Hernandez has posted comments on this change.
Change subject: core: Query construction for ldap vendors
......................................................................
Patch Set 9:
(7 comments)
....................................................
File
backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/ldap/query/LdapGroupAttributeMapper.java
Line 44: protected GroupAttributes[] getEnumValues() {
Line 45: return GroupAttributes.values();
Line 46: }
Line 47: @Override
Line 48: public LdapGroup mapEntity(Attributes attributes) throws
NamingException {
Why LdapGroup and not DirectoryGroup?
Line 49: LdapGroup group = new LdapGroup();
Line 50:
Line 51: String name = getAttribute(attributes, GroupAttributes.name);
Line 52: ExternalId id = getExternalId(attributes, GroupAttributes.id);
....................................................
File
backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/ldap/query/LdapUserAttributeMapper.java
Line 19: super(configuration, "query.user");
Line 20: this.directory = directory;
Line 21: }
Line 22:
Line 23: public static enum UserAttributes {
Would it be possible to use reflection to scan the properties of the entity
(DirectoryUser or DirectoryGroup) and thus avoid this enum?
Line 24: id,
Line 25: name,
Line 26: firstName,
Line 27: lastName,
Line 36: return UserAttributes.values();
Line 37: }
Line 38:
Line 39: @Override
Line 40: public LdapUser mapEntity(Attributes attributes) throws
NamingException {
Why are you using LdapUser instead of DirectoryUser? The LdapUser class
shouldn't be used at all in the new infrastructure.
Line 41: String name = getAttribute(attributes, UserAttributes.name);
Line 42: ExternalId id = getExternalId(attributes, UserAttributes.id);
Line 43: LdapUser user = new LdapUser();
Line 44: user.setUserId(id);
Line 49: user.setTitle(this.<String> getAttribute(attributes,
UserAttributes.title));
Line 50: user.setDepartment(this.<String> getAttribute(attributes,
UserAttributes.department));
Line 51: user.setMemberof(getMultipleValueAttribute(attributes,
UserAttributes.memberof));
Line 52: user.setDomainControler(directory.getName());
Line 53: return user;
Here we are assuming that the mapping is from one LDAP attribute to one entity
attribute. That may not be true. I think this can be much more flexible if you
use templates also for the generation of the values. For example, the
administrator can decide to generate the e-mail address from the user name and
the directory name. This can be achieved using the following in the
configuration file:
ldap.mapping.email=${uid}@${domain}
Line 54: }
Line 55:
Line 56: @Override
Line 57: protected EnumMap<UserAttributes, String>
createAttributeNamesMapping() {
....................................................
File
backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/ldap/query/MultipleTemplateQueryBuilder.java
Line 4: * Responsible for generating a query where the template can occur
several times
Line 5: * For example
Line 6: * (|(id=1)(id=100)(id=2344))
Line 7: */
Line 8: public class MultipleTemplateQueryBuilder extends QueryBuilder {
I think that you can do this using a more capable templating mechanism, as the
required loop can go in the template instead of in the Java code.
Line 9:
Line 10: private String prefix;
Line 11: private String suffix;
Line 12:
....................................................
File
backend/manager/modules/authentication/src/main/java/org/ovirt/engine/core/authentication/ldap/query/QueryBuilder.java
Line 6: *
Line 7: */
Line 8: public class QueryBuilder {
Line 9: protected String template;
Line 10: protected QueryParameter[] queryParams;
Why QueryParameter and not just java.lang.Object?
Line 11:
Line 12: public QueryBuilder (String template, QueryParameter...
queryParams) {
Line 13: this.template = template;
Line 14: this.queryParams = queryParams;
Line 14: this.queryParams = queryParams;
Line 15: }
Line 16:
Line 17: public String build() {
Line 18: return String.format(template, (Object[]) queryParams);
Did you consider using a more capable templating mechanism. Some time ago I
proposed something similar to JSP pages:
http://gerrit.ovirt.org/14650
This is an example of a template used to generate a LDAP query containing
multiple values:
http://gerrit.ovirt.org/#/c/14650/2/backend/manager/modules/utils/src/test/resources/org/ovirt/engine/core/utils/templates/plain_uuids.template
And a test that shows how to use them:
http://gerrit.ovirt.org/#/c/14650/2/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/templates/TemplateTest.java
Line 19: }
Line 20:
Line 21: public static void main(String[] args) {
Line 22: QueryParameter param = new QueryParameter("hello");
--
To view, visit http://gerrit.ovirt.org/22705
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I06e924a5f0f04f360a78746f16a4fee4dc1e2614
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
