Yair Zaslavsky has posted comments on this change. Change subject: aaa: Change builtin authenticators and directories initialization ......................................................................
Patch Set 5: (8 comments) http://gerrit.ovirt.org/#/c/25741/5/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 122: ExtensionsManager.getInstance().load(authConfig); Line 123: Line 124: Properties dirConfig = new Properties(); Line 125: dirConfig.put(ExtensionsManager.CLASS, "org.ovirt.engine.extensions.aaa.builtin.internal.InternalDirectory"); Line 126: dirConfig.put(ExtensionsManager.PROVIDES, "org.ovirt.engine.core.authorization"); > are you sure the provides is based on class name and not simple name of Aut PROVIDES hold entry of autheorization/authentication, didn't understand your comment. CLASS holds the full class name. Line 127: dirConfig.put(ExtensionsManager.ENABLED, "true"); Line 128: dirConfig.put(ExtensionsManager.MODULE, "org.ovirt.engine.extensions.builtin"); Line 129: dirConfig.put("config.authz.user.name", Config.<String> getValue(ConfigValues.AdminUser)); Line 130: dirConfig.put(ExtensionsManager.NAME, "builtin-authz-internal"); Line 126: dirConfig.put(ExtensionsManager.PROVIDES, "org.ovirt.engine.core.authorization"); Line 127: dirConfig.put(ExtensionsManager.ENABLED, "true"); Line 128: dirConfig.put(ExtensionsManager.MODULE, "org.ovirt.engine.extensions.builtin"); Line 129: dirConfig.put("config.authz.user.name", Config.<String> getValue(ConfigValues.AdminUser)); Line 130: dirConfig.put(ExtensionsManager.NAME, "builtin-authz-internal"); > move this up so config. will be at same block not sure i understood what to move up. for dirConfig we have only one entry that the prefix of its key is "config" - the "configuathz.user.name" - do u want it to be first/last at the dirConfig part? Line 131: dirConfig.put("ovirt.engine.aaa.authz.profile.name", "internal"); Line 132: ExtensionsManager.getInstance().load(dirConfig); Line 133: } Line 134: Line 127: dirConfig.put(ExtensionsManager.ENABLED, "true"); Line 128: dirConfig.put(ExtensionsManager.MODULE, "org.ovirt.engine.extensions.builtin"); Line 129: dirConfig.put("config.authz.user.name", Config.<String> getValue(ConfigValues.AdminUser)); Line 130: dirConfig.put(ExtensionsManager.NAME, "builtin-authz-internal"); Line 131: dirConfig.put("ovirt.engine.aaa.authz.profile.name", "internal"); > why do we need profile name at authz... I will start notice these more now Well, I assume that in several directory implementations I will require to know what is the profile name. For example, in the patch I previously squashed, I used the profile name inside KerbeorsLdapDirectory as the "domain name". If I don't use the context for it, then I can set the profile name during profile creation. Line 132: ExtensionsManager.getInstance().load(dirConfig); Line 133: } Line 134: Line 135: private void createKerberosLdapAAAConfigurations() { Line 143: authConfig.put(ExtensionsManager.ENABLED, "true"); Line 144: authConfig.put(ExtensionsManager.MODULE, "org.ovirt.engine.extensions.builtin"); Line 145: authConfig.put(ExtensionsManager.NAME, String.format("builtin-authn-%1$s", domain)); Line 146: authConfig.put("ovirt.engine.aaa.authn.profile.name", domain); Line 147: authConfig.put("ovirt.engine.aaa.authn.authz.plugin", String.format("builtin-authz-%1$s", domain)); > I would have added specific config so that we can implicitly have a key to please elaborate. the specific entry at vdc_options holds a domains list. why would i want to set it? i set a single domain from it, as i assume profile name == domain. Line 148: ExtensionsManager.getInstance().load(authConfig); Line 149: Line 150: Properties dirConfig = new Properties(); Line 151: dirConfig.put(ExtensionsManager.CLASS, http://gerrit.ovirt.org/#/c/25741/5/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthenticator.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthenticator.java: Line 16: public void authenticate(String user, String password) { Line 17: String adminUser = ((Properties)context.get(ExtensionProperties.CONFIGURATION)).getProperty("config.authn.user.name"); Line 18: String adminPassword = ((Properties)context.get(ExtensionProperties.CONFIGURATION)).getProperty("config.authn.user.password"); Line 19: if (!ObjectUtils.equals(user, adminUser) Line 20: || !ObjectUtils.equals(password, adminPassword)) { > hmmm... why not: Done Line 21: throw new AAAExtensionException(AAAExtensionException.AAAExtensionError.INCORRECT_CREDENTIALS, ""); Line 22: } Line 23: } Line 24: http://gerrit.ovirt.org/#/c/25741/5/backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManager.java File backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManager.java: Line 118: } Line 119: Line 120: private ExtensionsManager() { Line 121: for (File directory : EngineLocalConfig.getInstance().getExtensionsDirectories()) { Line 122: loadDirectory(directory); > one time used funciton^ why not perform the loop here? Done Line 123: } Line 124: } Line 125: Line 126: public void load(Properties configuration) { Line 139: } Line 140: Line 141: private synchronized void loadImpl(Properties props, File confFile) { Line 142: ExtensionEntry entry = new ExtensionEntry(props, confFile); Line 143: ExtensionEntry alreadyLoaded = loadedEntries.get(entry.getName()); > alreadyLoaded implies boolean... please rename to alreadyLoadedEntry or som Done Line 144: if (alreadyLoaded != null) { Line 145: throw new ConfigurationException(String.format("Could not load the configuration %1$s. %2$s", Line 146: String.format("'%1$s' %2$s", entry.getName(), entry.file != null ? "from file " + entry.file.getAbsolutePath(): ""), Line 147: alreadyLoaded.file != null ? String.format("The already loaded file %1$s contains a configuration with the same name", Line 148: alreadyLoaded.file.getAbsolutePath()) Line 149: : "")); Line 150: } Line 151: loadedEntries.put(entry.getName(), entry); Line 152: activate(entry); > one time used function, right? why not perform it here? Done Line 153: } Line 154: Line 155: private void loadDirectory(File directory) throws ConfigurationException { Line 156: // Check that the folder that contains the configuration files exists: -- To view, visit http://gerrit.ovirt.org/25741 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id8513cb992c5becef7e83c04a8da8bc7f1622348 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[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
