From 5d449c0851c09b9e45ea97b2cfe691009b15323c Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Tue, 6 Dec 2022 22:13:13 -0500 Subject: [PATCH] Skip subject login if already authenticated and adds logout method to Subject Signed-off-by: Darshit Chanpura --- .../java/org/opensearch/authn/Subject.java | 11 +++++++ .../authn/internal/InternalSubject.java | 32 +++++++++++++++++++ .../opensearch/identity/noop/NoopSubject.java | 14 ++++++-- .../org/opensearch/rest/RestController.java | 30 ++--------------- 4 files changed, 57 insertions(+), 30 deletions(-) diff --git a/sandbox/libs/authn/src/main/java/org/opensearch/authn/Subject.java b/sandbox/libs/authn/src/main/java/org/opensearch/authn/Subject.java index 80414402e48c6..d1edd30686998 100644 --- a/sandbox/libs/authn/src/main/java/org/opensearch/authn/Subject.java +++ b/sandbox/libs/authn/src/main/java/org/opensearch/authn/Subject.java @@ -29,4 +29,15 @@ public interface Subject { * throws SubjectDisabled */ void login(final AuthenticationToken token); + + /** + * Logs this subject out and kills any session associated with it + */ + void logout(); + + /** + * Checks the current authentication status of this subject + * @return true if authenticated, false otherwise + */ + boolean isAuthenticated(); } diff --git a/sandbox/libs/authn/src/main/java/org/opensearch/authn/internal/InternalSubject.java b/sandbox/libs/authn/src/main/java/org/opensearch/authn/internal/InternalSubject.java index 5874439ebdcc9..705baf76953b0 100644 --- a/sandbox/libs/authn/src/main/java/org/opensearch/authn/internal/InternalSubject.java +++ b/sandbox/libs/authn/src/main/java/org/opensearch/authn/internal/InternalSubject.java @@ -8,6 +8,7 @@ import java.security.Principal; import java.util.Objects; +import org.apache.shiro.session.Session; import org.opensearch.authn.AuthenticationTokenHandler; import org.opensearch.authn.tokens.AuthenticationToken; import org.opensearch.authn.Subject; @@ -67,4 +68,35 @@ public void login(AuthenticationToken authenticationToken) { // Login via shiro realm. shiroSubject.login(authToken); } + + /** + * Logs out this subject + * + * TODO: test this method + */ + @Override + public void logout() { + try { + if (shiroSubject == null) return; + shiroSubject.logout(); + // Get current session and kill it before proceeding to create a new session + // TODO: need to study the impact of this + Session session = shiroSubject.getSession(false); + if (session == null) return; + session.stop(); + } catch (Exception e) { + // Ignore all errors, as we're trying to silently kill the session + } + } + + /** + * A flag to indicate whether this subject is already authenticated + * + * @return true if authenticated, false otherwise + */ + @Override + public boolean isAuthenticated() { + return shiroSubject.isAuthenticated(); + } + } diff --git a/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java b/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java index 7bba6249b19b4..ee74613739553 100644 --- a/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java +++ b/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java @@ -44,11 +44,19 @@ public String toString() { return "NoopSubject(principal=" + getPrincipal() + ")"; } - /** - * Logs the user in - */ @Override public void login(AuthenticationToken authenticationToken) { // Do nothing as noop subject is always logged in } + + @Override + public void logout() { + // Do nothing as noop subject should not be logged out + } + + @Override + public boolean isAuthenticated() { + // Noop subject is always authenticated + return true; + } } diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index 51e1e84e364a6..a70eb060070c6 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -35,9 +35,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.message.ParameterizedMessage; -import org.apache.shiro.SecurityUtils; import org.apache.shiro.authc.AuthenticationException; -import org.apache.shiro.session.Session; import org.opensearch.OpenSearchException; import org.opensearch.authn.tokens.AuthenticationToken; import org.opensearch.authn.tokens.BasicAuthToken; @@ -64,7 +62,6 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; -import java.io.Serializable; import java.net.URI; import java.util.Collections; import java.util.HashMap; @@ -680,32 +677,11 @@ private AuthenticationToken tokenType(String authHeader) { * @param headerToken Attempt login using this token */ private static void getShiroSessionAndLogin(Subject subject, AuthenticationToken headerToken) { - // Get or create a new session for this subject when there is none to ensure passing tests. - // TODO: Ensure that shiro session is allowed to be created everytime - logoutCurrentSubjectAndClearSessionIfAny(); + // TODO: potential repercussions. i.e. How to handle this in password change situations + // TODO: should logout be implemented to handle this + if (subject.isAuthenticated()) return; subject.login(headerToken); } - /** - * Logs out current user and kills the session if any to prevent Shiro from throwing - * {@link org.apache.shiro.session.UnknownSessionException} when calling - * {@link org.apache.shiro.session.mgt.DefaultSessionManager#retrieveSessionFromDataSource(Serializable sessionId)} - * - */ - private static void logoutCurrentSubjectAndClearSessionIfAny() { - try { - // logout current subject - org.apache.shiro.subject.Subject subject = SecurityUtils.getSubject(); - if (subject == null) return; - subject.logout(); - // Get current session and kill it before proceeding to create a new session - // TODO: need to study the impact of this - Session session = subject.getSession(false); - if (session == null) return; - session.stop(); - } catch (Exception e) { - // Ignore all errors, as we're trying to silently kill the session - } - } }