Alon Bar-Lev has posted comments on this change. Change subject: aaa: Stopping to use proxies ......................................................................
Patch Set 9: (10 comments) 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 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 don't think i understood this comment, this class does not expose ExtMap well, I thought that the conversion code from search to query will be done when query is initiated at the place in which query is received. The continue as if the search backend provided the extension search format, so in future this conversion can simply dropped without changing any of the code. 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: } > About iteration/callback - understood. it can be int, not sure why a new cast is required... I try to use long whenever I can to limit the variety of types. it is set by extension during initialization, it can come from configuration if so extension wishes so. we must have a limit for the query size, as the ldap does not support 1G of search size, right? also 1M is difficult enough... so we need a sane limit, for example 100 entries... if we know that this will work with decent field sizes. this requires mostly for the sync, as we should limit the # of ids each query can have. so I am opened to suggested of how to add limit to the contract. 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: } > Currently search has no paging, when you search for example for a* - you wi if you expose the callback, then there is no problem to add the capability in the calling code. Line 221: Line 222: private static List<DirectoryGroup> populateGroups(final ExtensionProxy extension, Line 223: final ExtUUID command, Line 224: final ExtMap extMap) { Line 301: directoryGroups.add(mapGroupRecord(extension, group)); Line 302: } Line 303: } Line 304: } Line 305: return directoryUser; > i would prefer not to do that at the moment - think of DirectoryUser as an ExtMap cannot be used at UI as it is not serialize and uses reflection to check code, if we to use it at UI we need some wrapper class, we already don this task within the command as map. However, when the UI will be re-written using restapi then it will not be required. But I would have limited the use of this class only to these commands that do share entities with UI if we can. 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/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); > the line after that should be removed. for service name.... no? for indirect reference to other plugin... interesting idea, but it is misleading. for example, if we need two plugins, one for user conversion and one for authz, then I expect these to leave within the authn name space. ovirt.engine.aaa.authn.authz.plugin ovirt.engine.aaa.authn.convert.plugin 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 84 Line 85 Line 86 Line 87 Line 88 > why not? i would like to distingush if the kept string per domain is URL or this is ok, but used one time, single line private function... no need for it, move it to wherever it is being used? Line 87 Line 88 Line 89 Line 90 Line 91 > not sure i understand the comment. so you need to find http: or https: 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)" > Actually, we used in EXTENSION_NAME "authorization" and ""Authentication" - yes, better to drop these terms in favour of authz/authn or authentication/authorization to be consistent. Line 71: ).mput( Line 72: Base.ContextKeys.LICENSE, Line 73: "ASL 2.0" Line 74: ).mput( 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); > The output map should contain the fact the credentials were in correct in c well, I thought of you threw exception with the aaa exception with right code, and handling all failures at main loop... but not that important. 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); > we can do that, i would like to advance and see if we have enough time left nothing is critical... but I think it is trivial... :) 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 <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[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
