You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by ap...@apache.org on 2020/10/02 20:00:11 UTC
[incubator-pinot] branch master updated: [TE] Refactor. ThirdEye
Principal should be immutable. (#6085)
This is an automated email from the ASF dual-hosted git repository.
apucher pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 267abef [TE] Refactor. ThirdEye Principal should be immutable. (#6085)
267abef is described below
commit 267abefd2740aa60af61d26ab48d390b6d06ba3f
Author: Suvodeep Pyne <su...@users.noreply.github.com>
AuthorDate: Fri Oct 2 12:59:56 2020 -0700
[TE] Refactor. ThirdEye Principal should be immutable. (#6085)
ThirdEyePrincipal is used in authentication and should be an immutable object.
This change refactors the usages to ensure that the principal isn't modified.
Also, the ThirdEyePrincipal object had groups which are never used. Plus the members
are non private. All these issues have been addressed.
Refactor. No logic changes.
---
.../restclient/MockThirdEyeRcaRestClient.java | 12 ++++----
.../restclient/TestThirdEyeRcaRestClient.java | 3 +-
.../pinot/thirdeye/auth/ThirdEyeAuthFilter.java | 21 +++++++------
.../auth/ThirdEyeAuthenticatorDisabled.java | 5 +---
.../thirdeye/auth/ThirdEyeLdapAuthenticator.java | 3 +-
.../pinot/thirdeye/auth/ThirdEyePrincipal.java | 35 +++++-----------------
.../content/templates/MetricAnomaliesContent.java | 7 +++--
7 files changed, 31 insertions(+), 55 deletions(-)
diff --git a/thirdeye/thirdeye-dashboard/src/test/java/org/apache/pinot/thirdeye/common/restclient/MockThirdEyeRcaRestClient.java b/thirdeye/thirdeye-dashboard/src/test/java/org/apache/pinot/thirdeye/common/restclient/MockThirdEyeRcaRestClient.java
index 00a215f..dbc5e8a 100644
--- a/thirdeye/thirdeye-dashboard/src/test/java/org/apache/pinot/thirdeye/common/restclient/MockThirdEyeRcaRestClient.java
+++ b/thirdeye/thirdeye-dashboard/src/test/java/org/apache/pinot/thirdeye/common/restclient/MockThirdEyeRcaRestClient.java
@@ -19,6 +19,10 @@
package org.apache.pinot.thirdeye.common.restclient;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
import java.util.Map;
import javax.ws.rs.client.Client;
import javax.ws.rs.client.Invocation;
@@ -27,9 +31,6 @@ import javax.ws.rs.core.GenericType;
import javax.ws.rs.core.Response;
import org.apache.pinot.thirdeye.auth.ThirdEyePrincipal;
-import static org.mockito.Matchers.*;
-import static org.mockito.Mockito.*;
-
public class MockThirdEyeRcaRestClient {
@@ -47,9 +48,8 @@ public class MockThirdEyeRcaRestClient {
when(builder.get()).thenReturn(response);
when(response.readEntity(any(GenericType.class))).thenReturn(expectedResponse);
- ThirdEyePrincipal principal = new ThirdEyePrincipal();
- principal.setSessionKey("dummy");
+ ThirdEyePrincipal principal = new ThirdEyePrincipal(null, "dummy");
return new ThirdEyeRcaRestClient(client, principal);
}
-}
\ No newline at end of file
+}
diff --git a/thirdeye/thirdeye-dashboard/src/test/java/org/apache/pinot/thirdeye/common/restclient/TestThirdEyeRcaRestClient.java b/thirdeye/thirdeye-dashboard/src/test/java/org/apache/pinot/thirdeye/common/restclient/TestThirdEyeRcaRestClient.java
index 8d28d78..4919528 100644
--- a/thirdeye/thirdeye-dashboard/src/test/java/org/apache/pinot/thirdeye/common/restclient/TestThirdEyeRcaRestClient.java
+++ b/thirdeye/thirdeye-dashboard/src/test/java/org/apache/pinot/thirdeye/common/restclient/TestThirdEyeRcaRestClient.java
@@ -54,8 +54,7 @@ public class TestThirdEyeRcaRestClient {
Client client = MockAbstractRestClient.setupMockClient(expectedResponse);
- ThirdEyePrincipal principal = new ThirdEyePrincipal();
- principal.setSessionKey("dummy");
+ ThirdEyePrincipal principal = new ThirdEyePrincipal(null, "dummy");
ThirdEyeRcaRestClient rcaClient = new ThirdEyeRcaRestClient(client, principal);
Map<String, Object> result = rcaClient.getRootCauseHighlights(anomalyId);
diff --git a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeAuthFilter.java b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeAuthFilter.java
index fa4804c..7d0fb94 100644
--- a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeAuthFilter.java
+++ b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeAuthFilter.java
@@ -19,9 +19,6 @@
package org.apache.pinot.thirdeye.auth;
-import javax.ws.rs.core.SecurityContext;
-import org.apache.pinot.thirdeye.datalayer.bao.SessionManager;
-import org.apache.pinot.thirdeye.datalayer.dto.SessionDTO;
import io.dropwizard.auth.AuthFilter;
import io.dropwizard.auth.Authenticator;
import java.util.HashSet;
@@ -32,6 +29,9 @@ import javax.ws.rs.WebApplicationException;
import javax.ws.rs.container.ContainerRequestContext;
import javax.ws.rs.core.Cookie;
import javax.ws.rs.core.Response;
+import javax.ws.rs.core.SecurityContext;
+import org.apache.pinot.thirdeye.datalayer.bao.SessionManager;
+import org.apache.pinot.thirdeye.datalayer.dto.SessionDTO;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -62,9 +62,8 @@ public class ThirdEyeAuthFilter extends AuthFilter<ThirdEyeCredentials, ThirdEye
String uriPath = requestContext.getUriInfo().getPath();
LOG.info("Checking auth for {}", uriPath);
- ThirdEyePrincipal principal = new ThirdEyePrincipal();
-
- if (!isAuthenticated(requestContext, principal)) {
+ final ThirdEyePrincipal principal = getPrincipal(requestContext);
+ if (principal == null) {
// not authenticated, check exceptions
// authenticate end points should be out of auth filter
@@ -109,7 +108,7 @@ public class ThirdEyeAuthFilter extends AuthFilter<ThirdEyeCredentials, ThirdEye
}
}
- private boolean isAuthenticated(ContainerRequestContext containerRequestContext, ThirdEyePrincipal principal) {
+ private ThirdEyePrincipal getPrincipal(ContainerRequestContext containerRequestContext) {
Map<String, Cookie> cookies = containerRequestContext.getCookies();
if (cookies != null && cookies.containsKey(AUTH_TOKEN_NAME)) {
@@ -120,14 +119,14 @@ public class ThirdEyeAuthFilter extends AuthFilter<ThirdEyeCredentials, ThirdEye
SessionDTO sessionDTO = this.sessionDAO.findBySessionKey(sessionKey);
if (sessionDTO != null && System.currentTimeMillis() < sessionDTO.getExpirationTime()) {
// session exist in database and has not expired
- principal.setName(sessionDTO.getPrincipal());
- principal.setSessionKey(sessionKey);
+
+ final ThirdEyePrincipal principal = new ThirdEyePrincipal(sessionDTO.getPrincipal(), sessionKey);
LOG.info("Found valid session {} for user {}", sessionDTO.getSessionKey(), sessionDTO.getPrincipal());
- return true;
+ return principal;
}
}
}
- return false;
+ return null;
}
private static void setCurrentPrincipal(ThirdEyePrincipal principal) {
diff --git a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeAuthenticatorDisabled.java b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeAuthenticatorDisabled.java
index c959ead..e2e9643 100644
--- a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeAuthenticatorDisabled.java
+++ b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeAuthenticatorDisabled.java
@@ -36,9 +36,6 @@ public class ThirdEyeAuthenticatorDisabled implements Authenticator<ThirdEyeCred
public Optional<ThirdEyePrincipal> authenticate(ThirdEyeCredentials credentials) throws AuthenticationException {
LOG.info("Authentication is disabled. Accepting any credentials for {}.", credentials.getPrincipal());
- ThirdEyePrincipal principal = new ThirdEyePrincipal();
- principal.setName(credentials.getPrincipal());
-
- return Optional.of(principal);
+ return Optional.of(new ThirdEyePrincipal(credentials.getPrincipal()));
}
}
diff --git a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeLdapAuthenticator.java b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeLdapAuthenticator.java
index a3bc303..a0d5575 100644
--- a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeLdapAuthenticator.java
+++ b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyeLdapAuthenticator.java
@@ -93,8 +93,7 @@ public class ThirdEyeLdapAuthenticator implements Authenticator<ThirdEyeCredenti
}
if (authenticationResults.isAuthenticated()) {
- ThirdEyePrincipal principal = new ThirdEyePrincipal();
- principal.setName(env.get(Context.SECURITY_PRINCIPAL));
+ ThirdEyePrincipal principal = new ThirdEyePrincipal(env.get(Context.SECURITY_PRINCIPAL));
LOG.info("Successfully authenticated {} with LDAP", env.get(Context.SECURITY_PRINCIPAL));
return Optional.of(principal);
} else {
diff --git a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyePrincipal.java b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyePrincipal.java
index c63bdfc..54a06bc 100644
--- a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyePrincipal.java
+++ b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/auth/ThirdEyePrincipal.java
@@ -20,29 +20,18 @@
package org.apache.pinot.thirdeye.auth;
import java.security.Principal;
-import java.util.HashSet;
-import java.util.Set;
-
public class ThirdEyePrincipal implements Principal {
- String name; // 'username@domainName'
- Set<String> groups = new HashSet<>();
- String sessionKey;
-
- public ThirdEyePrincipal(String name, String token) {
- this.name = name;
- this.sessionKey = token;
- }
- public ThirdEyePrincipal() {
+ private final String name; // 'username@domainName'
+ private final String sessionKey;
+ public ThirdEyePrincipal(final String name) {
+ this(name, null);
}
- public String getSessionKey() {
- return sessionKey;
- }
-
- public void setSessionKey(String sessionKey) {
+ public ThirdEyePrincipal(final String name, final String sessionKey) {
+ this.name = name;
this.sessionKey = sessionKey;
}
@@ -51,15 +40,7 @@ public class ThirdEyePrincipal implements Principal {
return name;
}
- public void setName(String name) {
- this.name = name;
- }
-
- public Set<String> getGroups() {
- return groups;
- }
-
- public void setGroups(Set<String> groups) {
- this.groups = groups;
+ public String getSessionKey() {
+ return sessionKey;
}
}
diff --git a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/notification/content/templates/MetricAnomaliesContent.java b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/notification/content/templates/MetricAnomaliesContent.java
index 03df6db..de8d448 100644
--- a/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/notification/content/templates/MetricAnomaliesContent.java
+++ b/thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/notification/content/templates/MetricAnomaliesContent.java
@@ -74,9 +74,10 @@ public class MetricAnomaliesContent extends BaseNotificationContent {
this.configDAO = DAORegistry.getInstance().getDetectionConfigManager();
if (this.rcaClient == null) {
- ThirdEyePrincipal principal = new ThirdEyePrincipal();
- principal.setName(this.thirdEyeAnomalyConfig.getThirdEyeRestClientConfiguration().getAdminUser());
- principal.setSessionKey(this.thirdEyeAnomalyConfig.getThirdEyeRestClientConfiguration().getSessionKey());
+ final ThirdEyePrincipal principal = new ThirdEyePrincipal(
+ this.thirdEyeAnomalyConfig.getThirdEyeRestClientConfiguration().getAdminUser(),
+ this.thirdEyeAnomalyConfig.getThirdEyeRestClientConfiguration().getSessionKey()
+ );
this.rcaClient = new ThirdEyeRcaRestClient(principal, this.thirdEyeAnomalyConfig.getDashboardHost());
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org