Yair Zaslavsky has posted comments on this change. Change subject: aaa: Fix sync ......................................................................
Patch Set 11: (9 comments) http://gerrit.ovirt.org/#/c/28561/11/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 316: } Line 317: return directoryGroup; Line 318: } Line 319: Line 320: private static int queryFlagValue(boolean resolveGroups) { > so you really can remove this function... no? yes, i will do that. Line 321: int result = 0; Line 322: if (resolveGroups) { Line 323: result |= Authz.QueryFlags.RESOLVE_GROUPS; Line 324: } http://gerrit.ovirt.org/#/c/28561/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/aaa/DirectoryUtils.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/aaa/DirectoryUtils.java: Line 26: } Line 27: return results; Line 28: } Line 29: Line 30: public static void flatGroups(Set<DirectoryGroup> accumulator, Collection<DirectoryGroup> groupsFrom) { > I am unsure you are using this one any more.... you have dup ok, i will check, I guess i missed that one . Line 31: for (DirectoryGroup group : groupsFrom) { Line 32: flatGroups(accumulator, group.getGroups()); Line 33: accumulator.add(group); Line 34: } http://gerrit.ovirt.org/#/c/28561/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java: Line 64: } Line 65: } Line 66: Line 67: /** Line 68: * Load all the users from the database and refresh them. The refresh logic is as follows : 1. Fetch users from DB > please have each as new line sure, this was like at in the IDE, i guess the formatter got crazy :) Line 69: * 2. Resolve the groups of users (get 1st level groups) 3. Fetch groups from DB 4. Resolve the fetched groups (for Line 70: * each group that was not already resolved) 5. Resolve recursively all groups 6. Compare the user from db and the Line 71: * fetched user from the directory + store in DB if there is a change Line 72: */ Line 91: }; Line 92: Line 93: List<DbUser> refreshedUsers = new ArrayList<>(); Line 94: for (String authzName : authzNames) { Line 95: refreshedUsers.addAll(refreshAllUsers(EngineExtensionsManager.getInstance().getExtensionByName(authzName), > you need to deal with users added by extension that is removed. eventually i will have to to provide a "flat " group ids list to the users. This is why i preferred to do that in a one step. Line 96: usersPerAuthz.get(authzName), Line 97: groupsPerAuthz.get(authzName))); Line 98: } Line 99: Line 98: } Line 99: Line 100: for (DbUser dbUser : refreshedUsers) { Line 101: DbFacade.getInstance().getDbUserDao().update(dbUser); Line 102: } > what about updating the groups? please explain what you mean? If you look at the content of ad_groups tables (yes, sorry for the very bad name, it's from 2.x days...) you will see that there is no "memberof" field or something like that, if you feel this is needed, please let me know. Line 103: } Line 104: Line 105: private Collection<DbUser> refreshAllUsers(ExtensionProxy authz, List<DbUser> dbUsers, List<DbGroup> dbGroups) { Line 106: Line 140: while (!idsToFetch.isEmpty()) { Line 141: groupsToFetch = new HashSet<>(); Line 142: for (String namespace : idsToFetch.keySet()) { Line 143: List<DirectoryGroup> groups = Line 144: AuthzUtils.findGroupsByIds(authz, namespace, new ArrayList<>(idsToFetch.get(namespace)), true); > why do you need to cast it to ArrayList? idstofetch.get returns a set, which is ok. i would like the ids to be kept in a structure that enforces uniqueness - hence using a set. Line 145: idsToFetch = new HashMap<>(); Line 146: for (DirectoryGroup group : groups) { Line 147: cache.put(group.getId(), group); Line 148: for (DirectoryGroup memberOf: group.getGroups()) { Line 141: groupsToFetch = new HashSet<>(); Line 142: for (String namespace : idsToFetch.keySet()) { Line 143: List<DirectoryGroup> groups = Line 144: AuthzUtils.findGroupsByIds(authz, namespace, new ArrayList<>(idsToFetch.get(namespace)), true); Line 145: idsToFetch = new HashMap<>(); > idsToFetch.removeAll(idsToFetch.get(namespace)) ? Well, you probably meant remove all of the ids that belong to the namespace, and yes, you're right. Line 146: for (DirectoryGroup group : groups) { Line 147: cache.put(group.getId(), group); Line 148: for (DirectoryGroup memberOf: group.getGroups()) { Line 149: groupsToFetch.add(memberOf); Line 145: idsToFetch = new HashMap<>(); Line 146: for (DirectoryGroup group : groups) { Line 147: cache.put(group.getId(), group); Line 148: for (DirectoryGroup memberOf: group.getGroups()) { Line 149: groupsToFetch.add(memberOf); > only if not in cache? yes. Line 150: } Line 151: Line 152: } Line 153: Line 152: } Line 153: Line 154: Line 155: } Line 156: idsToFetch = getIdsFromGroups(groupsToFetch, cache); > why do you need the double conversion? you can drop the groupsToFetch in fa renaming - groupIdsToFetch is better? what do you think? In addition, about groupToFetch - cant a group be a member of various groups in various namespaces? the groupsToFetch is used in order to allow holding groups per namespaces.. Line 157: } Line 158: List<DbUser> refreshed = new ArrayList<>(); Line 159: for (DbUser dbUser : dbUsers) { Line 160: DirectoryUser directoryUser = directoryUsersByIds.get(dbUser.getExternalId()); -- To view, visit http://gerrit.ovirt.org/28561 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id49b51517a967c7a83e8e73f52181673baa31700 Gerrit-PatchSet: 11 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
