You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by achristianson <gi...@git.apache.org> on 2018/08/28 14:31:47 UTC
[GitHub] nifi pull request #2970: NIFI-5542 Added support for node groups to FileAcce...
GitHub user achristianson opened a pull request:
https://github.com/apache/nifi/pull/2970
NIFI-5542 Added support for node groups to FileAccessPolicyProvider
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?
- [x] 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)?
- [x] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [x] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [x] 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/achristianson/nifi NIFI-5542
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2970.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 #2970
----
commit 94e05575fec715a2c76dc1397e7f7124dcd1ab20
Author: Andrew I. Christianson <an...@...>
Date: 2018-08-23T15:50:54Z
NIFI-5542 Added support for node groups to FileAccessPolicyProvider
----
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi/pull/2970
@pepov is the second lookup you're referring to this line?
```java
Group nodeGroup = userGroupProvider.getGroup(nodeGroupIdentifier);
```
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by pepov <gi...@git.apache.org>.
Github user pepov commented on the issue:
https://github.com/apache/nifi/pull/2970
Thanks for pointing this out! Of course we need identity here, but if I'm right `identity` is called `name` in case of groups.
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by pepov <gi...@git.apache.org>.
Github user pepov commented on the issue:
https://github.com/apache/nifi/pull/2970
Some unit tests are failing according to the travis build that are relevant:
```
[ERROR] testOnConfiguredWhenInitialAdminAndLegacyUsersProvided(org.apache.nifi.authorization.FileAccessPolicyProviderTest) Time elapsed: 0.114 s <<< ERROR!
java.lang.Exception: Unexpected exception, expected<org.apache.nifi.authorization.exception.AuthorizerCreationException> but was<java.lang.NullPointerException>
at org.apache.nifi.authorization.FileAccessPolicyProviderTest.testOnConfiguredWhenInitialAdminAndLegacyUsersProvided(FileAccessPolicyProviderTest.java:523)
[ERROR] testOnConfiguredWhenBadLegacyUsersFileProvided(org.apache.nifi.authorization.FileAccessPolicyProviderTest) Time elapsed: 0.036 s <<< ERROR!
java.lang.Exception: Unexpected exception, expected<org.apache.nifi.authorization.exception.AuthorizerCreationException> but was<java.lang.NullPointerException>
at org.apache.nifi.authorization.FileAccessPolicyProviderTest.testOnConfiguredWhenBadLegacyUsersFileProvided(FileAccessPolicyProviderTest.java:509)
```
https://api.travis-ci.org/v3/job/422545351/log.txt
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by pepov <gi...@git.apache.org>.
Github user pepov commented on the issue:
https://github.com/apache/nifi/pull/2970
Even better, thanks!
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:
https://github.com/apache/nifi/pull/2970
Yes, that should work fine to lookup group by name. Good point regarding the field name of the Group object. Thanks for looking into this!
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by pepov <gi...@git.apache.org>.
Github user pepov commented on the issue:
https://github.com/apache/nifi/pull/2970
+1 looks good to me now
---
[GitHub] nifi pull request #2970: NIFI-5542 Added support for node groups to FileAcce...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2970#discussion_r213438246
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-file-authorizer/src/main/java/org/apache/nifi/authorization/FileAccessPolicyProvider.java ---
@@ -114,6 +114,7 @@ private static JAXBContext initializeJaxbContext(final String contextPath) {
static final String WRITE_CODE = "W";
static final String PROP_NODE_IDENTITY_PREFIX = "Node Identity ";
+ static final String PROP_NODE_GROUP = "Node Group";
--- End diff --
The [Admin Guide](https://github.com/apache/nifi/blob/master/nifi-docs/src/main/asciidoc/administration-guide.adoc#fileaccesspolicyprovider) and default [authorizers.xml](https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-resources/src/main/resources/conf/authorizers.xml) file should be updated to include this property and its usage.
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi/pull/2970
@pepov I took that lookup out. It appeared to be completely unnecessary.
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by pepov <gi...@git.apache.org>.
Github user pepov commented on the issue:
https://github.com/apache/nifi/pull/2970
@kevdoran Are you okay with the current state? Also @mcgilman can you take a look at this?
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi/pull/2970
@kevdoran @pepov it's now looking up group ID from the given name. Unit test updated to reflect such, and docs updated to reflect the new property.
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi/pull/2970
@pepov @mcgilman new PR where we authorize the node group rather than its users. Please take a look when you get a chance.
---
[GitHub] nifi pull request #2970: NIFI-5542 Added support for node groups to FileAcce...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2970
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by pepov <gi...@git.apache.org>.
Github user pepov commented on the issue:
https://github.com/apache/nifi/pull/2970
@kevdoran @achristianson created a follow up PR to avoid an issue with the default empty "Node Group" in authorizers.xml: https://github.com/apache/nifi/pull/3043
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by pepov <gi...@git.apache.org>.
Github user pepov commented on the issue:
https://github.com/apache/nifi/pull/2970
yes
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by pepov <gi...@git.apache.org>.
Github user pepov commented on the issue:
https://github.com/apache/nifi/pull/2970
Nitpicking but now we do 2 lookups. First we look up the group to get the identifier then we lookup the same group using that identifier. I beleive it would be enough to store the group name property and look it up in the populateNodes method only, but this is nitpicking, since there is no performance or memory penalty in it, so overall this looks good to me.
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi/pull/2970
@pepov @kevdoran thanks for the feedback. I'll update the docs to reflect the property change, and update the code to reflect that the group name is provided and look up the group identifier accordingly.
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by pepov <gi...@git.apache.org>.
Github user pepov commented on the issue:
https://github.com/apache/nifi/pull/2970
I'm not trying to suggest naming just to point out that the Group has the `name` property instead of the `identity` that the User has. The workaround should be as simple as looping through the groups and finding the one with a matching name, am I right?
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:
https://github.com/apache/nifi/pull/2970
Ok, thanks @pepov -- that clears up the intention of these changes to me.
For naming of variables/properties I'm good with either _identity_ or _name_, as you suggest - both are clear to me and distinct from _identifier_. I was more concerned with the usage of the method `UserGroupProvider.getGroup(String groupIdentifier)`, which will will have to change if the value is not actually an identifier.
For accessing users, the [UserGroupProvider](https://github.com/apache/nifi/blob/master/nifi-framework-api/src/main/java/org/apache/nifi/authorization/UserGroupProvider.java) interface provides both `getUser(String identifier)` and `getUserByIdentity(String identity)` methods. I don't see an equivalent method for groups that is a lookup by name/identity, so you'll have to work around that.
---
[GitHub] nifi issue #2970: NIFI-5542 Added support for node groups to FileAccessPolic...
Posted by achristianson <gi...@git.apache.org>.
Github user achristianson commented on the issue:
https://github.com/apache/nifi/pull/2970
The group id lookup logic was running even if group name was not specified. Fixed that. All tests are passing locally.
---