Martin Mucha has posted comments on this change. Change subject: core: added user permissions to macPools ......................................................................
Patch Set 4: (13 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. TODO removed 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 yes, I've proposed modification of this method in previous comments. This should be done, since double validation is nonsense. And you're right about doing rather double/validation instead of calling validate, since odds for this refactoring aren't probably good :) Line 75: return Collections.singletonList(new PermissionSubject(getMacPoolId(), Line 76: VdcObjectType.MacPool, Line 77: ActionGroup.DELETE_MAC_POOL)); Line 78: } 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. Done 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. Done 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. I'll remove this, due to problem with calling validation. This methods is too far from invoking second validation. 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. this *shit* is already in code base, I'm just "using it". When someone named field "_oldStoragePool" and I just need to create getter for it, I need to name it properly and that's indeed "get_oldStoragePool". I cannot refactor it's name in this patchset, since other reviewers would torn me to pieces. I'll do it in following patchset, ok? 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 Done 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. after changes, when user passes wrong input, Guid.Empty is used, so even if validate method would fail (for some reason) this method will return permissions which will not authorize user anything ~~ i.e. rather than returning empty list, which would grant privileges to user, 'disallowing' permissions is returned. This also fixes possibility of requestedMacPoolId being 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 ? Done 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 ? Done 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 ? Done 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. TODO removed. 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: Done 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 <mmu...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
