Yair Zaslavsky has posted comments on this change. Change subject: aaa: Stopping to use proxies ......................................................................
Patch Set 10: (17 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. also at PAGE_SIZE? didn't we agree that page_size will be 100? 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? I was in a dilemma whether to expose ExtMap or not. My reasoning was that I do not want to expose the API to BLL. The cast is anyway redundant, and I will remove it. I also checked LoginBaseCommand again, and since the API is already exposed to BLL, I will change the parameter to ExtMap. 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? Done 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? Done 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? Done 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... Done 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 { Done 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); Done 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.... done. nice, i wasn't aware of subList :) 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 creat true, see the next method. the above is copy-paste error and should be removed, thanks. Line 62: Line 63: } Line 64: Line 65: public static ExtMap generateQueryMap(List<String> ids, ExtUUID queryEntity) { 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 statemen Done 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. Done 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 Done 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 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 Done 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: 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, > - Done 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: 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 Done 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. As the directory implementation uses AuthenticationResult throughout its code + uthnticationResult holds not just the enum literal, but other information as well, I still suggest to revisit it at the end if we have time. I'm not against getting rid of this and work directly with AuthResult. I think there are more urgent things to handle first. 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
