Yair Zaslavsky has posted comments on this change. Change subject: aaa: Intorduce filters ......................................................................
Patch Set 6: (10 comments) http://gerrit.ovirt.org/#/c/28022/6/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 37: } Line 38: } Line 39: Line 40: public static boolean isAuthenticated(HttpServletRequest request) { Line 41: HttpSession session = request.getSession(); > This means that you are creating a session whenever you call this method, w thanks , i'll check that. Line 42: return session.getAttribute(Constants.AUTHENTICATED_KEY) != null Line 43: && (boolean) session.getAttribute(Constants.AUTHENTICATED_KEY); Line 44: } Line 45: http://gerrit.ovirt.org/#/c/28022/6/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 39: Line 40: /** Line 41: * We store a boolean flag in the HTTP session that indicates if the user has been already authenticated, this is Line 42: * the key for that flag. Line 43: */ > There is nothing corresponding to this comment. Done Line 44: Line 45: /** Line 46: * In order to support several alternative authenticators we store their names in a stack inside the HTTP session, Line 47: * this is the key for that stack. Line 42: * the key for that flag. Line 43: */ Line 44: Line 45: /** Line 46: * In order to support several alternative authenticators we store their names in a stack inside the HTTP session, > The comment is not accurate, there isn't a "authenticator" concept here. Done Line 47: * this is the key for that stack. Line 48: */ Line 49: private static final String STACK_ATTR = NegotiationFilter.class.getName() + ".stack"; Line 50: Line 70: if (profiles == null) { Line 71: schemes = new ArrayList<>(); Line 72: profiles = new ArrayList<AuthenticationProfile>(1); Line 73: long caps = 0; Line 74: for (String nego : req.getServletContext().getInitParameter(CAPABILITIES_PARAMETER).trim().split("*|*")) { > What if the parameter is missing? Also you don't need to get/trim/split for I agree about the init, I'll move it there. Line 75: try { Line 76: caps |= Authn.Capabilities.class.getField(nego).getInt(null); Line 77: } catch (IllegalArgumentException | IllegalAccessException | NoSuchFieldException ex) { Line 78: Line 73: long caps = 0; Line 74: for (String nego : req.getServletContext().getInitParameter(CAPABILITIES_PARAMETER).trim().split("*|*")) { Line 75: try { Line 76: caps |= Authn.Capabilities.class.getField(nego).getInt(null); Line 77: } catch (IllegalArgumentException | IllegalAccessException | NoSuchFieldException ex) { > This exception should be logged. Done Line 78: Line 79: } Line 80: } Line 81: Line 109: } Line 110: Line 111: private void doAuth(HttpServletRequest req, HttpServletResponse rsp, FilterChain chain) Line 112: throws IOException, ServletException { Line 113: HttpSession session = req.getSession(); > Take into account that this creates a session if it doesn't exist. Is that Done Line 114: Line 115: if (!FiltersHelper.isAuthenticated(req) && session.getAttribute(FiltersHelper.Constants.USER_KEY) == null) { Line 116: // We need to remember which of the profiles was managing the negotiation with the client, so we store a Line 117: // stack Line 130: // Resume the negotiation with the profile at the top of the stack: Line 131: AuthenticationProfile profile = AuthenticationProfileRepository.getInstance().getProfile(stack.peek()); Line 132: if (profile == null) { Line 133: continue; Line 134: } else { > You don't need an else after "continue". Done Line 135: ExtMap output = profile.getAuthn().invoke( Line 136: new ExtMap().mput( Line 137: Base.InvokeKeys.COMMAND, Line 138: Authn.InvokeCommands.AUTHENTICATE_NEGOTIATE http://gerrit.ovirt.org/#/c/28022/6/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 29: Line 30: @Override Line 31: public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, Line 32: ServletException { Line 33: HttpSession httpSession = ((HttpServletRequest) request).getSession(); > Do you want to create the HTTP session here unconditionally? Done Line 34: String engineSession = (String) httpSession.getAttribute(FiltersHelper.Constants.ENGINE_SESSION_ID_KEY); Line 35: if (engineSession != null) { Line 36: InitialContext ctx = null; Line 37: try { http://gerrit.ovirt.org/#/c/28022/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/LoginUserParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/LoginUserParameters.java: Line 6: private static final long serialVersionUID = -1660445011620552804L; Line 7: Line 8: private static class AuthenticationInformation { Line 9: private String loginName; Line 10: private String password; > One of the results of the new authentication mechanism should be to remove The extension API has the the ability to authenticate on credential basis which includes the password, or to authenticate using negotiation. Line 11: private Object authRecord; Line 12: } Line 13: Line 14: private AuthenticationInformation authInfo; Line 7: Line 8: private static class AuthenticationInformation { Line 9: private String loginName; Line 10: private String password; Line 11: private Object authRecord; > An "authRecord" can be anything? we do nont want common to depend on the extensions-api. This is not ideal, I know. Line 12: } Line 13: Line 14: private AuthenticationInformation authInfo; Line 15: -- 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: 6 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
