You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@zeppelin.apache.org by pd...@apache.org on 2021/01/19 07:27:56 UTC
[zeppelin] branch master updated: [ZEPPELIN-5130] Cleanup in
ShiroAuthentication and related realms
This is an automated email from the ASF dual-hosted git repository.
pdallig pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new 227fb92 [ZEPPELIN-5130] Cleanup in ShiroAuthentication and related realms
227fb92 is described below
commit 227fb9266d2ca232c774a72ec913ba4e97af3bf8
Author: Philipp Dallig <ph...@gmail.com>
AuthorDate: Wed Jan 13 16:50:48 2021 +0100
[ZEPPELIN-5130] Cleanup in ShiroAuthentication and related realms
### What is this PR for?
During #4021 I hit some files, which can be improved in readability, performance and stability. This PR requires #4021.
This PR includes:
- improve Logging performance
- use Java generics to avoid casting.
- remove dead code
- use DefaultLdapRealm instead of JndiLdapRealm (see [JavaDoc](https://shiro.apache.org/static/1.7.0/apidocs/org/apache/shiro/realm/ldap/JndiLdapRealm.html))
### What type of PR is it?
- Refactoring
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5130
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
Author: Philipp Dallig <ph...@gmail.com>
Closes #4020 from Reamer/cleanup_shiro and squashes the following commits:
ce2678d2e [Philipp Dallig] Use DefaultLdapRealm instead of JndiLdapRealm JndiLdapRealm has been replaced with DefaultLdapRealm https://shiro.apache.org/static/1.7.0/apidocs/org/apache/shiro/realm/ldap/JndiLdapRealm.html
bbc5cb60a [Philipp Dallig] Adding realm constant strings to check for equality
ea2cbbb36 [Philipp Dallig] Replacement of com.google.common.base.Joiner with native string implementation
36547bd3c [Philipp Dallig] Use @Override to indicate overridden method
51c63f828 [Philipp Dallig] Use private static final for the KEYSTORE_PASS
f62865c1d [Philipp Dallig] use Java generics to avoid casting.
eb4b3ebfa [Philipp Dallig] misc cleanup
df5d47918 [Philipp Dallig] Use foreach instead of a while Loop
ea09396e9 [Philipp Dallig] Remove dead code
a4065dc84 [Philipp Dallig] Improved logging in ShiroAuthenticationService and in some realms
---
.../zeppelin/realm/ActiveDirectoryGroupRealm.java | 76 +++++++----------
.../org/apache/zeppelin/realm/LdapGroupRealm.java | 9 +-
.../java/org/apache/zeppelin/realm/LdapRealm.java | 82 +++++++-----------
.../java/org/apache/zeppelin/realm/PamRealm.java | 4 -
.../apache/zeppelin/realm/ZeppelinHubRealm.java | 3 +-
.../service/ShiroAuthenticationService.java | 98 +++++++++++-----------
.../service/ShiroAuthenticationServiceTest.java | 12 ---
.../notebook/repo/VFSNotebookRepoTest.java | 3 +-
8 files changed, 117 insertions(+), 170 deletions(-)
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/realm/ActiveDirectoryGroupRealm.java b/zeppelin-server/src/main/java/org/apache/zeppelin/realm/ActiveDirectoryGroupRealm.java
index 6d461d2..5efa723 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/realm/ActiveDirectoryGroupRealm.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/realm/ActiveDirectoryGroupRealm.java
@@ -38,7 +38,6 @@ import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
-import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
@@ -61,11 +60,11 @@ import javax.naming.ldap.LdapContext;
* @since 0.1
*/
public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
- private static final Logger log = LoggerFactory.getLogger(ActiveDirectoryGroupRealm.class);
+ private static final Logger LOGGER = LoggerFactory.getLogger(ActiveDirectoryGroupRealm.class);
private static final String ROLE_NAMES_DELIMETER = ",";
- final String keystorePass = "activeDirectoryRealm.systemPassword";
+ private static final String KEYSTORE_PASS = "activeDirectoryRealm.systemPassword";
private String userSearchAttributeName = "sAMAccountName";
@@ -97,6 +96,7 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
LdapContextFactory ldapContextFactory;
+ @Override
protected void onInit() {
super.onInit();
this.getLdapContextFactory();
@@ -104,10 +104,7 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
public LdapContextFactory getLdapContextFactory() {
if (this.ldapContextFactory == null) {
- if (log.isDebugEnabled()) {
- log.debug("No LdapContextFactory specified - creating a default instance.");
- }
-
+ LOGGER.debug("No LdapContextFactory specified - creating a default instance.");
DefaultLdapContextFactory defaultFactory = new DefaultLdapContextFactory();
defaultFactory.setPrincipalSuffix(this.principalSuffix);
defaultFactory.setSearchBase(this.searchBase);
@@ -120,12 +117,12 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
return this.ldapContextFactory;
}
+ @Override
protected AuthenticationInfo doGetAuthenticationInfo(AuthenticationToken token)
throws AuthenticationException {
try {
- AuthenticationInfo info = this.queryForAuthenticationInfo(token,
+ return this.queryForAuthenticationInfo(token,
this.getLdapContextFactory());
- return info;
} catch (javax.naming.AuthenticationException var5) {
throw new AuthenticationException("LDAP authentication failed.", var5);
} catch (NamingException var6) {
@@ -134,11 +131,11 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
}
}
+ @Override
protected AuthorizationInfo doGetAuthorizationInfo(PrincipalCollection principals) {
try {
- AuthorizationInfo info = this.queryForAuthorizationInfo(principals,
+ return this.queryForAuthorizationInfo(principals,
this.getLdapContextFactory());
- return info;
} catch (NamingException var5) {
String msg = "LDAP naming error while attempting to " +
"retrieve authorization for user [" + principals + "].";
@@ -151,7 +148,7 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
if (StringUtils.isEmpty(this.hadoopSecurityCredentialPath)) {
password = this.systemPassword;
} else {
- password = LdapRealm.getSystemPassword(hadoopSecurityCredentialPath, keystorePass);
+ password = LdapRealm.getSystemPassword(hadoopSecurityCredentialPath, KEYSTORE_PASS);
}
return password;
}
@@ -168,6 +165,7 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
* @return an {@link AuthenticationInfo} instance containing information retrieved from LDAP.
* @throws NamingException if any LDAP errors occur during the search.
*/
+ @Override
protected AuthenticationInfo queryForAuthenticationInfo(AuthenticationToken token,
LdapContextFactory ldapContextFactory) throws NamingException {
UsernamePasswordToken upToken = (UsernamePasswordToken) token;
@@ -191,7 +189,7 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
return buildAuthenticationInfo(upToken.getUsername(), upToken.getPassword());
}
- private Boolean isValidPrincipalName(String userPrincipalName) {
+ private boolean isValidPrincipalName(String userPrincipalName) {
if (userPrincipalName != null) {
if (StringUtils.isNotEmpty(userPrincipalName) && userPrincipalName.contains("@")) {
String userPrincipalWithoutDomain = userPrincipalName.split("@")[0].trim();
@@ -228,6 +226,7 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
* @return the AuthorizationInfo for the given Subject principal.
* @throws NamingException if an error occurs when searching the LDAP server.
*/
+ @Override
protected AuthorizationInfo queryForAuthorizationInfo(PrincipalCollection principals,
LdapContextFactory ldapContextFactory) throws NamingException {
String username = (String) getAvailablePrincipal(principals);
@@ -263,22 +262,21 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
Object[] searchArguments = new Object[]{containString};
- NamingEnumeration answer = ldapContext.search(searchBase, searchFilter, searchArguments,
+ NamingEnumeration<SearchResult> answer = ldapContext.search(searchBase, searchFilter,
+ searchArguments,
searchCtls);
while (answer.hasMoreElements()) {
- SearchResult sr = (SearchResult) answer.next();
+ SearchResult sr = answer.next();
- if (log.isDebugEnabled()) {
- log.debug("Retrieving userprincipalname names for user [" + sr.getName() + "]");
- }
+ LOGGER.debug("Retrieving userprincipalname names for user [{}]", sr.getName());
Attributes attrs = sr.getAttributes();
if (attrs != null) {
- NamingEnumeration ae = attrs.getAll();
+ NamingEnumeration<? extends Attribute> ae = attrs.getAll();
while (ae.hasMore()) {
- Attribute attr = (Attribute) ae.next();
- if (attr.getID().toLowerCase().equals(this.getUserSearchAttributeName().toLowerCase())) {
+ Attribute attr = ae.next();
+ if (attr.getID().equalsIgnoreCase(this.getUserSearchAttributeName())) {
userNameList.addAll(LdapUtils.getAllAttributeValues(attr));
}
}
@@ -289,10 +287,8 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
public Map<String, String> getListRoles() {
Map<String, String> roles = new HashMap<>();
- Iterator it = this.groupRolesMap.entrySet().iterator();
- while (it.hasNext()) {
- Map.Entry pair = (Map.Entry) it.next();
- roles.put((String) pair.getValue(), "*");
+ for (Map.Entry<String, String> pair : groupRolesMap.entrySet()) {
+ roles.put(pair.getValue(), "*");
}
return roles;
}
@@ -311,33 +307,25 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
String searchFilter = String.format("(&(objectClass=*)(%s=%s))", this.getUserSearchAttributeName(), userPrincipalName);
Object[] searchArguments = new Object[]{userPrincipalName};
- NamingEnumeration answer = ldapContext.search(searchBase, searchFilter, searchArguments,
+ NamingEnumeration<SearchResult> answer = ldapContext.search(searchBase, searchFilter, searchArguments,
searchCtls);
while (answer.hasMoreElements()) {
- SearchResult sr = (SearchResult) answer.next();
+ SearchResult sr = answer.next();
- if (log.isDebugEnabled()) {
- log.debug("Retrieving group names for user [" + sr.getName() + "]");
- }
+ LOGGER.debug("Retrieving group names for user [{}]", sr.getName());
Attributes attrs = sr.getAttributes();
if (attrs != null) {
- NamingEnumeration ae = attrs.getAll();
+ NamingEnumeration<? extends Attribute> ae = attrs.getAll();
while (ae.hasMore()) {
- Attribute attr = (Attribute) ae.next();
+ Attribute attr = ae.next();
if (attr.getID().equals("memberOf")) {
-
Collection<String> groupNames = LdapUtils.getAllAttributeValues(attr);
-
- if (log.isDebugEnabled()) {
- log.debug("Groups found for user [" + username + "]: " + groupNames);
- }
-
- Collection<String> rolesForGroups = getRoleNamesForGroups(groupNames);
- roleNames.addAll(rolesForGroups);
+ LOGGER.debug("Groups found for user [{}]: {}", username, groupNames);
+ roleNames.addAll(getRoleNamesForGroups(groupNames));
}
}
}
@@ -361,14 +349,8 @@ public class ActiveDirectoryGroupRealm extends AbstractLdapRealm {
String strRoleNames = groupRolesMap.get(groupName);
if (strRoleNames != null) {
for (String roleName : strRoleNames.split(ROLE_NAMES_DELIMETER)) {
-
- if (log.isDebugEnabled()) {
- log.debug("User is member of group [" + groupName + "] so adding role [" +
- roleName + "]");
- }
-
+ LOGGER.debug("User is member of group [{}] so adding role [{}]", groupName, roleName);
roleNames.add(roleName);
-
}
}
}
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/realm/LdapGroupRealm.java b/zeppelin-server/src/main/java/org/apache/zeppelin/realm/LdapGroupRealm.java
index cdc2c22..5e8ffa5 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/realm/LdapGroupRealm.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/realm/LdapGroupRealm.java
@@ -18,7 +18,7 @@ package org.apache.zeppelin.realm;
import org.apache.shiro.authz.AuthorizationInfo;
import org.apache.shiro.authz.SimpleAuthorizationInfo;
-import org.apache.shiro.realm.ldap.JndiLdapRealm;
+import org.apache.shiro.realm.ldap.DefaultLdapRealm;
import org.apache.shiro.realm.ldap.LdapContextFactory;
import org.apache.shiro.subject.PrincipalCollection;
import org.slf4j.Logger;
@@ -39,9 +39,10 @@ import javax.naming.ldap.LdapContext;
/**
* Created for org.apache.zeppelin.server.
*/
-public class LdapGroupRealm extends JndiLdapRealm {
- private static final Logger LOG = LoggerFactory.getLogger(LdapGroupRealm.class);
+public class LdapGroupRealm extends DefaultLdapRealm {
+ private static final Logger LOGGER = LoggerFactory.getLogger(LdapGroupRealm.class);
+ @Override
public AuthorizationInfo queryForAuthorizationInfo(PrincipalCollection principals,
LdapContextFactory ldapContextFactory) throws NamingException {
String username = (String) getAvailablePrincipal(principals);
@@ -83,7 +84,7 @@ public class LdapGroupRealm extends JndiLdapRealm {
return roleNames;
} catch (Exception e) {
- LOG.error("Error", e);
+ LOGGER.error("Error", e);
}
return new HashSet<>();
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/realm/LdapRealm.java b/zeppelin-server/src/main/java/org/apache/zeppelin/realm/LdapRealm.java
index bf8692b..abb5d01 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/realm/LdapRealm.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/realm/LdapRealm.java
@@ -59,8 +59,8 @@ import org.apache.shiro.crypto.hash.DefaultHashService;
import org.apache.shiro.crypto.hash.Hash;
import org.apache.shiro.crypto.hash.HashRequest;
import org.apache.shiro.crypto.hash.HashService;
+import org.apache.shiro.realm.ldap.DefaultLdapRealm;
import org.apache.shiro.realm.ldap.JndiLdapContextFactory;
-import org.apache.shiro.realm.ldap.JndiLdapRealm;
import org.apache.shiro.realm.ldap.LdapContextFactory;
import org.apache.shiro.realm.ldap.LdapUtils;
import org.apache.shiro.session.Session;
@@ -125,7 +125,7 @@ import org.slf4j.LoggerFactory;
* <p>
* securityManager.realms = $ldapRealm
*/
-public class LdapRealm extends JndiLdapRealm {
+public class LdapRealm extends DefaultLdapRealm {
private static final SearchControls SUBTREE_SCOPE = new SearchControls();
private static final SearchControls ONELEVEL_SCOPE = new SearchControls();
@@ -141,11 +141,10 @@ public class LdapRealm extends JndiLdapRealm {
private static final String MATCHING_RULE_IN_CHAIN_FORMAT =
"(&(objectClass=%s)(%s:1.2.840.113556.1.4.1941:=%s))";
- private static final Pattern TEMPLATE_PATTERN = Pattern.compile("\\{(\\d+?)\\}");
private static final String DEFAULT_PRINCIPAL_REGEX = "(.*)";
private static final String MEMBER_SUBSTITUTION_TOKEN = "{0}";
private static final String HASHING_ALGORITHM = "SHA-1";
- private static final Logger log = LoggerFactory.getLogger(LdapRealm.class);
+ private static final Logger LOGGER = LoggerFactory.getLogger(LdapRealm.class);
static {
SUBTREE_SCOPE.setSearchScope(SearchControls.SUBTREE_SCOPE);
@@ -184,7 +183,7 @@ public class LdapRealm extends JndiLdapRealm {
private final Map<String, List<String>> permissionsByRole = new LinkedHashMap<>();
private String hadoopSecurityCredentialPath;
- final String keystorePass = "ldapRealm.systemPassword";
+ private static final String KEYSTORE_PASS = "ldapRealm.systemPassword";
private boolean authorizationEnabled;
@@ -214,12 +213,13 @@ public class LdapRealm extends JndiLdapRealm {
}
}
+ @Override
protected void onInit() {
super.onInit();
if (!org.apache.commons.lang3.StringUtils.isEmpty(this.hadoopSecurityCredentialPath)
&& getContextFactory() != null) {
((JndiLdapContextFactory) getContextFactory()).setSystemPassword(
- getSystemPassword(this.hadoopSecurityCredentialPath, keystorePass));
+ getSystemPassword(this.hadoopSecurityCredentialPath, KEYSTORE_PASS));
}
}
@@ -287,9 +287,7 @@ public class LdapRealm extends JndiLdapRealm {
return null;
}
final Set<String> roleNames = getRoles(principals, ldapContextFactory);
- if (log.isDebugEnabled()) {
- log.debug("RolesNames Authorization: " + roleNames);
- }
+ LOGGER.debug("RolesNames Authorization: {}", roleNames);
SimpleAuthorizationInfo simpleAuthorizationInfo = new SimpleAuthorizationInfo(roleNames);
Set<String> stringPermissions = permsFor(roleNames);
simpleAuthorizationInfo.setStringPermissions(stringPermissions);
@@ -303,7 +301,7 @@ public class LdapRealm extends JndiLdapRealm {
Set<String> roles = getRoles(principals, ldapContextFactory);
for (String allowedRole : allowedRolesForAuthentication) {
if (roles.contains(allowedRole)) {
- log.debug("Allowed role for user [" + allowedRole + "] found.");
+ LOGGER.debug("Allowed role for user [{}] found.", allowedRole);
allowed = true;
break;
}
@@ -322,7 +320,7 @@ public class LdapRealm extends JndiLdapRealm {
return rolesFor(principals, username, systemLdapCtx,
ldapContextFactory, SecurityUtils.getSubject().getSession());
} catch (Throwable t) {
- log.warn("Failed to get roles in current context for " + username, t);
+ LOGGER.warn("Failed to get roles in current context for " + username, t);
return Collections.emptySet();
} finally {
LdapUtils.closeContext(systemLdapCtx);
@@ -336,27 +334,25 @@ public class LdapRealm extends JndiLdapRealm {
final Set<String> groupNames = new HashSet<>();
final String userName;
if (getUserLowerCase()) {
- log.debug("userLowerCase true");
+ LOGGER.debug("userLowerCase true");
userName = userNameIn.toLowerCase();
} else {
userName = userNameIn;
}
-
+
String userDn = getUserDnForSearch(userName);
// Activate paged results
int pageSize = getPagingSize();
- if (log.isDebugEnabled()) {
- log.debug("Ldap PagingSize: " + pageSize);
- }
+ LOGGER.debug("Ldap PagingSize: {}", pageSize);
int numResults = 0;
byte[] cookie = null;
try {
ldapCtx.addToEnvironment(Context.REFERRAL, "ignore");
-
+
ldapCtx.setRequestControls(new Control[]{new PagedResultsControl(pageSize,
Control.NONCRITICAL)});
-
+
do {
// ldapsearch -h localhost -p 33389 -D
// uid=guest,ou=people,dc=hadoop,dc=apache,dc=org -w guest-password
@@ -377,7 +373,7 @@ public class LdapRealm extends JndiLdapRealm {
Attribute attribute = group.getAttributes().get(getGroupIdAttribute());
String groupName = attribute.get().toString();
-
+
String roleName = roleNameFor(groupName);
if (roleName != null) {
roleNames.add(roleName);
@@ -394,10 +390,8 @@ public class LdapRealm extends JndiLdapRealm {
searchFilter = expandTemplate(groupSearchFilter, userName);
//searchFilter = String.format("%1$s", groupSearchFilter);
}
- if (log.isDebugEnabled()) {
- log.debug("Group SearchBase|SearchFilter|GroupSearchScope: " + getGroupSearchBase()
- + "|" + searchFilter + "|" + groupSearchScope);
- }
+ LOGGER.debug("Group SearchBase|SearchFilter|GroupSearchScope: " + "{}|{}|{}",
+ getGroupSearchBase(), searchFilter, groupSearchScope);
searchResultEnum = ldapCtx.search(
getGroupSearchBase(),
searchFilter,
@@ -410,7 +404,7 @@ public class LdapRealm extends JndiLdapRealm {
}
}
} catch (PartialResultException e) {
- log.debug("Ignoring PartitalResultException");
+ LOGGER.debug("Ignoring PartitalResultException");
} finally {
if (searchResultEnum != null) {
searchResultEnum.close();
@@ -421,10 +415,9 @@ public class LdapRealm extends JndiLdapRealm {
cookie, Control.CRITICAL)});
} while (cookie != null);
} catch (SizeLimitExceededException e) {
- log.info("Only retrieved first " + numResults +
- " groups due to SizeLimitExceededException.");
+ LOGGER.info("Only retrieved first {} groups due to SizeLimitExceededException.", numResults);
} catch (IOException e) {
- log.error("Unabled to setup paged results");
+ LOGGER.error("Unabled to setup paged results");
}
// save role names and group names in session so that they can be
// easily looked up outside of this object
@@ -433,9 +426,7 @@ public class LdapRealm extends JndiLdapRealm {
if (!groupNames.isEmpty() && (principals instanceof MutablePrincipalCollection)) {
((MutablePrincipalCollection) principals).addAll(groupNames, getName());
}
- if (log.isDebugEnabled()) {
- log.debug("User RoleNames: " + userName + "::" + roleNames);
- }
+ LOGGER.debug("User RoleNames: {}::{}", userName, roleNames);
return roleNames;
}
@@ -532,10 +523,8 @@ public class LdapRealm extends JndiLdapRealm {
Set<String> perms = new LinkedHashSet<>(); // preserve order
for (String role : roleNames) {
List<String> permsForRole = permissionsByRole.get(role);
- if (log.isDebugEnabled()) {
- log.debug("PermsForRole: " + role);
- log.debug("PermByRole: " + permsForRole);
- }
+ LOGGER.debug("PermsForRole: {}", role);
+ LOGGER.debug("PermByRole: {}", permsForRole);
if (permsForRole != null) {
perms.addAll(permsForRole);
}
@@ -678,7 +667,7 @@ public class LdapRealm extends JndiLdapRealm {
}
private Map<String, List<String>> parsePermissionByRoleString(String permissionsByRoleStr) {
- Map<String, List<String>> perms = new HashMap<String, List<String>>();
+ Map<String, List<String>> perms = new HashMap<>();
// split by semicolon ; then by eq = then by comma ,
StringTokenizer stSem = new StringTokenizer(permissionsByRoleStr, ";");
@@ -691,7 +680,7 @@ public class LdapRealm extends JndiLdapRealm {
String role = stEq.nextToken().trim();
String perm = stEq.nextToken().trim();
StringTokenizer stCom = new StringTokenizer(perm, ",");
- List<String> permList = new ArrayList<String>();
+ List<String> permList = new ArrayList<>();
while (stCom.hasMoreTokens()) {
permList.add(stCom.nextToken().trim());
}
@@ -711,7 +700,7 @@ public class LdapRealm extends JndiLdapRealm {
return false;
}
- String searchBaseString = tokens[0].substring(tokens[0].lastIndexOf("/") + 1);
+ String searchBaseString = tokens[0].substring(tokens[0].lastIndexOf('/') + 1);
String searchScope = tokens[2];
String searchFilter = tokens[3];
@@ -719,14 +708,14 @@ public class LdapRealm extends JndiLdapRealm {
// do scope test
if ("base".equalsIgnoreCase(searchScope)) {
- log.debug("DynamicGroup SearchScope base");
+ LOGGER.debug("DynamicGroup SearchScope base");
return false;
}
if (!userLdapDn.toString().endsWith(searchBaseDn.toString())) {
return false;
}
if ("one".equalsIgnoreCase(searchScope) && (userLdapDn.size() != searchBaseDn.size() - 1)) {
- log.debug("DynamicGroup SearchScope one");
+ LOGGER.debug("DynamicGroup SearchScope one");
return false;
}
// search for the filter, substituting base with userDn
@@ -902,9 +891,7 @@ public class LdapRealm extends JndiLdapRealm {
if ((userSearchBase == null || userSearchBase.isEmpty()) || (userSearchAttributeName == null
&& userSearchFilter == null && !"object".equalsIgnoreCase(userSearchScope))) {
userDn = expandTemplate(userDnTemplate, matchedPrincipal);
- if (log.isDebugEnabled()) {
- log.debug("LDAP UserDN and Principal: " + userDn + "," + principal);
- }
+ LOGGER.debug("LDAP UserDN and Principal: {},{}", userDn, principal);
return userDn;
}
@@ -929,24 +916,19 @@ public class LdapRealm extends JndiLdapRealm {
NamingEnumeration<SearchResult> searchResultEnum = null;
try {
systemLdapCtx = getContextFactory().getSystemLdapContext();
- if (log.isDebugEnabled()) {
- log.debug("SearchBase,SearchFilter,UserSearchScope: " + searchBase
- + "," + searchFilter + "," + userSearchScope);
- }
+ LOGGER.debug("SearchBase,SearchFilter,UserSearchScope: {},{},{}", searchBase, searchFilter, userSearchScope);
searchResultEnum = systemLdapCtx.search(searchBase, searchFilter, searchControls);
// SearchResults contains all the entries in search scope
if (searchResultEnum.hasMore()) {
SearchResult searchResult = searchResultEnum.next();
userDn = searchResult.getNameInNamespace();
- if (log.isDebugEnabled()) {
- log.debug("UserDN Returned,Principal: " + userDn + "," + principal);
- }
+ LOGGER.debug("UserDN Returned,Principal: {},{}", userDn, principal);
return userDn;
} else {
throw new IllegalArgumentException("Illegal principal name: " + principal);
}
} catch (AuthenticationException ne) {
- ne.printStackTrace();
+ LOGGER.error("AuthenticationException in getUserDn", ne);
throw new IllegalArgumentException("Illegal principal name: " + principal);
} catch (NamingException ne) {
throw new IllegalArgumentException("Hit NamingException: " + ne.getMessage());
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/realm/PamRealm.java b/zeppelin-server/src/main/java/org/apache/zeppelin/realm/PamRealm.java
index 0622673..fec247d 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/realm/PamRealm.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/realm/PamRealm.java
@@ -28,9 +28,6 @@ import org.apache.shiro.subject.PrincipalCollection;
import org.jvnet.libpam.PAM;
import org.jvnet.libpam.PAMException;
import org.jvnet.libpam.UnixUser;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
import java.util.LinkedHashSet;
import java.util.Set;
@@ -38,7 +35,6 @@ import java.util.Set;
* An {@code AuthorizingRealm} based on libpam4j.
*/
public class PamRealm extends AuthorizingRealm {
- private static final Logger LOG = LoggerFactory.getLogger(PamRealm.class);
private String service;
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/realm/ZeppelinHubRealm.java b/zeppelin-server/src/main/java/org/apache/zeppelin/realm/ZeppelinHubRealm.java
index d53b389..8a0da48 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/realm/ZeppelinHubRealm.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/realm/ZeppelinHubRealm.java
@@ -16,7 +16,6 @@
*/
package org.apache.zeppelin.realm;
-import com.google.common.base.Joiner;
import com.google.gson.Gson;
import com.google.gson.JsonParseException;
import java.io.IOException;
@@ -131,7 +130,7 @@ public class ZeppelinHubRealm extends AuthorizingRealm {
protected User authenticateUser(String requestBody) {
String responseBody;
String userSession;
- HttpPut put = new HttpPut(Joiner.on("/").join(zeppelinhubUrl, USER_LOGIN_API_ENDPOINT));
+ HttpPut put = new HttpPut(String.join("/", zeppelinhubUrl, USER_LOGIN_API_ENDPOINT));
put.setEntity(new StringEntity(requestBody, ContentType.APPLICATION_JSON));
try (CloseableHttpResponse response = httpClient.execute(put)){
if (HttpStatus.SC_OK != response.getStatusLine().getStatusCode()) {
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java b/zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java
index 9b1895f..056eac7 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java
@@ -24,7 +24,6 @@ import java.sql.ResultSet;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
-import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -42,8 +41,8 @@ import org.apache.shiro.UnavailableSecurityManagerException;
import org.apache.shiro.authz.AuthorizationInfo;
import org.apache.shiro.realm.Realm;
import org.apache.shiro.realm.jdbc.JdbcRealm;
+import org.apache.shiro.realm.ldap.DefaultLdapRealm;
import org.apache.shiro.realm.ldap.JndiLdapContextFactory;
-import org.apache.shiro.realm.ldap.JndiLdapRealm;
import org.apache.shiro.realm.text.IniRealm;
import org.apache.shiro.subject.SimplePrincipalCollection;
import org.apache.shiro.subject.Subject;
@@ -61,7 +60,13 @@ import org.slf4j.LoggerFactory;
*/
public class ShiroAuthenticationService implements AuthenticationService {
- private final Logger LOGGER = LoggerFactory.getLogger(ShiroAuthenticationService.class);
+ private static final Logger LOGGER = LoggerFactory.getLogger(ShiroAuthenticationService.class);
+
+ private static final String INI_REALM = "org.apache.shiro.realm.text.IniRealm";
+ private static final String LDAP_REALM = "org.apache.zeppelin.realm.LdapRealm";
+ private static final String LDAP_GROUP_REALM = "org.apache.zeppelin.realm.LdapGroupRealm";
+ private static final String ACTIVE_DIRECTORY_GROUP_REALM = "org.apache.zeppelin.realm.ActiveDirectoryGroupRealm";
+ private static final String JDBC_REALM = "org.apache.shiro.realm.jdbc.JdbcRealm";
private final ZeppelinConfiguration conf;
@@ -75,7 +80,7 @@ public class ShiroAuthenticationService implements AuthenticationService {
((DefaultWebSecurityManager) org.apache.shiro.SecurityUtils.getSecurityManager())
.getRealms();
if (realms.size() > 1) {
- Boolean isIniRealmEnabled = false;
+ boolean isIniRealmEnabled = false;
for (Realm realm : realms) {
if (realm instanceof IniRealm && ((IniRealm) realm).getIni().get("users") != null) {
isIniRealmEnabled = true;
@@ -107,8 +112,9 @@ public class ShiroAuthenticationService implements AuthenticationService {
if (subject.isAuthenticated()) {
principal = extractPrincipal(subject);
if (conf.isUsernameForceLowerCase()) {
- LOGGER.debug("Converting principal name " + principal
- + " to lower case:" + principal.toLowerCase());
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Converting principal name {} to lower case: {}", principal, principal.toLowerCase());
+ }
principal = principal.toLowerCase();
}
} else {
@@ -157,17 +163,17 @@ public class ShiroAuthenticationService implements AuthenticationService {
if (realmsList != null) {
for (Realm realm : realmsList) {
String realClassName = realm.getClass().getName();
- LOGGER.debug("RealmClass.getName: " + realClassName);
- if (realClassName.equals("org.apache.shiro.realm.text.IniRealm")) {
+ LOGGER.debug("RealmClass.getName: {}", realClassName);
+ if (INI_REALM.equals(realClassName)) {
usersList.addAll(getUserList((IniRealm) realm));
- } else if (realClassName.equals("org.apache.zeppelin.realm.LdapGroupRealm")) {
- usersList.addAll(getUserList((JndiLdapRealm) realm, searchText, numUsersToFetch));
- } else if (realClassName.equals("org.apache.zeppelin.realm.LdapRealm")) {
+ } else if (LDAP_GROUP_REALM.equals(realClassName)) {
+ usersList.addAll(getUserList((DefaultLdapRealm) realm, searchText, numUsersToFetch));
+ } else if (LDAP_REALM.equals(realClassName)) {
usersList.addAll(getUserList((LdapRealm) realm, searchText, numUsersToFetch));
- } else if (realClassName.equals("org.apache.zeppelin.realm.ActiveDirectoryGroupRealm")) {
+ } else if (ACTIVE_DIRECTORY_GROUP_REALM.equals(realClassName)) {
usersList.addAll(
getUserList((ActiveDirectoryGroupRealm) realm, searchText, numUsersToFetch));
- } else if (realClassName.equals("org.apache.shiro.realm.jdbc.JdbcRealm")) {
+ } else if (JDBC_REALM.equals(realClassName)) {
usersList.addAll(getUserList((JdbcRealm) realm));
}
}
@@ -191,10 +197,10 @@ public class ShiroAuthenticationService implements AuthenticationService {
if (realmsList != null) {
for (Realm realm : realmsList) {
String name = realm.getClass().getName();
- LOGGER.debug("RealmClass.getName: " + name);
- if (name.equals("org.apache.shiro.realm.text.IniRealm")) {
+ LOGGER.debug("RealmClass.getName: {}", name);
+ if (INI_REALM.equals(name)) {
rolesList.addAll(getRolesList((IniRealm) realm));
- } else if (name.equals("org.apache.zeppelin.realm.LdapRealm")) {
+ } else if (LDAP_REALM.equals(name)) {
rolesList.addAll(getRolesList((LdapRealm) realm));
}
}
@@ -215,16 +221,16 @@ public class ShiroAuthenticationService implements AuthenticationService {
public Set<String> getAssociatedRoles() {
Subject subject = org.apache.shiro.SecurityUtils.getSubject();
HashSet<String> roles = new HashSet<>();
- Map allRoles = null;
+ Map<String, String> allRoles = null;
if (subject.isAuthenticated()) {
Collection<Realm> realmsList = getRealmsList();
for (Realm realm : realmsList) {
String name = realm.getClass().getName();
- if (name.equals("org.apache.shiro.realm.text.IniRealm")) {
+ if (INI_REALM.equals(name)) {
allRoles = ((IniRealm) realm).getIni().get("roles");
break;
- } else if (name.equals("org.apache.zeppelin.realm.LdapRealm")) {
+ } else if (LDAP_REALM.equals(name)) {
try {
AuthorizationInfo auth =
((LdapRealm) realm)
@@ -238,17 +244,15 @@ public class ShiroAuthenticationService implements AuthenticationService {
LOGGER.error("Can't fetch roles", e);
}
break;
- } else if (name.equals("org.apache.zeppelin.realm.ActiveDirectoryGroupRealm")) {
+ } else if (ACTIVE_DIRECTORY_GROUP_REALM.equals(name)) {
allRoles = ((ActiveDirectoryGroupRealm) realm).getListRoles();
break;
}
}
if (allRoles != null) {
- Iterator it = allRoles.entrySet().iterator();
- while (it.hasNext()) {
- Map.Entry pair = (Map.Entry) it.next();
- if (subject.hasRole((String) pair.getKey())) {
- roles.add((String) pair.getKey());
+ for (Map.Entry<String, String> pair : allRoles.entrySet()) {
+ if (subject.hasRole(pair.getKey())) {
+ roles.add(pair.getKey());
}
}
}
@@ -259,12 +263,10 @@ public class ShiroAuthenticationService implements AuthenticationService {
/** Function to extract users from shiro.ini. */
private List<String> getUserList(IniRealm r) {
List<String> userList = new ArrayList<>();
- Map getIniUser = r.getIni().get("users");
+ Map<String, String> getIniUser = r.getIni().get(IniRealm.USERS_SECTION_NAME);
if (getIniUser != null) {
- Iterator it = getIniUser.entrySet().iterator();
- while (it.hasNext()) {
- Map.Entry pair = (Map.Entry) it.next();
- userList.add(pair.getKey().toString().trim());
+ for (Map.Entry<String, String> pair : getIniUser.entrySet()) {
+ userList.add(pair.getKey().trim());
}
}
return userList;
@@ -278,22 +280,20 @@ public class ShiroAuthenticationService implements AuthenticationService {
*/
private List<String> getRolesList(IniRealm r) {
List<String> roleList = new ArrayList<>();
- Map getIniRoles = r.getIni().get("roles");
+ Map<String, String> getIniRoles = r.getIni().get(IniRealm.ROLES_SECTION_NAME);
if (getIniRoles != null) {
- Iterator it = getIniRoles.entrySet().iterator();
- while (it.hasNext()) {
- Map.Entry pair = (Map.Entry) it.next();
- roleList.add(pair.getKey().toString().trim());
+ for (Map.Entry<String, String> pair : getIniRoles.entrySet()) {
+ roleList.add(pair.getKey().trim());
}
}
return roleList;
}
/** Function to extract users from LDAP. */
- private List<String> getUserList(JndiLdapRealm r, String searchText, int numUsersToFetch) {
+ private List<String> getUserList(DefaultLdapRealm r, String searchText, int numUsersToFetch) {
List<String> userList = new ArrayList<>();
String userDnTemplate = r.getUserDnTemplate();
- String userDn[] = userDnTemplate.split(",", 2);
+ String[] userDn = userDnTemplate.split(",", 2);
String userDnPrefix = userDn[0].split("=")[0];
String userDnSuffix = userDn[1];
JndiLdapContextFactory cf = (JndiLdapContextFactory) r.getContextFactory();
@@ -304,10 +304,10 @@ public class ShiroAuthenticationService implements AuthenticationService {
constraints.setSearchScope(SearchControls.SUBTREE_SCOPE);
String[] attrIDs = {userDnPrefix};
constraints.setReturningAttributes(attrIDs);
- NamingEnumeration result =
+ NamingEnumeration<SearchResult> result =
ctx.search(userDnSuffix, "(" + userDnPrefix + "=*" + searchText + "*)", constraints);
while (result.hasMore()) {
- Attributes attrs = ((SearchResult) result.next()).getAttributes();
+ Attributes attrs = result.next().getAttributes();
if (attrs.get(userDnPrefix) != null) {
String currentUser = attrs.get(userDnPrefix).toString();
userList.add(currentUser.split(":")[1].trim());
@@ -316,14 +316,14 @@ public class ShiroAuthenticationService implements AuthenticationService {
} catch (Exception e) {
LOGGER.error("Error retrieving User list from Ldap Realm", e);
}
- LOGGER.info("UserList: " + userList);
+ LOGGER.info("UserList: {}", userList);
return userList;
}
/** Function to extract users from Zeppelin LdapRealm. */
private List<String> getUserList(LdapRealm r, String searchText, int numUsersToFetch) {
List<String> userList = new ArrayList<>();
- LOGGER.debug("SearchText: " + searchText);
+ LOGGER.debug("SearchText: {}", searchText);
String userAttribute = r.getUserSearchAttributeName();
String userSearchRealm = r.getUserSearchBase();
String userObjectClass = r.getUserObjectClass();
@@ -335,7 +335,7 @@ public class ShiroAuthenticationService implements AuthenticationService {
constraints.setCountLimit(numUsersToFetch);
String[] attrIDs = {userAttribute};
constraints.setReturningAttributes(attrIDs);
- NamingEnumeration result =
+ NamingEnumeration<SearchResult> result =
ctx.search(
userSearchRealm,
"(&(objectclass="
@@ -347,7 +347,7 @@ public class ShiroAuthenticationService implements AuthenticationService {
+ "*))",
constraints);
while (result.hasMore()) {
- Attributes attrs = ((SearchResult) result.next()).getAttributes();
+ Attributes attrs = result.next().getAttributes();
if (attrs.get(userAttribute) != null) {
String currentUser;
if (r.getUserLowerCase()) {
@@ -357,7 +357,7 @@ public class ShiroAuthenticationService implements AuthenticationService {
LOGGER.debug("userLowerCase false");
currentUser = (String) attrs.get(userAttribute).get();
}
- LOGGER.debug("CurrentUser: " + currentUser);
+ LOGGER.debug("CurrentUser: {}", currentUser);
userList.add(currentUser.trim());
}
}
@@ -377,11 +377,9 @@ public class ShiroAuthenticationService implements AuthenticationService {
List<String> roleList = new ArrayList<>();
Map<String, String> roles = r.getListRoles();
if (roles != null) {
- Iterator it = roles.entrySet().iterator();
- while (it.hasNext()) {
- Map.Entry pair = (Map.Entry) it.next();
- LOGGER.debug("RoleKeyValue: " + pair.getKey() + " = " + pair.getValue());
- roleList.add((String) pair.getKey());
+ for (Map.Entry<String, String> pair : roles.entrySet()) {
+ LOGGER.debug("RoleKeyValue: {} = {}", pair.getKey(), pair.getValue());
+ roleList.add(pair.getKey());
}
}
return roleList;
@@ -407,7 +405,7 @@ public class ShiroAuthenticationService implements AuthenticationService {
ResultSet rs = null;
DataSource dataSource = null;
String authQuery = "";
- String retval[];
+ String[] retval;
String tablename = "";
String username = "";
String userquery;
diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/service/ShiroAuthenticationServiceTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/service/ShiroAuthenticationServiceTest.java
index ed9e3d8..ef0e679 100644
--- a/zeppelin-server/src/test/java/org/apache/zeppelin/service/ShiroAuthenticationServiceTest.java
+++ b/zeppelin-server/src/test/java/org/apache/zeppelin/service/ShiroAuthenticationServiceTest.java
@@ -17,13 +17,9 @@
package org.apache.zeppelin.service;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.when;
import java.io.IOException;
-import java.lang.reflect.Field;
-import java.lang.reflect.Modifier;
import java.security.Principal;
import org.apache.commons.configuration.ConfigurationException;
@@ -86,14 +82,6 @@ public class ShiroAuthenticationServiceTest {
}
}
- private void setFinalStatic(Field field, Object newValue)
- throws NoSuchFieldException, IllegalAccessException {
- field.setAccessible(true);
- Field modifiersField = Field.class.getDeclaredField("modifiers");
- modifiersField.setAccessible(true);
- modifiersField.setInt(field, field.getModifiers() & ~Modifier.FINAL);
- field.set(null, newValue);
- }
public class TestPrincipal implements Principal {
private String username;
diff --git a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepoTest.java b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepoTest.java
index 8569b2e..dee35ec 100644
--- a/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepoTest.java
+++ b/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/VFSNotebookRepoTest.java
@@ -31,6 +31,7 @@ import org.junit.Test;
import java.io.File;
import java.io.IOException;
+import java.nio.charset.StandardCharsets;
import java.util.List;
import java.util.Map;
@@ -124,6 +125,6 @@ public class VFSNotebookRepoTest {
private void createNewNote(String content, String noteId, String noteName) throws IOException {
FileUtils.writeStringToFile(
- new File(notebookDir + "/" + noteName + "_" + noteId + ".zpln"), content);
+ new File(notebookDir + "/" + noteName + "_" + noteId + ".zpln"), content, StandardCharsets.UTF_8);
}
}