Alon Bar-Lev has posted comments on this change. Change subject: aaa : Add engine sso ......................................................................
Patch Set 65: (5 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? also add the original exception in the 2nd case as cause to enable full trace. if a new parameter is required for debug, such ash 'what' (OAuthAuthorizeServlet) then add one. 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? 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 based on token not code. code should only be used for interactive authentication, then it should be gone. 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? do you mean: urn:ovirt:params:oauth:grant-type:http 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? Line 51: } Line 52: } catch(OAuthException ex) { Line 53: SSOUtils.sendJsonDataWithMessage(response, ex); Line 54: } catch(AuthenticationException ex) { -- 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
