Alon Bar-Lev has posted comments on this change. Change subject: 6. [WIP] core: Introducing AuthenticationProfileRepository ......................................................................
Patch Set 9: (8 comments) http://gerrit.ovirt.org/#/c/24366/9/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthenticationProfileRepository.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthenticationProfileRepository.java: Line 11: Line 12: public class AuthenticationProfileRepository { Line 13: Line 14: private static final String AUTHN_PROFILE = "ovirt.engine.aaa.authn.profile.name"; Line 15: private static final String AUTHN_SERVICE = "org.ovirt.engine.core.authentication"; please rename services to org.ovirt.engine.extension.services.xxxx Line 16: private static final String AUTHN_AUTHZ_PLUGIN = "ovirt.engine.aaa.authn.authz.plugin"; Line 17: Line 18: Line 19: private static volatile AuthenticationProfileRepository instance = null; Line 30: } Line 31: return instance; Line 32: } Line 33: Line 34: public void createProfiles() throws ConfigurationException { shouldn't this be called by private constructor? what is the meaning of calling it twice on singleton? Line 35: Line 36: // Get the extensions that correspond to authn (authentication) service. Line 37: // For each extension - get the relevant authn extension. Line 38: Line 35: Line 36: // Get the extensions that correspond to authn (authentication) service. Line 37: // For each extension - get the relevant authn extension. Line 38: Line 39: List<ExtensionEntry> authnExtensions = ConfigurationLoader.getInstance().getExtensionsByService(AUTHN_SERVICE); any reason for this temp variable "authnExtensions"? :) Line 40: Line 41: Line 42: for (ExtensionEntry authnExtension: authnExtensions) { Line 43: String pluginName = authnExtension.getConfig().getString(AUTHN_AUTHZ_PLUGIN); Line 39: List<ExtensionEntry> authnExtensions = ConfigurationLoader.getInstance().getExtensionsByService(AUTHN_SERVICE); Line 40: Line 41: Line 42: for (ExtensionEntry authnExtension: authnExtensions) { Line 43: String pluginName = authnExtension.getConfig().getString(AUTHN_AUTHZ_PLUGIN); this too :) Line 44: ExtensionEntry authzExtension = ConfigurationLoader.getInstance().getExtensionByName(pluginName); Line 45: if (authzExtension == null) { Line 46: throw new ConfigurationException("The authentication extension " + authnExtension.getName() Line 47: + " does not have a matching authorization extension"); Line 48: Line 49: } Line 50: Line 51: Directory directory = DirectoryManager.getInstance().parseDirectory(authzExtension.getConfig()); Line 52: Authenticator authenticator = why the indent? why if there is an error these classes do not throw exception? they should so we can eliminate this code here. Line 53: AuthenticatorManager.getInstance().parseAuthenticator(authzExtension.getConfig()); Line 54: if (directory == null) { Line 55: throw new ConfigurationException("No directory could be obtained for authorization plugin " Line 56: + authzExtension.getName()); Line 64: throw new ConfigurationException("Profile name could not be obtained from configuration of authentication plugin " Line 65: + authnExtension.getName()); Line 66: } Line 67: Line 68: AuthenticationProfile profile = new AuthenticationProfile(profileName, authenticator, directory); why do you need temp variable "profile"? :) Line 69: profiles.put(profileName, profile); Line 70: } Line 71: } Line 72: Line 65: + authnExtension.getName()); Line 66: } Line 67: Line 68: AuthenticationProfile profile = new AuthenticationProfile(profileName, authenticator, directory); Line 69: profiles.put(profileName, profile); ok... if I understand it correctly, it all boils into: for (ExtensionEntry authnExtension: ConfigurationLoader.getInstance().getExtensionsByService(AUTHN_SERVICE)) { // getExtensionByName should throw an exception if name is not found. // you can add optional boolean parameter for these would like to get null // I expect the authentication to know what profile he is // and I expect the profile to have getName() that ask its embedded // authentication... // so we can simply do: // these should raise exception if unable to parse // also, consider getting extension and not config, to simplify // code registerProfile(new AuthenticationProfile( AuthenticatorManager.getInstance().parseAuthenticator(authzExtension.getConfig()), DirectoryManager.getInstance().parseDirectory( ConfigurationLoader.getInstance().getExtensionByName( authnExtension.getConfig().getString(AUTHN_AUTHZ_PLUGIN) ).getConfig() ) )); } *BUT* I really do not understand why we use singletons all over... why the parseDirectory is not static or within standard object. Line 70: } Line 71: } Line 72: Line 73: /** Line 97: * the profile to register Line 98: */ Line 99: public void registerProfile(String name, AuthenticationProfile profile) { Line 100: profiles.put(name, profile); Line 101: } why do we need register? where can it come from? name should be acquired from profile. -- 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: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
