Moti Asayag has posted comments on this change. Change subject: core: added user permissions to macPools ......................................................................
Patch Set 2: (4 comments) basically, 03_05_0760_add_permissions_to_mac_pools.sql looks good, minor 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 different role and objectType. 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 validateInputs() is being called from a single place only. invoking this prior to authorizing the user will provide more info to the user than required (i.e. existence/non existence of entities) you can just make this code null-safe, despite its ugliness. last, validateInput() does not fail the command by itself and won't prevent the following NPE to come. 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/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 78: validateInputs see previous comment. 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 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
