Alon Bar-Lev has posted comments on this change. Change subject: aaa : Add engine sso ......................................................................
Patch Set 56: (17 comments) 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. 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 one of the properties? if property it can be: for (x : Command) { commandsMap.put(x.getName(), x); } no? 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. 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 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. 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 Credentials will be nice. 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. 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? you just repeat its logic... if you just drop it you get the same result... if (isEmpty(profile)) { throw } if (isEmpty(user)) { throw } handleCredentials(); 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 authentication. 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. 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? 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 add none/void entity. 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? we can pass exception into this function when we have an error. we should also debug the exception. I guess it can be: sendJsonDataWithMessage( response, STATUS_UNAUTHORIZED, new Exception( String.format( "Authentication Failed %s: %s", getUserName(credentials), e.getMessage() ), e ) ); this way the sendJson... knows that it is an error, as throwable object is given in parameter, so it can log an error, debug the exception/stack, convert the exception message to text etc... please remember that if e.getMessage() == null, to use the e.getClass().getName() 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 can't this be achieved using the query entity? just trying to reduce endpoints. 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 can't this be achieved using the query entity? just trying to reduce endpoints. 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 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 very specific, I suggest to remove, in all cases please put N/A or similar when you do not have an important field such as user. Line 240: StringBuilder buf = new StringBuilder(""); Line 241: if (credentials != null) { Line 242: buf.append("for user "); Line 243: buf.append(credentials.getUsername()); -- 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
