Alon Bar-Lev has posted comments on this change. Change subject: aaa : Add engine sso ......................................................................
Patch Set 65: (4 comments) 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 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. 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 put it in single place (function). 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 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 password, so you need to PBE the secret before comparing. 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
