From 42940a8828424a53969af0e6117052956325ff76 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Fri, 10 Jul 2015 02:28:30 +0530 Subject: [PATCH] CLOUDSTACK-8622: Reinstate working sessions in browser - Login is based on sessionkey HttpOnly Cookie - ApiServlet does login verification using sessionKey from both the request cookies and the API parameters. In both cases, if either or both are passed they should match the sessionKey stored in the current session of the HttpRequest - UI: it no longer needs to read or set sessionkey cookie - UI: it no longer needs to return g_sessionKey value in the API requests, though to support a sso mechanism g_sessionKey is still passed in the API is not null - Secure jsessionid cookie is set to be HttpOnly and Secure - SAML login should also set HttpOnly cookie before redirecting to UI - SAML: listIdps & getSPMetadata APIs are readonly now, won't log out a logged in user Performed tests (login, saml login if applicable, page refreshes, opening multiple tabs, logout) with following combinations: - SAML disabled, normal auth as admin, domain-admin and user - SAML enabled, normal auth as admin, domain-admin and user; and saml sso as admin, domain-admin and user Signed-off-by: Rohit Yadav This closes #574 This closes #308 --- .../api/auth/APIAuthenticationType.java | 2 +- .../GetServiceProviderMetaDataCmd.java | 2 +- .../cloudstack/api/command/ListIdpsCmd.java | 2 +- .../SAML2LoginAPIAuthenticatorCmd.java | 2 +- .../GetServiceProviderMetaDataCmdTest.java | 2 +- .../SAML2LoginAPIAuthenticatorCmdTest.java | 2 +- server/src/com/cloud/api/ApiServlet.java | 67 ++++++++------ ui/scripts/cloudStack.js | 92 ++++++++++--------- ui/scripts/sharedFunctions.js | 5 +- ui/scripts/ui-custom/login.js | 77 ++++++---------- utils/src/com/cloud/utils/HttpUtils.java | 3 + 11 files changed, 131 insertions(+), 125 deletions(-) diff --git a/api/src/org/apache/cloudstack/api/auth/APIAuthenticationType.java b/api/src/org/apache/cloudstack/api/auth/APIAuthenticationType.java index e8c7f0f5eda..fac969dbfcc 100644 --- a/api/src/org/apache/cloudstack/api/auth/APIAuthenticationType.java +++ b/api/src/org/apache/cloudstack/api/auth/APIAuthenticationType.java @@ -17,5 +17,5 @@ package org.apache.cloudstack.api.auth; public enum APIAuthenticationType { - LOGIN_API, LOGOUT_API + LOGIN_API, LOGOUT_API, READONLY_API } diff --git a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java index 466a3ddc533..75353f2c3c4 100644 --- a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java +++ b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java @@ -256,7 +256,7 @@ public class GetServiceProviderMetaDataCmd extends BaseCmd implements APIAuthent @Override public APIAuthenticationType getAPIType() { - return APIAuthenticationType.LOGIN_API; + return APIAuthenticationType.READONLY_API; } @Override diff --git a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/ListIdpsCmd.java b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/ListIdpsCmd.java index 0592425a1a0..ad387044b5f 100644 --- a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/ListIdpsCmd.java +++ b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/ListIdpsCmd.java @@ -97,7 +97,7 @@ public class ListIdpsCmd extends BaseCmd implements APIAuthenticator { @Override public APIAuthenticationType getAPIType() { - return APIAuthenticationType.LOGIN_API; + return APIAuthenticationType.READONLY_API; } @Override diff --git a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java index 26a4c9b4b58..8e67408b5f0 100644 --- a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java +++ b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java @@ -328,13 +328,13 @@ public class SAML2LoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthent resp.addCookie(new Cookie("domainid", URLEncoder.encode(loginResponse.getDomainId(), HttpUtils.UTF_8))); resp.addCookie(new Cookie("role", URLEncoder.encode(loginResponse.getType(), HttpUtils.UTF_8))); resp.addCookie(new Cookie("username", URLEncoder.encode(loginResponse.getUsername(), HttpUtils.UTF_8))); - resp.addCookie(new Cookie(ApiConstants.SESSIONKEY, URLEncoder.encode(loginResponse.getSessionKey(), HttpUtils.UTF_8))); resp.addCookie(new Cookie("account", URLEncoder.encode(loginResponse.getAccount(), HttpUtils.UTF_8))); String timezone = loginResponse.getTimeZone(); if (timezone != null) { resp.addCookie(new Cookie("timezone", URLEncoder.encode(timezone, HttpUtils.UTF_8))); } resp.addCookie(new Cookie("userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20"))); + resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly", ApiConstants.SESSIONKEY, loginResponse.getSessionKey())); resp.sendRedirect(SAML2AuthManager.SAMLCloudStackRedirectionUrl.value()); return ApiResponseSerializer.toSerializedString(loginResponse, responseType); } diff --git a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/GetServiceProviderMetaDataCmdTest.java b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/GetServiceProviderMetaDataCmdTest.java index db1323fa6df..86009ac4e5c 100644 --- a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/GetServiceProviderMetaDataCmdTest.java +++ b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/GetServiceProviderMetaDataCmdTest.java @@ -99,6 +99,6 @@ public class GetServiceProviderMetaDataCmdTest { @Test public void testGetAPIType() { - Assert.assertTrue(new GetServiceProviderMetaDataCmd().getAPIType() == APIAuthenticationType.LOGIN_API); + Assert.assertTrue(new GetServiceProviderMetaDataCmd().getAPIType() == APIAuthenticationType.READONLY_API); } } diff --git a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmdTest.java b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmdTest.java index 6960b3bb502..0c72512082d 100644 --- a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmdTest.java +++ b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmdTest.java @@ -197,6 +197,6 @@ public class SAML2LoginAPIAuthenticatorCmdTest { @Test public void testGetAPIType() { - Assert.assertTrue(new GetServiceProviderMetaDataCmd().getAPIType() == APIAuthenticationType.LOGIN_API); + Assert.assertTrue(new SAML2LoginAPIAuthenticatorCmd().getAPIType() == APIAuthenticationType.LOGIN_API); } } diff --git a/server/src/com/cloud/api/ApiServlet.java b/server/src/com/cloud/api/ApiServlet.java index 2a2844ef015..e30a6ca2ba2 100644 --- a/server/src/com/cloud/api/ApiServlet.java +++ b/server/src/com/cloud/api/ApiServlet.java @@ -16,22 +16,13 @@ // under the License. package com.cloud.api; -import java.io.UnsupportedEncodingException; -import java.net.URLDecoder; -import java.util.Arrays; -import java.util.Collections; -import java.net.InetAddress; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import javax.inject.Inject; -import javax.servlet.ServletConfig; -import javax.servlet.ServletException; -import javax.servlet.http.HttpServlet; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import javax.servlet.http.HttpSession; +import com.cloud.user.Account; +import com.cloud.user.AccountService; +import com.cloud.user.User; +import com.cloud.utils.HttpUtils; +import com.cloud.utils.StringUtils; +import com.cloud.utils.db.EntityManager; +import com.cloud.utils.net.NetUtils; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ApiServerService; @@ -45,13 +36,22 @@ import org.apache.log4j.Logger; import org.springframework.stereotype.Component; import org.springframework.web.context.support.SpringBeanAutowiringSupport; -import com.cloud.user.Account; -import com.cloud.user.AccountService; -import com.cloud.user.User; -import com.cloud.utils.HttpUtils; -import com.cloud.utils.StringUtils; -import com.cloud.utils.db.EntityManager; -import com.cloud.utils.net.NetUtils; +import javax.inject.Inject; +import javax.servlet.ServletConfig; +import javax.servlet.ServletException; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; +import java.io.UnsupportedEncodingException; +import java.net.InetAddress; +import java.net.URLDecoder; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; @Component("apiServlet") @SuppressWarnings("serial") @@ -187,7 +187,7 @@ public class ApiServlet extends HttpServlet { } session = req.getSession(true); if (ApiServer.isSecureSessionCookieEnabled()) { - resp.setHeader("SET-COOKIE", "JSESSIONID=" + session.getId() + ";Secure;Path=/client"); + resp.setHeader("SET-COOKIE", String.format("JSESSIONID=%s;Secure;HttpOnly;Path=/client", session.getId())); if (s_logger.isDebugEnabled()) { if (s_logger.isDebugEnabled()) { s_logger.debug("Session cookie is marked secure!"); @@ -198,11 +198,15 @@ public class ApiServlet extends HttpServlet { try { responseString = apiAuthenticator.authenticate(command, params, session, InetAddress.getByName(remoteAddress), responseType, auditTrailSb, req, resp); + if (session != null && session.getAttribute(ApiConstants.SESSIONKEY) != null) { + resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly", ApiConstants.SESSIONKEY, session.getAttribute(ApiConstants.SESSIONKEY))); + } } catch (ServerApiException e) { httpResponseCode = e.getErrorCode().getHttpCode(); responseString = e.getMessage(); s_logger.debug("Authentication failure: " + e.getMessage()); } + if (apiAuthenticator.getAPIType() == APIAuthenticationType.LOGOUT_API) { if (session != null) { final Long userId = (Long) session.getAttribute("userid"); @@ -220,6 +224,9 @@ public class ApiServlet extends HttpServlet { } catch (final IllegalStateException ignored) { } } + Cookie sessionKeyCookie = new Cookie(ApiConstants.SESSIONKEY, ""); + sessionKeyCookie.setMaxAge(0); + resp.addCookie(sessionKeyCookie); } HttpUtils.writeHttpResponse(resp, responseString, httpResponseCode, responseType, ApiServer.getJSONContentType()); return; @@ -236,11 +243,15 @@ public class ApiServlet extends HttpServlet { if (!isNew) { userId = (Long)session.getAttribute("userid"); - final String account = (String)session.getAttribute("account"); + final String account = (String) session.getAttribute("account"); final Object accountObj = session.getAttribute("accountobj"); - final String sessionKey = (String)session.getAttribute(ApiConstants.SESSIONKEY); - final String[] sessionKeyParam = (String[])params.get(ApiConstants.SESSIONKEY); - if ((sessionKeyParam == null) || (sessionKey == null) || !sessionKey.equals(sessionKeyParam[0])) { + final String sessionKey = (String) session.getAttribute(ApiConstants.SESSIONKEY); + final String sessionKeyFromCookie = HttpUtils.findCookie(req.getCookies(), ApiConstants.SESSIONKEY); + final String[] sessionKeyFromParams = (String[]) params.get(ApiConstants.SESSIONKEY); + if ((sessionKey == null) + || (sessionKeyFromParams == null && sessionKeyFromCookie == null) + || (sessionKeyFromParams != null && !sessionKey.equals(sessionKeyFromParams[0])) + || (sessionKeyFromCookie != null && !sessionKey.equals(sessionKeyFromCookie))) { try { session.invalidate(); } catch (final IllegalStateException ise) { diff --git a/ui/scripts/cloudStack.js b/ui/scripts/cloudStack.js index 14d0cc202f3..6d08735381f 100644 --- a/ui/scripts/cloudStack.js +++ b/ui/scripts/cloudStack.js @@ -107,28 +107,11 @@ // Use this for checking the session, to bypass login screen bypassLoginCheck: function(args) { //determine to show or bypass login screen if (g_loginResponse == null) { //show login screen - /* - * Since we no longer store sessionKey in cookie, opening the - * 2nd browser window (of the same domain) will show login screen (i.e. user has to - * enter credentials again) and will cause the 1st browser window session timeout. - */ var unBoxCookieValue = function (cookieName) { - var cookieValue = $.cookie(cookieName); - if (cookieValue && cookieValue.length > 2 && cookieValue[0] === '"' && cookieValue[cookieValue.length-1] === '"') { - cookieValue = cookieValue.slice(1, cookieValue.length-1); - $.cookie(cookieName, cookieValue, { expires: 1 }); - } - return decodeURIComponent(cookieValue); + return decodeURIComponent($.cookie(cookieName)).replace(/"([^"]+(?="))"/g, '$1'); }; - unBoxCookieValue('sessionkey'); - // if sessionkey cookie exists use this to set g_sessionKey - // and destroy sessionkey cookie - if ($.cookie('sessionkey')) { - g_sessionKey = $.cookie('sessionkey'); - $.cookie('sessionkey', null); - } else { - g_sessionKey = unBoxCookieValue('JSESSIONID'); - } + // sessionkey is a HttpOnly cookie now, no need to pass as API param + g_sessionKey = null; g_role = unBoxCookieValue('role'); g_userid = unBoxCookieValue('userid'); g_domainid = unBoxCookieValue('domainid'); @@ -136,7 +119,7 @@ g_username = unBoxCookieValue('username'); g_userfullname = unBoxCookieValue('userfullname'); g_timezone = unBoxCookieValue('timezone'); - } else { //single-sign-on (bypass login screen) + } else { //single-sign-on (bypass login screen) g_sessionKey = encodeURIComponent(g_loginResponse.sessionkey); g_role = g_loginResponse.type; g_username = g_loginResponse.username; @@ -144,7 +127,7 @@ g_account = g_loginResponse.account; g_domainid = g_loginResponse.domainid; g_userfullname = g_loginResponse.firstname + ' ' + g_loginResponse.lastname; - g_timezone = g_loginResponse.timezone; + g_timezone = g_loginResponse.timezone; } var userValid = false; @@ -180,7 +163,25 @@ return true; } }); - + + // Populate IDP list + $.ajax({ + type: 'GET', + url: createURL('listIdps'), + dataType: 'json', + async: false, + success: function(data, textStatus, xhr) { + if (data && data.listidpsresponse && data.listidpsresponse.idp) { + var idpList = data.listidpsresponse.idp.sort(function (a, b) { + return a.orgName.localeCompare(b.orgName); + }); + g_idpList = idpList; + } + }, + error: function(xhr) { + }, + }); + return userValid ? { user: { userid: g_userid, @@ -227,14 +228,16 @@ async: false, success: function(json) { var loginresponse = json.loginresponse; - - g_sessionKey = encodeURIComponent(loginresponse.sessionkey); + // sessionkey is recevied as a HttpOnly cookie + // therefore reset any g_sessionKey value, an explicit + // param in the API call is no longer needed + g_sessionKey = null; g_role = loginresponse.type; g_username = loginresponse.username; g_userid = loginresponse.userid; g_account = loginresponse.account; g_domainid = loginresponse.domainid; - g_timezone = loginresponse.timezone; + g_timezone = loginresponse.timezone; g_userfullname = loginresponse.firstname + ' ' + loginresponse.lastname; $.cookie('username', g_username, { @@ -320,7 +323,7 @@ $.ajax({ url: createURL('logout'), async: false, - success: function() { + success: function() { g_sessionKey = null; g_username = null; g_account = null; @@ -331,15 +334,16 @@ g_kvmsnapshotenabled = null; g_regionsecondaryenabled = null; g_loginCmdText = null; - - $.cookie('JSESSIONID', null); - $.cookie('sessionkey', null); - $.cookie('username', null); - $.cookie('account', null); - $.cookie('domainid', null); - $.cookie('role', null); - $.cookie('timezone', null); - + + // Remove any cookies + var cookies = document.cookie.split(";"); + for (var i = 0; i < cookies.length; i++) { + var cookieName = $.trim(cookies[i].split("=")[0]); + if (['login-option', 'lang'].indexOf(cookieName) < 0) { + $.cookie(cookieName, null); + } + } + if (onLogoutCallback()) { //onLogoutCallback() will set g_loginResponse(single-sign-on variable) to null, then bypassLoginCheck() will show login screen. document.location.reload(); //when onLogoutCallback() returns true, reload the current document. } @@ -367,13 +371,15 @@ g_regionsecondaryenabled = null; g_loginCmdText = null; - $.cookie('JSESSIONID', null); - $.cookie('sessionkey', null); - $.cookie('username', null); - $.cookie('account', null); - $.cookie('domainid', null); - $.cookie('role', null); - $.cookie('timezone', null); + // Remove any cookies + var cookies = document.cookie.split(";"); + for (var i = 0; i < cookies.length; i++) { + var cookieName = $.trim(cookies[i].split("=")[0]); + if (['login-option', 'lang'].indexOf(cookieName) < 0) { + $.cookie(cookieName, null); + } + } + var url = 'samlSso'; if (args.data.idpid) { url = url + '&idpid=' + args.data.idpid; diff --git a/ui/scripts/sharedFunctions.js b/ui/scripts/sharedFunctions.js index c29956a1d65..14cd0f35b30 100644 --- a/ui/scripts/sharedFunctions.js +++ b/ui/scripts/sharedFunctions.js @@ -170,7 +170,10 @@ var pollAsyncJobResult = function(args) { function createURL(apiName, options) { if (!options) options = {}; - var urlString = clientApiUrl + "?" + "command=" + apiName + "&response=json&sessionkey=" + g_sessionKey; + var urlString = clientApiUrl + "?" + "command=" + apiName + "&response=json"; + if (g_sessionKey) { + urlString += "&sessionkey=" + g_sessionKey; + } if (cloudStack.context && cloudStack.context.projects && !options.ignoreProject) { urlString = urlString + '&projectid=' + cloudStack.context.projects[0].id; diff --git a/ui/scripts/ui-custom/login.js b/ui/scripts/ui-custom/login.js index 7c7e923a0e3..e1129583454 100644 --- a/ui/scripts/ui-custom/login.js +++ b/ui/scripts/ui-custom/login.js @@ -129,10 +129,6 @@ return false; }); - // Show SAML button if only SP is configured - $login.find('#login-dropdown').hide(); - $login.find('#saml-login').hide(); - $login.find('#cloudstack-login').hide(); var toggleLoginView = function (selectedOption) { $login.find('#login-submit').show(); @@ -160,56 +156,43 @@ } }); - $.ajax({ - type: 'GET', - url: createURL('listIdps'), - dataType: 'json', - async: false, - success: function(data, textStatus, xhr) { - if (xhr.status === 200) { - $login.find('#login-dropdown').show(); - $login.find('#login-submit').hide(); - } else { - $login.find('#cloudstack-login').show(); - $login.find('#login-submit').show(); - return; - } + // By Default hide login option dropdown + $login.find('#login-dropdown').hide(); + $login.find('#login-submit').show(); + $login.find('#cloudstack-login').show(); + $login.find('#saml-login').hide(); + // If any IdP servers were set, SAML is enabled + if (g_idpList && g_idpList.length > 0) { + $login.find('#login-dropdown').show(); + $login.find('#login-submit').hide(); + $login.find('#cloudstack-login').hide(); + $login.find('#saml-login').hide(); + + $login.find('#login-options') + .append($('