Yair Zaslavsky has posted comments on this change. Change subject: Introduction of filters to unify AAA flows for UI and REST-API ......................................................................
Patch Set 48: (7 comments) http://gerrit.ovirt.org/#/c/28022/48/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 27: } Line 28: } Line 29: Line 30: public static boolean isAuthenticated(HttpServletRequest request) { Line 31: return request.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY) != null; > this must look at both request and session. yep, thanks for that. Line 32: } Line 33: http://gerrit.ovirt.org/#/c/28022/48/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/LoginFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/LoginFilter.java: Line 62: } Line 63: } Line 64: doFilter = true; Line 65: } catch (Exception ex) { Line 66: log.error("Error in login to engine ", ex); > don't we want stack trace only in debug? juan has asked in previous rounds to stacktrace at error here. Line 67: ((HttpServletResponse) response).sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); Line 68: } Line 69: if (doFilter) { Line 70: chain.doFilter(request, response); http://gerrit.ovirt.org/#/c/28022/48/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/RestApiSessionMgmtFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/RestApiSessionMgmtFilter.java: Line 54: } catch (NumberFormatException ex) { Line 55: log.error("Session-TTL header was not passed. Not setting TTL value"); Line 56: } Line 57: } Line 58: chain.doFilter(request, response); > this should be outside of the try/catch scope done. actually, besides the NumberFormatException there is no real reason to wrap in additional try catch - the reason for tat was the naming exception (initial context), so actually IMHO the 2nd part after doFilter should be wrapped with exception. Line 59: if (!persistentAuth && FiltersHelper.isAuthenticated(req)) { Line 60: InitialContext ctx = new InitialContext(); Line 61: try { Line 62: FiltersHelper.getBackend(ctx).runAction( http://gerrit.ovirt.org/#/c/28022/48/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SessionMgmtFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SessionMgmtFilter.java: Line 23: public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, Line 24: ServletException { Line 25: HttpSession session = ((HttpServletRequest) request).getSession(false); Line 26: String engineSessionId = (String) request.getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY); Line 27: if (session != null && engineSessionId != null) { > why do you check if session != null? if you have engineSessionID you must c Done Line 28: session.setAttribute( Line 29: SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY, Line 30: engineSessionId Line 31: ); http://gerrit.ovirt.org/#/c/28022/48/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/security/auth/LoginValidator.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/security/auth/LoginValidator.java: Line 42: response.getMetadata().add(SessionUtils.JSESSIONID_HEADER, Line 43: httpSession.getId()); Line 44: } Line 45: } Line 46: sessionHelper.clean(); } > new line before } ? Done Line 47: Line 48: // Here to ease mocking it in the tester Line 49: protected HttpSession getCurrentSession(boolean create) { Line 50: return SessionUtils.getCurrentSession(create); http://gerrit.ovirt.org/#/c/28022/48/frontend/webadmin/modules/userportal-gwtp/src/main/webapp/WEB-INF/web.xml File frontend/webadmin/modules/userportal-gwtp/src/main/webapp/WEB-INF/web.xml: Line 70: </filter> Line 71: <filter-mapping> Line 72: <filter-name>SessionMgmtFilter</filter-name> Line 73: <url-pattern>/*</url-pattern> Line 74: </filter-mapping> > keep same indent as rest of this file? same, i would not mind to run some identiation tool at another patch, i would like to progress with this patch though. Line 75: Line 76: <filter-mapping> Line 77: <filter-name>LocaleFilter</filter-name> Line 78: <url-pattern>/*</url-pattern> http://gerrit.ovirt.org/#/c/28022/48/frontend/webadmin/modules/webadmin/src/main/webapp/WEB-INF/web.xml File frontend/webadmin/modules/webadmin/src/main/webapp/WEB-INF/web.xml: Line 73: </filter> Line 74: <filter-mapping> Line 75: <filter-name>SessionMgmtFilter</filter-name> Line 76: <url-pattern>/*</url-pattern> Line 77: </filter-mapping> > keep same indent as rest of this file? the file is not that indented to begin with. Line 78: <filter-mapping> Line 79: <filter-name>LocaleFilter</filter-name> Line 80: <url-pattern>/*</url-pattern> Line 81: <dispatcher>REQUEST</dispatcher> -- 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: 48 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@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
