Yair Zaslavsky has posted comments on this change. Change subject: aaa: Change builtin authenticators and directories initialization ......................................................................
Patch Set 1: (10 comments) http://gerrit.ovirt.org/#/c/25741/1/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 11: Line 12: import org.ovirt.engine.core.aaa.AuthenticationProfileRepository; Line 13: import org.ovirt.engine.extensions.aaa.builtin.kerberosldap.KerberosManager; Line 14: import org.ovirt.engine.extensions.aaa.builtin.kerberosldap.UsersDomainsCacheManagerService; Line 15: import org.ovirt.engine.extensions.aaa.builtin.kerberosldap.LdapBrokerUtils; > right, i forgot to add a comment on the patch that i still have to clear th Done Line 16: import org.ovirt.engine.core.bll.dwh.DwhHeartBeat; Line 17: import org.ovirt.engine.core.bll.gluster.GlusterJobsManager; Line 18: import org.ovirt.engine.core.bll.job.ExecutionHandler; Line 19: import org.ovirt.engine.core.bll.network.MacPoolManager; Line 56: // TODO: remove this later, and rely only on the custom and built in extensions directories configuration Line 57: Line 58: List<Properties> configurations = createInternalConfigurations(); Line 59: configurations.addAll(createKerberosLdapConfigurations()); Line 60: ExtensionManager.getInstance().load(configurations); > better to call load twice... and drop the temp var Done Line 61: AuthenticationProfileRepository.getInstance(); Line 62: KerberosManager.getInstance(); Line 63: UsersDomainsCacheManagerService.getInstance().init(); Line 64: DbUserCacheManager.getInstance().init(); Line 58: List<Properties> configurations = createInternalConfigurations(); Line 59: configurations.addAll(createKerberosLdapConfigurations()); Line 60: ExtensionManager.getInstance().load(configurations); Line 61: AuthenticationProfileRepository.getInstance(); Line 62: KerberosManager.getInstance(); > Needs to be moved to LdapKerberosAuthenticator.init() Done Line 63: UsersDomainsCacheManagerService.getInstance().init(); Line 64: DbUserCacheManager.getInstance().init(); Line 65: AsyncTaskManager.getInstance().initAsyncTaskManager(); Line 66: ResourceManager.getInstance().init(); Line 59: configurations.addAll(createKerberosLdapConfigurations()); Line 60: ExtensionManager.getInstance().load(configurations); Line 61: AuthenticationProfileRepository.getInstance(); Line 62: KerberosManager.getInstance(); Line 63: UsersDomainsCacheManagerService.getInstance().init(); > same. Done Line 64: DbUserCacheManager.getInstance().init(); Line 65: AsyncTaskManager.getInstance().initAsyncTaskManager(); Line 66: ResourceManager.getInstance().init(); Line 67: OvfDataUpdater.getInstance().initOvfDataUpdater(); Line 108: Line 109: new DwhHeartBeat().init(); Line 110: } Line 111: Line 112: private List<Properties> createInternalConfigurations() { > ok, continuing my previous comment... As we agreed, we will read from context at internalAuthenticator/Directory. Line 113: List<Properties> results = new ArrayList<>(); Line 114: Properties authConfig = new Properties(); Line 115: authConfig.put(ExtensionManager.CLASS, "org.ovirt.extensions.builtin.ldapkerberos.LdapKerberosAuthenticator"); Line 116: authConfig.put(ExtensionManager.PROVIDES, "org.ovirt.engine.core.authentication"); Line 152: dirConfig.put(ExtensionManager.PROVIDES, "org.ovirt.engine.core.authorization"); Line 153: dirConfig.put(ExtensionManager.ENABLED, true); Line 154: dirConfig.put(ExtensionManager.MODULE, "org.ovirt.engine.extensions.builtin"); Line 155: dirConfig.put(ExtensionManager.NAME, String.format("builtin-authz-%1$s", domain)); Line 156: dirConfig.put("org.ovirt.engine.aaa.authz.profile.name", domain); > you should probably add a variable for the domain name so that the provider profile == domain. for now we will use vdc_options inside the ldapkerberos authenticator and provider. Line 157: results.add(authConfig); Line 158: } Line 159: return results; Line 160: http://gerrit.ovirt.org/#/c/25741/1/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 54: } Line 55: } Line 56: Line 57: public ExtensionEntry(Properties props) { Line 58: this.file = null; > no need for this.file = null as it will be null. Done Line 59: context = new EnumMap<>(ExtensionProperties.class); Line 60: load(props); Line 61: } Line 62: Line 88: return (Properties) context.get(ExtensionProperties.CONFIGURATION); Line 89: } Line 90: Line 91: private void load(Properties props) { Line 92: enabled = props.get(ENABLED) != null ? Boolean.parseBoolean((String) props.get(ENABLED)) : true; > remove the (String) cast? Done Line 93: context.put(ExtensionProperties.CONFIGURATION, props); Line 94: context.put(ExtensionProperties.NAME, props.getProperty(NAME)); Line 95: context.put(ExtensionProperties.PROVIDES, props.getProperty(PROVIDES)); Line 96: } Line 134: Line 135: public void load(Collection<Properties> configurations) { Line 136: if (configurations == null) { Line 137: return; Line 138: } > hmmm... so why anyone will call it? better just to wait for null pointer ex Done Line 139: for (Properties configuration : configurations) { Line 140: ExtensionEntry entry = Line 141: new ExtensionEntry(configuration); Line 142: ExtensionEntry alreadyLoaded = loadedEntries.get(entry.getName()); Line 139: for (Properties configuration : configurations) { Line 140: ExtensionEntry entry = Line 141: new ExtensionEntry(configuration); Line 142: ExtensionEntry alreadyLoaded = loadedEntries.get(entry.getName()); Line 143: if (alreadyLoaded != null) { > it should be the same. Done Line 144: throw new ConfigurationException(String.format("Could not load the configuration '%1$s'. %2%s", Line 145: entry.getName(), Line 146: alreadyLoaded.file != null ? String.format("The already loaded file %1$s contains a configuration with the same name", Line 147: alreadyLoaded.file.getAbsolutePath()) -- 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: 1 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
