This is an automated email from the ASF dual-hosted git repository.
spolavarapu pushed a commit to branch ranger-2.2
in repository https://gitbox.apache.org/repos/asf/ranger.git
The following commit(s) were added to refs/heads/ranger-2.2 by this push:
new f84adce RANGER-3254 & RANGER-3232: Incorporated review comments and
also added couple of unit tests
f84adce is described below
commit f84adce7c6988419ac1d9d59b6f347ea09636dc2
Author: Sailaja Polavarapu <[email protected]>
AuthorDate: Tue May 4 12:17:00 2021 -0700
RANGER-3254 & RANGER-3232: Incorporated review comments and also added
couple of unit tests
---
.../apache/ranger/ugsyncutil/model/XGroupInfo.java | 7 +-
.../apache/ranger/ugsyncutil/model/XUserInfo.java | 7 +-
.../unixusersync/config/UserGroupSyncConfig.java | 2 +-
.../process/PolicyMgrUserGroupBuilder.java | 119 ++++++++++++---------
.../process/TestFileSourceUserGroupBuilder.java | 31 ++++++
.../PolicyMgrUserGroupBuilderTest.java | 24 ++++-
.../resources/usergroups-special-characters.csv | 5 +
7 files changed, 135 insertions(+), 60 deletions(-)
diff --git
a/ugsync-util/src/main/java/org/apache/ranger/ugsyncutil/model/XGroupInfo.java
b/ugsync-util/src/main/java/org/apache/ranger/ugsyncutil/model/XGroupInfo.java
index 5f5c9aa..a80e5dc 100644
---
a/ugsync-util/src/main/java/org/apache/ranger/ugsyncutil/model/XGroupInfo.java
+++
b/ugsync-util/src/main/java/org/apache/ranger/ugsyncutil/model/XGroupInfo.java
@@ -19,6 +19,7 @@
package org.apache.ranger.ugsyncutil.model;
+import java.util.HashMap;
import java.util.Map;
public class XGroupInfo {
@@ -30,7 +31,7 @@ public class XGroupInfo {
private String isVisible;
private String groupSource;
private String otherAttributes;
- private Map<String, String> otherAttrsMap;
+ private Map<String, String> otherAttrsMap = new HashMap<>();
public String getId() {
return id;
}
@@ -77,7 +78,9 @@ public class XGroupInfo {
}
public void setOtherAttrsMap(Map<String, String> otherAttrsMap) {
- this.otherAttrsMap = otherAttrsMap;
+ if (otherAttrsMap != null) {
+ this.otherAttrsMap = otherAttrsMap;
+ }
}
public String getOtherAttributes() {
diff --git
a/ugsync-util/src/main/java/org/apache/ranger/ugsyncutil/model/XUserInfo.java
b/ugsync-util/src/main/java/org/apache/ranger/ugsyncutil/model/XUserInfo.java
index 058b984..bc1a665 100644
---
a/ugsync-util/src/main/java/org/apache/ranger/ugsyncutil/model/XUserInfo.java
+++
b/ugsync-util/src/main/java/org/apache/ranger/ugsyncutil/model/XUserInfo.java
@@ -20,6 +20,7 @@
package org.apache.ranger.ugsyncutil.model;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -28,7 +29,7 @@ public class XUserInfo {
private String name;
private String description;
private String otherAttributes;
- private Map<String, String> otherAttrsMap;
+ private Map<String, String> otherAttrsMap = new HashMap<>();
private String userSource;
private String status;
private String isVisible;
@@ -109,7 +110,9 @@ public class XUserInfo {
}
public void setOtherAttrsMap(Map<String, String> otherAttrsMap) {
- this.otherAttrsMap = otherAttrsMap;
+ if (otherAttrsMap != null) {
+ this.otherAttrsMap = otherAttrsMap;
+ }
}
public String getOtherAttributes() {
diff --git
a/ugsync/src/main/java/org/apache/ranger/unixusersync/config/UserGroupSyncConfig.java
b/ugsync/src/main/java/org/apache/ranger/unixusersync/config/UserGroupSyncConfig.java
index 2271bd9..ce9d27f 100644
---
a/ugsync/src/main/java/org/apache/ranger/unixusersync/config/UserGroupSyncConfig.java
+++
b/ugsync/src/main/java/org/apache/ranger/unixusersync/config/UserGroupSyncConfig.java
@@ -273,7 +273,7 @@ public class UserGroupSyncConfig {
private static final boolean DEFAULT_UGSYNC_DELETES_ENABLED = false;
private static final String UGSYNC_DELETES_FREQUENCY =
"ranger.usersync.deletes.frequency";
private static final long DEFAULT_UGSYNC_DELETES_FREQUENCY = 10L; //
After every 10 sync cycles
- private static final String UGSYNC_NAME_VALIDATION_ENABLED =
"ranger.usersync.name.validation.enabled";
+ public static final String UGSYNC_NAME_VALIDATION_ENABLED =
"ranger.usersync.name.validation.enabled";
private static final boolean DEFAULT_UGSYNC_NAME_VALIDATION_ENABLED =
false;
private Properties prop = new Properties();
diff --git
a/ugsync/src/main/java/org/apache/ranger/unixusersync/process/PolicyMgrUserGroupBuilder.java
b/ugsync/src/main/java/org/apache/ranger/unixusersync/process/PolicyMgrUserGroupBuilder.java
index 4d8a32a..6420939 100644
---
a/ugsync/src/main/java/org/apache/ranger/unixusersync/process/PolicyMgrUserGroupBuilder.java
+++
b/ugsync/src/main/java/org/apache/ranger/unixusersync/process/PolicyMgrUserGroupBuilder.java
@@ -602,35 +602,38 @@ public class PolicyMgrUserGroupBuilder extends
AbstractUserGroupSource implement
String groupName = groupNameMap.get(groupDN);
if (StringUtils.isEmpty(groupName)) {
groupName =
groupNameTransform(newGroupAttrs.get(UgsyncCommonConstants.ORIGINAL_NAME).trim());
- if (!isValidString(groupName)) {
- LOG.warn("Ignoring invalid group " +
groupName + " Full name = " + groupDN);
- continue;
- }
- groupNameMap.put(groupDN, groupName);
}
- if (!groupCache.containsKey(groupName)) {
- XGroupInfo newGroup = addXGroupInfo(groupName,
newGroupAttrs, newGroupAttrsStr);
- deltaGroups.put(groupName, newGroup);
- noOfNewGroups++;
+ if (!isValidString(groupName)) {
+ LOG.warn("Ignoring invalid group " + groupName
+ " Full name = " + groupDN);
} else {
- XGroupInfo oldGroup = groupCache.get(groupName);
- Map<String, String> oldGroupAttrs =
oldGroup.getOtherAttrsMap();
- String oldGroupDN =
oldGroupAttrs.get(UgsyncCommonConstants.FULL_NAME);
- //LOG.info("DN from source = " + groupDN + "
saved DN = " + oldGroupDN);
- //LOG.info("OldGroupAttr = " + oldGroupAttrs +
" newGroupAttrs = " + newGroupAttrs);
- if (StringUtils.equalsIgnoreCase(groupDN,
oldGroupDN)
- &&
StringUtils.equalsIgnoreCase(oldGroupAttrs.get(UgsyncCommonConstants.SYNC_SOURCE),
newGroupAttrs.get(UgsyncCommonConstants.SYNC_SOURCE))
- &&
StringUtils.equalsIgnoreCase(oldGroupAttrs.get(UgsyncCommonConstants.LDAP_URL),
newGroupAttrs.get(UgsyncCommonConstants.LDAP_URL))) {
- String oldGroupAttrsStr =
gson.toJson(oldGroupAttrs);
-
if(!StringUtils.equalsIgnoreCase(oldGroupAttrsStr, newGroupAttrsStr)) {
-
oldGroup.setOtherAttributes(newGroupAttrsStr);
-
oldGroup.setOtherAttrsMap(newGroupAttrs);
- deltaGroups.put(groupName,
oldGroup);
- noOfModifiedGroups++;
- }
+ if (!groupCache.containsKey(groupName)) {
+ XGroupInfo newGroup =
addXGroupInfo(groupName, newGroupAttrs, newGroupAttrsStr);
+ deltaGroups.put(groupName, newGroup);
+ noOfNewGroups++;
+ groupNameMap.put(groupDN, groupName);
} else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Skipping to update "
+ groupName + " as same group name with different DN already exists");
+ XGroupInfo oldGroup =
groupCache.get(groupName);
+ String oldGroupAttrsStr =
oldGroup.getOtherAttributes();
+ if
(!StringUtils.equalsIgnoreCase(oldGroupAttrsStr, newGroupAttrsStr)) {
+ Map<String, String>
oldGroupAttrs = oldGroup.getOtherAttrsMap();
+ String oldGroupDN =
oldGroupAttrs.get(UgsyncCommonConstants.FULL_NAME);
+ if
(StringUtils.equalsIgnoreCase(groupDN, oldGroupDN)
+ &&
(StringUtils.isEmpty(oldGroupAttrs.get(UgsyncCommonConstants.SYNC_SOURCE))
+ ||
StringUtils.equalsIgnoreCase(oldGroupAttrs.get(UgsyncCommonConstants.SYNC_SOURCE),
+
newGroupAttrs.get(UgsyncCommonConstants.SYNC_SOURCE)))) {
+
oldGroup.setOtherAttributes(newGroupAttrsStr);
+
oldGroup.setOtherAttrsMap(newGroupAttrs);
+
deltaGroups.put(groupName, oldGroup);
+ noOfModifiedGroups++;
+
groupNameMap.put(groupDN, groupName);
+ } else {
+ if
(LOG.isDebugEnabled()) {
+
LOG.debug("Skipping to update " + groupName + " as same group name with
different DN or sync source already exists");
+ LOG.debug("old
group DN = " + oldGroupDN + " and new group DN = " + groupDN);
+ }
+ }
+ } else {
+ groupNameMap.put(groupDN,
groupName);
}
}
}
@@ -654,35 +657,39 @@ public class PolicyMgrUserGroupBuilder extends
AbstractUserGroupSource implement
String userName = userNameMap.get(userDN);
if (StringUtils.isEmpty(userName)) {
userName =
userNameTransform(newUserAttrs.get(UgsyncCommonConstants.ORIGINAL_NAME).trim());
- if (!isValidString(userName)) {
- LOG.warn("Ignoring invalid user " +
userName + " Full name = " + userDN);
- continue;
- }
- userNameMap.put(userDN, userName);
}
-
- if (!userCache.containsKey(userName)) {
-
- XUserInfo newUser = addXUserInfo(userName,
newUserAttrs, newUserAttrsStr);
- deltaUsers.put(userName, newUser);
- noOfNewUsers++;
+ if (!isValidString(userName)) {
+ LOG.warn("Ignoring invalid user " + userName +
" Full name = " + userDN);
} else {
- XUserInfo oldUser = userCache.get(userName);
- Map<String, String> oldUserAttrs =
oldUser.getOtherAttrsMap();
- String oldUserDN =
oldUserAttrs.get(UgsyncCommonConstants.FULL_NAME);
- if (StringUtils.equalsIgnoreCase(userDN,
oldUserDN)
- &&
StringUtils.equalsIgnoreCase(oldUserAttrs.get(UgsyncCommonConstants.SYNC_SOURCE),
newUserAttrs.get(UgsyncCommonConstants.SYNC_SOURCE))
- &&
StringUtils.equalsIgnoreCase(oldUserAttrs.get(UgsyncCommonConstants.LDAP_URL),
newUserAttrs.get(UgsyncCommonConstants.LDAP_URL))) {
- String oldUserAttrsStr =
gson.toJson(oldUserAttrs);
- if(
!StringUtils.equalsIgnoreCase(oldUserAttrsStr, newUserAttrsStr)) {
-
oldUser.setOtherAttributes(newUserAttrsStr);
-
oldUser.setOtherAttrsMap(newUserAttrs);
- deltaUsers.put(userName,
oldUser);
- noOfModifiedUsers++;
- }
+ if (!userCache.containsKey(userName)) {
+
+ XUserInfo newUser =
addXUserInfo(userName, newUserAttrs, newUserAttrsStr);
+ deltaUsers.put(userName, newUser);
+ noOfNewUsers++;
+ userNameMap.put(userDN, userName);
} else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Skipping to update "
+ userName + " as same username with different DN already exists");
+ XUserInfo oldUser =
userCache.get(userName);
+ String oldUserAttrsStr =
oldUser.getOtherAttributes();
+ if
(!StringUtils.equalsIgnoreCase(oldUserAttrsStr, newUserAttrsStr)) {
+ Map<String, String>
oldUserAttrs = oldUser.getOtherAttrsMap();
+ String oldUserDN =
oldUserAttrs.get(UgsyncCommonConstants.FULL_NAME);
+ if
(StringUtils.equalsIgnoreCase(userDN, oldUserDN)
+ &&
(StringUtils.isEmpty(oldUserAttrs.get(UgsyncCommonConstants.SYNC_SOURCE))
+ ||
StringUtils.equalsIgnoreCase(oldUserAttrs.get(UgsyncCommonConstants.SYNC_SOURCE),
+
newUserAttrs.get(UgsyncCommonConstants.SYNC_SOURCE)))) {
+
oldUser.setOtherAttributes(newUserAttrsStr);
+
oldUser.setOtherAttrsMap(newUserAttrs);
+
deltaUsers.put(userName, oldUser);
+ noOfModifiedUsers++;
+ userNameMap.put(userDN,
userName);
+ } else {
+ if
(LOG.isDebugEnabled()) {
+
LOG.debug("Skipping to update " + userName + " as same username with different
DN or sync source already exists");
+ LOG.debug("old
user DN = " + oldUserDN + " and new user DN = " + userDN);
+ }
+ }
+ } else {
+ userNameMap.put(userDN,
userName);
}
}
}
@@ -1589,7 +1596,7 @@ public class PolicyMgrUserGroupBuilder extends
AbstractUserGroupSource implement
return groupName;
}
- private boolean isValidString(final String name) {
+ protected boolean isValidString(final String name) {
if (StringUtils.isBlank(name)) {
return false;
}
@@ -1827,4 +1834,10 @@ public class PolicyMgrUserGroupBuilder extends
AbstractUserGroupSource implement
return ret;
}
+
+ //Only for testing purpose
+ protected void setUserSyncNameValidationEnabled(boolean
isNameValidationEnabled) {
+
config.setProperty(UserGroupSyncConfig.UGSYNC_NAME_VALIDATION_ENABLED,
"isNameValidationEnabled");
+ this.isUserSyncNameValidationEnabled =
config.isUserSyncNameValidationEnabled();
+ }
}
diff --git
a/ugsync/src/test/java/org/apache/ranger/unixusersync/process/TestFileSourceUserGroupBuilder.java
b/ugsync/src/test/java/org/apache/ranger/unixusersync/process/TestFileSourceUserGroupBuilder.java
index 42884af..2f26677 100644
---
a/ugsync/src/test/java/org/apache/ranger/unixusersync/process/TestFileSourceUserGroupBuilder.java
+++
b/ugsync/src/test/java/org/apache/ranger/unixusersync/process/TestFileSourceUserGroupBuilder.java
@@ -158,4 +158,35 @@ public class TestFileSourceUserGroupBuilder {
assertTrue(sink.getAllGroups().contains("group1"));
assertTrue(sink.getAllGroups().contains("group2"));
}
+
+ @Test
+ public void testUpdateSinkDisableInvalidNameCheck() throws Throwable {
+ config.setProperty(UserGroupSyncConfig.UGSYNC_SOURCE_FILE_PROC,
"src/test/resources/usergroups-special-characters.csv");
+
+ FileSourceUserGroupBuilder fileBuilder = new
FileSourceUserGroupBuilder();
+ fileBuilder.init();
+
+ PolicyMgrUserGroupBuilderTest sink = new
PolicyMgrUserGroupBuilderTest();
+ sink.init();
+ fileBuilder.updateSink(sink);
+
+ assertEquals(0, sink.getTotalInvalidGroups());
+ assertEquals(0, sink.getTotalInvalidUsers());
+ }
+
+ @Test
+ public void testUpdateSinkEnableInvalidNameCheck() throws Throwable {
+ config.setProperty(UserGroupSyncConfig.UGSYNC_SOURCE_FILE_PROC,
"src/test/resources/usergroups-special-characters.csv");
+
+ FileSourceUserGroupBuilder fileBuilder = new
FileSourceUserGroupBuilder();
+ fileBuilder.init();
+
+ PolicyMgrUserGroupBuilderTest sink = new
PolicyMgrUserGroupBuilderTest();
+ sink.init();
+ sink.setUserSyncNameValidationEnabled(true);
+ fileBuilder.updateSink(sink);
+
+ assertEquals(2, sink.getTotalInvalidGroups());
+ assertEquals(1, sink.getTotalInvalidUsers());
+ }
}
diff --git
a/ugsync/src/test/java/org/apache/ranger/usergroupsync/PolicyMgrUserGroupBuilderTest.java
b/ugsync/src/test/java/org/apache/ranger/usergroupsync/PolicyMgrUserGroupBuilderTest.java
index 4fb6b6c..930f688 100644
---
a/ugsync/src/test/java/org/apache/ranger/usergroupsync/PolicyMgrUserGroupBuilderTest.java
+++
b/ugsync/src/test/java/org/apache/ranger/usergroupsync/PolicyMgrUserGroupBuilderTest.java
@@ -30,6 +30,8 @@ public class PolicyMgrUserGroupBuilderTest extends
PolicyMgrUserGroupBuilder {
private Set<String> allGroups;
private Set<String> allUsers;
private Map<String, Set<String>> groupUsers;
+ private Set<String> invalidGroups;
+ private Set<String> invalidUsers;
public PolicyMgrUserGroupBuilderTest() {
super();
@@ -40,6 +42,8 @@ public class PolicyMgrUserGroupBuilderTest extends
PolicyMgrUserGroupBuilder {
allGroups = new HashSet<>();
allUsers = new HashSet<>();
groupUsers = new HashMap<>();
+ invalidGroups = new HashSet<>();
+ invalidUsers = new HashSet<>();
}
public int getTotalUsers() {
@@ -59,6 +63,14 @@ public class PolicyMgrUserGroupBuilderTest extends
PolicyMgrUserGroupBuilder {
return allUsers;
}
+ public int getTotalInvalidGroups() {
+ return invalidGroups.size();
+ }
+
+ public int getTotalInvalidUsers() {
+ return invalidUsers.size();
+ }
+
public int getGroupsWithNoUsers() {
int groupsWithNoUsers = 0;
for (String group : groupUsers.keySet()) {
@@ -77,11 +89,19 @@ public class PolicyMgrUserGroupBuilderTest extends
PolicyMgrUserGroupBuilder {
for (String userdn : sourceUsers.keySet()) {
//System.out.println("Username: " +
sourceUsers.get(userdn).get("original_name"));
-
allUsers.add(userNameTransform(sourceUsers.get(userdn).get("original_name")));
+ String username =
userNameTransform(sourceUsers.get(userdn).get("original_name"));
+ allUsers.add(username);
+ if (!isValidString(username)) {
+ invalidUsers.add(username);
+ }
}
for (String groupdn : sourceGroups.keySet()) {
//System.out.println("Groupname: " +
sourceGroups.get(groupdn).get("original_name"));
-
allGroups.add(groupNameTransform(sourceGroups.get(groupdn).get("original_name")));
+ String groupname =
groupNameTransform(sourceGroups.get(groupdn).get("original_name"));
+ allGroups.add(groupname);
+ if (!isValidString(groupname)) {
+ invalidGroups.add(groupname);
+ }
}
groupUsers = sourceGroupUsers;
//System.out.println("Username: " + user + " and associated
groups: " + groups);
diff --git a/ugsync/src/test/resources/usergroups-special-characters.csv
b/ugsync/src/test/resources/usergroups-special-characters.csv
new file mode 100644
index 0000000..8ba0285
--- /dev/null
+++ b/ugsync/src/test/resources/usergroups-special-characters.csv
@@ -0,0 +1,5 @@
+user$1,group1
+user2,group'2
+user3,group@1,group"2
+user-4,
+user5,group=
\ No newline at end of file
