exceptionfactory commented on code in PR #9115:
URL: https://github.com/apache/nifi/pull/9115#discussion_r1693961683
##########
nifi-commons/nifi-security-identity/src/main/java/org/apache/nifi/authorization/util/IdentityMappingUtil.java:
##########
@@ -140,8 +141,12 @@ public int compare(IdentityMapping m1, IdentityMapping m2)
{
* @return the mapped identity, or the same identity if no mappings matched
*/
public static String mapIdentity(final String identity,
List<IdentityMapping> mappings) {
- for (IdentityMapping mapping : mappings) {
- Matcher m = mapping.getPattern().matcher(identity);
+ final List<String> keys = mappings.stream().map(m ->
m.getKey()).collect(Collectors.toList());
+ Collections.sort(keys);
+
+ for (final String key : keys) {
+ final IdentityMapping mapping = mappings.stream().filter(m ->
m.getKey().equals(key)).findFirst().orElseThrow();
Review Comment:
Instead of iterating over the keys and then searching for the matching
IdentityMapping, did you consider creating a `TreeMap` of the key and the
`IdentityMapping` object? That would avoid the need for this stream, filter,
and findFirst approach.
##########
nifi-commons/nifi-security-identity/src/test/java/org/apache/nifi/authorization/util/IdentityMappingUtilTest.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.authorization.util;
+
+import org.apache.nifi.util.NiFiProperties;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+class IdentityMappingUtilTest {
+
+ @Test
+ public void testIdentityMappingGeneration() {
+ final Map<String, String> properties = new HashMap<>();
+ properties.putAll(getDnProperties());
+ properties.putAll(getLdapProperties("UPPER"));
+ final NiFiProperties niFiProperties = new NiFiProperties(properties);
+ final List<IdentityMapping> identityMappings =
IdentityMappingUtil.getIdentityMappings(niFiProperties);
+
+ final Optional<IdentityMapping> dnMapping =
identityMappings.stream().filter(im -> im.getKey().equals("dn")).findFirst();
+ final Optional<IdentityMapping> ldapMapping =
identityMappings.stream().filter(im -> im.getKey().equals("ldap")).findFirst();
+
+ Assertions.assertTrue(dnMapping.isPresent());
+ Assertions.assertEquals("NONE", dnMapping.get().getTransform().name());
+
+ Assertions.assertTrue(ldapMapping.isPresent());
+ Assertions.assertEquals("UPPER",
ldapMapping.get().getTransform().name());
+ }
+
+ // NIFI-13409 TC1
Review Comment:
Although it can be useful to know the Jira issue that prompted this change,
that information is already available in the commit message itself, so I
recommend removing the comments that reference the Jira issue.
##########
nifi-commons/nifi-security-identity/src/test/java/org/apache/nifi/authorization/util/IdentityMappingUtilTest.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.authorization.util;
+
+import org.apache.nifi.util.NiFiProperties;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+class IdentityMappingUtilTest {
+
+ @Test
+ public void testIdentityMappingGeneration() {
+ final Map<String, String> properties = new HashMap<>();
+ properties.putAll(getDnProperties());
+ properties.putAll(getLdapProperties("UPPER"));
+ final NiFiProperties niFiProperties = new NiFiProperties(properties);
+ final List<IdentityMapping> identityMappings =
IdentityMappingUtil.getIdentityMappings(niFiProperties);
+
+ final Optional<IdentityMapping> dnMapping =
identityMappings.stream().filter(im -> im.getKey().equals("dn")).findFirst();
+ final Optional<IdentityMapping> ldapMapping =
identityMappings.stream().filter(im -> im.getKey().equals("ldap")).findFirst();
+
+ Assertions.assertTrue(dnMapping.isPresent());
+ Assertions.assertEquals("NONE", dnMapping.get().getTransform().name());
+
+ Assertions.assertTrue(ldapMapping.isPresent());
+ Assertions.assertEquals("UPPER",
ldapMapping.get().getTransform().name());
+ }
+
+ // NIFI-13409 TC1
+ @Test
+ public void testMappingIdentity() {
+ final Map<String, String> properties = new HashMap<>();
+ properties.putAll(getDnProperties());
+ properties.putAll(getLdapProperties("UPPER"));
+ final NiFiProperties niFiProperties = new NiFiProperties(properties);
+
+ final List<IdentityMapping> identityMappings =
IdentityMappingUtil.getIdentityMappings(niFiProperties);
+ Assertions.assertEquals("nifi-node1",
IdentityMappingUtil.mapIdentity("CN=nifi-node1, ST=MD, C=US",
identityMappings));
+ Assertions.assertEquals("NIFIADMIN",
IdentityMappingUtil.mapIdentity("nifiadmin", identityMappings));
Review Comment:
As the identity and mapped identity values are repeated across multiple
methods, it would be helpful to create static variables that can be reused
across test methods.
##########
nifi-commons/nifi-security-identity/src/test/java/org/apache/nifi/authorization/util/IdentityMappingUtilTest.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.authorization.util;
+
+import org.apache.nifi.util.NiFiProperties;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+class IdentityMappingUtilTest {
+
+ @Test
+ public void testIdentityMappingGeneration() {
+ final Map<String, String> properties = new HashMap<>();
+ properties.putAll(getDnProperties());
+ properties.putAll(getLdapProperties("UPPER"));
+ final NiFiProperties niFiProperties = new NiFiProperties(properties);
+ final List<IdentityMapping> identityMappings =
IdentityMappingUtil.getIdentityMappings(niFiProperties);
+
+ final Optional<IdentityMapping> dnMapping =
identityMappings.stream().filter(im -> im.getKey().equals("dn")).findFirst();
+ final Optional<IdentityMapping> ldapMapping =
identityMappings.stream().filter(im -> im.getKey().equals("ldap")).findFirst();
+
+ Assertions.assertTrue(dnMapping.isPresent());
+ Assertions.assertEquals("NONE", dnMapping.get().getTransform().name());
+
+ Assertions.assertTrue(ldapMapping.isPresent());
+ Assertions.assertEquals("UPPER",
ldapMapping.get().getTransform().name());
Review Comment:
Assertions should use static imports for readability.
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
