Alon Bar-Lev has posted comments on this change. Change subject: aaa: Stopping to use proxies ......................................................................
Patch Set 10: (21 comments) http://gerrit.ovirt.org/#/c/26602/10/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 23: public ExtMap getCommandParameters(); Line 24: } Line 25: Line 26: private static final int QUERIES_RESULTS_LIMIT = 1000; Line 27: private static final long PAGE_SIZE = 100; we can have 1000 here, or at least 500. Line 28: Line 29: Line 30: public static String getName(ExtensionProxy proxy) { Line 31: return proxy.getContext().<String> get(Base.ContextKeys.INSTANCE_NAME); Line 36: Base.InvokeKeys.COMMAND, Line 37: Authz.InvokeCommands.FETCH_PRINCIPAL_RECORD Line 38: ).mput( Line 39: Authn.InvokeKeys.AUTH_RECORD, Line 40: (ExtMap) authRecord why do we need cast? why don't we get ExtMap as parameter? Line 41: )).<ExtMap> get(Authz.InvokeKeys.PRINCIPAL_RECORD)); Line 42: } Line 43: Line 44: public static List<DirectoryUser> findPrincipalsByQuery(final ExtensionProxy extension, final String query) { Line 45: return queryPrincipals(extension, SearchQueryParsingUtils.generateQueryMap(query, Authz.QueryEntity.PRINCIPAL)); Line 46: } Line 47: Line 48: public static List<DirectoryUser> findPrincipalsByIds(final ExtensionProxy extension, final List<String> ids) { Line 49: List<List<String>> idsBatches = getIdsBatches(extension, ids); why do you need this temp variable? Line 50: List<DirectoryUser> results = new ArrayList<>(); Line 51: for (List<String> batch : idsBatches) { Line 52: results.addAll( Line 53: queryPrincipals( Line 71: return queryGroups(extension, SearchQueryParsingUtils.generateQueryMap(query, Authz.QueryEntity.GROUP)); Line 72: } Line 73: Line 74: public static List<DirectoryGroup> findGroupsByIds(final ExtensionProxy extension, final List<String> ids) { Line 75: List<List<String>> idsBatches = getIdsBatches(extension, ids); why do you need this temp variable? Line 76: List<DirectoryGroup> results = new ArrayList<>(); Line 77: for (List<String> batch:idsBatches) { Line 78: results.addAll( Line 79: queryGroups( Line 138: Line 139: @Override Line 140: public ExtUUID getCommand() { Line 141: return command; Line 142: } why do you need command here? and not just pass it as argument? Line 143: Line 144: @Override Line 145: public ExtMap getCommandParameters() { Line 146: return extMap; Line 142: } Line 143: Line 144: @Override Line 145: public ExtMap getCommandParameters() { Line 146: return extMap; same... Line 147: } Line 148: }); Line 149: return directoryUsers; Line 150: } Line 194: ).mput( Line 195: handler.getCommandParameters() Line 196: ) Line 197: ).get(Authz.InvokeKeys.QUERY_OPAQUE); Line 198: while (true) { do { Line 199: List<ExtMap> result = extension.invoke(new ExtMap().mput( Line 200: Base.InvokeKeys.COMMAND, Line 201: Authz.InvokeCommands.QUERY_EXECUTE Line 202: ).mput( Line 212: } Line 213: if (!handler.handle(result)) { Line 214: break; Line 215: } Line 216: } } while(handler.handle(result); Line 217: } finally { Line 218: extension.invoke(new ExtMap().mput( Line 219: Base.InvokeKeys.COMMAND, Line 220: Authz.InvokeCommands.QUERY_CLOSE Line 280: batchOfIds.add(ids.get(index++)); Line 281: } Line 282: } Line 283: return batchOfIdsList; Line 284: } hmmm.... chunk = size-10; for (i=0;i<ids.size();i+=chunk) { out.addAll(ids.subList(i, i+chunk > ids.size() ? ids.size() : i+chunk); } Line 285: http://gerrit.ovirt.org/#/c/26602/10/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/SearchQueryParsingUtils.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/SearchQueryParsingUtils.java: Line 57: sb.append(")"); Line 58: if (ids.size() > 1) { Line 59: sb.append(")"); Line 60: } Line 61: return sb.toString(); we agreed that you generate the extension syntax directly, no need to create string and convert to extension syntax. Line 62: Line 63: } Line 64: Line 65: public static ExtMap generateQueryMap(List<String> ids, ExtUUID queryEntity) { Line 134: ).mput(Authz.QueryFilterRecord.FILTER, Line 135: filter Line 136: )); Line 137: } Line 138: return result; this is ugly... but if it works for you I leave it as-is for now. I would have done this via regexp and not complex parsing... but not important for our cause right now. Line 139: } Line 140: Line 141: private static ExtMap createMapForKeyAndValue(String field, String value) { Line 142: return createMapForKeyAndValue(mapRecordKey(field), value); Line 155: ); Line 156: } Line 157: Line 158: private static ExtKey mapRecordKey(String key) { Line 159: return attributesToKeys.get(key); again... one time used private function... please remove and embed statement. Line 160: } Line 161: http://gerrit.ovirt.org/#/c/26602/10/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 172: "org.ovirt.engine.extensions.aaa.builtin.kerberosldap.KerberosLdapAuthenticator"); Line 173: authConfig.put("ovirt.engine.aaa.authn.profile.name", domain); Line 174: authConfig.put("ovirt.engine.aaa.authn.authz.plugin", domain); Line 175: authConfig.put("org.ovirt.engine.extension.change.password.url", passwordChangeUrlPerDomain.get(domain)); Line 176: authConfig.put("org.ovirt.engine.extension.change.password.msg", passwordChangeMsgPerDomain.get(domain)); these should be config.<something> no? see internal for example. Line 177: ExtensionsManager.getInstance().load(authConfig); Line 178: Line 179: Properties dirConfig = new Properties(); Line 180: dirConfig.put(Base.ConfigKeys.NAME, domain); http://gerrit.ovirt.org/#/c/26602/10/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 60: context.mput( Line 61: Base.ContextKeys.AUTHOR, Line 62: "The oVirt Project").mput( Line 63: Base.ContextKeys.EXTENSION_NAME, Line 64: "Internal Authn (Built-in)" this belongs to previous patch, and please re-add the indent Line 65: ).mput( Line 66: Base.ContextKeys.LICENSE, Line 67: "ASL 2.0" Line 68: ).mput( http://gerrit.ovirt.org/#/c/26602/10/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 45: } else if (command.equals(Authz.InvokeCommands.QUERY_CLOSE)) { Line 46: // Do nothing Line 47: } else if (command.equals(Authz.InvokeCommands.QUERY_OPEN)) { Line 48: output.put(Authz.InvokeKeys.QUERY_OPAQUE, new Opaque(input.<ExtUUID> get(Authz.InvokeKeys.QUERY_ENTITY) Line 49: .equals(Authz.QueryEntity.PRINCIPAL))); this belongs to previous patch Line 50: } else if (command.equals(Authz.InvokeCommands.QUERY_EXECUTE)) { Line 51: doQueryExecute(input, output); Line 52: } else { Line 53: output.put( Line 86: private void doInit(ExtMap input, ExtMap output) { Line 87: context = input.<ExtMap> get(Base.InvokeKeys.CONTEXT); Line 88: context.mput( Line 89: Base.ContextKeys.EXTENSION_NAME, Line 90: "Internal Authz (Built-in)" this belongs to previous patch, and please re-add the indent Line 91: ).mput( Line 92: Base.ContextKeys.LICENSE, Line 93: "ASL 2.0" Line 94: ).mput( http://gerrit.ovirt.org/#/c/26602/10/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: do not forget to rename the class... Line 1: package org.ovirt.engine.extensions.aaa.builtin.kerberosldap; Line 2: Line 3: import java.util.List; Line 4: import java.util.Properties; Line 86: AdActionType.AuthenticateUser, Line 87: new LdapUserPasswordBaseParameters(input, output) Line 88: ); Line 89: // Putting these keys anyway, it's up to BLL to decide if to use them or not Line 90: output.mput(Authn.InvokeKeys.USER_MESSAGE, - Line 91: configuration.getProperty("org.ovirt.extension.change.password.msg") Line 92: ).mput( Line 93: Authn.InvokeKeys.CREDENTIALS_CHANGE_URL, Line 94: configuration.getProperty("org.ovirt.extension.change.password.url" http://gerrit.ovirt.org/#/c/26602/10/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: do not forget to rename the class 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 262: Line 263: if (query.<ExtMap> get(Authz.InvokeKeys.QUERY_FILTER) Line 264: .<List<ExtMap>> get(Authz.QueryFilterRecord.FILTER) Line 265: .size() > 1) { Line 266: result.insert(0, "(|").append(")"); why or? why don't you have mapping between operator and character and just construct it? how come this is not recursive? should be as simple as: f(queryfilterrecord r) { String ret; if (r.filter == null) { ret = format("(%s%s%s)", key, operator.get(r.operator), value); else { ret = format("(%s%s", operator.get(r.operaor)); for (e : r.filter) { ret += f(e); } ret += ")" } return ret; } Line 267: } Line 268: return result.insert(0, Line 269: query.get(Authz.InvokeKeys.QUERY_ENTITY).equals(Authz.QueryEntity.PRINCIPAL) ? USERS_QUERY_PREFIX Line 270: : GROUPS_QUERY_PREFIX) http://gerrit.ovirt.org/#/c/26602/10/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); I still think that it should be very easy to use the values directly. 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: 10 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
