Moti Asayag has posted comments on this change. Change subject: core: added user permissions to macPools ......................................................................
Patch Set 4: Code-Review-1 (14 comments) http://gerrit.ovirt.org/#/c/29846/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/PredefinedRoles.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/PredefinedRoles.java: Line 27: TAG_ADMIN(new Guid("DEF00011-0000-0000-0000-DEF000000013")), Line 28: BOOKMARK_ADMIN(new Guid("DEF00011-0000-0000-0000-DEF000000014")), Line 29: EVENT_NOTIFICATION_ADMIN(new Guid("DEF00011-0000-0000-0000-DEF000000015")), Line 30: Line 31: //TODO MM: there maybe is some rule how to for these GUIDs, verify. i think that they just need to be unique in this context. Line 32: MAC_POOL_ADMIN(new Guid("DEF00013-0000-0000-0000-DEF000000013")), Line 33: MAC_POOL_USER(new Guid("DEF00014-0000-0000-0000-DEF000000014")); Line 34: Line 35: private Guid id; http://gerrit.ovirt.org/#/c/29846/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMacPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMacPoolCommand.java: Line 70: Line 71: @Override Line 72: public List<PermissionSubject> getPermissionCheckSubjects() { Line 73: //TODO MM: can this be done better? Line 74: validateInputs(); as stated before - validateInputs() is no good here.e I think that it just be deleted from here, and this will work as planned. in addition, i wouldn't call validateInputs(); or any other sort of validations from this context. This method should be kept intact with a single responsibility: to produce list of entities on which examination should be executed. If planned to suggest alternative, i'd first suggest just to get over with this patch-set in the way it is being done today, and propose an alternative once we're done with this feature. The reason not to do it now is such change will have a wide impact on all command flows in the system and requires an approval from various maintainers. Putting this aside, the right place IMO for such validation is on: CommandBase.internalCanDoAction() where: returnValue = isUserAuthorizedToRunAction() && isBackwardsCompatible() && validateInputs() && acquireLock() && canDoAction() && internalValidateAndSetQuota(); should be modified to include some input validation prior to isUserAuthorizedToRunAction() check. Please raise this issue on @devel to reach a wider audience. Line 75: return Collections.singletonList(new PermissionSubject(getMacPoolId(), Line 76: VdcObjectType.MacPool, Line 77: ActionGroup.DELETE_MAC_POOL)); Line 78: } Line 86: Line 87: private MacPool getOldMacPool() { Line 88: if (oldMacPool == null) { Line 89: oldMacPool = getMacPoolDao().get(getMacPoolId()); Line 90: } please add a space line after '}' Line 91: return oldMacPool; Line 92: } http://gerrit.ovirt.org/#/c/29846/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMacPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMacPoolCommand.java: Line 74: Line 75: @Override Line 76: public List<PermissionSubject> getPermissionCheckSubjects() { Line 77: //TODO MM: can this be done better? Line 78: validateInputs(); samed as previous comment. Line 79: return Collections.singletonList(new PermissionSubject(getMacPoolId(), Line 80: VdcObjectType.MacPool, Line 81: ActionGroup.EDIT_MAC_POOL)); Line 82: } http://gerrit.ovirt.org/#/c/29846/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddEmptyStoragePoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddEmptyStoragePoolCommand.java: Line 99: } Line 100: Line 101: @Override Line 102: public List<PermissionSubject> getPermissionCheckSubjects() { Line 103: validateInputs(); same. Line 104: return Arrays.asList( Line 105: new PermissionSubject(Guid.SYSTEM, VdcObjectType.System, getActionType().getActionGroup()), Line 106: new PermissionSubject(getRequestedMacPoolId(), VdcObjectType.MacPool, ActionGroup.CONFIGURE_MAC_POOL) Line 107: ); http://gerrit.ovirt.org/#/c/29846/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StoragePoolManagementCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StoragePoolManagementCommandBase.java: Line 53: return (sps == null || sps.isEmpty()); Line 54: } Line 55: Line 56: protected Guid getRequestedMacPoolId() { Line 57: return getParameters().getStoragePool().getMacPoolId(); this should be NPE safe. Line 58: } http://gerrit.ovirt.org/#/c/29846/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommand.java: Line 306: private Guid getOldMacPoolId() { Line 307: return get_oldStoragePool().getMacPoolId(); Line 308: } Line 309: Line 310: private StoragePool get_oldStoragePool() { please use a proper naming convention. Line 311: if (_oldStoragePool == null) { Line 312: _oldStoragePool = getStoragePoolDAO().get(getStoragePool().getId()); Line 313: } Line 314: return _oldStoragePool; Line 309: Line 310: private StoragePool get_oldStoragePool() { Line 311: if (_oldStoragePool == null) { Line 312: _oldStoragePool = getStoragePoolDAO().get(getStoragePool().getId()); Line 313: } please add a space line Line 314: return _oldStoragePool; Line 315: } Line 316: Line 317: @Override Line 318: public List<PermissionSubject> getPermissionCheckSubjects() { Line 319: final List<PermissionSubject> result = new ArrayList<>(super.getPermissionCheckSubjects()); Line 320: Line 321: final boolean changingPoolDefinition = getRequestedMacPoolId().equals(getOldMacPoolId()); Line 322: if (changingPoolDefinition) { getRequestedMacPoolId() might be null. Line 323: result.add(new PermissionSubject(getRequestedMacPoolId(), VdcObjectType.MacPool, ActionGroup.CONFIGURE_MAC_POOL)); Line 324: } Line 325: Line 326: return result; http://gerrit.ovirt.org/#/c/29846/4/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/model/PermitType.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/model/PermitType.java: Line 119: Line 120: // Mac Pool action groups Line 121: CREATE_MAC_POOL, Line 122: EDIT_MAC_POOL, Line 123: DELETE_MAC_POOL; CONFIGURE_MAC_POOL ? Line 124: Line 125: public String value() { Line 126: return name().toLowerCase(); Line 127: } http://gerrit.ovirt.org/#/c/29846/4/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/PermitMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/PermitMapper.java: Line 196: return PermitType.CREATE_MAC_POOL; Line 197: case EDIT_MAC_POOL: Line 198: return PermitType.EDIT_MAC_POOL; Line 199: case DELETE_MAC_POOL: Line 200: return PermitType.DELETE_MAC_POOL; what about CONFIGURE_MAC_POOL ? Line 201: default: Line 202: return null; Line 203: } Line 204: } Line 352: return ActionGroup.EVENT_NOTIFICATION_MANAGEMENT; Line 353: case MANIPULATE_AFFINITY_GROUPS: Line 354: return ActionGroup.MANIPULATE_AFFINITY_GROUPS; Line 355: case ADD_USERS_AND_GROUPS_FROM_DIRECTORY: Line 356: return ActionGroup.ADD_USERS_AND_GROUPS_FROM_DIRECTORY; same here ? Line 357: case CREATE_MAC_POOL: Line 358: return ActionGroup.CREATE_MAC_POOL; Line 359: case EDIT_MAC_POOL: Line 360: return ActionGroup.EDIT_MAC_POOL; http://gerrit.ovirt.org/#/c/29846/4/packaging/dbscripts/upgrade/03_05_0760_add_permissions_to_mac_pools.sql File packaging/dbscripts/upgrade/03_05_0760_add_permissions_to_mac_pools.sql: Line 33: --TODO should be readonly true or false? Line 34: INSERT INTO roles (id, Line 35: name, Line 36: description, Line 37: is_readonly, it should be readonly since it is predefined role. Line 38: role_type, Line 39: allows_viewing_children, Line 40: app_mode) Line 41: SELECT Line 65: Line 66: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_ADMIN, v_CREATE_MAC_POOL); Line 67: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_ADMIN, v_EDIT_MAC_POOL); Line 68: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_ADMIN, v_DELETE_MAC_POOL); Line 69: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_ADMIN, v_LOGIN); Please add: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_ADMIN, v_CONFIGURE_MAC_POOL); Line 70: Line 71: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_USER, v_CONFIGURE_MAC_POOL); Line 72: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_USER, v_LOGIN); Line 73: -- To view, visit http://gerrit.ovirt.org/29846 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1f5d080b6628f86ab2ff88f8e2dfaab21d367c7f Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Moti Asayag <[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
