Alon Bar-Lev has posted comments on this change. Change subject: 9. [WIP] core: Introducing AuthenticationProfileRepository ......................................................................
Patch Set 26: (8 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: Topic: AAA 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? 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 :) Line 42: http://gerrit.ovirt.org/#/c/24366/26/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/Directory.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/Directory.java: Line 16: * Line 17: */ Line 18: private static final long serialVersionUID = -8724317446083142917L; Line 19: Line 20: public String getName() { > I kept the methods for convenience, and removed the fields (I rely on the c that's perfectly ok Line 21: return (String) context.get(ExtensionProperties.NAME); Line 22: } Line 23: Line 24: public String getProfileName() { 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. 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? so it contains a map for your to use, dropping this addition in utils? 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 query the repository at each iteration. Line 43: Line 44: } Line 45: Line 46: public void init() { http://gerrit.ovirt.org/#/c/24366/26/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitBackendServicesOnStartupBean.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitBackendServicesOnStartupBean.java: Line 82: authenticator.init(); Line 83: Line 84: AuthenticationProfile profile = new AuthenticationProfile(authenticator, directory); Line 85: Line 86: AuthenticationProfileRepository.getInstance().registerProfile(profile); why do we need the above? we can put the service for these and configuration file so it will be loaded using our extension manager. Line 87: } Line 88: Line 89: ExtensionManager.getInstance().load(EngineLocalConfig.getInstance()); Line 90: AuthenticationProfileRepository.getInstance(); -- 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
