-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NIFI-375: Added operation policy #2990
Conversation
Will review... |
@mcgilman Updated how API exposes required data to operate components. Although I need to do the similar refactoring with RGPs, the most complex ControllerService implementation is done. I updated the PR description, too, to illustrate how ControllerService enable/disable operations are authorized. It'd be great if you can review this while I'm working on the remaining RPG stuffs. Thanks! |
722fa60
to
a538639
Compare
@mcgilman I hope this PR is now fully ready for review. Thanks. |
@ijokarumawak Thanks for the additional commits! Will review... |
@ijokarumawak Thanks for the update. I'm still in the process of reviewing but one thing that concerns me is where we've identified Service Only in the scenarios above. Currently (before the PR) the Enable case we allow the user to specify if they want to enable just this service or this service and all components that reference it (including other services and their referencing components). During the Disable case, we require that the user disables this service and all referencing components. This is because the referencing components require this service's availability to continue running. The issue that we're hitting now is that a user with permissions outlined above with Service Only will be able to Enable this service but will be unable to subsequently disable it. Because of this, I'm wondering if we need to be even more strict to prevent this cases via the UI. I don't think its too restrictive as this is more of a corner case. The more common use case here will be granting operators permissions to the read policies and operation policies for these components. Thoughts? |
...ork-cluster/src/main/java/org/apache/nifi/cluster/manager/ControllerServiceEntityMerger.java
Outdated
Show resolved
Hide resolved
...k/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ControllerServiceResource.java
Outdated
Show resolved
Hide resolved
...framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/InputPortResource.java
Outdated
Show resolved
Hide resolved
...ramework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/OutputPortResource.java
Outdated
Show resolved
Hide resolved
...framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ProcessorResource.java
Outdated
Show resolved
Hide resolved
.../nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/RemoteProcessGroupResource.java
Outdated
Show resolved
Hide resolved
.../nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/RemoteProcessGroupResource.java
Outdated
Show resolved
Hide resolved
.../nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/RemoteProcessGroupResource.java
Outdated
Show resolved
Hide resolved
...ework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ReportingTaskResource.java
Outdated
Show resolved
Hide resolved
@@ -68,10 +75,13 @@ default void merge(final EntityType clientEntity, final Map<NodeIdentifier, Enti | |||
if (clientEntity.getBulletins().size() > MAX_BULLETINS_PER_COMPONENT) { | |||
clientEntity.setBulletins(clientEntity.getBulletins().subList(0, MAX_BULLETINS_PER_COMPONENT)); | |||
} | |||
} else { | |||
clientEntity.setBulletins(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to continue to set the component to null when canRead
is false. It may have been changed to accommodate an earlier iteration of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks for caching this!
The operation policy allows that a user to operate components even if they does not have direct READ/WRITE permission of the component. Following operations are controlled by the new operate policy: - Start/stop/enable/disable Processors, ControllerServices, ReportingTasks, Input/OuputPorts - Enable/disable transmission of RemoteInput/OutputPorts and RemoteProcessGroups - Terminate Processor threads
The previous commit let API exposes few fields in DTO. But we should avoid returning partial DTO as it complicates authorization logic. Instead, this commit adds StatusDTO for ReportingTaskEntity and ControllerServiceEntity, so that it can be returned regardless of having READ permission. Component DTO can only be returned with a READ permission.
- Cleaned up merger classes - Recreate DTO instance at each function during two phase commmit
a538639
to
2a1e6a1
Compare
@mcgilman Rebased with the latest master and incorporated the feedback. About the concern:
I added another commit to address this, but I'm going to remove that commit. Because... I confirmed that even before this PR, NiFi UI works like this if the user has READ/WRITE permissions for a CS, but only has READ to its referencing components. Such user can enable the CS if 'Service Only', but can not disable the CS because the referencing component can not be written. |
@ijokarumawak I see. You're correct that the existing codebase also exhibits this behavior. However, I think that its an issue we should consider fixing separately from this. Generally speaking, I think the UI should not allow the user to change something that they cannot change back (assuming the same permissions). Let's proceed here and we can address this concern in a future effort. Thanks... I'll continue reviewing! |
.../nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/RemoteProcessGroupResource.java
Outdated
Show resolved
Hide resolved
.../nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/RemoteProcessGroupResource.java
Outdated
Show resolved
Hide resolved
...uthorization/src/main/java/org/apache/nifi/authorization/resource/OperationAuthorizable.java
Outdated
Show resolved
Hide resolved
...uthorization/src/main/java/org/apache/nifi/authorization/resource/OperationAuthorizable.java
Outdated
Show resolved
Hide resolved
.../nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-remote-process-group.js
Outdated
Show resolved
Hide resolved
...ework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-settings.js
Outdated
Show resolved
Hide resolved
- Renamed confusing static method names and its parameters - Removed unnecessary permission checks from UI condition
...fi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/dto/DtoFactory.java
Outdated
Show resolved
Hide resolved
@ijokarumawak This is looking really good! Just seeing a couple minor things aside from the NPE I commented in the code. It looks like the Also, I think we need to apply the new Thanks again! |
@mcgilman Incorporated the last review comments. The original intent of having I wasn't consistent enough while I was refactoring the code at some point and added it to ComponentEntity. But I still think the operatePermissions should exist with only component those can be operated. So, I removed operatePermissions from ComponentEntity. I hope this we have polished this PR enough to get merged. Thanks for your comprehensive review comments! |
@ijokarumawak Thanks this looks good. I'm going to squash and merged to master. I've created two new JIRAs that are related to this effort but separate from the initial concern. Thanks again for working through all the review comments! https://issues.apache.org/jira/browse/NIFI-5610 |
The operation policy allows that a user to operate components even if they does not have direct READ/WRITE permission of the component. Following operations are controlled by the new operate policy: - Start/stop/enable/disable Processors, ControllerServices, ReportingTasks, Input/OuputPorts - Enable/disable transmission of RemoteInput/OutputPorts and RemoteProcessGroups - Terminate Processor threads Refactored what API exposes The previous commit let API exposes few fields in DTO. But we should avoid returning partial DTO as it complicates authorization logic. Instead, this commit adds StatusDTO for ReportingTaskEntity and ControllerServiceEntity, so that it can be returned regardless of having READ permission. Component DTO can only be returned with a READ permission. Refactor RPG same as ControllerService. WIP incorporating review comments. Incorporated review comments - Cleaned up merger classes - Recreate DTO instance at each function during two phase commmit Restrict enabling ControllerService without read permission Revert the last commit. Fix review comments. - Renamed confusing static method names and its parameters - Removed unnecessary permission checks from UI condition Fixed delete action display condition. Fixed NPE at Summary. Apply operation policy to activateControllerServices. Removed OperationPermissible from ComponentEntity. This closes apache#2990
The operation policy allows that a user to operate components even if they does not have direct READ/WRITE permission of the component.
Following operations are controlled by the new operate policy:
ReportingTasks, Input/OuputPorts
RemoteProcessGroups
ControllerServices table view
Enabling/Disabling ControllerService
This PR supports following spec based on the ControllerService and its referencing components permissions:
controller-services/{id}/run-status
can be successful, and can be used from operational scripts. NOTE Technically we can update the UI to do the same thing, but currently this PR doesn't allow this from UI.ReportingTasks view
Thank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.