Alon Bar-Lev has posted comments on this change. Change subject: aaa : Add engine sso ......................................................................
Patch Set 62: (10 comments) 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. 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 send an error. 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. 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. 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 75: SSOUtils.getRequestParameter(request, "password", "password"), Line 76: (SSOConfig) request.getServletContext().getAttribute(SSOUtils.SSO_CONFIG)); Line 77: String code = null; Line 78: if (credentials != null && credentials.isValid()) { Line 79: code = AuthenticationUtils.handleCredentials(request, false, credentials); you do not need code in this case, maybe better to modify the session data to be based on token not code. Line 80: } Line 81: Map<String, Object> sessionData = SSOUtils.getSessionData(request.getServletContext(), code); Line 82: log.debug("Sending json response"); Line 83: SSOUtils.sendJsonData(response, buildResponse(sessionData)); Line 89: private static void issueTokenUsingHttpHeaders(HttpServletRequest request, HttpServletResponse response, String redirectUri) throws Exception { Line 90: log.debug("Entered issueTokenUsingHttpHeaders"); Line 91: Credentials credentials = null; Line 92: try { Line 93: String code = ExternalAuthUtils.doAuth(request, response, false); again, there is no code in this case, only token. and, this external authentication can be negotiation using multiple iterations with remote, so you need to run until exhausted all options. Line 94: if (StringUtils.isEmpty(code)) { Line 95: log.debug("External auth failed trying basic auth"); Line 96: credentials = SSOUtils.getUserCredentialsFromHeader(request); Line 97: if (credentials != null && credentials.isValid()) { 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... but... :) 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. 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 reached? Line 27: } https://gerrit.ovirt.org/#/c/36119/62/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 349: if (!Boolean.parseBoolean(clientInfo.get(TRUSTED))) { Line 350: throw new OAuthException(ERR_CODE_ACCESS_DENIED, ERR_CODE_ACCESS_DENIED_MSG); Line 351: } Line 352: Line 353: if (StringUtils.isNotEmpty(clientSecret) && !clientInfo.get(CLIENT_SECRET).equals(clientSecret)) { the client secret should be a PBE not plain, I guess you left it for future, please add TODO comments for these cases. Line 354: throw new OAuthException(ERR_CODE_INVALID_REQUEST, ERR_CODE_INVALID_REQUEST_MSG); Line 355: } Line 356: Line 357: 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: 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
