You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by markobean <gi...@git.apache.org> on 2018/05/15 00:37:26 UTC
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
GitHub user markobean opened a pull request:
https://github.com/apache/nifi/pull/2703
NIFI-4907: add 'view provenance' component policy
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:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [x] Have you ensured that format looks appropriate for the output in which it is rendered?
### 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/markobean/nifi NIFI-4907
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2703.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 #2703
----
commit eed76e9aa2891eeccb7ef4bb4a5890a178aa7a8f
Author: Mark Bean <ma...@...>
Date: 2018-05-12T20:32:29Z
NIFI-4907: add 'view provenance' component policy
----
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190939869
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1489,6 +1492,12 @@ public int compare(AttributeDTO a1, AttributeDTO a2) {
dto.setChildUuids(childUuids);
}
+ // lineage duration
+ if (event.getLineageStartDate() > 0) {
--- End diff --
If we don't piggyback off of summarization, I believe this can be moved back.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r191052573
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
nifi.security.identity.mapping.pattern.kerb=^(.*?)/instance@(.*?)$
nifi.security.identity.mapping.value.kerb=$1@$2
-nifi.security.identity.mapping.transform.kerb=NONE
----
The last segment of each property is an identifier used to associate the pattern with the replacement value. When a user makes a request to NiFi, their identity is checked to see if it matches each of those patterns in lexicographical order. For the first one that matches, the replacement specified in the `nifi.security.identity.mapping.value.xxxx` property is used. So a login with `CN=localhost, OU=Apache NiFi, O=Apache, L=Santa Monica, ST=CA, C=US` matches the DN mapping pattern above and the DN mapping value `$1@$2` is applied. The user is normalized to `localhost@Apache NiFi`.
-In addition to mapping a transform may be applied. The supported versions are NONE (no transform applied), LOWER (identity lowercased), and UPPER (identity uppercased). If not specified, the default value is NONE.
--- End diff --
Somehow, there was a bad rebase to master which removed some recently modified lines. Re-rebased to master.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190921858
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/FlowController.java ---
@@ -3464,10 +3462,6 @@ public ReportingTaskNode createReportingTask(final String type, final String id,
LoggableComponent<ReportingTask> task = null;
boolean creationSuccessful = true;
-
- // make sure the first reference to LogRepository happens outside of a NarCloseable so that we use the framework's ClassLoader
- final LogRepository logRepository = LogRepositoryFactory.getRepository(id);
--- End diff --
I don't think we can move this line. This needs to happen outside of the NarCloseable. Please refer to JIRA it was added for additional information. https://issues.apache.org/jira/browse/NIFI-5136
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190909631
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
nifi.security.identity.mapping.pattern.kerb=^(.*?)/instance@(.*?)$
nifi.security.identity.mapping.value.kerb=$1@$2
-nifi.security.identity.mapping.transform.kerb=NONE
----
The last segment of each property is an identifier used to associate the pattern with the replacement value. When a user makes a request to NiFi, their identity is checked to see if it matches each of those patterns in lexicographical order. For the first one that matches, the replacement specified in the `nifi.security.identity.mapping.value.xxxx` property is used. So a login with `CN=localhost, OU=Apache NiFi, O=Apache, L=Santa Monica, ST=CA, C=US` matches the DN mapping pattern above and the DN mapping value `$1@$2` is applied. The user is normalized to `localhost@Apache NiFi`.
-In addition to mapping a transform may be applied. The supported versions are NONE (no transform applied), LOWER (identity lowercased), and UPPER (identity uppercased). If not specified, the default value is NONE.
--- End diff --
Did you intend to remove this?
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194499578
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1389,104 +1420,119 @@ private ProvenanceEventDTO createProvenanceEventDto(final ProvenanceEventRecord
// sets the component details if it can find the component still in the flow
setComponentDetails(dto);
- // only include all details if not summarizing
- if (!summarize) {
- // convert the attributes
- final Comparator<AttributeDTO> attributeComparator = new Comparator<AttributeDTO>() {
- @Override
- public int compare(AttributeDTO a1, AttributeDTO a2) {
- return Collator.getInstance(Locale.US).compare(a1.getName(), a2.getName());
- }
- };
+// try {
+// AuthorizationResult result = flowController.checkConnectableAuthorization(event.getComponentId());
+ AuthorizationResult result = checkConnectableAuthorization(event.getComponentId());
+ if (Result.Denied.equals(result.getResult())) {
+ dto.setComponentType("Processor"); // is this always a Processor?
+ dto.setComponentName(dto.getComponentId());
+ dto.setEventType("UNKNOWN");
+ }
- final SortedSet<AttributeDTO> attributes = new TreeSet<>(attributeComparator);
+// authorizeData(event);
+ final AuthorizationResult dataResult = checkAuthorizationForData(event); //(authorizer, RequestAction.READ, user, event.getAttributes());
- final Map<String, String> updatedAttrs = event.getUpdatedAttributes();
- final Map<String, String> previousAttrs = event.getPreviousAttributes();
+ // only include all details if not summarizing and approved
+ if (!summarize && Result.Approved.equals(dataResult.getResult())) {
--- End diff --
If the user is not authorized for the data of a component we should still be able to return a non-summary. In this case, we should just be leaving out any of the data fields in the ProvenanceEventDto. I would consider these fields data fields as they are associated with either attributes, content, or replay (all of which requires data policies to execute).
```
private Collection<AttributeDTO> attributes;
private Boolean contentEqual;
private Boolean inputContentAvailable;
private String inputContentClaimSection;
private String inputContentClaimContainer;
private String inputContentClaimIdentifier;
private Long inputContentClaimOffset;
private String inputContentClaimFileSize;
private Long inputContentClaimFileSizeBytes;
private Boolean outputContentAvailable;
private String outputContentClaimSection;
private String outputContentClaimContainer;
private String outputContentClaimIdentifier;
private Long outputContentClaimOffset;
private String outputContentClaimFileSize;
private Long outputContentClaimFileSizeBytes;
private Boolean replayAvailable;
private String replayExplanation;
private String sourceConnectionIdentifier;
```
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r192493603
--- Diff: nifi-nar-bundles/nifi-provenance-repository-bundle/nifi-persistent-provenance-repository/src/main/java/org/apache/nifi/provenance/authorization/UserEventAuthorizer.java ---
@@ -65,12 +61,7 @@ public void authorize(final ProvenanceEventRecord event) {
return;
}
- final Authorizable eventAuthorizable;
- if (event.isRemotePortType()) {
- eventAuthorizable = resourceFactory.createRemoteDataAuthorizable(event.getComponentId());
- } else {
- eventAuthorizable = resourceFactory.createLocalDataAuthorizable(event.getComponentId());
- }
+ final Authorizable eventAuthorizable = resourceFactory.createProvenanceDataAuthorizable(event.getComponentId());
eventAuthorizable.authorize(authorizer, RequestAction.READ, user, event.getAttributes());
--- End diff --
I don't think the attributes are necessary here. I'm pretty sure the event attributes would be necessary for authorizing access to attributes/content.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194718895
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1489,6 +1492,12 @@ public int compare(AttributeDTO a1, AttributeDTO a2) {
dto.setChildUuids(childUuids);
}
+ // lineage duration
+ if (event.getLineageStartDate() > 0) {
--- End diff --
lineage duration was pulled out specifically because there was a case in which the duration was not properly populated. This was during early testing and may now be corrected by other changes. I will try to replicate.
---
[GitHub] nifi issue #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/2703
Will review...
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r191052561
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
--- End diff --
Somehow, there was a bad rebase to master which removed some recently modified lines. Re-rebased to master.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194495873
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1389,104 +1420,119 @@ private ProvenanceEventDTO createProvenanceEventDto(final ProvenanceEventRecord
// sets the component details if it can find the component still in the flow
setComponentDetails(dto);
- // only include all details if not summarizing
- if (!summarize) {
- // convert the attributes
- final Comparator<AttributeDTO> attributeComparator = new Comparator<AttributeDTO>() {
- @Override
- public int compare(AttributeDTO a1, AttributeDTO a2) {
- return Collator.getInstance(Locale.US).compare(a1.getName(), a2.getName());
- }
- };
+// try {
+// AuthorizationResult result = flowController.checkConnectableAuthorization(event.getComponentId());
+ AuthorizationResult result = checkConnectableAuthorization(event.getComponentId());
--- End diff --
Why not check the authorization within `setComponentDetails`? In there you already have the components to authorize and you'll know the corresponding type.
---
[GitHub] nifi issue #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/2703
@markobean I just ran your PR and I'm not seeing the same behavior you are describing. Even without the component policy, I'm able to view the provenance event. This is the behavior I was expecting to see following the discussion on the JIRA. It's possible that we're using the same language to refer to different things. Let me try to elaborate/clarify a bit here.
/processors/1234 - component policy (controls access to a component and its config)
/provenance-data/processors/1234 - comopnent provenance event policy (controls access to the provenance events from a component)
/data/processors/1234 - component data policy (controls access to the data from a component including flowfile attributes)
The line you referenced should only verify access to the component provenance event (and it appears that's how it's working). It should not be checking the component policy. My suggestion was to additionally check the component policy prior to populating the component details (`setComponentDetails`). This would be in line with your initial comment on this JIRA.
With your most recent changes, I'm not sure its functionality is different than before. It seems that it would be impossible to get a non-summarized event without permissions to the data of the component. I think we only need to verify permissions to data of a component for the attributes and the content specific fields. Other fields should be ok, allowing for a non-summarized event for folks without access to a component's data.
It appears that `checkAuthorizationForReplay` was also verifying that the connection that would be replayed into still exists. This would affect the availability of the replay action. Also, while a little nit-picky, I would also suggest using the `checkAuthorization...` methods which return an `AuthorizationResult` instead of relying on an `Exception` during a non-exceptional case. The generation of the stack trace is an expensive operation.
Also, it does not seem like you updated or replied to my comment regarding the need to include the flowfile attributes when authorizing access to component's provenance events. I think these are only necessary when authorizing access to a component's data.
Please do not squash additional commits. It makes it difficult to review when I cannot easily see the incremental changes.
Thanks!
---
[GitHub] nifi issue #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on the issue:
https://github.com/apache/nifi/pull/2703
I believe the latest changes are demonstrating the intended functionality. Provenance events are only listed in the query results if the user has 'view provenance' on the corresponding component; flowfile content in the event details is only visible based on 'view the data' policy; component name and type is only visible based on 'view the component' policy (replaced with generic info such as UUID in place of name when policy is lacking.) I'll do some further testing today.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r191444067
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1359,7 +1363,12 @@ public ProvenanceEventDTO getProvenanceEvent(final Long eventId) {
} else {
dataAuthorizable = flowController.createLocalDataAuthorizable(event.getComponentId());
}
- dataAuthorizable.authorize(authorizer, RequestAction.READ, NiFiUserUtils.getNiFiUser(), attributes);
+ // If not authorized for 'view the data', create only summarized provenance event
--- End diff --
@markobean The summary concept was introduced for performance reasons [1]. The summary represents the details required to render a row in the table. Some events can contain a lot of details (many children/parents UUIDs, flowfile attributes, etc) which was causing the table to load extremely slowly. The fully populated event (not summary) is returned once a dialog is opened and those details can be rendered.
My suggestion would be to not modify the summary concept. Returning more details in the summary for users with access to the event but not the data will begin to regress NIFI-1135. Artificially withholding event fields they should have access to also doesn't seem right.
Since we're moving to this super granular approach, I would recommend the following.
1) `createProvenanceEventDto(...)` is only invoked once we know the user has permissions to the event.
2) Within `createProvenanceEventDto(...)` I would check if the user is allowed to access that component to populate the component details. If the user does not have access, I would use the ID in place of the name and 'Processor' in place of the fully qualified class name (for Processors).
3) Within `createProvenanceEventDto(...)` I would check if the user is allowed to access the component's data to populate the attributes and content details. If the user does not have access, I would leave those fields unset.
This should retain the summary concept while introducing the granular approach we're looking for. Thoughts?
[1] https://issues.apache.org/jira/browse/NIFI-1135
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190921803
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/FlowController.java ---
@@ -1152,10 +1153,6 @@ public ProcessorNode createProcessor(final String type, String id, final BundleC
boolean creationSuccessful;
LoggableComponent<Processor> processor;
-
- // make sure the first reference to LogRepository happens outside of a NarCloseable so that we use the framework's ClassLoader
- final LogRepository logRepository = LogRepositoryFactory.getRepository(id);
--- End diff --
I don't think we can move this line. This needs to happen outside of the NarCloseable. Please refer to JIRA it was added for additional information. https://issues.apache.org/jira/browse/NIFI-5136
---
[GitHub] nifi issue #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/2703
Thanks for having a look. I'll include these when I merge in your changes.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194512038
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1389,104 +1420,119 @@ private ProvenanceEventDTO createProvenanceEventDto(final ProvenanceEventRecord
// sets the component details if it can find the component still in the flow
setComponentDetails(dto);
- // only include all details if not summarizing
- if (!summarize) {
- // convert the attributes
- final Comparator<AttributeDTO> attributeComparator = new Comparator<AttributeDTO>() {
- @Override
- public int compare(AttributeDTO a1, AttributeDTO a2) {
- return Collator.getInstance(Locale.US).compare(a1.getName(), a2.getName());
- }
- };
+// try {
+// AuthorizationResult result = flowController.checkConnectableAuthorization(event.getComponentId());
+ AuthorizationResult result = checkConnectableAuthorization(event.getComponentId());
+ if (Result.Denied.equals(result.getResult())) {
+ dto.setComponentType("Processor"); // is this always a Processor?
+ dto.setComponentName(dto.getComponentId());
+ dto.setEventType("UNKNOWN");
--- End diff --
Yes, I agree. The event type should be controlled by the new provenance event policy. It is not controlled by the component policy that protects the component name and component type.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r191052587
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-file-authorizer/src/main/java/org/apache/nifi/authorization/RoleAccessPolicy.java ---
@@ -63,7 +63,7 @@ public String getAction() {
final Set<RoleAccessPolicy> provenancePolicies = new HashSet<>();
provenancePolicies.add(new RoleAccessPolicy(ResourceType.Provenance.getValue(), READ_ACTION));
if (rootGroupId != null) {
- provenancePolicies.add(new RoleAccessPolicy(ResourceType.Data.getValue() + ResourceType.ProcessGroup.getValue() + "/" + rootGroupId, READ_ACTION));
+ provenancePolicies.add(new RoleAccessPolicy(ResourceType.ProvenanceData.getValue() + ResourceType.ProcessGroup.getValue() + "/" + rootGroupId, READ_ACTION));
--- End diff --
Agree. Added Data back in.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r192493635
--- Diff: nifi-nar-bundles/nifi-provenance-repository-bundle/nifi-volatile-provenance-repository/src/main/java/org/apache/nifi/provenance/VolatileProvenanceRepository.java ---
@@ -280,12 +276,7 @@ protected void authorize(final ProvenanceEventRecord event, final NiFiUser user)
return;
}
- final Authorizable eventAuthorizable;
- if (event.isRemotePortType()) {
- eventAuthorizable = resourceFactory.createRemoteDataAuthorizable(event.getComponentId());
- } else {
- eventAuthorizable = resourceFactory.createLocalDataAuthorizable(event.getComponentId());
- }
+ final Authorizable eventAuthorizable = resourceFactory.createProvenanceDataAuthorizable(event.getComponentId());
eventAuthorizable.authorize(authorizer, RequestAction.READ, user, event.getAttributes());
--- End diff --
I don't think the attributes are necessary here. I'm pretty sure the event attributes would be necessary for authorizing access to attributes/content.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r191052564
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
--- End diff --
Somehow, there was a bad rebase to master which removed some recently modified lines. Re-rebased to master.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190921900
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/FlowController.java ---
@@ -3669,12 +3664,10 @@ public FlowRegistryClient getFlowRegistryClient() {
@Override
public ControllerServiceNode createControllerService(final String type, final String id, final BundleCoordinate bundleCoordinate, final Set<URL> additionalUrls, final boolean firstTimeAdded) {
- // make sure the first reference to LogRepository happens outside of a NarCloseable so that we use the framework's ClassLoader
- final LogRepository logRepository = LogRepositoryFactory.getRepository(id);
--- End diff --
I don't think we can move this line. This needs to happen outside of the NarCloseable. Please refer to JIRA it was added for additional information. https://issues.apache.org/jira/browse/NIFI-5136
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194718350
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1338,6 +1339,67 @@ private void authorizeReplay(final ProvenanceEventRecord event) {
dataAuthorizable.authorize(authorizer, RequestAction.WRITE, user, eventAttributes);
}
+ private AuthorizationResult checkAuthorizationForData(ProvenanceEventRecord event) {
+ final NiFiUser user = NiFiUserUtils.getNiFiUser();
+ final Authorizable dataAuthorizable;
+ if (event.isRemotePortType()) {
+ dataAuthorizable = flowController.createRemoteDataAuthorizable(event.getComponentId());
+ } else {
+ dataAuthorizable = flowController.createLocalDataAuthorizable(event.getComponentId());
+ }
+
+ final Map<String, String> eventAttributes = event.getAttributes();
+
+ // ensure we can read the data
+ return dataAuthorizable.checkAuthorization(authorizer, RequestAction.READ, user, eventAttributes);
+ }
+
+ private AuthorizationResult checkAuthorizationForProvenanceData(final ProvenanceEventRecord event) {
--- End diff --
I modified this method and checkConnectableAuthorization() to accomodate a Process Group being the event component. This is the case for DOWNLOAD provenance events.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r191052566
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
nifi.security.identity.mapping.pattern.kerb=^(.*?)/instance@(.*?)$
nifi.security.identity.mapping.value.kerb=$1@$2
-nifi.security.identity.mapping.transform.kerb=NONE
--- End diff --
Somehow, there was a bad rebase to master which removed some recently modified lines. Re-rebased to master.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190919423
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
--- End diff --
Did you intend to remove this?
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r191052568
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
nifi.security.identity.mapping.pattern.kerb=^(.*?)/instance@(.*?)$
nifi.security.identity.mapping.value.kerb=$1@$2
-nifi.security.identity.mapping.transform.kerb=NONE
--- End diff --
Somehow, there was a bad rebase to master which removed some recently modified lines. Re-rebased to master.
---
[GitHub] nifi issue #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on the issue:
https://github.com/apache/nifi/pull/2703
Fixed conflicts with master.
Added NIFI-5207 since it is from the same policy refactor and required minimal code change.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190919443
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
nifi.security.identity.mapping.pattern.kerb=^(.*?)/instance@(.*?)$
nifi.security.identity.mapping.value.kerb=$1@$2
-nifi.security.identity.mapping.transform.kerb=NONE
--- End diff --
Did you intend to remove this?
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190909604
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
nifi.security.identity.mapping.pattern.kerb=^(.*?)/instance@(.*?)$
nifi.security.identity.mapping.value.kerb=$1@$2
-nifi.security.identity.mapping.transform.kerb=NONE
--- End diff --
Did you intend to remove this?
---
[GitHub] nifi issue #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/2703
Things are looking pretty good. I'd like to propose a few additional changes which I'm implemented here [1]. Please review them and let me know your thoughts. Thanks!
[1] https://github.com/mcgilman/nifi/commit/eed1be3dffcdd11d82746c1ba04bfd1ff68b5fc9
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194513579
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/FlowController.java ---
@@ -4919,6 +4925,22 @@ private void updateRemoteProcessGroups() {
return new ArrayList<>(provenanceRepository.getEvents(firstEventId, maxRecords));
}
+ public AuthorizationResult checkConnectableAuthorization(final String componentId) {
--- End diff --
Correct. This was moved to ControllerFacade.java. I will remove it from FlowController.java.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194495379
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/FlowController.java ---
@@ -4919,6 +4925,22 @@ private void updateRemoteProcessGroups() {
return new ArrayList<>(provenanceRepository.getEvents(firstEventId, maxRecords));
}
+ public AuthorizationResult checkConnectableAuthorization(final String componentId) {
--- End diff --
I don't believe this is called.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190919397
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
nifi.security.identity.mapping.pattern.kerb=^(.*?)/instance@(.*?)$
nifi.security.identity.mapping.value.kerb=$1@$2
-nifi.security.identity.mapping.transform.kerb=NONE
----
The last segment of each property is an identifier used to associate the pattern with the replacement value. When a user makes a request to NiFi, their identity is checked to see if it matches each of those patterns in lexicographical order. For the first one that matches, the replacement specified in the `nifi.security.identity.mapping.value.xxxx` property is used. So a login with `CN=localhost, OU=Apache NiFi, O=Apache, L=Santa Monica, ST=CA, C=US` matches the DN mapping pattern above and the DN mapping value `$1@$2` is applied. The user is normalized to `localhost@Apache NiFi`.
-In addition to mapping a transform may be applied. The supported versions are NONE (no transform applied), LOWER (identity lowercased), and UPPER (identity uppercased). If not specified, the default value is NONE.
--- End diff --
Did you intend to remove this?
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194496260
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1389,104 +1420,119 @@ private ProvenanceEventDTO createProvenanceEventDto(final ProvenanceEventRecord
// sets the component details if it can find the component still in the flow
setComponentDetails(dto);
- // only include all details if not summarizing
- if (!summarize) {
- // convert the attributes
- final Comparator<AttributeDTO> attributeComparator = new Comparator<AttributeDTO>() {
- @Override
- public int compare(AttributeDTO a1, AttributeDTO a2) {
- return Collator.getInstance(Locale.US).compare(a1.getName(), a2.getName());
- }
- };
+// try {
+// AuthorizationResult result = flowController.checkConnectableAuthorization(event.getComponentId());
+ AuthorizationResult result = checkConnectableAuthorization(event.getComponentId());
+ if (Result.Denied.equals(result.getResult())) {
+ dto.setComponentType("Processor"); // is this always a Processor?
+ dto.setComponentName(dto.getComponentId());
+ dto.setEventType("UNKNOWN");
--- End diff --
Do you think that we need to redact the event type when the user does not have permissions to the component policy? I would have considered this field under the new provenance event policy.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r192493531
--- Diff: nifi-nar-bundles/nifi-provenance-repository-bundle/nifi-persistent-provenance-repository/src/main/java/org/apache/nifi/provenance/PersistentProvenanceRepository.java ---
@@ -403,12 +399,7 @@ public void authorize(final ProvenanceEventRecord event, final NiFiUser user) {
return;
}
- final Authorizable eventAuthorizable;
- if (event.isRemotePortType()) {
- eventAuthorizable = resourceFactory.createRemoteDataAuthorizable(event.getComponentId());
- } else {
- eventAuthorizable = resourceFactory.createLocalDataAuthorizable(event.getComponentId());
- }
+ final Authorizable eventAuthorizable = resourceFactory.createProvenanceDataAuthorizable(event.getComponentId());
eventAuthorizable.authorize(authorizer, RequestAction.READ, user, event.getAttributes());
--- End diff --
I don't think the attributes are necessary here. I'm pretty sure the event attributes would be necessary for authorizing access to attributes/content.
---
[GitHub] nifi issue #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/2703
Thanks @markobean! This has been merged to master.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190919318
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-file-authorizer/src/main/java/org/apache/nifi/authorization/RoleAccessPolicy.java ---
@@ -63,7 +63,7 @@ public String getAction() {
final Set<RoleAccessPolicy> provenancePolicies = new HashSet<>();
provenancePolicies.add(new RoleAccessPolicy(ResourceType.Provenance.getValue(), READ_ACTION));
if (rootGroupId != null) {
- provenancePolicies.add(new RoleAccessPolicy(ResourceType.Data.getValue() + ResourceType.ProcessGroup.getValue() + "/" + rootGroupId, READ_ACTION));
+ provenancePolicies.add(new RoleAccessPolicy(ResourceType.ProvenanceData.getValue() + ResourceType.ProcessGroup.getValue() + "/" + rootGroupId, READ_ACTION));
--- End diff --
In order to be consistent with our 0.x concept of provenance access, this should include both ProvenanceData and Data.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2703
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194498155
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1389,104 +1420,119 @@ private ProvenanceEventDTO createProvenanceEventDto(final ProvenanceEventRecord
// sets the component details if it can find the component still in the flow
setComponentDetails(dto);
- // only include all details if not summarizing
- if (!summarize) {
- // convert the attributes
- final Comparator<AttributeDTO> attributeComparator = new Comparator<AttributeDTO>() {
- @Override
- public int compare(AttributeDTO a1, AttributeDTO a2) {
- return Collator.getInstance(Locale.US).compare(a1.getName(), a2.getName());
- }
- };
+// try {
+// AuthorizationResult result = flowController.checkConnectableAuthorization(event.getComponentId());
+ AuthorizationResult result = checkConnectableAuthorization(event.getComponentId());
+ if (Result.Denied.equals(result.getResult())) {
+ dto.setComponentType("Processor"); // is this always a Processor?
+ dto.setComponentName(dto.getComponentId());
+ dto.setEventType("UNKNOWN");
+ }
- final SortedSet<AttributeDTO> attributes = new TreeSet<>(attributeComparator);
+// authorizeData(event);
+ final AuthorizationResult dataResult = checkAuthorizationForData(event); //(authorizer, RequestAction.READ, user, event.getAttributes());
--- End diff --
We only need to authorize for the data if the event is a non-summary. For instance, when we're pulling back 1000 summaries to load the provenance table we don't need to check any data policies.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194730011
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1338,6 +1339,67 @@ private void authorizeReplay(final ProvenanceEventRecord event) {
dataAuthorizable.authorize(authorizer, RequestAction.WRITE, user, eventAttributes);
}
+ private AuthorizationResult checkAuthorizationForData(ProvenanceEventRecord event) {
+ final NiFiUser user = NiFiUserUtils.getNiFiUser();
+ final Authorizable dataAuthorizable;
+ if (event.isRemotePortType()) {
+ dataAuthorizable = flowController.createRemoteDataAuthorizable(event.getComponentId());
+ } else {
+ dataAuthorizable = flowController.createLocalDataAuthorizable(event.getComponentId());
+ }
+
+ final Map<String, String> eventAttributes = event.getAttributes();
+
+ // ensure we can read the data
+ return dataAuthorizable.checkAuthorization(authorizer, RequestAction.READ, user, eventAttributes);
+ }
+
+ private AuthorizationResult checkAuthorizationForProvenanceData(final ProvenanceEventRecord event) {
+ final ProcessGroup rootGroup = flowController.getGroup(getRootGroupId());
+ final NiFiUser user = NiFiUserUtils.getNiFiUser();
+ final String componentId = event.getComponentId();
+ Connectable connectable;
+ String targetId = null;
+ // check if the component is the rootGroup
+ if (getRootGroupId().equals(componentId)) {
+ targetId = componentId;
+ }
+ if (targetId == null) {
+ // check if the component is a processor
+ connectable = rootGroup.findProcessor(componentId);
+ if (connectable == null) {
+ // if the component id is not a processor then consider a connection
+ connectable = rootGroup.findConnection(componentId).getSource();
+
+ if (connectable == null) {
+ throw new ResourceNotFoundException("The component that generated this event is no longer part of the data flow");
+ }
+ }
+ targetId = connectable.getIdentifier();
+ }
+ final Authorizable provenanceDataAuthorizable = flowController.createProvenanceDataAuthorizable(targetId);
+
+ return provenanceDataAuthorizable.checkAuthorization(authorizer, RequestAction.READ, user);
+ }
+
+ private AuthorizationResult checkConnectableAuthorization(final String componentId) {
+ final ProcessGroup rootGroup = flowController.getGroup(getRootGroupId());
+ final NiFiUser user = NiFiUserUtils.getNiFiUser();
+ if (rootGroup.getIdentifier().equals(componentId)) {
+ return rootGroup.checkAuthorization(authorizer, RequestAction.READ, user);
+ }
+ Connectable connectable = rootGroup.findLocalConnectable(componentId);
--- End diff --
Will findLocalConnectable() versus findProcessor() include connections as well? If so, then this should return to findProcessor() to account for connections and subsequently finding the connection's source component.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194503331
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1389,104 +1420,119 @@ private ProvenanceEventDTO createProvenanceEventDto(final ProvenanceEventRecord
// sets the component details if it can find the component still in the flow
setComponentDetails(dto);
- // only include all details if not summarizing
- if (!summarize) {
- // convert the attributes
- final Comparator<AttributeDTO> attributeComparator = new Comparator<AttributeDTO>() {
- @Override
- public int compare(AttributeDTO a1, AttributeDTO a2) {
- return Collator.getInstance(Locale.US).compare(a1.getName(), a2.getName());
- }
- };
+// try {
+// AuthorizationResult result = flowController.checkConnectableAuthorization(event.getComponentId());
+ AuthorizationResult result = checkConnectableAuthorization(event.getComponentId());
+ if (Result.Denied.equals(result.getResult())) {
+ dto.setComponentType("Processor"); // is this always a Processor?
+ dto.setComponentName(dto.getComponentId());
+ dto.setEventType("UNKNOWN");
+ }
- final SortedSet<AttributeDTO> attributes = new TreeSet<>(attributeComparator);
+// authorizeData(event);
+ final AuthorizationResult dataResult = checkAuthorizationForData(event); //(authorizer, RequestAction.READ, user, event.getAttributes());
--- End diff --
Also, it appears that we're checking the checkAuthorizationForData is verifying READ to the data of the corresponding component. This check is already done as part of the checkAuthorizationForReplay method. It appears that is the only place the replay authorization check is performed. It likely makes sense to refactor some of this so that we're only checking permissions for READ to the data of the corresponding component once. The remainder of the replay authorization check only needs to be performed when we're populating the data fields (READ to the data of the corresponding component is approved). See below.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r192492247
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1359,7 +1363,12 @@ public ProvenanceEventDTO getProvenanceEvent(final Long eventId) {
} else {
dataAuthorizable = flowController.createLocalDataAuthorizable(event.getComponentId());
}
- dataAuthorizable.authorize(authorizer, RequestAction.READ, NiFiUserUtils.getNiFiUser(), attributes);
+ // If not authorized for 'view the data', create only summarized provenance event
--- End diff --
The original JIRA called to make this more granular because using the data policies was too blunt. In the PR as-is, for each event it appears that we authorize the event and then authorize the data policies twice. We are authorizing the data policy to determine if we should summarize and then again to determine if replay is authorized. The replay portion is not changed/new in this PR but is an area for improvement we could make now.
Since we're taking this more granular approach I agree with your originally filed JIRA to add the additional component based check. This shouldn't introduce too much additional cost. The component checks do not consider flow file attributes and the results should be easily cached.
Another improvement that I didn't call out specifically above, is that we really only need to check the data policies if we are not summarizing. Whether the user is approved for data of a component would only be relevant if we were returning the fully populated event.
In order to return the summary, we only need to check the policies for the event and the component. Like the component policies, I don't _think_ the flow file attributes would need to be considered for the event policies. I believe the attributes would only need to be considered for the data policies where we are actually returning the attributes and content. This should help with some of the performance concerns regarding frequent authorization.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r191053294
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1359,7 +1363,12 @@ public ProvenanceEventDTO getProvenanceEvent(final Long eventId) {
} else {
dataAuthorizable = flowController.createLocalDataAuthorizable(event.getComponentId());
}
- dataAuthorizable.authorize(authorizer, RequestAction.READ, NiFiUserUtils.getNiFiUser(), attributes);
+ // If not authorized for 'view the data', create only summarized provenance event
--- End diff --
The summarized event does seem to exclude other details that do not fall under 'view the data' (i.e. attributes and content.) For example, event duration and parent/child UUIDs. It seems either more event details besides lineageStartDate need to be moved out of the "if (!summarized)" block, or... what else would you suggest? A new method to generate the ProvenanceEventDTO which explicitly excludes all attributes and content?
---
[GitHub] nifi issue #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on the issue:
https://github.com/apache/nifi/pull/2703
I like the proposed changes. It makes the authorization process a bit cleaner.
+1
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r191052575
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
nifi.security.identity.mapping.pattern.kerb=^(.*?)/instance@(.*?)$
nifi.security.identity.mapping.value.kerb=$1@$2
-nifi.security.identity.mapping.transform.kerb=NONE
----
The last segment of each property is an identifier used to associate the pattern with the replacement value. When a user makes a request to NiFi, their identity is checked to see if it matches each of those patterns in lexicographical order. For the first one that matches, the replacement specified in the `nifi.security.identity.mapping.value.xxxx` property is used. So a login with `CN=localhost, OU=Apache NiFi, O=Apache, L=Santa Monica, ST=CA, C=US` matches the DN mapping pattern above and the DN mapping value `$1@$2` is applied. The user is normalized to `localhost@Apache NiFi`.
-In addition to mapping a transform may be applied. The supported versions are NONE (no transform applied), LOWER (identity lowercased), and UPPER (identity uppercased). If not specified, the default value is NONE.
--- End diff --
Somehow, there was a bad rebase to master which removed some recently modified lines. Re-rebased to master.
---
[GitHub] nifi issue #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on the issue:
https://github.com/apache/nifi/pull/2703
When calling getEvent() from the provenance repository, the user is authorized for the event (including component level authorization). See ControllerFacade.java:1353. This getEvent() method call is prior to createProvenanceEventDto(). So, it would be redundant to authorize the user for the event inside createProvenanceEventDto() as any unauthorized events will have already been filtered out. The original approach was to exclude all events from a provenance query result for which the user is not authorized (e.g. the user is not in the 'view provenance' component level policy). Therefore, it should not be necessary to perform your point #2 above.
For point #3 and a slight refactor of authorizeReplay(), I've renamed it to authorizeData(). And, removed the duplicate authorization block from getProvenanceEvent(). Instead, the createProvenanceEventDto() will perform the data authorization prior to the if !summarize block. In this way, the event will need to be authorized for data access as well as not summarized in order for the dto to populate the attributes and content.
I also updated some authorization unit tests with more detailed expected results. And, rebased to master.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r194510876
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1389,104 +1420,119 @@ private ProvenanceEventDTO createProvenanceEventDto(final ProvenanceEventRecord
// sets the component details if it can find the component still in the flow
setComponentDetails(dto);
- // only include all details if not summarizing
- if (!summarize) {
- // convert the attributes
- final Comparator<AttributeDTO> attributeComparator = new Comparator<AttributeDTO>() {
- @Override
- public int compare(AttributeDTO a1, AttributeDTO a2) {
- return Collator.getInstance(Locale.US).compare(a1.getName(), a2.getName());
- }
- };
+// try {
+// AuthorizationResult result = flowController.checkConnectableAuthorization(event.getComponentId());
+ AuthorizationResult result = checkConnectableAuthorization(event.getComponentId());
+ if (Result.Denied.equals(result.getResult())) {
+ dto.setComponentType("Processor"); // is this always a Processor?
+ dto.setComponentName(dto.getComponentId());
+ dto.setEventType("UNKNOWN");
--- End diff --
If we choose to _not_ redact event type, that makes life easier. Currently, it displays "UNKNOWN" in the table (when 'view provenance' is enabled and 'view the component' is not). But, the event type IS diplayed in the lineage graph. We need to get to consistency one way or the other on this. I'm leaning towards allowing the event type info to be visible since this is a characteristic of provenance (i.e. 'view provenance') and not a characteristic of 'view the component'.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190938627
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1359,7 +1363,12 @@ public ProvenanceEventDTO getProvenanceEvent(final Long eventId) {
} else {
dataAuthorizable = flowController.createLocalDataAuthorizable(event.getComponentId());
}
- dataAuthorizable.authorize(authorizer, RequestAction.READ, NiFiUserUtils.getNiFiUser(), attributes);
+ // If not authorized for 'view the data', create only summarized provenance event
--- End diff --
I believe the event summaries are what's necessary to populate the table. However, even if the user does not have 'view the data' they can still open the event dialog. Shouldn't we be returning more than a summary? The event should include everything but the attributes and content fields. Piggybacking on the summarization concept could inadvertently change this if we ever change what comprises a summary (if we change the table for instance).
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r190909574
--- Diff: nifi-docs/src/main/asciidoc/administration-guide.adoc ---
@@ -3424,27 +3429,13 @@ The following examples demonstrate normalizing DNs from certificates and princip
----
nifi.security.identity.mapping.pattern.dn=^CN=(.*?), OU=(.*?), O=(.*?), L=(.*?), ST=(.*?), C=(.*?)$
nifi.security.identity.mapping.value.dn=$1@$2
-nifi.security.identity.mapping.transform.dn=NONE
--- End diff --
Did you intend to remove this?
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r192493550
--- Diff: nifi-nar-bundles/nifi-provenance-repository-bundle/nifi-persistent-provenance-repository/src/main/java/org/apache/nifi/provenance/WriteAheadProvenanceRepository.java ---
@@ -226,12 +226,7 @@ private void authorize(final ProvenanceEventRecord event, final NiFiUser user) {
return;
}
- final Authorizable eventAuthorizable;
- if (event.isRemotePortType()) {
- eventAuthorizable = resourceFactory.createRemoteDataAuthorizable(event.getComponentId());
- } else {
- eventAuthorizable = resourceFactory.createLocalDataAuthorizable(event.getComponentId());
- }
+ final Authorizable eventAuthorizable = resourceFactory.createProvenanceDataAuthorizable(event.getComponentId());
eventAuthorizable.authorize(authorizer, RequestAction.READ, user, event.getAttributes());
--- End diff --
I don't think the attributes are necessary here. I'm pretty sure the event attributes would be necessary for authorizing access to attributes/content.
---
[GitHub] nifi pull request #2703: NIFI-4907: add 'view provenance' component policy
Posted by markobean <gi...@git.apache.org>.
Github user markobean commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2703#discussion_r192413226
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/controller/ControllerFacade.java ---
@@ -1359,7 +1363,12 @@ public ProvenanceEventDTO getProvenanceEvent(final Long eventId) {
} else {
dataAuthorizable = flowController.createLocalDataAuthorizable(event.getComponentId());
}
- dataAuthorizable.authorize(authorizer, RequestAction.READ, NiFiUserUtils.getNiFiUser(), attributes);
+ // If not authorized for 'view the data', create only summarized provenance event
--- End diff --
My only concern with the approach you outlined is the additional authorizations calls to determine "if the user is allowed". What you suggest requires up to 2 additional authorizations per provenance event. Already on busy systems, we have observed authorizing the user to each provenance event as a limiting factor (it can result in provenance becoming unusable).
Having said that, unless you think of another approach which would require fewer authorizations calls, I'll proceed as you recommend. I suspect there may be a future JIRA ticket to address the provenance query/authorization impact anyhow; if so, this can be addressed at that time. We won't know for sure if this is a problem until we get the current fix into an appropriately loaded test environment.
---