Alon Bar-Lev has posted comments on this change. Change subject: aaa: Intorduce filters ......................................................................
Patch Set 4: (10 comments) http://gerrit.ovirt.org/#/c/28022/4/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 25: public final static String PROFILE_KEY = "profile"; Line 26: public final static String PASSWORD_KEY = "password"; Line 27: public final static String AUTH_RECORD_KEY = "auth_record"; Line 28: public final static String UNAUTHORIZED_KEY = "unauthorized"; Line 29: } inner classes first please Line 30: Line 31: public static void closeContext(InitialContext ctx) { Line 32: try { Line 33: ctx.close(); http://gerrit.ovirt.org/#/c/28022/4/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 64: synchronized (this) { Line 65: if (profiles == null) { Line 66: profiles = new ArrayList<AuthenticationProfile>(1); Line 67: long caps = 0; Line 68: for (String nego : req.getServletContext().getInitParameter(CAPABILITIES_PARAMETER).split("|")) { Change to .trim().split(" *| *") and remove bellow trims. Line 69: if (nego.trim().equals("AUTHENTICATE_NEGOTIATE_INTERACTIVE")) { Line 70: caps |= Authn.Capabilities.AUTHENTICATE_NEGOTIATE_INTERACTIVE; Line 71: } else if (nego.trim().equals("AUTHENTICATE_NEGOTIATE_NON_INTERACTIVE")) { Line 72: caps |= Authn.Capabilities.AUTHENTICATE_NEGOTIATE_NON_INTERACTIVE; Line 65: if (profiles == null) { Line 66: profiles = new ArrayList<AuthenticationProfile>(1); Line 67: long caps = 0; Line 68: for (String nego : req.getServletContext().getInitParameter(CAPABILITIES_PARAMETER).split("|")) { Line 69: if (nego.trim().equals("AUTHENTICATE_NEGOTIATE_INTERACTIVE")) { or... better: caps |= Authn.Capabilities.class.getField(nego).getInt(null); Line 70: caps |= Authn.Capabilities.AUTHENTICATE_NEGOTIATE_INTERACTIVE; Line 71: } else if (nego.trim().equals("AUTHENTICATE_NEGOTIATE_NON_INTERACTIVE")) { Line 72: caps |= Authn.Capabilities.AUTHENTICATE_NEGOTIATE_NON_INTERACTIVE; Line 73: } Line 74: } Line 75: Line 76: for (AuthenticationProfile profile : AuthenticationProfileRepository.getInstance().getProfiles()) { Line 77: if (profile != null) { Line 78: if ((profile.getAuthn().getContext().<Long> get(Authn.ContextKeys.CAPABILITIES).longValue() & caps) != 0) { != caps Line 79: profiles.add(0, profile); Line 80: } Line 81: } Line 82: } Line 92: // authentication to perform: Line 93: findNegotiatingProfiles(req); Line 94: if (profiles.isEmpty()) { Line 95: chain.doFilter(req, rsp); Line 96: return; please do not return at middle Line 97: } Line 98: Line 99: // Perform the authentication: Line 100: doFilter((HttpServletRequest) req, (HttpServletResponse) rsp, chain); Line 96: return; Line 97: } Line 98: Line 99: // Perform the authentication: Line 100: doFilter((HttpServletRequest) req, (HttpServletResponse) rsp, chain); very confusing you call the same name for something of ours. please rename to doAuthentication or something. Line 101: } Line 102: Line 103: private void doFilter(HttpServletRequest req, HttpServletResponse rsp, FilterChain chain) Line 104: throws IOException, ServletException { Line 123: while (!stack.isEmpty()) { Line 124: // Resume the negotiation with the profile at the top of the stack: Line 125: AuthenticationProfile profile = AuthenticationProfileRepository.getInstance().getProfile(stack.peek()); Line 126: if (profile == null) { Line 127: FiltersHelper.markUnauthorized(session); if not authorized then it is unauthorized... why do we need this flag? Line 128: } else { Line 129: ExtMap output = profile.getAuthn().invoke( Line 130: new ExtMap().mput( Line 131: Base.InvokeKeys.COMMAND, Line 149: case Authn.AuthResult.NEGOTIATION_UNAUTHORIZED: Line 150: moveToNextProfile(session, stack); Line 151: break; Line 152: Line 153: default: please log unexpected result Line 154: moveToNextProfile(session, stack); Line 155: } Line 156: } Line 157: } Line 155: } Line 156: } Line 157: } Line 158: if (session.getAttribute(FiltersHelper.Constants.AUTH_RECORD_KEY) == null) { Line 159: FiltersHelper.markUnauthorized(session); I am unsure why this is needed.... as you simply continue to next in change within AUTH_RECORD and without authenticated. Line 160: } Line 161: chain.doFilter(req, rsp); Line 162: } Line 163: http://gerrit.ovirt.org/#/c/28022/4/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 47: log.debug("", ex); Line 48: } Line 49: } finally { Line 50: FiltersHelper.closeContext(ctx); Line 51: chain.doFilter(request, response); I am unsure that in case of exception here we should move to next filter. if we have a problem such as database down or any we should just return the error, there is no sense to attempt in allowing access to application. Line 52: } Line 53: } Line 54: } Line 55: -- 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: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
