Ravi Nori has posted comments on this change. Change subject: aaa : Add engine sso ......................................................................
Patch Set 56: (25 comments) https://gerrit.ovirt.org/#/c/36119/56/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/LoginPhase4Servlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/LoginPhase4Servlet.java: Line 22: throws ServletException, IOException { Line 23: HttpSession session = request.getSession(true); Line 24: if (SSOUtils.isUserAuthenticated(session)) { Line 25: log.debug("User is authenticated forwarding to module"); Line 26: SSOUtils.redirectToModule( > instead redirect to module, maybe consider to redirect back to the interact Let me finish everything else and think about this one Line 27: session, Line 28: request, Line 29: response); Line 30: } else { https://gerrit.ovirt.org/#/c/36119/56/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOServiceBatchServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOServiceBatchServlet.java: Line 29: private static final String VERSION = "version"; Line 30: private static final String AUTHN_CREDENTIALS = "authn-credentials"; Line 31: private static final String INVALIDATE_SESSION = "sso-token-invalidate"; Line 32: private static final String VALIDATE_SESSION = "sso-token-validate"; Line 33: private static final String QUERY_ENTITY = "sso-query-entity"; > we can remove the sso- from the query-entity command name. Done Line 34: private static final String STATUS = "status"; Line 35: private static final String MESSAGE = "MESSAGE"; Line 36: private static final String STATUS_OK = "OK"; Line 37: private static final String STATUS_INVALID_TOKEN = "INVALID_TOKEN"; Line 93: Line 94: @Override Line 95: public void init() throws ServletException { Line 96: commandsMap = new HashMap<>(); Line 97: commandsMap.put(AUTHN_CREDENTIALS, Command.AuthnCredentials); > the enum is nice, why not add this AUTHN_CREDNETIALS as the enum name or o Done Line 98: commandsMap.put(INVALIDATE_SESSION, Command.InvalidateSession); Line 99: commandsMap.put(QUERY_ENTITY, Command.QueryEntity); Line 100: commandsMap.put(VALIDATE_SESSION, Command.ValidateSession); Line 101: commandsMap.put(VERSION, Command.Version); Line 105: protected void service(HttpServletRequest request, HttpServletResponse response) Line 106: throws ServletException, IOException { Line 107: Line 108: String command = request.getParameter(COMMAND); Line 109: String version = request.getParameter(VERSION); > please debug the command and version. Done Line 110: try { Line 111: if (StringUtils.isEmpty(request.getParameter(SSOUtils.VERSION))) { Line 112: throw new RuntimeException("No version found in request."); Line 113: } Line 118: Command cmd = commandsMap.get(command); Line 119: if (cmd == null) { Line 120: log.error("Unknown command found in request: {} ", command); Line 121: throw new RuntimeException(String.format("Unknown command found in request %s", command)); Line 122: } else { > no need for else in exceptional pattern Done Line 123: cmd.execute(request, response); Line 124: } Line 125: } catch (Exception ex) { Line 126: sendJsonDataWithMessage(response, STATUS_INVALID_TOKEN, ex.getMessage()); Line 122: } else { Line 123: cmd.execute(request, response); Line 124: } Line 125: } catch (Exception ex) { Line 126: sendJsonDataWithMessage(response, STATUS_INVALID_TOKEN, ex.getMessage()); > better to send the exception and not just the message, so you can debug it. Done Line 127: } Line 128: } Line 129: Line 130: public static void authCredentials(HttpServletRequest request, HttpServletResponse response) throws IOException { Line 127: } Line 128: } Line 129: Line 130: public static void authCredentials(HttpServletRequest request, HttpServletResponse response) throws IOException { Line 131: Credentials credentials = SSOUtils.getUserCredentialsFromHeader(request); > please debug the credentials without password a toString() method of Creden Done Line 132: try { Line 133: if (credentials == null) { Line 134: throw new AuthenticationException("Unable to extract credentials from header"); Line 135: } else if (!credentials.isValid()) { Line 131: Credentials credentials = SSOUtils.getUserCredentialsFromHeader(request); Line 132: try { Line 133: if (credentials == null) { Line 134: throw new AuthenticationException("Unable to extract credentials from header"); Line 135: } else if (!credentials.isValid()) { > no need for else in exceptional pattern. Done Line 136: if (StringUtils.isEmpty(credentials.getUsername())) { Line 137: throw new AuthenticationException(String.format("No user name found in credentials")); Line 138: } Line 139: if (StringUtils.isEmpty(credentials.getProfile())) { Line 133: if (credentials == null) { Line 134: throw new AuthenticationException("Unable to extract credentials from header"); Line 135: } else if (!credentials.isValid()) { Line 136: if (StringUtils.isEmpty(credentials.getUsername())) { Line 137: throw new AuthenticationException(String.format("No user name found in credentials")); > question... why does not the isValid() throws the exception? Done Line 138: } Line 139: if (StringUtils.isEmpty(credentials.getProfile())) { Line 140: throw new AuthenticationException(String.format("No profile found in credentials")); Line 141: } Line 139: if (StringUtils.isEmpty(credentials.getProfile())) { Line 140: throw new AuthenticationException(String.format("No profile found in credentials")); Line 141: } Line 142: if (StringUtils.isEmpty(credentials.getPassword())) { Line 143: throw new AuthenticationException(String.format("No password found in credentials")); > empty password should be valid, you should get an error during authenticati Done Line 144: } Line 145: } else { Line 146: AuthenticationUtils.handleCredentials( Line 147: request.getSession(true), Line 141: } Line 142: if (StringUtils.isEmpty(credentials.getPassword())) { Line 143: throw new AuthenticationException(String.format("No password found in credentials")); Line 144: } Line 145: } else { > you can drop the isValid() and remove the else and achieve the same. Done Line 146: AuthenticationUtils.handleCredentials( Line 147: request.getSession(true), Line 148: credentials.getUsername(), Line 149: credentials.getPassword(), Line 146: AuthenticationUtils.handleCredentials( Line 147: request.getSession(true), Line 148: credentials.getUsername(), Line 149: credentials.getPassword(), Line 150: credentials.getProfile()); > won't it better to pass the credentials object? Done Line 151: String returnPayload = request.getParameter("payload"); Line 152: if (StringUtils.isEmpty(returnPayload) || "0".equals(returnPayload)) { Line 153: response.getWriter().print(SSOUtils.getSsoToken(request.getSession(true))); Line 154: } else { Line 151: String returnPayload = request.getParameter("payload"); Line 152: if (StringUtils.isEmpty(returnPayload) || "0".equals(returnPayload)) { Line 153: response.getWriter().print(SSOUtils.getSsoToken(request.getSession(true))); Line 154: } else { Line 155: sendJsonData(response, STATUS_OK, SSOUtils.getTokenPayload(request.getSession(true))); > better to replace the payload with entity and reuse the query, no? we can a We decided to use a parameter to determine what to return. This reduced round trip as we return payload right here. Do you want to change? Line 156: } Line 157: } Line 158: } catch (Exception e) { Line 159: String message = String.format("Authentication Failed %s: %s", getUserName(credentials), e.getMessage()); Line 157: } Line 158: } catch (Exception e) { Line 159: String message = String.format("Authentication Failed %s: %s", getUserName(credentials), e.getMessage()); Line 160: log.error(message); Line 161: sendJsonDataWithMessage(response, STATUS_UNAUTHORIZED, message); > any reason not to log the error at the sendJsonDataWithMessage? Done Line 162: } Line 163: } Line 164: Line 165: public static void getSsoVersion(HttpServletResponse response) throws IOException { Line 168: sendJsonData(response, STATUS_OK, data); Line 169: } Line 170: Line 171: public static void getTokenUser(HttpServletRequest request, HttpServletResponse response) throws IOException, GeneralSecurityException, SQLException { Line 172: String ssoToken = request.getParameter("sso_token"); > please debug the token This is invoked for entity=user not a direct end point Line 173: if (StringUtils.isEmpty(ssoToken) || Line 174: !((Map<String, Map>) request.getServletContext().getAttribute(SSOUtils.SSO_SESSION_DATA)).containsKey(ssoToken)) { Line 175: sendJsonDataWithMessage(response, STATUS_INVALID_TOKEN, Line 176: String.format("Get user failed, session not found for token %s", ssoToken)); Line 180: } Line 181: } Line 182: Line 183: public static void getTokenPayload(HttpServletRequest request, HttpServletResponse response) throws IOException, GeneralSecurityException, SQLException { Line 184: String ssoToken = request.getParameter("sso_token"); > please debug the token This is invoked for entity=token not a direct end point Line 185: if (StringUtils.isEmpty(ssoToken) || Line 186: !((Map<String, Map>) request.getServletContext().getAttribute(SSOUtils.SSO_SESSION_DATA)).containsKey(ssoToken)) { Line 187: sendJsonDataWithMessage(response, STATUS_INVALID_TOKEN, Line 188: String.format("Payload not found for token %s", ssoToken)); Line 191: } Line 192: } Line 193: Line 194: public static void invalidateSession(HttpServletRequest request, HttpServletResponse response) throws IOException { Line 195: String ssoToken = request.getParameter("sso_token"); > please debug the token Done Line 196: if (StringUtils.isEmpty(ssoToken) || Line 197: !((Map<String, HttpSession>) request.getServletContext().getAttribute(SSOUtils.SSO_SESSIONS)).containsKey(ssoToken)) { Line 198: sendJsonDataWithMessage(response, STATUS_INVALID_TOKEN, Line 199: String.format("Invalidate session failed, session not found for token %s", ssoToken)); Line 235: sendJsonData(response, STATUS_OK); Line 236: } Line 237: } Line 238: Line 239: private static String getUserName(Credentials credentials) { > this function is used only once and the "for user " addition suggests it is Using toString method of credentials Line 240: StringBuilder buf = new StringBuilder(""); Line 241: if (credentials != null) { Line 242: buf.append("for user "); Line 243: buf.append(credentials.getUsername()); Line 265: String jsonPayload = SSOUtils.getJson(payload); Line 266: response.setContentType("application/json"); Line 267: response.setContentLength(jsonPayload.length()); Line 268: os.write(jsonPayload.getBytes("UTF-8")); Line 269: } > please trace the output. Done Line 270: } https://gerrit.ovirt.org/#/c/36119/56/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOServiceInteractiveServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOServiceInteractiveServlet.java: Line 180: } Line 181: if (StringUtils.isNotEmpty(request.getParameter(SSOUtils.POST_LOGOUT_URL))) { Line 182: response.sendRedirect(request.getParameter(SSOUtils.POST_LOGOUT_URL)); Line 183: } else { Line 184: response.sendRedirect(((SSOConfig) request.getServletContext().getAttribute(SSOUtils.SSO_CONFIG)).getSsoPostLoginDefaultUrl()); > this should be a function that takes url and returns default if empty. so: Done Line 185: } Line 186: } Line 187: Line 188: public static void getSsoVersion(HttpServletResponse response) throws IOException, ServletException { Line 188: public static void getSsoVersion(HttpServletResponse response) throws IOException, ServletException { Line 189: response.getWriter().println(SSOUtils.SSO_VERSION); Line 190: } Line 191: Line 192: private static void getToken(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { > can we add query command instead of getSsoVersion and getToken? this can re Added query entity for version and token. Currently did not change to return token as json as it is returned to WelcomeServlet as a parameter. Line 193: if (StringUtils.isEmpty(request.getParameter(SSOUtils.POST_COMMAND_URL))) { Line 194: throw new RuntimeException("No post action url found in request."); Line 195: } Line 196: https://gerrit.ovirt.org/#/c/36119/56/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 12: Line 13: @Override Line 14: public void sessionCreated(HttpSessionEvent se) { Line 15: String sso_token = SSOUtils.generateSsoToken(); Line 16: se.getSession().setAttribute(SSOUtils.SSO_TOKEN, sso_token); > are you sure we need to create sso token in each session? maybe we do not w What will be key for session data map if we don't generate sso_token here and save it in session Line 17: ((Map<String, Map>) se.getSession().getServletContext().getAttribute(SSOUtils.SSO_SESSION_DATA)).put(sso_token, new HashMap<String, Object>()); Line 18: ((Map<String, HttpSession>) se.getSession().getServletContext().getAttribute(SSOUtils.SSO_SESSIONS)).put(sso_token, se.getSession()); Line 19: } Line 20: Line 14: public void sessionCreated(HttpSessionEvent se) { Line 15: String sso_token = SSOUtils.generateSsoToken(); Line 16: se.getSession().setAttribute(SSOUtils.SSO_TOKEN, sso_token); Line 17: ((Map<String, Map>) se.getSession().getServletContext().getAttribute(SSOUtils.SSO_SESSION_DATA)).put(sso_token, new HashMap<String, Object>()); Line 18: ((Map<String, HttpSession>) se.getSession().getServletContext().getAttribute(SSOUtils.SSO_SESSIONS)).put(sso_token, se.getSession()); > why not put the session object into the above map? Done Line 19: } Line 20: Line 21: @Override Line 22: public void sessionDestroyed(HttpSessionEvent se) { https://gerrit.ovirt.org/#/c/36119/56/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 33: public static final String OPAQUE = "opaque"; Line 34: public static final String VERSION = "version"; Line 35: public static final String SSO_VERSION = "0"; Line 36: public static final String POST_LOGOUT_URL = "post_logout_url"; Line 37: public static final String POST_COMMAND_URL = "post_command_url"; > can we merge these two? Removed POST_LOGOUT_URL Line 38: public static final String SSO_CONFIG = "config"; Line 39: public static final String SSO_SESSION_DATA = "session_data"; Line 40: public static final String SSO_SESSIONS = "sessions"; Line 41: public static final String AUTH_PROFILE_REPOSITORY = "auth_profile_repository"; Line 61: return sessionData.get(SSO_PRINCIPAL_RECORD_ATTR_NAME) != null && Line 62: "true".equals(sessionData.get(SSO_IS_EXTERNAL_AUTHN)); Line 63: } Line 64: Line 65: public static void redirectToModule(HttpSession session, > probably this should go into the interactive interface. This is used from Interactive servlet and LoginPhase4Servlet Line 66: HttpServletRequest request, Line 67: HttpServletResponse response) Line 68: throws IOException { Line 69: try { -- 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: 56 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
