Martin Mucha has posted comments on this change. Change subject: core: added user permissions to macPools ......................................................................
Patch Set 2: (3 comments) http://gerrit.ovirt.org/#/c/29846/2/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 86: private void addPermission(Guid userId, Guid macPoolId) { Line 87: addPermission(userId, macPoolId, PredefinedRoles.MAC_POOL_ADMIN, VdcObjectType.MacPool); Line 88: } Line 89: Line 90: private static void addPermission(Guid userId, Guid entityId, PredefinedRoles role, VdcObjectType objectType) { > why is this method needed ? it isn't being called elsewhere with a differen this is the method from NetworkHelper you've pointed me to. I just copy it from there to here, since a) is private there b) mine action is not related to network. In following refactoring patch I'm removing this duplicity and placing this method where it rather should be defined instead of NetworkHelper. after that only the first method of these two will remain here. I can squash these changes, no problem. Line 91: Permissions perms = new Permissions(); Line 92: perms.setad_element_id(userId); Line 93: perms.setObjectType(objectType); Line 94: perms.setObjectId(entityId); http://gerrit.ovirt.org/#/c/29846/2/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(); > there is a certain order of actions in command execution, and validateInput I've proposed some solution on command base. This duplicity is hell not right. I've looked at hierarchy and if IDE is not mistaken, there's no overriding of "validateInput" and no overriding of "validateObject". So there's super easy solution: create validateParameters method which *only* checks javax.validation, I'm not aware of custom validators accessing DB, so this should be OK. And this method will be evaluated prior to authorization. There's nothing insecure about exposing rules of public api. And validateInputs would remain empty as template method to override if necessary. you're right about validateInput() I trust the method name and forget what it does. 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/2/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 removed. it isn;t used Done Line 9: v_MAC_POOL_USER UUID; Line 10: v_SUPER_USER_ID UUID; Line 11: Line 12: v_CREATE_MAC_POOL INTEGER; -- 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: 2 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
