Juan Hernandez has posted comments on this change. Change subject: aaa: Intorduce filters ......................................................................
Patch Set 6: (18 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, which may interfere with handling of the "Prefer: persistent-auth" header. Try to call "getSession(false)" and check if the result is null. 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. 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. 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 69: synchronized (this) { Line 70: if (profiles == null) { Line 71: schemes = new ArrayList<>(); Line 72: profiles = new ArrayList<AuthenticationProfile>(1); Line 73: long caps = 0; Why use an integer to store a bits when you can use a EnumSet? 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) { 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 each request, better do it once, in the "init" method. Line 75: try { Line 76: caps |= Authn.Capabilities.class.getField(nego).getInt(null); Line 77: } catch (IllegalArgumentException | IllegalAccessException | NoSuchFieldException ex) { Line 78: Line 72: profiles = new ArrayList<AuthenticationProfile>(1); 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); Another construct that isn't checked by the compiler, and impossible to understand. Line 77: } catch (IllegalArgumentException | IllegalAccessException | NoSuchFieldException ex) { Line 78: Line 79: } Line 80: } 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. Line 78: Line 79: } Line 80: } Line 81: Line 81: Line 82: for (AuthenticationProfile profile : AuthenticationProfileRepository.getInstance().getProfiles()) { Line 83: if (profile != null) { Line 84: ExtMap authnContext = profile.getAuthn().getContext(); Line 85: if ((authnContext.<Long> get(Authn.ContextKeys.CAPABILITIES).longValue() & caps) != caps) { Another thing that the compiler won't check. If the "get" method returns anything other than long the error will only be detected during run-time. Line 86: profiles.add(0, profile); Line 87: } Line 88: schemes.addAll(authnContext.<List<String>>get(Authn.ContextKeys.HTTP_AUTHENTICATION_SCHEME, Collections.<String>emptyList())); Line 89: } Line 84: ExtMap authnContext = profile.getAuthn().getContext(); Line 85: if ((authnContext.<Long> get(Authn.ContextKeys.CAPABILITIES).longValue() & caps) != caps) { Line 86: profiles.add(0, profile); Line 87: } Line 88: schemes.addAll(authnContext.<List<String>>get(Authn.ContextKeys.HTTP_AUTHENTICATION_SCHEME, Collections.<String>emptyList())); Same, try to avoid this bad practice. Line 89: } Line 90: } Line 91: } Line 92: } 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 compatible with the "Prefer: persistent-auth" handling? 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". Line 135: ExtMap output = profile.getAuthn().invoke( Line 136: new ExtMap().mput( Line 137: Base.InvokeKeys.COMMAND, Line 138: Authn.InvokeCommands.AUTHENTICATE_NEGOTIATE Line 142: ).mput( Line 143: Authn.InvokeKeys.HTTP_SERVLET_RESPONSE, Line 144: rsp Line 145: ) Line 146: ); This is impossible to understand. Any mistake you make here in names or types will go unchecked by the compiler and just appear, hopefully, while testing. I will -1 the complete change because of this. Line 147: Line 148: switch (output.<Integer> get(Authn.InvokeKeys.RESULT)) { Line 149: case Authn.AuthResult.SUCCESS: Line 150: ExtMap authRecord = output.<ExtMap> get(Authn.InvokeKeys.AUTH_RECORD); Line 144: rsp Line 145: ) Line 146: ); Line 147: Line 148: switch (output.<Integer> get(Authn.InvokeKeys.RESULT)) { Another hidden cast. Line 149: case Authn.AuthResult.SUCCESS: Line 150: ExtMap authRecord = output.<ExtMap> get(Authn.InvokeKeys.AUTH_RECORD); Line 151: session.setAttribute(FiltersHelper.Constants.AUTH_RECORD_KEY, authRecord); Line 152: session.removeAttribute(STACK_ATTR); Line 146: ); Line 147: Line 148: switch (output.<Integer> get(Authn.InvokeKeys.RESULT)) { Line 149: case Authn.AuthResult.SUCCESS: Line 150: ExtMap authRecord = output.<ExtMap> get(Authn.InvokeKeys.AUTH_RECORD); Another hidden cast. Line 151: session.setAttribute(FiltersHelper.Constants.AUTH_RECORD_KEY, authRecord); Line 152: session.removeAttribute(STACK_ATTR); Line 153: break; Line 154: 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? 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/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java: Line 162: Line 163: // Check that the authenticator provided by the profile supports password authentication: Line 164: authnExtension = profile.getAuthn(); Line 165: SessionDataContainer.getInstance().setAuthn(authnExtension); Line 166: authRecord = (ExtMap) getParameters().getAuthRecord(); More casts. Line 167: if (authRecord == null) { Line 168: Line 169: // Verify that the login name and password have been provided: Line 170: String loginName = getParameters().getLoginName(); 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 concept of password from the application. 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? 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: 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
