Yair Zaslavsky has posted comments on this change. Change subject: aaa: Intorduce filters ......................................................................
Patch Set 4: (8 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 Done 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. Done 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: Done 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 Done Line 79: profiles.add(0, profile); Line 80: } Line 81: } Line 82: } 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 Done 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? i'll remove, i wanted that in the next filter, Login BLL will not be called, if "unauhtorized" is marked. 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 Done 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 Done Line 160: } Line 161: chain.doFilter(req, rsp); Line 162: } Line 163: -- 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 <[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
