Yair Zaslavsky has posted comments on this change. Change subject: 9. [WIP] core: Introducing AuthenticationProfileRepository ......................................................................
Patch Set 26: (6 comments) http://gerrit.ovirt.org/#/c/24366/26//COMMIT_MSG Commit Message: Line 8: Line 9: This class will replace AuthenticationProfileManager in loading the profiles. Line 10: The class will construct a profile based on two separate configurations - Line 11: one of authenticator (authn) and one of directory (authz) Line 12: > please add: Done Line 13: Change-Id: If375fccea98544c64d9ec41cc9dbcb855bf02fb7 http://gerrit.ovirt.org/#/c/24366/26/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/Authenticator.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/Authenticator.java: Line 19: } Line 20: Line 21: public String getProfileName() { Line 22: Properties config = (Properties) context.get(ExtensionProperties.CONFIGURATION); Line 23: return config.getProperty("ovirt.engine.aaa.authz.profile.name"); > why do you need config temp variable? Done Line 24: } Line 25: Line 26: Line 27: @Override Line 37: Line 38: protected Authenticator() { Line 39: } Line 40: Line 41: protected Map<Extension.ExtensionProperties, Object> context; > why member is not on top? maybe I missed yet another convention :) Done Line 42: http://gerrit.ovirt.org/#/c/24366/26/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/internal/InternalAuthenticator.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/internal/InternalAuthenticator.java: Line 27: } Line 28: Line 29: @Override Line 30: public void init() { Line 31: } > move constructor statements into init, constructor should not do anything. Not sure I follow, there is no CTOR here. did you confuse with the method "authenticate"? http://gerrit.ovirt.org/#/c/24366/26/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/aaa/DirectoryUtils.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/aaa/DirectoryUtils.java: Line 30: return sb.toString(); Line 31: } Line 32: Line 33: public static Directory getDirectoryByName(String profileName) { Line 34: for (AuthenticationProfile profile : AuthenticationProfileRepository.getInstance().getProfiles()) { > why can't this function reside within the AuthetnciationProfileRepository? this was added in previous patches before introduction of AuthenticationProfileRepository. it can indeed moved there. Line 35: if (profile.getDirectory().getProfileName().equals(profileName)) { Line 36: return profile.getDirectory(); Line 37: } Line 38: } http://gerrit.ovirt.org/#/c/24366/26/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java: Line 38: Line 39: private DbUserCacheManager() { Line 40: for (AuthenticationProfile profile : AuthenticationProfileRepository.getInstance().getProfiles()) { Line 41: directoriesMap.put(profile.getDirectory().getName(), profile.getDirectory()); Line 42: } > I am unsure why you need to cache yet another instance of this map... just Done Line 43: Line 44: } Line 45: Line 46: public void init() { -- To view, visit http://gerrit.ovirt.org/24366 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If375fccea98544c64d9ec41cc9dbcb855bf02fb7 Gerrit-PatchSet: 26 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [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
