Alon Bar-Lev has posted comments on this change. Change subject: aaa: Adding resolve groups ......................................................................
Patch Set 10: (8 comments) most comments you can ignore, the javadoc, please fix... http://gerrit.ovirt.org/#/c/28368/10/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 251: Collections.sort(groupNamesFromDirectory); Line 252: List<String> groupNamesFromDb = Arrays.asList(dbUser.getGroupNames().split(",")); Line 253: Collections.sort(groupNamesFromDb); Line 254: if (!groupNamesFromDb.equals(groupNamesFromDirectory)) { Line 255: dbUser.setGroupNames(StringUtils.join(groupNamesFromDirectory, ",")); why can't you use two sets and compare instead of need of sort and compare? I would have deferred the sort to the last point in which setGroupNames is actually required Line 256: update = true; Line 257: } Line 258: Line 259: if (!groupIds.equals(dbUser.getGroupIds())) { http://gerrit.ovirt.org/#/c/28368/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java: Line 251: // getPermissionCheckSubjects mechanism, because we need the user to be logged in to Line 252: // the system in order to perform this check. The user is indeed logged in when running every command Line 253: // except the login command Line 254: if (!checkUserAndGroupsAuthorization(dbUser.getId(), Line 255: StringUtils.join(dbUser.getGroupIds(), ","), won't it better to modify checkUserAndGroupsAuthorization to do this internally? Line 256: getActionType().getActionGroup(), Line 257: MultiLevelAdministrationHandler.BOTTOM_OBJECT_ID, Line 258: VdcObjectType.Bottom, Line 259: true)) { http://gerrit.ovirt.org/#/c/28368/10/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DbUserDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DbUserDAODbFacadeImpl.java: Line 44: entity.setNamespace(rs.getString("namespace")); Line 45: return entity; Line 46: } Line 47: Line 48: private HashSet<Guid> convertToSet(String string, String delimiter) { I would have put char as delimiter and split by the usual: s.trim().split(String.format(" *%s *", delimiter)) also function name should be convertToGuidSet or something as it is specific. Line 49: HashSet<Guid> results = new HashSet<>(); Line 50: for (String id : Arrays.asList(string.split(delimiter))) { Line 51: results.add(Guid.createGuidFromString(id)); Line 52: } Line 77: } Line 78: Line 79: private String convertToString(HashSet<Guid> groupIds) { Line 80: return StringUtils.join(groupIds, ","); Line 81: } but this near the above function... and it can be: private String convertToString(Collection<?> groupIds) { right? not that it is that important. Line 82: } Line 83: Line 84: @Override Line 85: public DbUser get(Guid id) { http://gerrit.ovirt.org/#/c/28368/10/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 94: * Invoke commands. Line 95: */ Line 96: public static class InvokeCommands { Line 97: /** Line 98: * Fetch principal record. Used for user login. I would like Used for... to be back to own line... :) Line 99: * Line 100: * <p> Line 101: * Input: Line 102: * <ul> Line 95: */ Line 96: public static class InvokeCommands { Line 97: /** Line 98: * Fetch principal record. Used for user login. Line 99: * - Line 100: * <p> Line 101: * Input: Line 102: * <ul> Line 103: * <li>{@link Authn.InvokeKeys#AUTH_RECORD}[M] - authentication record.</li> Line 103: * <li>{@link Authn.InvokeKeys#AUTH_RECORD}[M] - authentication record.</li> Line 104: * <li>{@link InvokeKeys#QUERY_FLAGS}[O] - query flags.</li> Line 105: * </ul> Line 106: * </p> Line 107: * - Line 108: * <p> Line 109: * Output: Line 110: * <ul> Line 111: * <li>{@link InvokeKeys#PRINCIPAL_RECORD}</li> Line 110: * <ul> Line 111: * <li>{@link InvokeKeys#PRINCIPAL_RECORD}</li> Line 112: * </ul> Line 113: * </p> Line 114: * - Line 115: * @see Authn.AuthRecord Line 116: */ Line 117: public static final ExtUUID FETCH_PRINCIPAL_RECORD = new ExtUUID("AAA_AUTHZ_FETCH_PRINCIPAL_RECORD", "5a5bf9bb-9336-4376-a823-26efe1ba26df"); Line 118: /** -- To view, visit http://gerrit.ovirt.org/28368 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3249b7f18c8bf609c9577e60aafa948a0aa55101 Gerrit-PatchSet: 10 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
