Ravi Nori has posted comments on this change. Change subject: aaa : Add engine sso ......................................................................
Patch Set 62: (20 comments) https://gerrit.ovirt.org/#/c/36119/62//COMMIT_MSG Commit Message: Line 5: CommitDate: 2015-05-28 13:20:25 -0400 Line 6: Line 7: aaa : Add engine sso Line 8: Line 9: Add sso module that is oAuth2 compliant. > OAuth2 :) Done Line 10: Supports authentication using basic auth, Line 11: external auth using headers and falls back Line 12: to form based authentication. Line 13: https://gerrit.ovirt.org/#/c/36119/62/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/OAuthAuthorizeServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/OAuthAuthorizeServlet.java: Line 29: SSOUtils.validateClientRequest(request, clientId, null, scope); Line 30: Line 31: if (!responseType.equals("code")) { Line 32: throw new OAuthException(SSOUtils.ERR_CODE_INVALID_REQUEST, Line 33: String.format("The request contains unsupported parameter value %s for parameter %s.", > please always quote string parameters '%s' will provide a chance to parse. Done Line 34: responseType, SSOUtils.RESPONSE_TYPE)); Line 35: } Line 36: Line 37: login(request, response, scope); Line 38: } catch (OAuthException ex) { Line 39: log.error("OAuthException {}: {}", ex.getCode(), ex.getMessage()); Line 40: log.debug("OAuthException:", ex); Line 41: String redirect_uri = SSOUtils.getParameter(request, SSOUtils.REDIRECT_URI); Line 42: response.sendRedirect(String.format("%s&error_code=%s&error=%s", redirect_uri, ex.getCode(), ex.getMessage())); > I believe you need to uriencode these, no? Done Line 43: } catch (Exception ex) { Line 44: log.error(ex.getMessage()); Line 45: log.debug("Exception in OAuthAuthorizeServlet:", ex); Line 46: SSOUtils.redirectToErrorPage(request, response, ex); Line 42: response.sendRedirect(String.format("%s&error_code=%s&error=%s", redirect_uri, ex.getCode(), ex.getMessage())); Line 43: } catch (Exception ex) { Line 44: log.error(ex.getMessage()); Line 45: log.debug("Exception in OAuthAuthorizeServlet:", ex); Line 46: SSOUtils.redirectToErrorPage(request, response, ex); > maybe better to use the same method to redirect to error page in above as w The SSOUtils.redirectToErrorPage redirects to internal error page SSO Error URL. Should I always redirect back to redirect url and drop the internal error page? Line 47: } Line 48: } Line 49: Line 50: private static void login(HttpServletRequest request, HttpServletResponse response, String scope) throws IOException, ServletException { Line 54: log.debug("Saving parameters map in session"); Line 55: request.getSession(true).setAttribute(SSOUtils.PARAMS_MAP, new HashMap<>(request.getParameterMap())); Line 56: } Line 57: Line 58: if ("ovirt-auth-force-internal".equals(scope)) { > scope is space delimitered string, when accepting, you can do the usual mag Done Line 59: log.debug("Performing switch user queryString: {}", request.getQueryString()); Line 60: SSOConfig config = (SSOConfig) request.getServletContext().getAttribute(SSOUtils.SSO_CONFIG); Line 61: if (!config.isAllowDisableExternalAuth()) { Line 62: throw new RuntimeException("Switch user is not permitted"); Line 58: if ("ovirt-auth-force-internal".equals(scope)) { Line 59: log.debug("Performing switch user queryString: {}", request.getQueryString()); Line 60: SSOConfig config = (SSOConfig) request.getServletContext().getAttribute(SSOUtils.SSO_CONFIG); Line 61: if (!config.isAllowDisableExternalAuth()) { Line 62: throw new RuntimeException("Switch user is not permitted"); > OAuthException with access denied? So who is responsible for showing the error page "Switch user is not permitted" Line 63: } Line 64: request.getSession(true).invalidate(); Line 65: if (!request.getParameterMap().isEmpty()) { Line 66: log.debug("Saving parameters map in session"); Line 60: SSOConfig config = (SSOConfig) request.getServletContext().getAttribute(SSOUtils.SSO_CONFIG); Line 61: if (!config.isAllowDisableExternalAuth()) { Line 62: throw new RuntimeException("Switch user is not permitted"); Line 63: } Line 64: request.getSession(true).invalidate(); > you create a session only for destroying it :) Done Line 65: if (!request.getParameterMap().isEmpty()) { Line 66: log.debug("Saving parameters map in session"); Line 67: request.getSession(true).setAttribute(SSOUtils.PARAMS_MAP, new HashMap<>(request.getParameterMap())); Line 68: } Line 63: } Line 64: request.getSession(true).invalidate(); Line 65: if (!request.getParameterMap().isEmpty()) { Line 66: log.debug("Saving parameters map in session"); Line 67: request.getSession(true).setAttribute(SSOUtils.PARAMS_MAP, new HashMap<>(request.getParameterMap())); > dup as above, maybe can be deferred to be just before redirection and have Done Line 68: } Line 69: log.debug("Redirecting to LoginPhase2Servlet"); Line 70: String redirect = request.getContextPath() + SSOUtils.LOGIN_PHASE2_URI; Line 71: log.debug("Redirecting to url: {}", redirect); Line 72: response.sendRedirect(redirect); Line 73: } else if (SSOUtils.isUserAuthenticated(request.getSession(true))) { Line 74: log.debug("User is authenticated redirecting to LoginPhase4Servlet"); Line 75: request.getRequestDispatcher(SSOUtils.LOGIN_PHASE4_URI).forward(request, response); Line 76: } else if ("ovirt-auth-identity".equals(scope)) { > scope note as well. Done Line 77: String redirect_uri = SSOUtils.getParameter(request, SSOUtils.REDIRECT_URI); Line 78: response.sendRedirect(String.format("%s&error_code=%s&error=%s", redirect_uri, SSOUtils.ERR_CODE_ACCESS_DENIED, SSOUtils.ERR_CODE_ACCESS_DENIED_MSG)); Line 79: } else { Line 80: SSOConfig config = (SSOConfig) request.getServletContext().getAttribute(SSOUtils.SSO_CONFIG); https://gerrit.ovirt.org/#/c/36119/62/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/OAuthRevokeServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/OAuthRevokeServlet.java: Line 23: throws ServletException, IOException { Line 24: log.debug("Entered OAuthRevokeServlet"); Line 25: try { Line 26: String clientId = SSOUtils.getRequestParameter(request, SSOUtils.CLIENT_ID, "CLIENT_ID"); Line 27: String clientSecret = SSOUtils.getRequestParameter(request, SSOUtils.CLIENT_SECRET, "CLIENT_SECRET"); > client id and client secret should be either in query string or Basic Autho Done Line 28: String token = SSOUtils.getRequestParameter(request, SSOUtils.TOKEN, SSOUtils.TOKEN); Line 29: String scope = SSOUtils.getRequestParameter(request, SSOUtils.SCOPE, SSOUtils.SCOPE); Line 30: Line 31: SSOUtils.validateClientRequest(request, clientId, clientSecret, scope); Line 29: String scope = SSOUtils.getRequestParameter(request, SSOUtils.SCOPE, SSOUtils.SCOPE); Line 30: Line 31: SSOUtils.validateClientRequest(request, clientId, clientSecret, scope); Line 32: Line 33: String authCode = SSOUtils.getAuthenticationCode(request.getServletContext(), token); > why do you use the authCode and not token as a key? 1. /sso/oauth/authorize returns the authCode to user. 2. When client requests token he passes authCode. On SSo side at 1. I save the auth record, principal record using authCode as key, so when user sends auth code when requesting token info I can find the records Line 34: HttpSession existingSession = SSOUtils.getSession(request.getServletContext(), authCode); Line 35: if (existingSession != null) { Line 36: log.debug("Exisiting Session found for token: {}, invalidating session", token); Line 37: existingSession.invalidate(); Line 41: SSOUtils.sendJsonData(response, new HashMap<String, Object>()); Line 42: } catch (OAuthException ex) { Line 43: SSOUtils.sendJsonDataWithMessage(response, ex); Line 44: } catch (Exception ex) { Line 45: SSOUtils.sendJsonDataWithMessage(response, new OAuthException(SSOUtils.ERR_CODE_SERVER_ERROR, ex.getMessage())); > please put the ex as cause so you can debug it at the sendJsonDataWith... Done Line 46: } Line 47: } https://gerrit.ovirt.org/#/c/36119/62/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/OAuthTokenInfo.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/OAuthTokenInfo.java: Line 26: throws ServletException, IOException { Line 27: log.debug("Entered OAuthTokenInfo"); Line 28: try { Line 29: String clientId = SSOUtils.getRequestParameter(request, SSOUtils.CLIENT_ID, "CLIENT_ID"); Line 30: String clientSecret = SSOUtils.getRequestParameter(request, SSOUtils.CLIENT_SECRET, "CLIENT_SECRET"); > same, the client id and client secret can be query parameters, post paramet Done Line 31: String token = SSOUtils.getRequestParameter(request, SSOUtils.TOKEN, SSOUtils.TOKEN); Line 32: SSOUtils.validateClientRequest(request, clientId, clientSecret, null); Line 33: String accessCode = SSOUtils.getAuthenticationCode(request.getServletContext(), token); Line 34: if (StringUtils.isEmpty(accessCode)) { https://gerrit.ovirt.org/#/c/36119/62/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/OAuthTokenServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/OAuthTokenServlet.java: Line 29: throws ServletException, IOException { Line 30: try { Line 31: log.debug("Entered OAuthTokenServlet Query String: {}", request.getQueryString()); Line 32: String clientId = SSOUtils.getRequestParameter(request, SSOUtils.CLIENT_ID, "CLIENT_ID"); Line 33: String clientSecret = SSOUtils.getRequestParameter(request, SSOUtils.CLIENT_SECRET, "CLIENT_SECRET"); > same, authorization headers. Done Line 34: String grantType = SSOUtils.getRequestParameter(request, SSOUtils.GRANT_TYPE, SSOUtils.GRANT_TYPE); Line 35: String scope = SSOUtils.getRequestParameter(request, SSOUtils.SCOPE, SSOUtils.SCOPE); Line 36: String redirectUri = SSOUtils.getRequestParameter(request, SSOUtils.REDIRECT_URI, SSOUtils.REDIRECT_URI); Line 37: Line 43: break; Line 44: case "password": Line 45: issueTokenForPasswd(request, response, redirectUri); Line 46: break; Line 47: default: > please do not use default in security aware application, default should sen Done Line 48: issueTokenUsingHttpHeaders(request, response, redirectUri); Line 49: } Line 50: } catch(OAuthException ex) { Line 51: SSOUtils.sendJsonDataWithMessage(response, ex); Line 44: case "password": Line 45: issueTokenForPasswd(request, response, redirectUri); Line 46: break; Line 47: default: Line 48: issueTokenUsingHttpHeaders(request, response, redirectUri); > always use break... to avoid reordering issues. Done Line 49: } Line 50: } catch(OAuthException ex) { Line 51: SSOUtils.sendJsonDataWithMessage(response, ex); Line 52: } catch(AuthenticationException ex) { Line 60: } Line 61: Line 62: private static void issueTokenForAuthCode(HttpServletRequest request, HttpServletResponse response, String redirectUri) throws Exception { Line 63: log.debug("Entered issueTokenForAuthCode"); Line 64: String code = SSOUtils.getRequestParameter(request, SSOUtils.AUTHORIZATION_CODE, SSOUtils.AUTHORIZATION_CODE); > must check that code was issued for this specific client. I tried doing it using redirectUri but did not work for the following scenario 1. Use from welcome page clicks login from drop down (recirect uri = '/ovirt-engine') 2. Now user user accesses webadmin, redirect uri = '/ovirt-engine/webadmin' so I cant use the redirect uri to determine if it is the same client. Remote Address? Line 65: Map<String, Object> sessionData = SSOUtils.getSessionData(request.getServletContext(), code); Line 66: log.debug("Sending json response"); Line 67: SSOUtils.sendJsonData(response, buildResponse(sessionData)); Line 68: } Line 112: Map<String, Object> payload = new HashMap<>(); Line 113: payload.put(SSOUtils.ACCESS_TOKEN, sessionData.get(SSOUtils.ACCESS_TOKEN)); Line 114: payload.put(SSOUtils.SCOPE, sessionData.get(SSOUtils.SCOPE)); Line 115: payload.put(SSOUtils.EXPIRES_IN, sessionData.get(SSOUtils.VALID_TO)); Line 116: payload.put("token_type", "bearer"); > why do you mix constants and primitives? well, I know I prefer primitives.. Will replace with constant Line 117: return payload; Line 118: } https://gerrit.ovirt.org/#/c/36119/62/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOContextListener.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOContextListener.java: Line 28: config.setSsoExtensionsManager(new SSOExtensionsManager(config.getDbUtils(), localConfig)); Line 29: config.setSsoLocalConfig(localConfig); Line 30: Line 31: AuthenticationProfileRepository repo = new AuthenticationProfileRepository(config.getSsoExtensionsManager()); Line 32: ctx.setAttribute(SSOUtils.AUTH_PROFILE_REPOSITORY, repo); > not sure you need the repo temp var. Done Line 33: Line 34: ctx.setAttribute(SSOUtils.SSO_CONFIG, config); Line 35: Line 36: try { https://gerrit.ovirt.org/#/c/36119/62/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOSessionListener.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOSessionListener.java: Line 22: if (StringUtils.isNotEmpty(accessToken)) { Line 23: ((Map<String, Map>) se.getSession().getServletContext().getAttribute(SSOUtils.SSO_ACCESS_TOKEN_AUTH_CODE_MAP)).remove(accessToken); Line 24: } Line 25: ((Map<String, Map>) se.getSession().getServletContext().getAttribute(SSOUtils.SSO_SESSION_DATA)).remove(authCode); Line 26: } > unrelated question... when do we expire sessions when the valid-to is reach We do not have the logic yet Line 27: } -- To view, visit https://gerrit.ovirt.org/36119 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4894fc12653027271b6abd4dd5313b10593703fa Gerrit-PatchSet: 62 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
