Alon Bar-Lev has posted comments on this change. Change subject: aaa: Introduce sso query service ......................................................................
Patch Set 1: (5 comments) https://gerrit.ovirt.org/#/c/38499/1/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/FiltersHelper.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/FiltersHelper.java: Line 83: if (StringUtils.isNotEmpty(sessionId)) { Line 84: try { Line 85: SSOQueryServiceUtils.getFromSsoQueryService(req, Line 86: String.format("command=sso-token-validate&sso_token=%s", sessionId)); Line 87: isValid = true; won't it better to have it as args... ? SSOQueryServiceUtils.getFromSsoQueryService("command=sso-token-validate", String.format("sso_token=%s", sessionId); or even as: SSOQueryServiceUtils.getFromSsoQueryService(req, "command=sso-token-validate&sso_token=%s", sessionId); Line 88: } catch (Exception e) { Line 89: log.error("Session not valid session id = " + sessionId, e.getMessage()); Line 90: } Line 91: } https://gerrit.ovirt.org/#/c/38499/1/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOQueryServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOQueryServlet.java: Line 51: switch (command) { Line 52: case AUTHN_CREDENTIALS: Line 53: authCredentials(request, response); Line 54: break; Line 55: case GET_TOKEN: get ticket? Line 56: getToken(request, response); Line 57: break; Line 58: case INVALIDATE_SESSION: Line 59: HttpSession existingSession = request.getSession(false); Line 67: case VALIDATE_SESSION: Line 68: validateSession(request, response); Line 69: break; Line 70: case VERSION: Line 71: response.getWriter().print(SSOUtils.SSO_VERSION); I think it will be best if we always return json Line 72: break; Line 73: default: Line 74: log.error("Unknown command " + command); Line 75: response.setStatus(HttpURLConnection.HTTP_INTERNAL_ERROR); Line 137: } Line 138: } Line 139: } catch (Exception e) { Line 140: log.error("Unable to authenticate {}", e.getMessage()); Line 141: response.sendError(HttpServletResponse.SC_UNAUTHORIZED); will it be easier for user to accept json? for example we need to set different status code for password change / user locked. which reminds me... we need a service for that :))) Line 142: } Line 143: } https://gerrit.ovirt.org/#/c/38499/1/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: are you sure this is usable? I mean shouldn't it be in the service implementation as it should be used only there. Line 1: package org.ovirt.engine.core.sso.utils; Line 2: Line 3: import org.apache.commons.codec.binary.Base64; Line 4: import org.apache.commons.lang.StringUtils; -- To view, visit https://gerrit.ovirt.org/38499 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic2f20efa531b8311218a204d942fbfca1fc615d6 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
