modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/CoreGUI.java | 16 modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/UserSessionManager.java | 176 +++++++--- modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/GWTServiceLookup.java | 6 modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/SubjectGWTService.java | 4 modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/util/rpc/TrackingRemoteServiceProxy.java | 10 modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/server/gwt/SubjectGWTServiceImpl.java | 4 6 files changed, 161 insertions(+), 55 deletions(-)
New commits: commit 41a92e1565e584c08cd4b45c17a8cef3d8a10f87 Author: Jay Shaughnessy jshaughn@redhat.com Date: Mon Jan 17 15:59:14 2011 -0500
Session Management Work - multiple fixes - Most importantly, coreGui initiated server side logout was never happening. Although requested the logout() command was getting filtered. This created a long window where, after a coreGui logout a user could F5 (browser refresh) and hijack the previous session. - The above fix still left a short interval allowintg the same sort of F5 session hijack. This is BZ 661785 and is now fixed. A second cookie had to be stored for this fix since the browser refresh didn't leave us enough state information to make the check. The new cookie is "RHQ_DoomedSession". It also required a new session state of UNKNOWN, which is the initial state. - When switching users after a session timeout the new user was navigated to the previous user's path. This is fixed, now goes to default path. - When switching users after a session timeout, from the dashboards view, the new user was left with the previous user's dashboard. This is fixed, the current user's dashboard will now be presented. - now explicitly users sessionId signature for server side logout, as opposed to Subject (imo its one step closer to allowing multiple sessions for the same user, and also it helped with these changes) - wrapped a few log messages in isLevelEnabled
diff --git a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/CoreGUI.java b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/CoreGUI.java index eb0df84..431f873 100644 --- a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/CoreGUI.java +++ b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/CoreGUI.java @@ -151,7 +151,7 @@ public class CoreGUI implements EntryPoint, ValueChangeHandler<String> { }
public void buildCoreUI() { - // If the core gui is already built (eg. from previous login, just refire event) + // If the core gui is already built (eg. from previous login) skip the build and just refire event if (rootCanvas == null) { MenuBarView menuBarView = new MenuBarView("TopMenu"); menuBarView.setWidth("100%"); @@ -176,12 +176,14 @@ public class CoreGUI implements EntryPoint, ValueChangeHandler<String> { }
if (History.getToken().equals("") || History.getToken().equals("LogOut")) { - // request a redraw of the MenuBarItem to ensure the correct sessio info is displayed - rootCanvas.getMember(0).markForRedraw(); + + // init the rootCanvas to ensure a clean slate for a new user + rootCanvas.initCanvas();
// go to default view if user doesn't specify a history token History.newItem(getDefaultView()); } else { + // otherwise just fire an event for the bookmarked URL they are returning to History.fireCurrentHistoryState(); } @@ -346,6 +348,14 @@ public class CoreGUI implements EntryPoint, ValueChangeHandler<String> { return "RHQ: " + path.toString(); }
+ public void initCanvas() { + // request a redraw of the MenuBarItem to ensure the correct session info is displayed + getMember(0).markForRedraw(); + + // remove any current viewId so the next requested path generates new content + this.currentViewId = null; + } + public void renderView(final ViewPath viewPath) { Window.setTitle(getViewPathTitle(viewPath));
diff --git a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/UserSessionManager.java b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/UserSessionManager.java index 661f5b0..d3fa43e 100644 --- a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/UserSessionManager.java +++ b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/UserSessionManager.java @@ -29,6 +29,7 @@ import com.google.gwt.http.client.RequestCallback; import com.google.gwt.http.client.RequestException; import com.google.gwt.http.client.Response; import com.google.gwt.user.client.Cookies; +import com.google.gwt.user.client.History; import com.google.gwt.user.client.Timer; import com.google.gwt.user.client.rpc.AsyncCallback;
@@ -52,6 +53,7 @@ import org.rhq.enterprise.gui.coregui.client.util.preferences.UserPreferences; * If logged out on server-side, LoginView will be shown, which sets local loggedIn bit to false. * * @author Joseph Marques + * @author Jay Shaughnessy */ public class UserSessionManager { private static final Messages MSG = CoreGUI.getMessages(); @@ -59,37 +61,43 @@ public class UserSessionManager { public static int SESSION_TIMEOUT = 29 * 60 * 1000; // 29 mins, just shorter than the 30-min web session timeout private static int LOGOUT_DELAY = 5 * 1000; // wait 5 seconds for in-flight requests to complete before logout
+ // The web session public static final String SESSION_NAME = "RHQ_Session";
+ // The web session scheduled to be logged out on the server side + private static final String DOOMED_SESSION_NAME = "RHQ_DoomedSession"; private static final String LOCATOR_ID = "SessionManagerLogin";
private static Subject sessionSubject; private static UserPreferences userPreferences;
- enum State { - IS_LOGGED_IN, // - IS_REGISTERING, // - IS_LOGGED_OUT; - } - - private static State sessionState = State.IS_LOGGED_OUT; - private static Timer sessionTimer = new Timer() { + private static Timer logoutTimer = new Timer() { @Override public void run() { - Log.info("Session timer expired."); - sessionState = State.IS_LOGGED_OUT; - new LoginView(LOCATOR_ID).showLoginDialog(); // log user out, show login dialog + logoutServerSide(); } }; - private static Timer logoutTimer = new Timer() { + + private static Timer sessionTimer = new Timer() { @Override public void run() { - logoutServerSide(); + Log.info("Session timer expired."); + new LoginView(LOCATOR_ID).showLoginDialog(); } };
+ enum State { + IS_LOGGED_IN, // + IS_REGISTERING, // + IS_LOGGED_OUT, // + IS_UNKNOWN; + } + + // At entry or browser refresh set state IS_UNKNOWN + private static State sessionState = State.IS_UNKNOWN; + private UserSessionManager() { - // static access only + // static access only }
public static void checkLoginStatus(final String user, final String password, final AsyncCallback<Subject> callback) { @@ -99,10 +107,11 @@ public class UserSessionManager { try { b.setCallback(new RequestCallback() { public void onResponseReceived(final Request request, final Response response) { + Log.info("response text = " + response.getText()); String sessionIdString = response.getText();
- //Checks for valid session strings + // If a session is active it will return valid session strings if (sessionIdString != null && sessionIdString.length() > 0) {
String[] parts = sessionIdString.split(":"); @@ -113,8 +122,27 @@ public class UserSessionManager { Log.info("sessionAccess-sessionId: " + sessionId); Log.info("sessionAccess-lastAccess: " + lastAccess);
+ // There is a window of LOGOUT_DELAY ms where the coreGui session is logged out but the + // server session is valid (to allow in-flight requests to process successfully). During + // this window prevent a browser refresh (F5) from being able to bypass the + // loginView and hijack the still-valid server session. We need to allow: + // 1) a browser refresh when coreGui is logged in (no doomedSession) + // 2) a valid, quick re-login (sessionState loggedOut, not unknown) + // Being careful of these scenarios, catch the bad refresh situation and + // redirect back to loginView + if (State.IS_UNKNOWN == sessionState && sessionId.equals(getDoomedSessionId())) { + + // a browser refresh kills any existing logoutTimer. Reschedule the logout. + sessionState = State.IS_LOGGED_OUT; + scheduleLogoutServerSide(sessionId); + + new LoginView(LOCATOR_ID).showLoginDialog(); + return; + } + String previousSessionId = getPreviousSessionId(); // may be null Log.info("sessionAccess-previousSessionId: " + previousSessionId); + if (previousSessionId == null || previousSessionId.equals(sessionId) == false) {
// persist sessionId if different from previously saved sessionId @@ -141,13 +169,38 @@ public class UserSessionManager { sessionTimer.schedule((int) expiryMillis); }
+ // Certain logins may not follow a "LogOut" history item. Specifically, if the session timer + // causes a logout the History token will be the user's current view. If the same user + // logs in again his view should be maintained, but if the subsequent login is for a + // different user we want him to start fresh, so in this case ensure a proper + // History token is set. + if (!History.getToken().equals("LogOut")) { + + if (null != sessionSubject && sessionSubject.getId() != subjectId) { + + // on user change register the logout + History.newItem("LogOut", false); + + } + // TODO else { + + // We don't currently capture enough state info to solve this scenario: + // 1) session expires + // 2) browser refresh + // 3) log in as different user. + // In this case the previous user's path will be the initial view for the new user. To + // solve this we'd need to somehow flag that a browser refresh has occurred. This may + // be doable by looking for state transitions from UNKNOWN to other states. + // } + } + // set the session subject, so the fetch to load the configuration works final Subject subject = new Subject(); subject.setId(subjectId); subject.setSessionId(Integer.valueOf(sessionId)); sessionSubject = subject;
- //populate the username for the subject for isUserWithPrincipal check in ldap processing + // populate the username for the subject for isUserWithPrincipal check in ldap processing subject.setName(user);
if (subject.getId() == 0) {//either i)ldap new user registration ii)ldap case sensitive match @@ -160,7 +213,7 @@ public class UserSessionManager { return; }
- //BZ-586435: insert case insensitivity for usernames with ldap auth + // BZ-586435: insert case insensitivity for usernames with ldap auth // locate first matching subject and attach. SubjectCriteria subjectCriteria = new SubjectCriteria(); subjectCriteria.setCaseSensitive(false); @@ -275,7 +328,9 @@ public class UserSessionManager { } }); }//end of server side session check; - } else {//invalid client session. Back to login + } else { + + //invalid client session. Back to login sessionState = State.IS_LOGGED_OUT; new LoginView(LOCATOR_ID).showLoginDialog(); return; @@ -304,19 +359,12 @@ public class UserSessionManager { * @param password */ public static void login(String user, String password) { + checkLoginStatus(user, password, new AsyncCallback<Subject>() { public void onSuccess(Subject result) { // will build UI if necessary, then fires history event sessionState = State.IS_LOGGED_IN;
- // subject and session may have been updated during this login request - if (sessionSubject.getSessionId() != result.getSessionId()) {//update - Log.trace("A new subject and session were returned. Updating sessionSubject."); - sessionSubject = result; - //update savedSessionId for browser refresh - saveSessionId(String.valueOf(sessionSubject.getSessionId())); - } - CoreGUI.get().buildCoreUI(); }
@@ -338,19 +386,26 @@ public class UserSessionManager { return Cookies.getCookie(SESSION_NAME); }
+ private static void saveDoomedSessionId(String doomedSessionId) { + Cookies.setCookie(DOOMED_SESSION_NAME, doomedSessionId); + } + + private static String getDoomedSessionId() { + return Cookies.getCookie(DOOMED_SESSION_NAME); + } + + private static void removeDoomedSessionId() { + Cookies.removeCookie(DOOMED_SESSION_NAME); + } + public static void refresh() { refresh(SESSION_TIMEOUT); }
private static void refresh(int millis) { - // if quickly logging back in, first cancel the logout timer so that we - // don't have race conditions to the server where the login request beats - // the logout request, which would appear to the user for the login to - // have failed since it will boot them back to the login screen. - logoutTimer.cancel(); - // now continue with the rest of the login logic sessionState = State.IS_LOGGED_IN; + Log.info("Refreshing session timer..."); sessionTimer.schedule(millis); } @@ -366,30 +421,63 @@ public class UserSessionManager {
// log out the web session on the server-side in a delayed fashion, // allowing enough time to pass to let in-flight requests complete + scheduleLogoutServerSide(String.valueOf(sessionSubject.getSessionId())); + + } + + private static void scheduleLogoutServerSide(String sessionId) { + if (null == sessionId) { + return; + } + + String doomedSessionId = getDoomedSessionId(); + + // if we are requesting an already doomed sessionId be logged out then cancel any existing timer + if (sessionId.equals(doomedSessionId)) { + logoutTimer.cancel(); + } + + saveDoomedSessionId(sessionId); logoutTimer.schedule(LOGOUT_DELAY); }
+ // only call this from the logoutTimer, all logout requests should be scheduled via logoutTimer(String) private static void logoutServerSide() { + final Integer doomedSessionId = Integer.valueOf(getDoomedSessionId()); + removeDoomedSessionId(); + + if (null == doomedSessionId) { + return; + } + + // if the subject scheduled for logout is now logged in again, don't log him out. Unlikely but + // possible for a quick re-login of the same user. + if (State.IS_LOGGED_IN == sessionState && null != sessionSubject + && doomedSessionId.equals(sessionSubject.getSessionId())) { + return; + } + try { - if (getSessionSubject() != null) { - GWTServiceLookup.getSubjectService().logout(UserSessionManager.getSessionSubject(), - new AsyncCallback<Void>() { - public void onFailure(Throwable caught) { - CoreGUI.getErrorHandler().handleError(MSG.util_userSession_logoutFail(), caught); - } + GWTServiceLookup.getSubjectService().logout(doomedSessionId, new AsyncCallback<Void>() { + public void onFailure(Throwable caught) { + CoreGUI.getErrorHandler().handleError(MSG.util_userSession_logoutFail(), caught); + }
- public void onSuccess(Void result) { - Log.trace("Logged out."); - } - }); - } + public void onSuccess(Void result) { + if (Log.isTraceEnabled()) { + Log.trace("Logged out: " + doomedSessionId); + } + } + }); } catch (Throwable caught) { CoreGUI.getErrorHandler().handleError(MSG.util_userSession_logoutFail(), caught); } }
public static boolean isLoggedIn() { - Log.trace("isLoggedIn = " + sessionState); + if (Log.isTraceEnabled()) { + Log.trace("isLoggedIn = " + sessionState); + } return sessionState == State.IS_LOGGED_IN; }
diff --git a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/GWTServiceLookup.java b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/GWTServiceLookup.java index 1e8a94e..ccbbdae 100644 --- a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/GWTServiceLookup.java +++ b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/GWTServiceLookup.java @@ -202,8 +202,10 @@ public class GWTServiceLookup {
String sessionId = UserSessionManager.getSessionId(); if (sessionId != null) { - Log.debug("SessionRpcRequestBuilder is adding sessionId(" + sessionId + ") to request(" - + serviceEntryPoint + ")"); + if (Log.isDebugEnabled()) { + Log.debug("SessionRpcRequestBuilder is adding sessionId(" + sessionId + ") to request(" + + serviceEntryPoint + ")"); + } rb.setHeader(UserSessionManager.SESSION_NAME, sessionId); } else { Log.error("SessionRpcRequestBuilder missing sessionId for request(" + serviceEntryPoint + ") "); diff --git a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/SubjectGWTService.java b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/SubjectGWTService.java index a09cdc1..379b403 100644 --- a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/SubjectGWTService.java +++ b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/gwt/SubjectGWTService.java @@ -84,9 +84,9 @@ public interface SubjectGWTService extends RemoteService { /** * Logs out a user. * - * @param subject The username for the current user + * @param sessionId The sessionId for the subject */ - void logout(Subject subject) throws RuntimeException; + void logout(int sessionId) throws RuntimeException;
/** * Updates an existing subject with new data. This does <b>not</b> cascade any changes to the roles, but it will save diff --git a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/util/rpc/TrackingRemoteServiceProxy.java b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/util/rpc/TrackingRemoteServiceProxy.java index 0697489..b361fb9 100644 --- a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/util/rpc/TrackingRemoteServiceProxy.java +++ b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/util/rpc/TrackingRemoteServiceProxy.java @@ -49,9 +49,11 @@ import org.rhq.enterprise.gui.coregui.client.UserSessionManager; */ public class TrackingRemoteServiceProxy extends RemoteServiceProxy {
+ /** Don't block methods used during the login or logout process. Declare the exceptions here. */ private static final Set<String> bypassMethods = new HashSet<String>(); static { bypassMethods.add("SubjectGWTService_Proxy.findSubjectsByCriteria"); + bypassMethods.add("SubjectGWTService_Proxy.logout"); }
public TrackingRemoteServiceProxy(String moduleBaseURL, String remoteServiceRelativePath, @@ -74,7 +76,9 @@ public class TrackingRemoteServiceProxy extends RemoteServiceProxy {
String sessionId = UserSessionManager.getSessionId(); if (sessionId != null) { - Log.debug("SessionRpcRequestBuilder is adding sessionId to request for (" + methodName + ")"); + if (Log.isDebugEnabled()) { + Log.debug("SessionRpcRequestBuilder is adding sessionId to request for (" + methodName + ")"); + } rb.setHeader(UserSessionManager.SESSION_NAME, sessionId); } else { Log.error("SessionRpcRequestBuilder missing sessionId for request (" + methodName + ")"); @@ -100,7 +104,9 @@ public class TrackingRemoteServiceProxy extends RemoteServiceProxy { protected <T> Request doInvoke(ResponseReader responseReader, String methodName, int invocationCount, String requestData, AsyncCallback<T> callback) {
- Log.debug("RPC method invocation: " + methodName); + if (Log.isDebugEnabled()) { + Log.debug("RPC method invocation: " + methodName); + } if (bypassMethods.contains(methodName) || !UserSessionManager.isLoggedOut()) { return super.doInvoke(responseReader, methodName, invocationCount, requestData, callback); } diff --git a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/server/gwt/SubjectGWTServiceImpl.java b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/server/gwt/SubjectGWTServiceImpl.java index df34cab..17423f5 100644 --- a/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/server/gwt/SubjectGWTServiceImpl.java +++ b/modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/server/gwt/SubjectGWTServiceImpl.java @@ -79,9 +79,9 @@ public class SubjectGWTServiceImpl extends AbstractGWTServiceImpl implements Sub } }
- public void logout(Subject subject) throws RuntimeException { + public void logout(int sessionId) throws RuntimeException { try { - subjectManager.logout(subject.getSessionId()); + subjectManager.logout(sessionId); } catch (Throwable t) { throw new RuntimeException(ThrowableUtil.getAllMessages(t)); }
rhq-commits@lists.fedorahosted.org