Alon Bar-Lev has posted comments on this change. Change subject: aaa: Added mapper usage ......................................................................
Patch Set 5: (5 comments) http://gerrit.ovirt.org/#/c/26970/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java: Line 349: ) Line 350: ); Line 351: } Line 352: result = outputMap.<ExtMap> get(Authn.InvokeKeys.AUTH_RECORD); Line 353: what about the map user? it should be in this command as well, no? Line 354: } Line 355: return result; Line 356: } Line 357: http://gerrit.ovirt.org/#/c/26970/5/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapMapper.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapMapper.java: Line 11: public class KerberosLdapMapper implements Extension { Line 12: Line 13: private ExtMap context; Line 14: private Properties config; Line 15: private Object authzName; why object? Line 16: Line 17: @Override Line 18: public void invoke(ExtMap input, ExtMap output) { Line 19: try { Line 49: ).mput( Line 50: Base.ContextKeys.BUILD_INTERFACE_VERSION, Line 51: Base.INTERFACE_VERSION_CURRENT); Line 52: config = context.<Properties> get(Base.ContextKeys.CONFIGURATION); Line 53: authzName = config.get("org.ovirt.engine.config.authz.name"); it is not authz name at all, it is default suffix. but I thought we agree that adding default suffix is so common use case that we add it to authn plugin. why have you decided to do this in specific extension? anyway, this extension should be generic if we add it... it should be based on regular expression.... configuration should be something like: suffix = domain1 type.1 = re pattern.1= ^(?<name>[^@]*)$ replace.1 = ${name}@${suffix} type.2 = tolower so it may be usable for other use cases as well. Line 54: } Line 55: Line 56: private void doMapping(ExtMap input, ExtMap output) { Line 57: ExtMap authRecord = input.<ExtMap> get(Authn.InvokeKeys.AUTH_RECORD); Line 54: } Line 55: Line 56: private void doMapping(ExtMap input, ExtMap output) { Line 57: ExtMap authRecord = input.<ExtMap> get(Authn.InvokeKeys.AUTH_RECORD); Line 58: ExtMap outRecord = (ExtMap) authRecord.clone(); interesting if we need clone... I was thinking of doing all by reference. what do you think? Line 59: if (!authRecord.<String> get(Authn.AuthRecord.PRINCIPAL).contains("@")) { Line 60: outRecord.put(Authn.AuthRecord.PRINCIPAL, authRecord.<String> get(Authn.AuthRecord.PRINCIPAL) + "@" Line 61: + authzName); Line 62: http://gerrit.ovirt.org/#/c/26970/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 90: Line 91: public ExtensionProxy getExtensionByName(String name) throws ConfigurationException { Line 92: ExtensionEntry entry = loadedEntries.get(name); Line 93: if (entry == null) { Line 94: return null; so you need to be consistent, change anything that returns nothing? for example null also if extension is not activated. squash this hank to the extension manager changes. Line 95: Line 96: } Line 97: if (!entry.activated) { Line 98: throw new ConfigurationException(String.format( -- To view, visit http://gerrit.ovirt.org/26970 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib5ac853e9011bb6118796a4cd14c0b7425308f3b 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: [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
