Alon Bar-Lev has posted comments on this change. Change subject: aaa: Intorduce filters ......................................................................
Patch Set 9: (9 comments) http://gerrit.ovirt.org/#/c/28022/9/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/BasicAuthenticationFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/BasicAuthenticationFilter.java: Line 51: String[] creds = new String( Line 52: Base64.decodeBase64(headerValue.substring("Basic".length())), Line 53: Charset.forName("UTF-8") Line 54: ).split(":", 2); Line 55: handleCredentials(request, creds); why don't you pass user and password? Line 56: } Line 57: } Line 58: chain.doFilter(request, response); Line 59: } Line 66: } else if (userNameFormat == UserNameFormat.RESTAPI_SPECIFIC && qualified.indexOf("@") == -1) { Line 67: result = qualified.indexOf("\\"); Line 68: } Line 69: return result; Line 70: } why getSeparator? won't it better to have translateUser that returns: class XXX { String profile; String user; } Line 71: Line 72: // private void handleCredentials(ServletRequest request, String qualified, String password, int index) { Line 73: private void handleCredentials(ServletRequest request, String[] creds) { Line 74: if (creds != null && creds.length == 2 && getSeparator(creds[0]) != -1) { Line 68: } Line 69: return result; Line 70: } Line 71: Line 72: // private void handleCredentials(ServletRequest request, String qualified, String password, int index) { remove comment? Line 73: private void handleCredentials(ServletRequest request, String[] creds) { Line 74: if (creds != null && creds.length == 2 && getSeparator(creds[0]) != -1) { Line 75: int index = getSeparator(creds[0]); Line 76: String user = null, profileName = null; Line 70: } Line 71: Line 72: // private void handleCredentials(ServletRequest request, String qualified, String password, int index) { Line 73: private void handleCredentials(ServletRequest request, String[] creds) { Line 74: if (creds != null && creds.length == 2 && getSeparator(creds[0]) != -1) { if creds are null you should not call this function. the length should also be checked at caller. the user parsing is something you should do here... but much cleaner. Line 75: int index = getSeparator(creds[0]); Line 76: String user = null, profileName = null; Line 77: if (creds[0].charAt(index) == '@') { Line 78: // UPN format: user@profile Line 81: } else { Line 82: // legacy format: profile\\user Line 83: profileName = creds[0].substring(0, index); Line 84: user = creds[0].substring(index + 1); Line 85: } please move the above including the getSeparator into own function that parses the user into domain, user Line 86: Line 87: AuthenticationProfile profile = AuthenticationProfileRepository.getInstance().getProfile(profileName); Line 88: if (profile == null) { Line 89: String msg = String.format("Error in obtaining profile %1$s", profileName); Line 88: if (profile == null) { Line 89: String msg = String.format("Error in obtaining profile %1$s", profileName); Line 90: log.error(msg); Line 91: throw new RuntimeException(msg); Line 92: } this should return 401 or 403 with proper message, or just ignore as you ignore the error in login bellow. Line 93: Line 94: ExtMap outputMap = profile.getAuthn().invoke(new ExtMap().mput( Line 95: Base.InvokeKeys.COMMAND, Line 96: Authn.InvokeCommands.AUTHENTICATE_CREDENTIALS http://gerrit.ovirt.org/#/c/28022/9/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/NegotiationFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/NegotiationFilter.java: Line 52: @Override Line 53: public void init(FilterConfig filterConfig) throws ServletException { Line 54: String capsParam = filterConfig.getInitParameter(CAPABILITIES_PARAMETER); Line 55: if (capsParam == null) { Line 56: caps = 0; this should be outside of conditional caps = 0 if (capsParam != null) { for... } but if you initialize it at class scope, why do you need it here? Line 57: } else { Line 58: for (String nego : capsParam.trim().split("\\|")) { Line 59: try { Line 60: caps |= Authn.Capabilities.class.getField(nego).getInt(null); Line 54: String capsParam = filterConfig.getInitParameter(CAPABILITIES_PARAMETER); Line 55: if (capsParam == null) { Line 56: caps = 0; Line 57: } else { Line 58: for (String nego : capsParam.trim().split("\\|")) { " *\\| *" Line 59: try { Line 60: caps |= Authn.Capabilities.class.getField(nego).getInt(null); Line 61: } catch (IllegalArgumentException | IllegalAccessException | NoSuchFieldException ex) { Line 62: log.error(String.format("Error calculating authn capabilities while accessing constant %1$s", nego)); http://gerrit.ovirt.org/#/c/28022/9/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SessionValidationFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SessionValidationFilter.java: Line 40: httpSession.setAttribute( Line 41: FiltersHelper.Constants.AUTHENTICATED_KEY, Line 42: FiltersHelper.getBackend(ctx).runPublicQuery(VdcQueryType.ValidateSession, parameters).getSucceeded() Line 43: ); Line 44: FiltersHelper.closeContext(ctx); why not in finally? Line 45: } catch (Exception ex) { Line 46: log.error(String.format("An error has occurred while session validation. Message is %1$s", ex.getMessage())); Line 47: if (log.isDebugEnabled()) { Line 48: log.debug("", ex); -- To view, visit http://gerrit.ovirt.org/28022 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia5536d123b6407acf41b6946dde796bd67d1e073 Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
