Alon Bar-Lev has posted comments on this change. Change subject: aaa: Fix sync ......................................................................
Patch Set 4: (11 comments) http://gerrit.ovirt.org/#/c/28561/4/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 146 Line 147 Line 148 Line 149 Line 150 if you do first datbase groups then user, you fetch more groups than first users and then groups. as when fetch users we get some of the groups, and we do not have control what we actually query. Line 32: import org.ovirt.engine.core.utils.timer.OnTimerMethodAnnotation; Line 33: import org.ovirt.engine.core.utils.timer.SchedulerUtilQuartzImpl; Line 34: Line 35: public class DbUserCacheManager { Line 36: private static final String EVERYONE_GROUP_ID = "eee00000-0000-0000-0000-123456789eee"; is this engine specific uuid? Line 37: private static final Log log = LogFactory.getLog(DbUserCacheManager.class); Line 38: private static final DbUserCacheManager _instance = new DbUserCacheManager(); Line 39: private boolean initialized = false; Line 40: private Map<String, List<DbGroup>> dbGroups = new HashMap<>(); Line 38: private static final DbUserCacheManager _instance = new DbUserCacheManager(); Line 39: private boolean initialized = false; Line 40: private Map<String, List<DbGroup>> dbGroups = new HashMap<>(); Line 41: Line 42: // private final Map<String, Map<String, DbGroup>> groupsMap = new HashMap<>(); - Line 43: Line 44: public static DbUserCacheManager getInstance() { Line 45: return _instance; Line 46: } Line 85: DbUserDAO dao = DbFacade.getInstance().getDbUserDao(); Line 86: Line 87: // Retrieve all the users from the database: Line 88: List<DbUser> dbUsers = dao.getAll(); Line 89: dbGroups.clear(); if you clear it then it probably need to be a parameter and not object state Line 90: for (DbGroup dbGroup : DbFacade.getInstance().getDbGroupDao().getAll()) { Line 91: MultiValueMapUtils.addToMap(dbGroup.getNamespace(), dbGroup, dbGroups); Line 92: } Line 93: // Classify the users by directory. Note that the resulting map may have an entry with a null key, that Line 87: // Retrieve all the users from the database: Line 88: List<DbUser> dbUsers = dao.getAll(); Line 89: dbGroups.clear(); Line 90: for (DbGroup dbGroup : DbFacade.getInstance().getDbGroupDao().getAll()) { Line 91: MultiValueMapUtils.addToMap(dbGroup.getNamespace(), dbGroup, dbGroups); I may miss something bug of dbGroups is shared among all, why do we need to store it for each entry? Line 92: } Line 93: // Classify the users by directory. Note that the resulting map may have an entry with a null key, that Line 94: // corresponds to the users whose directory has been removed from the configuration. Line 95: Map<String, List<DbUser>> authzToPrincipalMap = new HashMap<>(); Line 152: directoryUsers = Collections.emptyList(); Line 153: } Line 154: Line 155: Line 156: - Line 157: // Build a map of users indexed by directory id to simplify the next step where we want to find the directory Line 158: // user corresponding to each database user: Line 159: Map<String, DirectoryUser> index = new HashMap<>(); Line 160: Set<DirectoryGroup> groupsToFetch = new HashSet<>(); Line 215: Map<String, DirectoryGroup> fetchedGroups, Line 216: Collection<DirectoryGroup> groupsToFetch Line 217: ) { Line 218: Line 219: if ((authz.getContext().<Long> get(Authz.ContextKeys.CAPABILITIES, 0L) & Authz.Capabilities.RECURSIVE_GROUPS_RESOLVING) != 0L) { == 0? the condition is probably reversed as you need to flat only if it does recursive. Line 220: for (DirectoryGroup group : groupsToFetch) { Line 221: fetchedGroups.put(group.getId(), group); Line 222: } Line 223: } else { Line 223: } else { Line 224: for (DirectoryGroup group : groupsToFetch) { Line 225: if (!fetchedGroups.containsKey(group.getId())) { Line 226: Set<DirectoryGroup> flatGroups = new HashSet<>(); Line 227: DirectoryGroup fetchedGroup = AuthzUtils.findGroupById(authz, group.getNamespace(), group.getId(), true, true); we do not need two flags now... only if fetch groups or not. Line 228: fetchedGroups.put(fetchedGroup.getId(), fetchedGroup); Line 229: DirectoryUtils.flatGroups(flatGroups, fetchedGroup.getGroups()); Line 230: for (DirectoryGroup fetchedMemberOf : flatGroups) { Line 231: fetchedGroups.put(fetchedMemberOf.getId(), fetchedMemberOf); Line 231: fetchedGroups.put(fetchedMemberOf.getId(), fetchedMemberOf); Line 232: } Line 233: } Line 234: } Line 235: } I do not understand the algorithm.... either you do recursive or you do a list based. if you do a list based I expect, something like: cache = new Map<>() tofetch = ids while (!tofetch.empty) { current = tofetch; tofetch.clear(); for (groupid : current) { group = fetch(groupid); add group to cache for (subgroup : group.groups()) { if (subgroup not in cache) { cache.add(subgroup); tofetch.add(subgroup); } } } } Line 236: Line 237: } Line 238: Line 239: /** http://gerrit.ovirt.org/#/c/28561/4/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthz.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthz.java: Line 123: Authz.PrincipalRecord.ID, Line 124: configuration.getProperty("config.authz.user.id") Line 125: ).mput( Line 126: Authz.PrincipalRecord.GROUPS, Line 127: Collections.emptyList() no need, you should assume no groups if no key Line 128: ); Line 129: Line 130: } Line 131: http://gerrit.ovirt.org/#/c/28561/4/backend/manager/modules/extensions-api-root/extensions-api/src/main/java/org/ovirt/engine/api/extensions/aaa/Authz.java File backend/manager/modules/extensions-api-root/extensions-api/src/main/java/org/ovirt/engine/api/extensions/aaa/Authz.java: Line 33: * @see Capabilities Line 34: */ Line 35: public static final ExtKey CAPABILITIES = new ExtKey("AAA_AUTHZ_CAPABILITIES", Line 36: Long.class, Line 37: "de243950-6774-45f8-a852-f4b656ff69aa"); - Line 38: } Line 39: Line 40: /** Line 41: * Capabilities. -- 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: 4 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
