Moti Asayag has posted comments on this change. Change subject: core: added user permissions to macPools ......................................................................
Patch Set 6: (5 comments) I think we're pretty close to the end of this permissions sage :-) 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 value to the permissions check, and it will fail with the same "User in not authorized to...", the same way as Guid.Empty will. I don't expect any case in which an empty id will be added to the mac pool table, but the condition above isn't required. return Collections.singletonList(new PermissionSubject(getMacPoolId(), VdcObjectType.MacPool, ActionGroup.DELETE_MAC_POOL)); 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, lines 78-83 can be simplified to: Guid macPoolIdToUse = getParameters().getMacPool() == null ? null : getParameters().getMacPool().getId(); 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: Guid requestedMacPoolId = getStoragePool() == null ? null : getStoragePool().getMacPoolId(); but only if the mac pool id is not null, we should add it to the list. The reason is to maintain backward compatibility with existing code that doesn't demand a mac pool in order to create a new data center. 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 of this file could be sent on its own patch to reduce noise from the current patch. Such cleanup patch is easy to review and to approve and it prevents obscuring logic of the current patch. 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 also the need to maintain backward compatibility. We should check for permissions on the mac pool id only if it was changed to something other than null, since we don't enforce mac pool id to be provided as part of this command, do we ? 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 <mmu...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: 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
