Yair Zaslavsky has posted comments on this change. Change subject: aaa: Stopping to use proxies ......................................................................
Patch Set 9: (23 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 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 th done in the separate class that deals with the query parsing. 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 hopeful yes. done. 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? We can, but this class is not "symmetric" to the API, for example you have here several query methods. Anyway, I'll change here as you suggested + use the term Principal in method names instead of user. Done. 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 a Oh, thanks for elaborating that, I did not understand this before (another thing to document better at the API once we finish...) Done. 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 i don't think i understood this comment, this class does not expose ExtMap in its interface. 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. About iteration/callback - understood. I think the definition of QUERY_MAX_FILTER_SIZE is a big vague - /** * Maximum query filter size. * Limit the number of entries within {@link InvokeKeys#QUERY_FILTER}. * No more than this may be provided. */ How do you define an entry? a QUERY_FILTER may contain operator, nested filter, record field and value.... Do I have to traverse the ExtMap that contains the QUERY_FILTER and count how many elements does it have? In addition QUERY_MAX_FILTER_SIZE is a context key. So it should be set on the context of the authorization provider. Who should set it? Should this be read from the configuration of the provider? In addition I don't see a reason for QUERY_MAX_FILTER_SIZE to be long. I would like to change this to integer (will save me some casting in the code). 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 go Currently search has no paging, when you search for example for a* - you will get all users that match a* (I think i've sent you by email the exact generated query ). So unfortunately, at the moment it is expected to contain all queries. For sync - we can take the list of IDs and split it to batches, for example - if you have 10,000 ids, split it to 100 batches of 100 ids in each batch, and call findUsersByIds 100 times, per each batch. However, we go to this direction, do we still need to implement the iteration code (as you suggested above) in this class? and what about the MAX_FILTER_SIZE - will it set the batch size somehow? 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 go see above. 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? i would prefer not to do that at the moment - think of DirectoryUser as an entity from engine/bll side. It is also used in UI/API - sooner or later I'll have to convert, or to use all over - do you want to use ExtMap at UI code as well? 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? As you can see, this is an "on going patch" - the proxy will be removed at the final step. I still need it to extend Directory for the profiles repository handling. There was a reason I called this patch "stopping to use proxies" and I do intend to stop using them, and remove them :) 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? the line after that should be removed. We agreed that instead of "ovirt.engine.aaa.authn.authz.plugin" we will use Authz.class.getName(). 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? Done Line 84 Line 85 Line 86 Line 87 Line 88 > I do not think this function is required. why not? i would like to distingush if the kept string per domain is URL or not. there is only one entry in vdc_options for both the msgs and URL. Line 87 Line 88 Line 89 Line 90 Line 91 > should be also ':' as suffix not sure i understand the comment. : can appear at: http://www.bla.com https://www.bla.com http://www.bla.com:8080 (and of course here you can have more parameters) https://www.bla.com:8080 Can you please elaborate? 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. No need for these maps anymore, I pass the info from InitOnStartup. 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? Actually, we used in EXTENSION_NAME "authorization" and ""Authentication" - I don't mind changing it if you wish. and for internal - well, copy paste error :) I will fix. 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? see above comments on the maps. 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? let's revisit class names later on. i dont object in general. 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: > - Done 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? See previous comment on Authn regarding this. If you would like to rename from Authorization to Authz , and same for Authentication to authn, I don't mind, we agreed on this string before, but i don't mind changing it. 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. not sure i fully understood your comment, but anyway i revised the code, and improved it a bit, let's discuss this further after i upload the next patch (which i'll do after we close the more important issues). 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? The output map should contain the fact the credentials were in correct in case the authentication failed. This should be propgated to the LdapKerberosAuthenticator which calls this command. 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? we can do that, i would like to advance and see if we have enough time left to handle this, IMHO this is a less critical change at this point. 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
