Juan Hernandez has posted comments on this change. Change subject: 6. core: Introduce new authentication interfaces ......................................................................
Patch Set 32: (5 comments) http://gerrit.ovirt.org/#/c/15596/32/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationResult.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationResult.java: Line 1: package org.ovirt.engine.core.authentication; Line 2: Line 3: import java.util.List; Line 4: Line 5: import org.ovirt.engine.core.common.errors.VdcBllMessages; I think that we should try to avoid using classes from modules like common, as this means that the developers that create new authenticators need to understand how things like VdcBllMessages work. Line 6: Line 7: /** Line 8: * This class represents a result returned by an Authenticator Line 9: */ Line 8: * This class represents a result returned by an Authenticator Line 9: */ Line 10: public abstract class AuthenticationResult<T> { Line 11: Line 12: protected T detailedInfo; Instead of assuming that the authenticator will always contain an object for this I would suggest to add to this class a method that the user can use to get the details: /** * Returns a string containing the details of the authentication * result. The caller can specify what MIME type and locale should * be used for the result, and the authenticator will return {@code * null} if it doesn't support that combination. * * @param mimeType the MIME type describing the requested data * @param locale the locale of the requested data * @return a string describing the outcume, may be {@code null} * if the authenticator doesn't know how to describe the result * with the given mime type and locale */ public abstract String getDetails(String mimeType, Locale locale); This way the authenticator can support returning a complex document, for example the document describing how to change the password. Line 13: Line 14: protected AuthenticationResult(T detailedInfo) { Line 15: this.detailedInfo = detailedInfo; Line 16: } Line 32: /** Line 33: * Resolves the detailed information into VdcBll messages Line 34: * @return Line 35: */ Line 36: public abstract List<VdcBllMessages> resolveMessage(); The conversion from the details provided by the authenticator to VdcBllMessages should be done by the backend, not by the authenticator. http://gerrit.ovirt.org/#/c/15596/32/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GroupMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GroupMapper.java: Line 27: public static Group map(LdapGroup entity, Group template) { Line 28: Group model = template != null ? template : new Group(); Line 29: model.setName(entity.getname()); Line 30: model.setId(entity.getid().toString()); Line 31: model.setExternalId(entity.getid().toString()); Please remove this line, in the RESTAPI representations directory users and groups only have one identifier, the external one, and it is stored in "Id", not in "ExternalId". Line 32: if (!StringUtils.isEmpty(entity.getdomain())) { Line 33: Domain dom = new Domain(); Line 34: dom.setId(new Guid(entity.getdomain().getBytes(), true).toString()); Line 35: model.setDomain(dom); http://gerrit.ovirt.org/#/c/15596/32/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/UserMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/UserMapper.java: Line 40: @Mapping(from = LdapUser.class, to = User.class) Line 41: public static User map(LdapUser entity, User template) { Line 42: User model = template != null ? template : new User(); Line 43: model.setName(entity.getName()); Line 44: model.setExternalId(entity.getUserId().toString()); Please remove this line. Line 45: model.setUserName(entity.getUserName()); Line 46: model.setId(entity.getUserId().toString()); Line 47: model.setLastName(entity.getSurName()); Line 48: model.setEmail(entity.getEmail()); -- 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: 32 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: mooli tayer <mta...@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
