Omer Frenkel has posted comments on this change. Change subject: core: VirtIO console access key management ......................................................................
Patch Set 8: (2 comments) looks much better, thanks! one minor comment (that relevant for all commands) and a concern regarding permissions.. https://gerrit.ovirt.org/#/c/39512/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddUserProfileCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddUserProfileCommand.java: Line 34: return getSucceeded() ? AuditLogType.USER_ADD_PROFILE : AuditLogType.USER_ADD_PROFILE_FAILED; Line 35: } Line 36: Line 37: @Override Line 38: protected void setActionMessageParameters() { this method should only contain the parameters for the generic messages, means var-action-x and var-type-y and it is called automatically inside commandBase, so no need to call it again the specific failure messages should be added inside the canDoAction so in the canDoAction you should only have: if (somethingIsWrong) { result = failCanDoAction(msg); } (just a note - usually we use 'quick-return' in canDoActions, means once you found an issue, return. so it makes the method more readable (no need ti check the value of 'result' or complicated if-else, but here its not a must since its a short one) Line 39: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__ADD); Line 40: addCanDoActionMessage(VdcBllMessages.VAR__TYPE__USER_PROFILE); Line 41: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_PROFILE_ALREADY_EXISTS); Line 42: } https://gerrit.ovirt.org/#/c/39512/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UserProfilesOperationCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UserProfilesOperationCommandBase.java: Line 42: return result; Line 43: } Line 44: Line 45: @Override Line 46: public List<PermissionSubject> getPermissionCheckSubjects() { i wonder about this, i dont think any user has this permission for himself. since we enforce the user in the commands, we might skip permissions.. what do you think? unless we'd like to allow users to manipulate other users profiles, then we would need to find some other solution.. and just a note, since we specify the action group for each action in the VdcActionType enum, here you could just use getActionGroup() Line 47: return Collections.singletonList(new PermissionSubject(getUserId(), VdcObjectType.User, ActionGroup.MANIPULATE_USERS)); Line 48: } -- To view, visit https://gerrit.ovirt.org/39512 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5ff5403823e752e695ebde76a4b7fb83e07099b6 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@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
