Juan Hernandez has posted comments on this change. Change subject: Introduction of filters to unify AAA flows for UI and REST-API ......................................................................
Patch Set 47: (6 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); > Hi, you are right, I have another question though that just popped in my he I don't see any problem with setting the inactive interval with each call. Setting the inactive interval to the same value again doesn't have any effect, so I think that we don't need an additional boolean value for that. 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"); 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 > hi, in what sense? i don't mind to switch back, i saw a mix in the code of The first advantage is that slf4j is basically an interface, without implementation, so it gives the user the freedom to actually use the logging framework that better fits his needs. Second advantage is that slf4j supports methods with multiple parameters, and supports the {} syntax in the messages, replacing them with the parameters. For example, you are currently doing the following: log.error(String.format("whatever %s", parameter)) It is very well known that String.format is at least an order of magnitude slower than plain string concatenation, and the that "parameter" is converted to string regardless of the log level settings. So developers that care about performace tend to do this: if (log.isErrorEnabled()) { log.error("whatever " + parameter); } Slf4j supports the following instead: log.error("whatever {}", parameter); The main point here is that sf4j is smart enough to avoid the conversion of "parameter" to string when the log level settings are such that this message is disabled, so the "if (log.isErrorEnabled()) {" isn't needed in general. In addition the interpolation used by slf4j is faster than String.format. Finally, inside JBoss AS, the slf4j interface is implemented directly by the JBoss logging subsystem, which makes it faster and lighter. 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"; > IMHO it is not used just in the aaa subsystem. If it isn't used in the aaa subsystem then why rename it? 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; > what do you mean? Do you mean the concept of CurrentPreProcessor or somethi I mean any "import org.jboss.resteasy....". 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: } > ok, but any reason not to invalidate the session after you perform backend. Well, this code runs outside of the control of the web container, in one of the threads of the thread pool, so you can't assume that you have access to the HTTP session at all. That session may have been expired, or passivated by the web container, etc. It is only because we are lucky that this works in JBoss AS 7. 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 > hi, correct me if i'm wrong but all of the code in the if block is in the You are right. But I'm asking what happens to the treatment of "async" that goes inside the if: invalidating the session only if async=false, setting the JSESSIONID header only if async=false, etc. Where is that done now? -- 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
