Alon Bar-Lev has posted comments on this change. Change subject: aaa: Stopping to use proxies ......................................................................
Patch Set 9: (25 comments) http://gerrit.ovirt.org/#/c/26602/9/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthenticationFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthenticationFilter.java: I am unsure we should leave this in tree and not have own package. Line 1: package org.ovirt.engine.core.aaa; Line 2: Line 3: import java.io.IOException; Line 4: import java.util.ArrayDeque; http://gerrit.ovirt.org/#/c/26602/9/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthzUtils.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthzUtils.java: Line 45: Line 46: public ExtMap getCommandParameters(); Line 47: } Line 48: Line 49: private static ExtMap generateQuery(String query, ExtUUID queryEntity) { please add big fat comment of why do we do the reverse parsing... a note that we should attend to fix the search backend for ease of use. Line 50: String queryPrefix = Authz.QueryEntity.PRINCIPAL.equals(queryEntity) ? USERS_QUERY_PREFIX : GROUPS_QUERY_PREFIX; Line 51: ExtMap result = new ExtMap(); Line 52: result.mput( Line 53: Authz.InvokeKeys.QUERY_ENTITY, Line 118: } Line 119: Line 120: private static ExtKey mapRecordKey(String key) { Line 121: return attributesToKeys.get(key); Line 122: } can we split the search parsing from the others into own class that hopefully in future will be removed? Line 123: Line 124: public static String getName(ExtensionProxy proxy) { Line 125: return proxy.getContext().<String> get(Base.ContextKeys.INSTANCE_NAME); Line 126: } Line 124: public static String getName(ExtensionProxy proxy) { Line 125: return proxy.getContext().<String> get(Base.ContextKeys.INSTANCE_NAME); Line 126: } Line 127: Line 128: public static DirectoryUser findUser(final ExtensionProxy extension, final String name) { can we sync terms with api? fetchPrincipalRecord? Line 129: return mapPrincipalRecord(extension, extension.invoke(new ExtMap().mput( Line 130: Base.InvokeKeys.COMMAND, Line 131: Authz.InvokeCommands.FETCH_PRINCIPAL_RECORD Line 132: ).mput( Line 133: Authn.InvokeKeys.AUTH_RECORD, Line 134: new ExtMap().mput( Line 135: Authn.AuthRecord.PRINCIPAL, Line 136: name Line 137: ) you cannot construct the auth record your-self, this must be of output of authentication, so that authn can pass info to authz. Line 138: )).<ExtMap> get(Authz.InvokeKeys.PRINCIPAL_RECORD)); Line 139: } Line 140: Line 141: public static List<DirectoryUser> findUsersByQuery(final ExtensionProxy extension, final String query) { Line 137: ) Line 138: )).<ExtMap> get(Authz.InvokeKeys.PRINCIPAL_RECORD)); Line 139: } Line 140: Line 141: public static List<DirectoryUser> findUsersByQuery(final ExtensionProxy extension, final String query) { I prefer you call explicit conversion between query->ext and then call find users, so that the interface of this class will be kept. Line 142: return queryUsers(extension, generateQuery(query, Authz.QueryEntity.PRINCIPAL)); Line 143: } Line 144: Line 145: public static List<DirectoryUser> findUsersByIds(final ExtensionProxy extension, final List<String> ids) { Line 144: Line 145: public static List<DirectoryUser> findUsersByIds(final ExtensionProxy extension, final List<String> ids) { Line 146: return queryUsers(extension, Line 147: generateQuery(generateIdsListQuery(Authz.QueryEntity.PRINCIPAL, ids), Authz.QueryEntity.PRINCIPAL)); Line 148: } I really do not want to see this unlimited nature of functions any more. As you do not know how many ids you cannot have a function without iteration, this is why you should either have callback or iterator. Also, you must limit each search with QUERY_MAX_FILTER_SIZE. Line 149: Line 150: public static DirectoryUser findUserById(final ExtensionProxy extension, final String id) { Line 151: List<DirectoryUser> users = findUsersByIds(extension, Arrays.asList(id)); Line 152: if (users.isEmpty()) { Line 216: return extMap; Line 217: } Line 218: }); Line 219: return directoryUsers; Line 220: } this implementation assumes all can be contained in memory, which is not good for large processing. Line 221: Line 222: private static List<DirectoryGroup> populateGroups(final ExtensionProxy extension, Line 223: final ExtUUID command, Line 224: final ExtMap extMap) { Line 242: } Line 243: Line 244: }); Line 245: return directoryGroups; Line 246: } this implementation assumes all can be contained in memory, which is not good for large processing. Line 247: Line 248: private static void queryImpl(final ExtensionProxy extension, final QueryResultHandler handler) { Line 249: Object opaque = null; Line 250: try { Line 301: directoryGroups.add(mapGroupRecord(extension, group)); Line 302: } Line 303: } Line 304: } Line 305: return directoryUser; can we drpo this DirectoryUser in favour of the ExtMap? Line 306: } Line 307: Line 308: private static DirectoryGroup mapGroupRecord(final ExtensionProxy extension, final ExtMap group) { Line 309: DirectoryGroup directoryGroup = null; http://gerrit.ovirt.org/#/c/26602/9/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/DirectoryProxy.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/DirectoryProxy.java: Line 58: Line 59: @Override Line 60: public List<DirectoryGroup> queryGroups(String query) { Line 61: // TODO Auto-generated method stub Line 62: return null; if you keep the proxy, why do you need the utils? Line 63: } http://gerrit.ovirt.org/#/c/26602/9/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 145: authConfig.put(Base.ConfigKeys.MODULE, "org.ovirt.engine.extensions.builtin"); Line 146: authConfig.put(Base.ConfigKeys.CLASS, Line 147: "org.ovirt.engine.extensions.aaa.builtin.kerberosldap.KerberosLdapAuthenticator"); Line 148: authConfig.put("ovirt.engine.aaa.authn.profile.name", domain); Line 149: authConfig.put(Authz.class.getName(), domain); why is this needed? Line 150: authConfig.put("ovirt.engine.aaa.authn.authz.plugin", domain); Line 151: ExtensionsManager.getInstance().load(authConfig); Line 152: Line 153: Properties dirConfig = new Properties(); http://gerrit.ovirt.org/#/c/26602/9/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthenticator.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthenticator.java: Line 8 Line 9 Line 10 Line 11 Line 12 how can we use the config here? please pass the required parameters within the initbean as properties. Line 84 Line 85 Line 86 Line 87 Line 88 I do not think this function is required. Line 87 Line 88 Line 89 Line 90 Line 91 should be also ':' as suffix please rename to authn Line 1: package org.ovirt.engine.extensions.aaa.builtin.kerberosldap; Line 2: Line 3: import java.io.UnsupportedEncodingException; Line 4: import java.net.URLDecoder; Line 30: private LdapBroker broker; Line 31: private ExtMap context; Line 32: private Properties configuration; Line 33: private static volatile Map<String, String> passwordChangeMsgPerDomain = null; Line 34: private static volatile Map<String, String> passwordChangeUrlPerDomain = null; please avoid using static if not initialized in static context. Line 35: Line 36: Line 37: Line 38: public KerberosLdapAuthenticator() { Line 66: context.mput( Line 67: Base.ContextKeys.AUTHOR, Line 68: "The oVirt Project").mput( Line 69: Base.ContextKeys.EXTENSION_NAME, Line 70: "Internal Authentication (Built-in)" Kerberos/LDAP? Authn? Line 71: ).mput( Line 72: Base.ContextKeys.LICENSE, Line 73: "ASL 2.0" Line 74: ).mput( Line 86: synchronized (KerberosLdapAuthenticator.class) { Line 87: if (passwordChangeMsgPerDomain == null) { Line 88: passwordChangeMsgPerDomain = new HashMap<>(); Line 89: passwordChangeUrlPerDomain = new HashMap<>(); Line 90: String[] pairs = Config.<String> getValue(ConfigValues.ChangePasswordMsg).split(","); you know which domain, why do you need anything static? Line 91: for (String pair : pairs) { Line 92: // Split the pair in such a way that if the URL contains :, it will not be split to strings Line 93: String[] pairParts = pair.split(":", 2); Line 94: if (pairParts.length >= 2) { http://gerrit.ovirt.org/#/c/26602/9/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapDirectory.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapDirectory.java: rename to authz? Line 1: package org.ovirt.engine.extensions.aaa.builtin.kerberosldap; Line 2: Line 3: import java.util.ArrayList; Line 4: import java.util.HashMap; Line 141: broker.runAdAction(AdActionType.GetAdUserByUserName, new LdapSearchByUserNameParameters(null, Line 142: getDirectoryName(), Line 143: input.<ExtMap> get(Authn.InvokeKeys.AUTH_RECORD).<String> get(Authn.AuthRecord.PRINCIPAL))); Line 144: output.mput(Authz.InvokeKeys.PRINCIPAL_RECORD, mapLdapUser(((LdapUser) ldapResult.getReturnValue()))); Line 145: - Line 146: } Line 147: Line 148: private void doQueryExecute(ExtMap input, ExtMap output) { Line 149: Opaque opaque = input.<Opaque> get(Authz.InvokeKeys.QUERY_OPAQUE); Line 173: context.mput( Line 174: Base.ContextKeys.AUTHOR, Line 175: "The oVirt Project").mput( Line 176: Base.ContextKeys.EXTENSION_NAME, Line 177: "Internal Authorization (Built-in)" Kerberos/LDAP? Authz? Line 178: ).mput( Line 179: Base.ContextKeys.LICENSE, Line 180: "ASL 2.0" Line 181: ).mput( Line 261: } Line 262: } Line 263: if (multipleFilters) { Line 264: result.append(")"); Line 265: } why don't you have a map of operators? then it should be a trivial loop. ( ) ^ ( & ( ) ) ^ ( & (a=b) ) ^ Line 266: return result.append(")").toString(); Line 267: Line 268: } Line 269: http://gerrit.ovirt.org/#/c/26602/9/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/LdapAuthenticateUserCommand.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/LdapAuthenticateUserCommand.java: Line 61: } else { Line 62: log.errorFormat("Failed authenticating. Domain is {0}. User is {1}. The user doesn't have a UPN", Line 63: getAuthenticationDomain(), Line 64: getLoginName()); Line 65: getParameters().getOutputMap().put(Authn.InvokeKeys.RESULT, Authn.AuthResult.CREDENTIALS_INCORRECT); why do you update the output map? Line 66: } Line 67: } Line 68: Line 69: if (!getSucceeded()) { http://gerrit.ovirt.org/#/c/26602/9/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/LdapBrokerCommandBase.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/LdapBrokerCommandBase.java: Line 47: Authn.AuthResult.CREDENTIALS_EXPIRED); Line 48: resultsMap.put(AuthenticationResult.USER_ACCOUNT_DISABLED_OR_LOCKED, Line 49: Authn.AuthResult.ACCOUNT_LOCKED); Line 50: resultsMap.put(AuthenticationResult.WRONG_REALM, Line 51: Authn.AuthResult.CREDENTIALS_INCORRECT); can't we use the api constants directly and not map them? Line 52: } Line 53: Line 54: private Map<String, LdapGroup> globalGroupsDict = new HashMap<>(); Line 55: -- To view, visit http://gerrit.ovirt.org/26602 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I916012eab61a96bdb0f366d9dc8462325d7f726f Gerrit-PatchSet: 9 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
