You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@zeppelin.apache.org by zj...@apache.org on 2020/01/31 06:57:24 UTC
[zeppelin] branch master updated: [ZEPPELIN-4578] Cleanup of
LoginRestApi
This is an automated email from the ASF dual-hosted git repository.
zjffdu 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 be57e34 [ZEPPELIN-4578] Cleanup of LoginRestApi
be57e34 is described below
commit be57e343f3b2fcb6df23c9e4d47b97d0e3849c7d
Author: Lars Francke <la...@gmail.com>
AuthorDate: Fri Jan 24 08:47:56 2020 +0100
[ZEPPELIN-4578] Cleanup of LoginRestApi
### What is this PR for?
I've had to debug a few Zeppelin issues and looked at the LoginRestApi code.
It's using raw parameterized classes all over the place and a few other weird things but what really made me look is that every log in attempt (succesful or not) is logged as WARN.
I decided to fix the warnings my IDE shows me and changed the Log statements from WARN to INFO
### What type of PR is it?
Improvement
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-4578
### How should this be tested?
This does not change any actual functionality so I rely on existing tests.
Travis CI ran and there were a bunch of failures but they all look unrelated (Livy, Timeouts, ...)
Author: Lars Francke <la...@gmail.com>
Closes #3617 from lfrancke/ZEPPELIN-4578 and squashes the following commits:
2886ce469 [Lars Francke] ZEPPELIN-4578 Cleanup of LoginRestApi
b050bb019 [Lars Francke] ZEPPELIN4578 Cleanup of LoginRestApi
---
.../org/apache/zeppelin/rest/LoginRestApi.java | 119 ++++++++++-----------
.../zeppelin/service/AuthenticationService.java | 4 +-
.../zeppelin/service/NoAuthenticationService.java | 4 +-
.../service/ShiroAuthenticationService.java | 17 ++-
4 files changed, 67 insertions(+), 77 deletions(-)
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/LoginRestApi.java b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/LoginRestApi.java
index cb5d598..c7b563d 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/LoginRestApi.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/LoginRestApi.java
@@ -16,11 +16,9 @@
*/
package org.apache.zeppelin.rest;
-import com.google.gson.Gson;
import java.text.ParseException;
import java.util.Collection;
import java.util.HashMap;
-import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import javax.inject.Inject;
@@ -36,6 +34,8 @@ import javax.ws.rs.core.HttpHeaders;
import javax.ws.rs.core.Response;
import javax.ws.rs.core.Response.Status;
+import com.google.gson.Gson;
+import org.apache.shiro.SecurityUtils;
import org.apache.shiro.authc.AuthenticationException;
import org.apache.shiro.authc.AuthenticationToken;
import org.apache.shiro.authc.UsernamePasswordToken;
@@ -43,8 +43,8 @@ import org.apache.shiro.realm.Realm;
import org.apache.shiro.subject.Subject;
import org.apache.zeppelin.annotation.ZeppelinApi;
import org.apache.zeppelin.conf.ZeppelinConfiguration;
-import org.apache.zeppelin.notebook.Notebook;
import org.apache.zeppelin.notebook.AuthorizationService;
+import org.apache.zeppelin.notebook.Notebook;
import org.apache.zeppelin.realm.jwt.JWTAuthenticationToken;
import org.apache.zeppelin.realm.jwt.KnoxJwtRealm;
import org.apache.zeppelin.realm.kerberos.KerberosRealm;
@@ -63,11 +63,11 @@ import org.slf4j.LoggerFactory;
@Singleton
public class LoginRestApi {
private static final Logger LOG = LoggerFactory.getLogger(LoginRestApi.class);
- private static final Gson gson = new Gson();
- private ZeppelinConfiguration zConf;
+ private static final Gson GSON = new Gson();
+ private final ZeppelinConfiguration zConf;
- private AuthenticationService authenticationService;
- private AuthorizationService authorizationService;
+ private final AuthenticationService authenticationService;
+ private final AuthorizationService authorizationService;
@Inject
public LoginRestApi(Notebook notebook,
@@ -81,12 +81,12 @@ public class LoginRestApi {
@GET
@ZeppelinApi
public Response getLogin(@Context HttpHeaders headers) {
- JsonResponse response = null;
+ JsonResponse<Map<String, String>> response = null;
if (isKnoxSSOEnabled()) {
KnoxJwtRealm knoxJwtRealm = getJTWRealm();
Cookie cookie = headers.getCookies().get(knoxJwtRealm.getCookieName());
if (cookie != null && cookie.getValue() != null) {
- Subject currentUser = org.apache.shiro.SecurityUtils.getSubject();
+ Subject currentUser = SecurityUtils.getSubject();
JWTAuthenticationToken token = new JWTAuthenticationToken(null, cookie.getValue());
try {
String name = knoxJwtRealm.getName(token);
@@ -100,43 +100,42 @@ public class LoginRestApi {
if (response == null) {
Map<String, String> data = new HashMap<>();
data.put("redirectURL", constructKnoxUrl(knoxJwtRealm, knoxJwtRealm.getLogin()));
- response = new JsonResponse(Status.OK, "", data);
+ response = new JsonResponse<>(Status.OK, "", data);
}
return response.build();
- } else {
- KerberosRealm kerberosRealm = getKerberosRealm();
- if (null != kerberosRealm) {
- try {
- Map<String, Cookie> cookies = headers.getCookies();
- KerberosToken kerberosToken = KerberosRealm.getKerberosTokenFromCookies(cookies);
- if (null != kerberosToken) {
- Subject currentUser = org.apache.shiro.SecurityUtils.getSubject();
- String name = (String) kerberosToken.getPrincipal();
- if (!currentUser.isAuthenticated() || !currentUser.getPrincipal().equals(name)) {
- response = proceedToLogin(currentUser, kerberosToken);
- }
- }
- if (null == response) {
- LOG.warn("No Kerberos token received");
- response = new JsonResponse(Status.UNAUTHORIZED, "", null);
+ }
+
+ KerberosRealm kerberosRealm = getKerberosRealm();
+ if (null != kerberosRealm) {
+ try {
+ Map<String, Cookie> cookies = headers.getCookies();
+ KerberosToken kerberosToken = KerberosRealm.getKerberosTokenFromCookies(cookies);
+ if (null != kerberosToken) {
+ Subject currentUser = SecurityUtils.getSubject();
+ String name = (String) kerberosToken.getPrincipal();
+ if (!currentUser.isAuthenticated() || !currentUser.getPrincipal().equals(name)) {
+ response = proceedToLogin(currentUser, kerberosToken);
}
- return response.build();
- } catch (AuthenticationException e){
- LOG.error("Error in Login: " + e);
}
+ if (null == response) {
+ LOG.warn("No Kerberos token received");
+ response = new JsonResponse<>(Status.UNAUTHORIZED, "", null);
+ }
+ return response.build();
+ } catch (AuthenticationException e){
+ LOG.error("Error in Login", e);
}
}
- return new JsonResponse(Status.METHOD_NOT_ALLOWED).build();
+ return new JsonResponse<>(Status.METHOD_NOT_ALLOWED).build();
}
private KerberosRealm getKerberosRealm() {
- Collection realmsList = authenticationService.getRealmsList();
+ Collection<Realm> realmsList = authenticationService.getRealmsList();
if (realmsList != null) {
- for (Iterator<Realm> iterator = realmsList.iterator(); iterator.hasNext(); ) {
- Realm realm = iterator.next();
+ for (Realm realm : realmsList) {
String name = realm.getClass().getName();
- LOG.debug("RealmClass.getName: " + name);
+ LOG.debug("RealmClass.getName: {}", name);
if (name.equals("org.apache.zeppelin.realm.kerberos.KerberosRealm")) {
return (KerberosRealm) realm;
@@ -147,13 +146,12 @@ public class LoginRestApi {
}
private KnoxJwtRealm getJTWRealm() {
- Collection realmsList = authenticationService.getRealmsList();
+ Collection<Realm> realmsList = authenticationService.getRealmsList();
if (realmsList != null) {
- for (Iterator<Realm> iterator = realmsList.iterator(); iterator.hasNext(); ) {
- Realm realm = iterator.next();
+ for (Realm realm : realmsList) {
String name = realm.getClass().getName();
- LOG.debug("RealmClass.getName: " + name);
+ LOG.debug("RealmClass.getName: {}", name);
if (name.equals("org.apache.zeppelin.realm.jwt.KnoxJwtRealm")) {
return (KnoxJwtRealm) realm;
@@ -164,12 +162,11 @@ public class LoginRestApi {
}
private boolean isKnoxSSOEnabled() {
- Collection realmsList = authenticationService.getRealmsList();
+ Collection<Realm> realmsList = authenticationService.getRealmsList();
if (realmsList != null) {
- for (Iterator<Realm> iterator = realmsList.iterator(); iterator.hasNext(); ) {
- Realm realm = iterator.next();
+ for (Realm realm : realmsList) {
String name = realm.getClass().getName();
- LOG.debug("RealmClass.getName: " + name);
+ LOG.debug("RealmClass.getName: {}", name);
if (name.equals("org.apache.zeppelin.realm.jwt.KnoxJwtRealm")) {
return true;
}
@@ -178,8 +175,8 @@ public class LoginRestApi {
return false;
}
- private JsonResponse proceedToLogin(Subject currentUser, AuthenticationToken token) {
- JsonResponse response = null;
+ private JsonResponse<Map<String, String>> proceedToLogin(Subject currentUser, AuthenticationToken token) {
+ JsonResponse<Map<String, String>> response = null;
try {
logoutCurrentUser();
currentUser.getSession(true);
@@ -187,19 +184,14 @@ public class LoginRestApi {
Set<String> roles = authenticationService.getAssociatedRoles();
String principal = authenticationService.getPrincipal();
- String ticket;
- if ("anonymous".equals(principal)) {
- ticket = "anonymous";
- } else {
- ticket = TicketContainer.instance.getTicket(principal);
- }
+ String ticket = "anonymous".equals(principal) ? "anonymous" : TicketContainer.instance.getTicket(principal);
Map<String, String> data = new HashMap<>();
data.put("principal", principal);
- data.put("roles", gson.toJson(roles));
+ data.put("roles", GSON.toJson(roles));
data.put("ticket", ticket);
- response = new JsonResponse(Response.Status.OK, "", data);
+ response = new JsonResponse<>(Status.OK, "", data);
// if no exception, that's it, we're done!
// set roles for user in NotebookAuthorization module
@@ -226,14 +218,14 @@ public class LoginRestApi {
@ZeppelinApi
public Response postLogin(@FormParam("userName") String userName,
@FormParam("password") String password) {
- LOG.debug("userName:" + userName);
- JsonResponse response = null;
+ LOG.debug("userName: {}", userName);
// ticket set to anonymous for anonymous user. Simplify testing.
- Subject currentUser = org.apache.shiro.SecurityUtils.getSubject();
+ Subject currentUser = SecurityUtils.getSubject();
if (currentUser.isAuthenticated()) {
currentUser.logout();
}
- LOG.debug("currentUser: " + currentUser);
+ LOG.debug("currentUser: {}", currentUser);
+ JsonResponse<Map<String, String>> response = null;
if (!currentUser.isAuthenticated()) {
UsernamePasswordToken token = new UsernamePasswordToken(userName, password);
@@ -242,10 +234,10 @@ public class LoginRestApi {
}
if (response == null) {
- response = new JsonResponse(Response.Status.FORBIDDEN, "", "");
+ response = new JsonResponse<>(Response.Status.FORBIDDEN, "", null);
}
- LOG.warn(response.toString());
+ LOG.info(response.toString());
return response.build();
}
@@ -253,9 +245,8 @@ public class LoginRestApi {
@Path("logout")
@ZeppelinApi
public Response logout() {
- JsonResponse response;
logoutCurrentUser();
- Status status = null;
+ Status status;
Map<String, String> data = new HashMap<>();
if (zConf.isAuthorizationHeaderClear()) {
status = Status.UNAUTHORIZED;
@@ -268,11 +259,9 @@ public class LoginRestApi {
KnoxJwtRealm knoxJwtRealm = getJTWRealm();
data.put("redirectURL", constructKnoxUrl(knoxJwtRealm, knoxJwtRealm.getLogout()));
data.put("isLogoutAPI", knoxJwtRealm.getLogoutAPI().toString());
- response = new JsonResponse(status, "", data);
- } else {
- response = new JsonResponse(status, "", data);
}
- LOG.warn(response.toString());
+ JsonResponse<Map<String, String>> response = new JsonResponse<>(status, "", data);
+ LOG.info(response.toString());
return response.build();
}
@@ -286,7 +275,7 @@ public class LoginRestApi {
}
private void logoutCurrentUser() {
- Subject currentUser = org.apache.shiro.SecurityUtils.getSubject();
+ Subject currentUser = SecurityUtils.getSubject();
TicketContainer.instance.removeTicket(authenticationService.getPrincipal());
currentUser.getSession().stop();
currentUser.logout();
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/service/AuthenticationService.java b/zeppelin-server/src/main/java/org/apache/zeppelin/service/AuthenticationService.java
index a048a20..eaab4d4 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/service/AuthenticationService.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/service/AuthenticationService.java
@@ -21,6 +21,8 @@ import java.util.Collection;
import java.util.List;
import java.util.Set;
+import org.apache.shiro.realm.Realm;
+
/**
* Interface for Zeppelin Security.
* //TODO(zjffdu) rename it to AuthenticationService
@@ -39,7 +41,7 @@ public interface AuthenticationService {
*/
Set<String> getAssociatedRoles();
- Collection getRealmsList();
+ Collection<Realm> getRealmsList();
boolean isAuthenticated();
diff --git a/zeppelin-server/src/main/java/org/apache/zeppelin/service/NoAuthenticationService.java b/zeppelin-server/src/main/java/org/apache/zeppelin/service/NoAuthenticationService.java
index 317ffad..478d402 100644
--- a/zeppelin-server/src/main/java/org/apache/zeppelin/service/NoAuthenticationService.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/service/NoAuthenticationService.java
@@ -24,6 +24,8 @@ import java.util.Collections;
import java.util.List;
import java.util.Set;
import javax.inject.Inject;
+
+import org.apache.shiro.realm.Realm;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -47,7 +49,7 @@ public class NoAuthenticationService implements AuthenticationService {
}
@Override
- public Collection getRealmsList() {
+ public Collection<Realm> getRealmsList() {
return Collections.emptyList();
}
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 f768fd0..9b1895f 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
@@ -130,10 +130,9 @@ public class ShiroAuthenticationService implements AuthenticationService {
}
@Override
- public Collection getRealmsList() {
- DefaultWebSecurityManager defaultWebSecurityManager;
+ public Collection<Realm> getRealmsList() {
String key = ThreadContext.SECURITY_MANAGER_KEY;
- defaultWebSecurityManager = (DefaultWebSecurityManager) ThreadContext.get(key);
+ DefaultWebSecurityManager defaultWebSecurityManager = (DefaultWebSecurityManager) ThreadContext.get(key);
return defaultWebSecurityManager.getRealms();
}
@@ -154,7 +153,7 @@ public class ShiroAuthenticationService implements AuthenticationService {
public List<String> getMatchedUsers(String searchText, int numUsersToFetch) {
List<String> usersList = new ArrayList<>();
try {
- Collection<Realm> realmsList = (Collection<Realm>) getRealmsList();
+ Collection<Realm> realmsList = getRealmsList();
if (realmsList != null) {
for (Realm realm : realmsList) {
String realClassName = realm.getClass().getName();
@@ -188,10 +187,9 @@ public class ShiroAuthenticationService implements AuthenticationService {
public List<String> getMatchedRoles() {
List<String> rolesList = new ArrayList<>();
try {
- Collection realmsList = getRealmsList();
+ Collection<Realm> realmsList = getRealmsList();
if (realmsList != null) {
- for (Iterator<Realm> iterator = realmsList.iterator(); iterator.hasNext(); ) {
- Realm realm = iterator.next();
+ for (Realm realm : realmsList) {
String name = realm.getClass().getName();
LOGGER.debug("RealmClass.getName: " + name);
if (name.equals("org.apache.shiro.realm.text.IniRealm")) {
@@ -220,9 +218,8 @@ public class ShiroAuthenticationService implements AuthenticationService {
Map allRoles = null;
if (subject.isAuthenticated()) {
- Collection realmsList = getRealmsList();
- for (Iterator<Realm> iterator = realmsList.iterator(); iterator.hasNext(); ) {
- Realm realm = iterator.next();
+ Collection<Realm> realmsList = getRealmsList();
+ for (Realm realm : realmsList) {
String name = realm.getClass().getName();
if (name.equals("org.apache.shiro.realm.text.IniRealm")) {
allRoles = ((IniRealm) realm).getIni().get("roles");