Ravi Nori has posted comments on this change. Change subject: aaa : Add engine sso ......................................................................
Patch Set 65: (11 comments) https://gerrit.ovirt.org/#/c/36119/65/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 42: SSOUtils.redirectToErrorPage(request, response, ex); 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, new OAuthException(SSOUtils.ERR_CODE_SERVER_ERROR, ex.getMessage())); > why not move the log in both cases into the redirect function? Done Line 47: } Line 48: } Line 49: Line 50: private static void login(HttpServletRequest request, HttpServletResponse response, String scope) throws IOException, ServletException { Line 70: log.debug("User is authenticated redirecting to LoginPhase4Servlet"); Line 71: redirectUrl = request.getContextPath() + SSOUtils.LOGIN_PHASE4_URI; Line 72: } else if (SSOUtils.scopeAsList(scope).contains("ovirt-auth-identity")) { Line 73: String redirect_uri = SSOUtils.getParameter(request, SSOUtils.REDIRECT_URI); Line 74: redirectUrl = String.format("%s&error_code=%s&error=%s", redirect_uri, SSOUtils.ERR_CODE_ACCESS_DENIED_IDENTITY, SSOUtils.ERR_CODE_ACCESS_DENIED_MSG); > shouldn't you encode the parameters? Done Line 75: } else { Line 76: SSOConfig config = (SSOConfig) request.getServletContext().getAttribute(SSOUtils.SSO_CONFIG); Line 77: if (config.isEnableExternalAuth()) { Line 78: log.debug("Redirecting to ExternalAuthServlet"); https://gerrit.ovirt.org/#/c/36119/65/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 26: try { Line 27: String token = SSOUtils.getRequestParameter(request, SSOUtils.TOKEN, SSOUtils.TOKEN); Line 28: String scope = SSOUtils.getRequestParameter(request, SSOUtils.SCOPE, SSOUtils.SCOPE); Line 29: Line 30: String authCode = SSOUtils.getAuthorizationCode(request.getServletContext(), token); > why do we need the code at revoke? all we have is the token, data should be We have a map of auth code to token which needs to be cleaned up. Line 31: if (SSOUtils.scopeAsList(scope).contains("ovirt-auth-revoke-all")) { Line 32: String[] clientIdAndSecret = SSOUtils.getClientIdClientSecret(request); Line 33: SSOUtils.validateClientRequest(request, clientIdAndSecret[0], clientIdAndSecret[1], scope); Line 34: HttpSession existingSession = SSOUtils.getSession(request.getServletContext(), token); https://gerrit.ovirt.org/#/c/36119/65/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 42: break; Line 43: case "password": Line 44: issueTokenForPasswd(request, response, redirectUri); Line 45: break; Line 46: case "": > when is it empty? yes Line 47: issueTokenUsingHttpHeaders(request, response, redirectUri); Line 48: break; Line 49: default: Line 50: new OAuthException(SSOUtils.ERR_CODE_UNSUPPORTED_GRANT_TYPE, SSOUtils.ERR_CODE_UNSUPPORTED_GRANT_TYPE_MSG); Line 46: case "": Line 47: issueTokenUsingHttpHeaders(request, response, redirectUri); Line 48: break; Line 49: default: Line 50: new OAuthException(SSOUtils.ERR_CODE_UNSUPPORTED_GRANT_TYPE, SSOUtils.ERR_CODE_UNSUPPORTED_GRANT_TYPE_MSG); > throw? Done Line 51: } Line 52: } catch(OAuthException ex) { Line 53: SSOUtils.sendJsonDataWithMessage(response, ex); Line 54: } catch(AuthenticationException ex) { https://gerrit.ovirt.org/#/c/36119/65/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/UnauthorizedServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/UnauthorizedServlet.java: Line 20: SSOConfig config = (SSOConfig) request.getServletContext().getAttribute(SSOUtils.SSO_CONFIG); Line 21: if (config.isAllowDisableExternalAuth()) { Line 22: if (config.isEnforceNego()) { Line 23: SSOUtils.redirectToErrorPage(request, response, Line 24: new OAuthException(SSOUtils.ERR_CODE_UNAUTHORIZED_CLIENT, "HTTP Status 401 - authentication required.")); > the message goes to user, you can avoid technical details, I refer to the h Changed to "Authentication required." Line 25: } else { Line 26: request.getSession(true).setAttribute(SSOUtils.CHECK_AUTHENTICATION, "0"); Line 27: response.sendRedirect(request.getContextPath() + SSOUtils.LOGIN_FORM_URI); Line 28: } https://gerrit.ovirt.org/#/c/36119/65/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUtils.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUtils.java: Line 78: public static final String ERR_CODE_UNSUPPORTED_GRANT_TYPE = "unsupported_grant_type"; Line 79: public static final String ERR_CODE_INVALID_REQUEST = "invalid_request"; Line 80: public static final String ERR_CODE_UNAUTHORIZED_CLIENT = "unauthorized_client"; Line 81: public static final String ERR_CODE_ACCESS_DENIED = "access_denied"; Line 82: public static final String ERR_CODE_ACCESS_DENIED_IDENTITY = "access_denied_identity"; > do you mean unauthorized_client or access_denied? please make sure all your Done Line 83: public static final String ERR_CODE_INVALID_SCOPE = "invalid_scope"; Line 84: public static final String ERR_CODE_SERVER_ERROR = "server_error"; Line 85: Line 86: public static final String ERR_CODE_INVALID_GRANT_MSG = "The provided authorization grant is no longer valid or not valid for the requested redirect uri."; Line 127: } Line 128: } Line 129: Line 130: public static void redirectToErrorPage(HttpServletRequest request, HttpServletResponse response, OAuthException ex) { Line 131: log.debug("Entered redirectToErrorPage"); > here you can debug/log the error and remove it from caller. Done Line 132: try { Line 133: String redirectUrl = SSOUtils.getParameter(request, SSOUtils.REDIRECT_URI); Line 134: response.sendRedirect(String.format("%s&error_code=%s&error=%s", redirectUrl, response.encodeURL(ex.getCode()), response.encodeURL(ex.getMessage()))); Line 135: log.debug("Redirecting back to module: {}", redirectUrl); Line 138: log.debug("Error redirecting to error page", e); Line 139: throw new RuntimeException(ex); Line 140: } finally { Line 141: request.getSession(true).removeAttribute(PARAMS_MAP); Line 142: request.getSession(true).removeAttribute(CHECK_AUTHENTICATION); > I see this more than once, and this is security sensitive, so I suggest to Done Line 143: } Line 144: } Line 145: Line 146: private static String getRedirectUrl(String url, String defaultUrl) { Line 219: Line 220: public static String[] getClientIdClientSecretFromHeader(HttpServletRequest request) { Line 221: String[] retVal = new String[2]; Line 222: String header = request.getHeader(HEADER_AUTHORIZATION); Line 223: if (StringUtils.isNotEmpty(header)) { > please reject anything that is not Basic Done Line 224: String[] creds = new String( Line 225: Base64.decodeBase64(header.substring("Basic".length())), Line 226: Charset.forName("UTF-8") Line 227: ).split(":", 2); Line 358: if (!Boolean.parseBoolean(clientInfo.get(TRUSTED))) { Line 359: throw new OAuthException(ERR_CODE_ACCESS_DENIED, ERR_CODE_ACCESS_DENIED_MSG); Line 360: } Line 361: Line 362: if (StringUtils.isNotEmpty(clientSecret) && !clientInfo.get(CLIENT_SECRET).equals(clientSecret)) { > please note that the client secret within the clientInfo is PBE not plain p Done Line 363: throw new OAuthException(ERR_CODE_INVALID_REQUEST, ERR_CODE_INVALID_REQUEST_MSG); Line 364: } Line 365: Line 366: if (StringUtils.isNotEmpty(scope)) { -- 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: 65 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
