Alon Bar-Lev has posted comments on this change. Change subject: aaa: Change builtin authenticators and directories initialization ......................................................................
Patch Set 5: (12 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 Authentication / 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"); 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 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 as I can see something actually happens. authn has profile name and has reference to authz so you got all information without this. and.... two or more separate authn can reference the same authz. 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 the vdc_table which may be other name than the profile. Line 148: ExtensionsManager.getInstance().load(authConfig); Line 149: Line 150: Properties dirConfig = new Properties(); Line 151: dirConfig.put(ExtensionsManager.CLASS, Line 153: dirConfig.put(ExtensionsManager.PROVIDES, "org.ovirt.engine.core.authorization"); Line 154: dirConfig.put(ExtensionsManager.ENABLED, "true"); Line 155: dirConfig.put(ExtensionsManager.MODULE, "org.ovirt.engine.extensions.builtin"); Line 156: dirConfig.put(ExtensionsManager.NAME, String.format("builtin-authz-%1$s", domain)); Line 157: dirConfig.put("ovirt.engine.aaa.authz.profile.name", domain); again... I do not think that this^ is required. Line 158: ExtensionsManager.getInstance().load(dirConfig); Line 159: } Line 160: } Line 161: 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 14: Line 15: @Override 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"); store these as member during init? although not that important, it is good as is. Line 19: if (!ObjectUtils.equals(user, adminUser) Line 20: || !ObjectUtils.equals(password, adminPassword)) { Line 21: throw new AAAExtensionException(AAAExtensionException.AAAExtensionError.INCORRECT_CREDENTIALS, ""); Line 22: } 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: !(equals && equals) :) 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? 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 something like that. 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 142: ExtensionEntry entry = new ExtensionEntry(props, confFile); Line 143: ExtensionEntry alreadyLoaded = loadedEntries.get(entry.getName()); 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(): ""), please indent.... String.format( format, arg1, arg2, ... ) makes it much easier to understand. also, if you have long argument, indent it: String.format( format, ( long argument multilined ), arg2, ... ) but this is way too complex... I think that display of null within file for builtin extensions is not a problem... 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 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? 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: Line 246: } Line 247: } Line 248: } Line 249: Line 250: public void logEnabledExtensions() { dump() ? :) Line 251: log.info("Start of enabled extensions list"); Line 252: for (ExtensionEntry entry: loadedEntries.values()) { Line 253: if (entry.extension != null) { Line 254: Map<ExtensionProperties, Object> context = entry.extension.getContext(); -- 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 <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
