You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by jtstorck <gi...@git.apache.org> on 2016/07/20 19:29:00 UTC
[GitHub] nifi pull request #694: NIFI-1876 Implements merging of responses to success...
GitHub user jtstorck opened a pull request:
https://github.com/apache/nifi/pull/694
NIFI-1876 Implements merging of responses to successful requests base\u2026
\u2026d on authorization, returning the most restrictive response
Added StandardHttpResponseMergerSpec for testing response merging
Added Permissible interface
Added nifi-api/controller/archive to ProcessGroupEndpointMerger
Removed AbstractMultiEntityEndpoint.java, not used anymore
Implemented reponse merging for GET requests where there are some successful and problematic responses, returning most restrictive one.
Updated nf-settings.js with ControllerConfigurationEntity property rename from controllerConfiguration to component
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jtstorck/nifi NIFI-1876
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/694.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #694
----
commit 6801e5501d45939487046e321895ff82cfd34362
Author: Jeff Storck <jt...@gmail.com>
Date: 2016-07-02T00:49:49Z
NIFI-1876 Implements merging of responses to successful requests based on authorization, returning the most restrictive response
Added StandardHttpResponseMergerSpec for testing response merging
Added Permissible interface
Added nifi-api/controller/archive to ProcessGroupEndpointMerger
Removed AbstractMultiEntityEndpoint.java, not used anymore
Implemented reponse merging for GET requests where there are some successful and problematic responses, returning most restrictive one.
Updated nf-settings.js with ControllerConfigurationEntity property rename from controllerConfiguration to component
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/694
@jtstorck I think we also need a CounterEndpointMerger.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/694
Reviewing
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by jtstorck <gi...@git.apache.org>.
Github user jtstorck commented on the issue:
https://github.com/apache/nifi/pull/694
Regarding the concerns with the viewing data, our current implementation looks correct. The queries for provenance events are run on each node invidually, and each event is unique to the node on which it occurred; there is no concept of merging a particular provenance event across nodes in the cluster. Per node, the configured authorizer is used to check for the current permissions for each event returned by the query. The results returned to the client are correct based on the per-node authorizer checks performed at the time of the query. Using the AbstractPolicyBasedAuthorizer, this case will not occur, since the policies are forced to be in sync across the cluster. Using any delegating authorizer, NiFi does not have control over the actual policies, and therefore can only operate based on the decision made by the authorizer on the particular node from which it was called.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/694
Hey Jeff, I checked out your most recent commit and it looks like there are some bugs with the Summary page. I am able to see names and types of processors that I do not have "view" permissions for. Also I see JS errors when I try to view the cluster-wise breakdown of stats. Screenshots for both are below.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/694
+1
Visually verified code, did a contrib check build. In a 3 node secure cluster I modified 2 of the nodes to always return accept and the third to use policies like normal. I then went through every possible way I could think of that would interact with policies to be sure they worked as expected. Looks good @jtstorck, I will merge it in.
Also I put in a fix to the URI pattern of counters here: https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/main/java/org/apache/nifi/cluster/coordination/node/NodeClusterCoordinator.java#L86-L86
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by jtstorck <gi...@git.apache.org>.
Github user jtstorck commented on the issue:
https://github.com/apache/nifi/pull/694
After doing some investigation, I saw that the FlowMerger wasn't merging labels or funnels, it was just picking the first instance of the component from each node. I added the use of the mergers for labels and funnels to the FlowMerger and tested on a three-node cluster, and the merging looks good now. With one node denying the read permissions on labels and funnels, the UI is correctly disabling them. Ready for review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/694
@jtstorck, testing using one node that acts normally and 2 that always allow.
Labels don't seem to get their permissions merged properly. I verified on master that they work as expected when you have a cluster that is homogenous but on in my test cluster I am able to see regardless what the policy is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by jtstorck <gi...@git.apache.org>.
Github user jtstorck commented on the issue:
https://github.com/apache/nifi/pull/694
Rebased on master and resolved conflicts. Ready for review again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by jtstorck <gi...@git.apache.org>.
Github user jtstorck commented on the issue:
https://github.com/apache/nifi/pull/694
Ready for review. This PR implements the changes requested in NIFI-1876 and NIFI-2264.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/694
Hey @jtstorck, the "View the data" and "Modify the data" policies are not being merged/properly taken into account when querying/working with provenance events. (In both scenarios the user is a part of the "query provenance" policy)
First when a user doesn't have "Modify the data" on a component on one node, it will correctly deny any replay requests that are of events that originated on that node. That said, if an event that originated on another node is submitted for replay it will succeed.
A potential problem with "View the data" comes about when one node doesn't have the "view the data" policy but the others do and you attempt to query provenance. As a user I would expect the most strict policy (deny) to be merged and I would not be able to "View the data" from any node. Unfortunately the way it works currently (I believe) is that the query gets sent to the node to vet and it will take into account any policies and return the events. Then the events are merged. This means that the user will be able to see events from the allowing nodes. I'm not sure there is currently a way to merge these properly/effectively.
@mcgilman may have more insight.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #694: NIFI-1876 Implements merging of responses to success...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/694#discussion_r72731831
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/dto/EntityFactory.java ---
@@ -77,6 +77,8 @@ public ProcessorEntity createProcessorEntity(final ProcessorDTO dto, final Revis
if (dto != null) {
entity.setPermissions(permissions);
entity.setStatus(status);
+ status.setCanRead(permissions.getCanRead());
--- End diff --
Yup, hit it when I was trying to delete a connection:
2016-07-28 21:41:44,415 INFO [NiFi Web Server-231] org.apache.nifi.web.filter.RequestLogger Attempting request for (anonymous) DELETE http://localhost:8080/nifi-api/connections/344c598c-0156-1000-0000-000053115355 (source ip: 127.0.0.1)
2016-07-28 21:41:44,429 ERROR [NiFi Web Server-231] o.a.nifi.web.api.config.ThrowableMapper An unexpected error has occurred: java.lang.NullPointerException. Returning Internal Server Error response.
java.lang.NullPointerException: null
at org.apache.nifi.web.api.dto.EntityFactory.createConnectionEntity(EntityFactory.java:251) ~[classes/:na]
at org.apache.nifi.web.StandardNiFiServiceFacade.deleteConnection(StandardNiFiServiceFacade.java:858) ~[classes/:1.0.0-SNAPSHOT]
at org.apache.nifi.web.StandardNiFiServiceFacade$$FastClassBySpringCGLIB$$358780e0.invoke(<generated>) ~[classes/:1.0.0-SNAPSHOT]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by jtstorck <gi...@git.apache.org>.
Github user jtstorck commented on the issue:
https://github.com/apache/nifi/pull/694
PropertyDescriptor merging has been added, and the Summary view now shows components property based on permissions. Ready for review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by jtstorck <gi...@git.apache.org>.
Github user jtstorck commented on the issue:
https://github.com/apache/nifi/pull/694
@JPercivall Labels don't currently have a merger, and I will create one today. Need to do some research on the data viewing policies.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by jtstorck <gi...@git.apache.org>.
Github user jtstorck commented on the issue:
https://github.com/apache/nifi/pull/694
Updated to merge labels, funnels, and controller service references, with unit tests. Ready for review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the issue:
https://github.com/apache/nifi/pull/694
Hey @jtstorck, I just tried testing the latest commit and I don't think everything was taken into account from the UI perspective with Labels and Funnels. A couple of things I noticed (in a cluster 2 saying always Yes another is policy based):
- Without Write permission for a Label, the UI still allows the user to open the configuration up and attempt to make a change.
- I am able to see a label and it's contents without having the view permission
- For Funnels (without having Read or Write) I can still attempt to move it, copy/paste and drag the connections.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #694: NIFI-1876 Implements merging of responses to success...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi/pull/694#discussion_r72728226
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/dto/EntityFactory.java ---
@@ -77,6 +77,8 @@ public ProcessorEntity createProcessorEntity(final ProcessorDTO dto, final Revis
if (dto != null) {
entity.setPermissions(permissions);
entity.setStatus(status);
+ status.setCanRead(permissions.getCanRead());
--- End diff --
I believe the potential for a NPE was introduce here. Permissions doesn't get checked for null until line 85.
This also applies to the other changes in this file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by jtstorck <gi...@git.apache.org>.
Github user jtstorck commented on the issue:
https://github.com/apache/nifi/pull/694
Addressed the EntityFactory NPE concerns, rebased into a single commit. Ready for review again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #694: NIFI-1876 Implements merging of responses to success...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/694
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #694: NIFI-1876 Implements merging of responses to successful req...
Posted by jtstorck <gi...@git.apache.org>.
Github user jtstorck commented on the issue:
https://github.com/apache/nifi/pull/694
Resolved conflicts to StandardNifiServiceFacade.java. Ready for review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---