mosermw commented on code in PR #9868:
URL: https://github.com/apache/nifi/pull/9868#discussion_r2129716950
##########
nifi-framework-bundle/nifi-framework/nifi-file-authorizer/src/main/java/org/apache/nifi/authorization/FileUserGroupProvider.java:
##########
@@ -168,10 +171,21 @@ public void onConfigured(AuthorizerConfigurationContext
configurationContext) th
// extract any node identities
initialUserIdentities = new HashSet<>();
+ initialGroupIdentities = new HashSet<>();
for (Map.Entry<String, String> entry :
configurationContext.getProperties().entrySet()) {
- Matcher matcher =
INITIAL_USER_IDENTITY_PATTERN.matcher(entry.getKey());
- if (matcher.matches() &&
!StringUtils.isBlank(entry.getValue())) {
-
initialUserIdentities.add(IdentityMappingUtil.mapIdentity(entry.getValue(),
identityMappings));
+ Matcher userMatcher =
INITIAL_USER_IDENTITY_PATTERN.matcher(entry.getKey());
+ if (userMatcher.matches() &&
!StringUtils.isBlank(entry.getValue())) {
+ if (StringUtils.isNotBlank(entry.getValue())) {
Review Comment:
Since the first if() uses && !StringUtils.isBlank(), the second if(
StringUtils.isNotBlank() ) is unnecessary and can be removed.
##########
nifi-framework-bundle/nifi-framework/nifi-file-authorizer/src/test/java/org/apache/nifi/authorization/FileUserGroupProviderTest.java:
##########
@@ -196,6 +229,27 @@ public void
testOnConfiguredWhenTenantsExistAndInitialUsersProvided() throws Exc
assertTrue(users.contains(new
User.Builder().identifier("user-2").identity("user-2").build()));
}
+ @Test
+ public void testOnConfiguredWhenTenantsExistAndInitialGroupsProvided()
throws Exception {
+ final String adminGroupIdentity = "admin-group";
+ final String otherGroupIdentity = "other-group";
+
+ // despite setting initial groups, they will not be loaded as the
tenants file is non-empty
+
when(configurationContext.getProperty(eq(FileUserGroupProvider.PROP_INITIAL_GROUP_IDENTITY_PREFIX
+ "1")))
+ .thenReturn(new StandardPropertyValue(adminGroupIdentity,
null, ParameterLookup.EMPTY));
+
when(configurationContext.getProperty(eq(FileUserGroupProvider.PROP_INITIAL_GROUP_IDENTITY_PREFIX
+ "2")))
+ .thenReturn(new StandardPropertyValue(otherGroupIdentity,
null, ParameterLookup.EMPTY));
+
+ writeFile(primaryTenants, SIMPLE_TENANTS_BY_USER);
+ userGroupProvider.onConfigured(configurationContext);
+
+ final Set<User> users = userGroupProvider.getUsers();
+ assertEquals(2, users.size());
Review Comment:
This is checking the number of Users but instead it should be checking the
number of Groups. In this case, since the tenants is not empty, verify that 0
groups are created.
##########
nifi-framework-bundle/nifi-framework/nifi-file-authorizer/src/main/java/org/apache/nifi/authorization/FileUserGroupProvider.java:
##########
@@ -737,6 +758,35 @@ private void createUser(final Tenants tenants, final
String userIdentity) {
}
}
+ /**
+ * Finds the Group with the given name, or creates a new one and adds it
to Tenants.
+ *
+ * @param tenants the Tenants reference
+ * @param groupName the name of the group to look for
+ */
+ private void createGroup(final Tenants tenants, final String groupName) {
+ if (StringUtils.isBlank(groupName)) {
+ return;
+ }
+
+ // TODO stream api?
Review Comment:
Since createGroup() and createUser() are so similar, I think you can remove
this TODO and keep what you have here.
##########
nifi-framework-bundle/nifi-framework/nifi-file-authorizer/src/main/java/org/apache/nifi/authorization/FileUserGroupProvider.java:
##########
@@ -168,10 +171,21 @@ public void onConfigured(AuthorizerConfigurationContext
configurationContext) th
// extract any node identities
initialUserIdentities = new HashSet<>();
+ initialGroupIdentities = new HashSet<>();
for (Map.Entry<String, String> entry :
configurationContext.getProperties().entrySet()) {
- Matcher matcher =
INITIAL_USER_IDENTITY_PATTERN.matcher(entry.getKey());
- if (matcher.matches() &&
!StringUtils.isBlank(entry.getValue())) {
-
initialUserIdentities.add(IdentityMappingUtil.mapIdentity(entry.getValue(),
identityMappings));
+ Matcher userMatcher =
INITIAL_USER_IDENTITY_PATTERN.matcher(entry.getKey());
+ if (userMatcher.matches() &&
!StringUtils.isBlank(entry.getValue())) {
+ if (StringUtils.isNotBlank(entry.getValue())) {
+
initialUserIdentities.add(IdentityMappingUtil.mapIdentity(entry.getValue(),
identityMappings));
+ }
+ continue;
+ }
+
+ Matcher groupMatcher =
INITIAL_GROUP_IDENTITY_PATTERN.matcher(entry.getKey());
+ if (groupMatcher.matches() &&
!StringUtils.isBlank(entry.getValue())) {
+ if (StringUtils.isNotBlank(entry.getValue())) {
Review Comment:
Same ... second if (StringUtils.isNotBlank() ) can be removed.
##########
nifi-framework-bundle/nifi-framework/nifi-file-authorizer/src/main/java/org/apache/nifi/authorization/FileUserGroupProvider.java:
##########
@@ -670,6 +684,7 @@ private synchronized void load() throws JAXBException,
IllegalStateException {
if (emptyTenants) {
populateInitialUsers(tenants);
+ populateInitialGroups(tenants);
Review Comment:
This is more of an idea than a comment. How would you feel about adding all
of the initial users as a member of each of the initial groups? As I was
testing this, I found myself wanting to add myself (the Initial Admin) to my
Initial Group, because managing policies at the group level has always proven
to be easier.
##########
nifi-framework-bundle/nifi-framework/nifi-file-authorizer/src/main/java/org/apache/nifi/authorization/FileUserGroupProvider.java:
##########
@@ -737,6 +758,35 @@ private void createUser(final Tenants tenants, final
String userIdentity) {
}
}
+ /**
+ * Finds the Group with the given name, or creates a new one and adds it
to Tenants.
+ *
+ * @param tenants the Tenants reference
+ * @param groupName the name of the group to look for
+ */
+ private void createGroup(final Tenants tenants, final String groupName) {
+ if (StringUtils.isBlank(groupName)) {
+ return;
+ }
+
+ // TODO stream api?
+ org.apache.nifi.authorization.file.tenants.generated.Group foundGroup
= null;
+ for (org.apache.nifi.authorization.file.tenants.generated.Group group
: tenants.getGroups().getGroup()) {
+ if (group.getName().equals(groupName)) {
+ foundGroup = group;
+ break;
+ }
+ }
+
+ if (foundGroup == null) {
+ final Group newGroup = new
Group.Builder().identifierGenerateFromSeed(groupName).name(groupName).build();
Review Comment:
Looking at this line and comparing it to the same line in the createUser()
method above, I think we should do a small refactor on the createUser() method.
Would you modify createUser() to use
User.Builder().identifierGenerateFromSeed()? I believe this would allow us to
remove the IdentifierUtil.java file entirely because it would not be used
anymore.
##########
nifi-framework-bundle/nifi-framework/nifi-file-authorizer/src/test/java/org/apache/nifi/authorization/FileAccessPolicyProviderTest.java:
##########
@@ -385,6 +414,121 @@ public void
testOnConfiguredWhenInitialAdminProvidedAndFlowIsNull() throws Excep
assertFalse(foundRootGroupPolicy);
}
+ @Test
+ public void testOnConfiguredWhenInitialAdminGroupProvided() throws
Exception {
+ final String adminGroupName = "admin-group";
+
+
when(configurationContext.getProperty(eq(FileAccessPolicyProvider.PROP_INITIAL_ADMIN_GROUP)))
+ .thenReturn(new StandardPropertyValue(adminGroupName, null,
ParameterLookup.EMPTY));
+
+ writeFile(primaryAuthorizations, EMPTY_AUTHORIZATIONS_CONCISE);
+ writeFile(primaryTenants, EMPTY_TENANTS_CONCISE);
+
+ userGroupProvider.onConfigured(configurationContext);
+ accessPolicyProvider.onConfigured(configurationContext);
+
+ final Set<Group> groups = userGroupProvider.getGroups();
+ final Group adminGroup = groups.iterator().next();
+ assertEquals(adminGroupName, adminGroup.getName());
+
+ final Set<AccessPolicy> policies =
accessPolicyProvider.getAccessPolicies();
+ assertEquals(12, policies.size());
+
+ final String rootGroupResource = ResourceType.ProcessGroup.getValue()
+ "/" + ROOT_GROUP_ID;
+
+ boolean foundRootGroupPolicy = false;
+ for (AccessPolicy policy : policies) {
+ if (policy.getResource().equals(rootGroupResource)) {
+ foundRootGroupPolicy = true;
+ break;
+ }
+ }
+
+ assertTrue(foundRootGroupPolicy);
+ }
+
+ @Test
+ public void testOnConfiguredWhenInitialAdminGroupProvidedAndNoFlowExists()
throws Exception {
+ // setup NiFi properties to return a file that does not exist
+ properties = mock(NiFiProperties.class);
+
when(properties.getRestoreDirectory()).thenReturn(restoreAuthorizations.getParentFile());
+ when(properties.getFlowConfigurationFile()).thenReturn(new
File("src/test/resources/does-not-exist.json.gz"));
+
+ userGroupProvider.setNiFiProperties(properties);
+ accessPolicyProvider.setNiFiProperties(properties);
+
+ final String adminGroupName = "admin-group";
+
+
when(configurationContext.getProperty(eq(FileAccessPolicyProvider.PROP_INITIAL_ADMIN_GROUP)))
+ .thenReturn(new StandardPropertyValue(adminGroupName, null,
ParameterLookup.EMPTY));
+
+ writeFile(primaryAuthorizations, EMPTY_AUTHORIZATIONS_CONCISE);
+ writeFile(primaryTenants, EMPTY_TENANTS_CONCISE);
+
+ userGroupProvider.onConfigured(configurationContext);
+ accessPolicyProvider.onConfigured(configurationContext);
+
+ final Set<Group> groups = userGroupProvider.getGroups();
+ final Group adminGroup = groups.iterator().next();
+ assertEquals(adminGroupName, adminGroup.getName());
+
+ final Set<AccessPolicy> policies =
accessPolicyProvider.getAccessPolicies();
+ assertEquals(8, policies.size());
+
+ final String rootGroupResource = ResourceType.ProcessGroup.getValue()
+ "/" + ROOT_GROUP_ID;
+
+ boolean foundRootGroupPolicy = false;
+ for (AccessPolicy policy : policies) {
+ if (policy.getResource().equals(rootGroupResource)) {
+ foundRootGroupPolicy = true;
+ break;
+ }
+ }
+
+ assertFalse(foundRootGroupPolicy);
+ }
+
+ @Test
+ public void testOnConfiguredWhenInitialAdminGroupProvidedAndFlowIsNull()
throws Exception {
+ // setup NiFi properties to return a file that does not exist
+ properties = mock(NiFiProperties.class);
+
when(properties.getRestoreDirectory()).thenReturn(restoreAuthorizations.getParentFile());
+ when(properties.getFlowConfigurationFile()).thenReturn(new
File("src/test/resources/does-not-exist.json.gz"));
Review Comment:
Looks like testOnConfiguredWhenInitialAdminGroupProvidedAndNoFlowExists()
and testOnConfiguredWhenInitialAdminGroupProvidedAndFlowIsNull() are exactly
the same. I believe this line should be
when(properties.getFlowConfigurationFile()).thenReturn(null);
Would you also fix the
testOnConfiguredWhenInitialAdminProvidedAndFlowIsNull() method where this same
change is needed? I believe this was a mistake when removing support for
flow.xml.gz
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
