Alon Bar-Lev has posted comments on this change. Change subject: aaa: Change builtin authenticators and directories initialization ......................................................................
Patch Set 2: (6 comments) http://gerrit.ovirt.org/#/c/25741/2/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 116: authConfig.put(ExtensionManager.NAME, "builtin-authn-internal"); Line 117: authConfig.put("ovirt.engine.aaa.authn.profile.name", "internal"); Line 118: authConfig.put("ovirt.engine.aaa.authn.authz.plugin", "builtin-authz-internal"); Line 119: authConfig.put("ovirt.engine.aaa.authn.user", Config.<String> getValue(ConfigValues.AdminUser)); Line 120: authConfig.put("ovirt.engine.aaa.authn.password", Config.<String> getValue(ConfigValues.AdminPassword)); ok... sorry for the late comment... but we need a new optional extension manager property... ovirt.engine.extension.sensitiveKeys in this case it should hold ovirt.engine.aaa.authn.password. this will enable us not to log sensitive extension keys in debug. please rename these provider specific attribute to own name space. config.authn.user.name config.authn.user.password as we done in generic's design Line 121: results.add(authConfig); Line 122: Line 123: Properties dirConfig = new Properties(); Line 124: dirConfig.put(ExtensionManager.CLASS, "org.ovirt.engine.extensions.aaa.builtin.internal.InternalDirectory"); http://gerrit.ovirt.org/#/c/25741/2/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalDirectory.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalDirectory.java: Line 32 Line 33 Line 34 Line 35 Line 36 I would have put the above in config as well.... however, first you need to figure out why you need to use external id... and how is it possible, as now that we are split, we have our own id which is mapped by the engine using <profile, id> -> id so how can we map to admin user using current api? we should have attribute on user or any other kind of solution. http://gerrit.ovirt.org/#/c/25741/2/backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionManager.java File backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionManager.java: Line 136: } Line 137: activate(); Line 138: } Line 139: Line 140: private void load(ExtensionEntry entry) { bad name that matches public interface. and it should not be used see bellow. Line 141: ExtensionEntry alreadyLoaded = loadedEntries.get(entry.getName()); Line 142: if (alreadyLoaded != null) { Line 143: throw new ConfigurationException(String.format("Could not load the configuration %1$s. %2$s", Line 144: String.format("'%1$s' %2$s", entry.getName(), entry.file != null ? "from file " + entry.file.getAbsolutePath(): ""), Line 174: if (file.getName().endsWith(".properties")) { Line 175: try { Line 176: ExtensionEntry entry = Line 177: new ExtensionEntry(file); Line 178: load(entry); simpler implementation.... use: public void load(Properties props) { _load(props, null); } public void load(File file) { try (InputStream...) { _load(props, file); } } make the ExtensionEntry constructor use only Properties and get optional (null) file. the name of the internal/private method of load should not match the public API to avoid confusion, find the java naming to do that. Line 179: } catch (IOException exception) { Line 180: throw new ConfigurationException(String.format("Can't load object configuration file '%1$s'", Line 181: file.getAbsolutePath())); Line 182: } Line 233: for (ExtensionEntry entry : loadedEntries.values()) { Line 234: //Engine local config might override the enabled property of the configuration Line 235: // if a proper entry exists at the engine config. Line 236: entry.enabled = config.getBoolean(ENGINE_EXTENSION_ENABLED + entry.getName(), entry.enabled); Line 237: if (entry.enabled && entry.extension == null) { now we can have race... maybe we should synchronize the load. Line 238: try { Line 239: entry.extension = (Extension) lookupService( Line 240: Extension.class, Line 241: entry.getConfig().getProperty(CLASS), Line 261: } Line 262: } Line 263: } Line 264: Line 265: public void logEnabledExtensions() { > Changed logEnabledExtensions to public as log it is called from InitOnStart ok Line 266: log.info("Start of enabled extensions list"); Line 267: for (ExtensionEntry entry: loadedEntries.values()) { Line 268: if (entry.extension != null) { Line 269: 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: 2 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
