Alon Bar-Lev has posted comments on this change. Change subject: aaa : Add engine sso ......................................................................
Patch Set 62: (16 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 :) 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. 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? 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 well? Line 47: } Line 48: } Line 49: Line 50: private static void login(HttpServletRequest request, HttpServletResponse response, String scope) throws IOException, ServletException { Line 52: Line 53: if (!request.getParameterMap().isEmpty()) { Line 54: log.debug("Saving parameters map in session"); Line 55: request.getSession(true).setAttribute(SSOUtils.PARAMS_MAP, new HashMap<>(request.getParameterMap())); Line 56: } are you sure you want to override the session if already logged in? maybe this should be differed when actually needed? Line 57: 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 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 magic of: new HashSet(Arrays.asList(scope.trim().split("\s *"))) 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? 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 :) won't it be nicer to invalidate only if there is a session? this way you avoid calling listeners and such. 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 redirection at one place. 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. also, I think this scope should be checked first, as it is short circute. 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 Authorization header. there are no CLIENT_ID and CLIENT_SECRET headers. 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 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"); Line 28: String token = SSOUtils.getRequestParameter(request, SSOUtils.TOKEN, SSOUtils.TOKEN); we should also allow Bearer authorization header to accept token from, I updated the document, this way you can "authenticate" using your own ticket to revoke. Line 29: String scope = SSOUtils.getRequestParameter(request, SSOUtils.SCOPE, SSOUtils.SCOPE); Line 30: Line 31: SSOUtils.validateClientRequest(request, clientId, clientSecret, scope); Line 32: 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? 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 33: String authCode = SSOUtils.getAuthenticationCode(request.getServletContext(), token); 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(); this should be done only if scope is ovirt-auth-revoke-all, otherwise you revoke only the token for this specific client. if a special scope is required, you must verify the client_id/client_secret, otherwise you can revoke the token without client authentication. this is usable for restapi use case for example... end-user use the urn:ovirt:params:oauth:grant-type:http and then should be able to revoke his token. Line 38: } else { Line 39: log.debug("No exisiting Session found for token: {}, cannot invalidate session", token); Line 40: } Line 41: SSOUtils.sendJsonData(response, new HashMap<String, Object>()); 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... 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 parameters or within authorization headers, there are no specific headers for these. 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)) { -- 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
