Martin Mucha has posted comments on this change. Change subject: core: added user permissions to macPools ......................................................................
Patch Set 6: (5 comments) http://gerrit.ovirt.org/#/c/29846/6/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 73: final Guid macPoolId = getMacPoolId(); Line 74: boolean invalidParameters = macPoolId == null; Line 75: //in case of invalid user input, return some improbable to succeed ID, and parameter validation Line 76: // yet to be invoked will handle this. Line 77: Guid macPoolIdToUse = invalidParameters ? Guid.Empty : macPoolId; > this isn't necessary. If the id is null, it can be provided as a valid valu Done Line 78: Line 79: return Collections.singletonList(new PermissionSubject(macPoolIdToUse, Line 80: VdcObjectType.MacPool, Line 81: ActionGroup.DELETE_MAC_POOL)); http://gerrit.ovirt.org/#/c/29846/6/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 79: final Guid macPoolId = macPool == null ? null : macPool.getId(); Line 80: boolean invalidParameters = macPoolId == null; Line 81: //in case of invalid user input, return some improbable to succeed ID, and parameter validation Line 82: // yet to be invoked will handle this. Line 83: Guid macPoolIdToUse = invalidParameters ? Guid.Empty : macPoolId; > based of previous comment regarding the usage of Guid.Empty, Done Line 84: Line 85: return Collections.singletonList(new PermissionSubject(macPoolIdToUse, Line 86: VdcObjectType.MacPool, Line 87: ActionGroup.EDIT_MAC_POOL)); http://gerrit.ovirt.org/#/c/29846/6/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: > Lines 103-109 could be replaced with: thanks for the correction about backward compatibility. You right, but in that case we need to do it differently. Since when NO macPoolId is sent, default pool is queried and used instead. So we have to check for access right for that pool, not don't do any access checks at all. Default mac pool should have rights for everyone, but if not, this should fail. I've changed code to adopt this. I hope you'll agree with this. Line 104: final StoragePool storagePool = getParameters().getStoragePool(); Line 105: final Guid macPoolId = storagePool == null ? null : storagePool.getMacPoolId(); Line 106: boolean invalidParameters = macPoolId == null; Line 107: //in case of invalid user input, return some improbable to succeed ID, and parameter validation http://gerrit.ovirt.org/#/c/29846/6/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 61: public UpdateStoragePoolCommand(T parameters, CommandContext commandContext) { Line 62: super(parameters, commandContext); Line 63: } Line 64: Line 65: private StoragePool oldStoragePool; > as a note to future features and patches to be, the refactor/naming cleanup this is the problem of "per-reviewer-conventions": I've encountered conflicting requests: "why do you do this in *this* patch when you don't need it in *this* patch? Do this in patch where it's required". So far I've found only one general rule of thumb. Regardless of whichever way I do it, during CR it's required to do it alternatively. Small commits must become bigger, big smaller, efficient code need to be crippled, inefficient improved, ... Next time, I'll try to do it in separate patch preceding one requiring change. Line 66: private StorageDomain masterDomainForPool; Line 67: Line 68: @Override Line 69: protected void executeCommand() { Line 324: boolean invalidParameters = macPoolId == null; Line 325: //in case of invalid user input, return some improbable to succeed ID, and parameter validation Line 326: // yet to be invoked will handle this. Line 327: Guid requestedMacPoolId = invalidParameters ? Guid.Empty : macPoolId; Line 328: > please see both comments regarding not using Guid.Empty as a default and al Done Line 329: Line 330: final boolean changingPoolDefinition = requestedMacPoolId.equals(getOldMacPoolId()); Line 331: if (changingPoolDefinition) { Line 332: result.add(new PermissionSubject(requestedMacPoolId, VdcObjectType.MacPool, ActionGroup.CONFIGURE_MAC_POOL)); -- 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: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Lior Vernia <[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
