Moti Asayag has posted comments on this change. Change subject: <core | restapi | tools | history | engine | userportal | webadmin>: ......................................................................
Patch Set 1: (6 comments) http://gerrit.ovirt.org/#/c/29846/1/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: Line 72: @Override Line 73: public List<PermissionSubject> getPermissionCheckSubjects() { Line 74: return Collections.singletonList(new PermissionSubject(Guid.SYSTEM, the permission should be asked for the mac pool itself, there fore instead of Guid.SYSTEM you should provide the getParameters().getMacPoolId() Line 75: VdcObjectType.System, Line 76: ActionGroup.DELETE_MAC_POOL)); Line 77: } Line 78: http://gerrit.ovirt.org/#/c/29846/1/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(); since the permission check is being executed prior to input validation, the call to this method might produce NPE if the parameters aren't complete. I'd verify getParameters().getStoragePool() before accessing its fields. Line 58: } http://gerrit.ovirt.org/#/c/29846/1/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 317: @Override Line 318: public List<PermissionSubject> getPermissionCheckSubjects() { Line 319: final List<PermissionSubject> result = new ArrayList<>(super.getPermissionCheckSubjects()); Line 320: Line 321: if (changingPoolDefinition()) { very nice Line 322: result.add(new PermissionSubject(getRequestedMacPoolId(), VdcObjectType.MacPool, ActionGroup.CONFIGURE_MAC_POOL)); Line 323: } Line 324: Line 325: return result; Line 324: Line 325: return result; Line 326: } Line 327: Line 328: private boolean changingPoolDefinition() { Can be simplified to the suggested below ? or is there another assumption here ? return Objects.equal(getRequestedMacPoolId(), etOldMacPoolId()) Line 329: return getRequestedMacPoolId() == null Line 330: ? getOldMacPoolId() != null Line 331: : getRequestedMacPoolId().equals(getOldMacPoolId()); Line 332: } http://gerrit.ovirt.org/#/c/29846/1/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 27: v_DELETE_MAC_POOL := 1662; Line 28: v_CONFIGURE_MAC_POOL := 1663; Line 29: v_LOGIN := 1300; Line 30: Line 31: v_APP_MODE := 255; i think the right mode is ApplicationMode.VirtOnly(2), since it is a feature relevant for vms eventually. +this is how you set it on: backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/ActionGroup.java Line 32: Line 33: --TODO should be readonly true or false? Line 34: INSERT INTO roles (id, name, description, is_readonly, role_type, allows_viewing_children, app_mode) SELECT Line 35: v_MAC_POOL_ADMIN, Line 52: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_ADMIN, v_CREATE_MAC_POOL); Line 53: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_ADMIN, v_EDIT_MAC_POOL); Line 54: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_ADMIN, v_DELETE_MAC_POOL); Line 55: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_ADMIN, v_CONFIGURE_MAC_POOL); Line 56: INSERT INTO roles_groups (role_id, action_group_id) VALUES (v_MAC_POOL_ADMIN, v_LOGIN); you should add action groups to v_MAC_POOL_USER as well. Line 57: Line 58: INSERT INTO permissions (id, Line 59: role_id, Line 60: ad_element_id, -- 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: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: 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
