Yair Zaslavsky has posted comments on this change. Change subject: Introduction of filters to unify AAA flows for UI and REST-API ......................................................................
Patch Set 47: (8 comments) http://gerrit.ovirt.org/#/c/28022/47/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 43: HttpSession session = req.getSession(); Line 44: try { Line 45: int ttlValue = Integer.parseInt(req.getHeader("Session-TTL")) * SECONDS_IN_MINUTE; Line 46: if (ttlValue >= MINIMAL_SESSION_TTL) { Line 47: session = req.getSession(true); > You already got/created the session in line 43, why do you need to get/crea Hi, you are right, I have another question though that just popped in my head - if I keep the session alive , and in all the requests I will send the TTL header, won't it exceed in each call the time the session will be kept alive? is this a desired behavior? if not, can you please suggest an approach? maybe keep a boolean value on the session indicating whether TTL was already set? Thanks! Line 48: session.setMaxInactiveInterval(ttlValue); Line 49: } Line 50: } catch (NumberFormatException ex) { Line 51: log.error("Session-TTL header was not passed. Not setting TTL value"); Line 65: ctx.close(); Line 66: } Line 67: } Line 68: } catch (Exception ex) { Line 69: log.error(String.format("Error in REST-API session management. Message is: %1$s", ex.getMessage())); > The exception stack trace should always go to the log, not only when debug Done Line 70: if (log.isDebugEnabled()) { Line 71: log.debug("", ex); Line 72: } Line 73: } http://gerrit.ovirt.org/#/c/28022/47/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 55: } Line 56: } Line 57: doFilter = true; Line 58: } catch (Exception ex) { Line 59: log.error(String.format("An error has occurred while session validation. Message is %1$s", ex.getMessage())); > The exception stack trace should always go to the log, not only when debug i have seen this in many other places in the code, but i'll fix. Line 60: if (log.isDebugEnabled()) { Line 61: log.debug("", ex); Line 62: } Line 63: ((HttpServletResponse) response).sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); http://gerrit.ovirt.org/#/c/28022/47/backend/manager/modules/aaa/src/main/modules/org/ovirt/engine/core/aaa/main/module.xml File backend/manager/modules/aaa/src/main/modules/org/ovirt/engine/core/aaa/main/module.xml: Line 13 Line 14 Line 15 Line 16 Line 17 > Slf4j is better than log4j and commons logging in many aspects. Why are you hi, in what sense? i don't mind to switch back, i saw a mix in the code of both commons logging and slf4j. http://gerrit.ovirt.org/#/c/28022/47/backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/security/auth/SessionUtils.java File backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/security/auth/SessionUtils.java: Line 14: /* Line 15: * This class contains useful session utils Line 16: */ Line 17: public class SessionUtils { Line 18: public final static String ENGINE_SESSION_ID_KEY = "ovirt_aaa_engineSessionId"; > If this constant is to be used only by the AAA subsystem then it shouldn't IMHO it is not used just in the aaa subsystem. In addition, i have just renamed the value, it existed before in the code. Line 19: public final static String PREFER_HEADER_FIELD = "Prefer"; Line 20: public final static String SESSION_TTL_HEADER_FIELD = "Session-TTL"; Line 21: public final static String PERSIST_FIELD_VALUE = "persistent-auth"; Line 22: public final static String JSESSIONID_HEADER = "JSESSIONID"; http://gerrit.ovirt.org/#/c/28022/47/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/preprocessors/CurrentPreProcessor.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/preprocessors/CurrentPreProcessor.java: Line 8: import org.jboss.resteasy.core.ResourceMethod; Line 9: import org.jboss.resteasy.core.ServerResponse; Line 10: import org.jboss.resteasy.spi.Failure; Line 11: import org.jboss.resteasy.spi.HttpRequest; Line 12: import org.jboss.resteasy.spi.interception.PreProcessInterceptor; > Please avoid introducing more usages of RESTEasy internal classes, it makes what do you mean? Do you mean the concept of CurrentPreProcessor or something else? Line 13: import org.ovirt.engine.api.common.invocation.Current; Line 14: import org.ovirt.engine.api.common.security.auth.SessionUtils; Line 15: import org.ovirt.engine.api.restapi.util.SessionHelper; Line 16: import org.ovirt.engine.core.common.config.ConfigCommon; http://gerrit.ovirt.org/#/c/28022/47/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendResource.java: Line 282: backend.logoff(sh.sessionize(new LogoutUserParameters(user.getId()))); Line 283: HttpSession session = SessionUtils.getCurrentSession(false); Line 284: if (session != null) { Line 285: session.invalidate(); Line 286: } > Avoid using the session in the RESTAPI code. REST applications aren't suppo ok, but any reason not to invalidate the session after you perform backend.logoff? Line 287: } Line 288: sh.clean(); Line 289: } Line 290: } http://gerrit.ovirt.org/#/c/28022/47/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 163 Line 164 Line 165 Line 166 Line 167 > What happens now with the special treatment for "async"? hi, correct me if i'm wrong but all of the code in the if block is in the case this is not async. in case of async, we run just sessionHelper.clean after the block, but this is also when in the case of not async. am i missing something? -- 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: 47 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
