Martin Mucha has posted comments on this change. Change subject: <core | restapi | tools | history | engine | userportal | webadmin>: ......................................................................
Patch Set 1: (7 comments) http://gerrit.ovirt.org/#/c/29846/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddMacPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddMacPoolCommand.java: Line 55: return getParameters().getMacPool(); Line 56: } Line 57: Line 58: @Override Line 59: protected void executeCommand() { > if you'll take a look at AddNetworkCommand.executeCommand() you'll notice Done Line 60: getMacPoolEntity().setId(Guid.newGuid()); Line 61: getMacPoolDao().save(getMacPoolEntity()); Line 62: Line 63: MacPoolPerDcSingleton.getInstance().createPool(getMacPoolEntity()); 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 Done 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 I know that; this must have slipped me off. In updatedc I've called validateInputs in getPermissionCheckSubject() where I decided upon "changingPoolDefinition()" so when needed I've validatedInputs prior to it. But I don't find this 'solution' good though, maybe that's why I removed it forgetting foolishly about it? Nevertheless why's input validation done prior to authorizing? There's nothing wrong (from security perspective) in input validation executed before authorization, but even if we do not like it, we can split validation method to two of them, and one, strictly verifying input (nulls, empty collections), usually using javax.validation can be executed prior to authorizing leaving second validation method, which can touch DB and potentially do some computations after authorization. Manually checking for NPE seems like a non-structural hack (although fastest and easiest). And change proposed above isn't anything which is any hard to do. I've added check back as mentioned above, but conceptually, getting something through constructor which is in addition to that sometimes used before it's fully initialized, this is wrong. Namely what's passed through constructor should be complete or completed in constructor. I'd propose: a) getting rid of parameterized constructor, replacing it with setter. ~~ which would allow us less problematic command creation b) setting parameter only after it was validated. c) raising exception when trying to use not yet validated parameter. This would discover any potentially existing & prevent new gotchas in code. 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 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 h thanks, wasn't aware of this 1.7 class. I'll drop it entirely since by validation RequestedMacPoolId() cannot be null; And due to simplifications inline this method to equally named variable (to give a comment without actual comment). DONE. 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 4: AS $procedure$ Line 5: DECLARE Line 6: v_EVERYONE UUID; Line 7: v_MAC_POOL_ADMIN UUID; Line 8: v_LOCAL_ADMIN_ID UUID; > this should be replaced with SUPER_USER(new Guid("00000000-0000-0000-0000-0 I'm not sure if I understand. v_LOCAL_ADMIN_ID is ID of record from table USERS. So it (probably) cannot be replaced with ID from table ROLES. So if I'm right, you've meant to remove adding new permission bounding new rolegroups to v_LOCAL_ADMIN_ID via their specific roles, but bound those actiongroups directly to SUPER_USER role. Right? I hope you've meant that, since that's what I'm going to write :) Line 9: v_MAC_POOL_USER UUID; Line 10: Line 11: v_CREATE_MAC_POOL INTEGER; Line 12: v_EDIT_MAC_POOL INTEGER; 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 featur crap. DONE. 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. I've also removed CONFIGURE_MAC_POOL since I do believe there shouldn't be overlap. Am I right? 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 <[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
