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: (15 comments) http://gerrit.ovirt.org/#/c/28022/47/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/BasicAuthenticationFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/BasicAuthenticationFilter.java: Line 107: return result; Line 108: } Line 109: Line 110: Line 111: private void handleCredentials(HttpServletRequest request, String user, String password) { This method is impossible to understand and review, full of casts and untyped method calls. Line 112: UserProfile userProfile = translateUser(user); Line 113: if (userProfile == null || userProfile.profile == null) { Line 114: log.error(String.format("Error in obtaining profile %1$s", userProfile.profile)); Line 115: } else { http://gerrit.ovirt.org/#/c/28022/47/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 51: authRecord, Line 52: loginAsAdmin ? VdcActionType.LoginAdminUser : VdcActionType.LoginUser) Line 53: ); Line 54: if (returnValue.getSucceeded()) { Line 55: HttpSession session = req.getSession(true); This creates a session even if the user didn't sent "Prefer: persistent-auth". Line 56: session.setAttribute( Line 57: SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY, Line 58: returnValue.getSessionId() Line 59: ); http://gerrit.ovirt.org/#/c/28022/47/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 35: /** Line 36: * The authentication profiles used to perform the authentication process. Line 37: */ Line 38: private volatile List<AuthenticationProfile> profiles; Line 39: private long caps = 0; Don't use bit sets, use EnumSet instead. Line 40: Line 41: Line 42: Line 43: /** Line 82: Line 83: for (AuthenticationProfile profile : AuthenticationProfileRepository.getInstance().getProfiles()) { Line 84: if (profile != null) { Line 85: ExtMap authnContext = profile.getAuthn().getContext(); Line 86: if ((authnContext.<Long> get(Authn.ContextKeys.CAPABILITIES).longValue() & caps) == caps) { Please don't use hidden casts. Line 87: profiles.add(0, profile); Line 88: schemes.addAll(authnContext.<List<String>>get(Authn.ContextKeys.HTTP_AUTHENTICATION_SCHEME, Collections.<String>emptyList())); Line 89: } Line 90: } Line 134: if (profile == null) { Line 135: continue; Line 136: } Line 137: Line 138: ExtMap output = profile.getAuthn().invoke( This is impossible to understand and review. Line 139: new ExtMap().mput( Line 140: Base.InvokeKeys.COMMAND, Line 141: Authn.InvokeCommands.AUTHENTICATE_NEGOTIATE Line 142: ).mput( 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 1: package org.ovirt.engine.core.aaa.filters; If this class is RESTAPI specific it should be in one of the RESTAPI packages/modules. Line 2: Line 3: import java.io.IOException; Line 4: import java.util.Collections; Line 5: 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/create it again here? 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 is enabled. 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 is enabled. 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 switching to an older and worse logging framework? 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 be here, but in some AAA package/module. 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/interface/common/jaxrs/src/test/java/org/ovirt/engine/api/common/security/auth/ChallengerTest.java File backend/manager/modules/restapi/interface/common/jaxrs/src/test/java/org/ovirt/engine/api/common/security/auth/ChallengerTest.java: Line 42 Line 43 Line 44 Line 45 Line 46 What replaces all these tests? 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 even harder migrating to Wildfly. 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: } Assuming that the session is available here makes it more difficult to migrate to Wildfly, please avoid it. 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"? -- 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
