Alon Bar-Lev has posted comments on this change. Change subject: aaa: Intorduce filters ......................................................................
Patch Set 22: (26 comments) http://gerrit.ovirt.org/#/c/28022/22/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 40: try { Line 41: userNameFormat = UserNameFormat.valueOf(filterConfig.getInitParameter("user-name-format")); Line 42: } catch (Exception ex) { Line 43: log.error(String.format("The value %1$s is not a valid UserNameFormat. setting UPN as default", filterConfig.getInitParameter("user-name-format"))); Line 44: userNameFormat = UserNameFormat.UPN; better to initialize the variable at class scope than setting it here Line 45: } Line 46: Line 47: } Line 48: Line 78: } else if (userNameFormat == UserNameFormat.RESTAPI_SPECIFIC) { Line 79: separator = translateFrom.indexOf("\\"); Line 80: result.profile = translateFrom.substring(0, separator); Line 81: result.userName = translateFrom.substring(separator+1); Line 82: } again, as you told me... the restapi supports both notations... how does it reflected here? Line 83: return result; Line 84: } Line 85: Line 86: private void handleCredentials(HttpServletRequest request, String user, String password) { Line 87: UserProfile userProfile = translateUser(user); Line 88: AuthenticationProfile profile = AuthenticationProfileRepository.getInstance().getProfile(userProfile.profile); Line 89: if (profile == null) { Line 90: String msg = String.format("Error in obtaining profile %1$s", userProfile.profile); Line 91: log.error(msg); do we really really really need this msg temp var? Line 92: } else { Line 93: ExtMap outputMap = profile.getAuthn().invoke(new ExtMap().mput( Line 94: Base.InvokeKeys.COMMAND, Line 95: Authn.InvokeCommands.AUTHENTICATE_CREDENTIALS Line 102: ) Line 103: ); Line 104: if (outputMap.<Integer> get(Base.InvokeKeys.RESULT) == Base.InvokeResult.SUCCESS && Line 105: outputMap.<Integer> get(Authn.InvokeKeys.RESULT) == Authn.AuthResult.SUCCESS) { Line 106: request.getSession(true); not needed Line 107: request.setAttribute(FiltersHelper.Constants.REQUEST_AUTH_RECORD_KEY, Line 108: outputMap.<ExtMap> get(Authn.InvokeKeys.AUTH_RECORD)); Line 109: request.setAttribute(FiltersHelper.Constants.REQUEST_PROFILE_KEY, userProfile.profile); Line 110: } else { http://gerrit.ovirt.org/#/c/28022/22/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 13: public final static String SESSION_ENGINE_SESSION_ID_KEY = "engineSessionId"; Line 14: public final static String REQUEST_AUTH_RECORD_KEY = "auth_record"; Line 15: public final static String REQUEST_SCHEMES_KEY = "schemes"; Line 16: public final static String REQUEST_PROFILE_KEY = "profile"; Line 17: public static final String SESSION_APPLICATION_MODE_KEY = "app_mode"; why this ^ in session? Line 18: public static final String REQUEST_USER_KEY = "user"; Line 19: public static final String REQUEST_ALREADY_LOGGED_OUT_KEY = "already_logged_out"; Line 20: } Line 21: Line 15: public final static String REQUEST_SCHEMES_KEY = "schemes"; Line 16: public final static String REQUEST_PROFILE_KEY = "profile"; Line 17: public static final String SESSION_APPLICATION_MODE_KEY = "app_mode"; Line 18: public static final String REQUEST_USER_KEY = "user"; Line 19: public static final String REQUEST_ALREADY_LOGGED_OUT_KEY = "already_logged_out"; can't these be removed? user is to be gotten from engine session or auth record, already logged out is detected if no engine session. Line 20: } Line 21: Line 22: public static BackendLocal getBackend(Context context) { Line 23: http://gerrit.ovirt.org/#/c/28022/22/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/ForceLoginFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/ForceLoginFilter.java: it is not force login, but enforce authenticated or anything similar. it does not enforce the login. Line 1: package org.ovirt.engine.core.aaa.filters; Line 2: Line 3: import java.io.IOException; Line 4: import java.util.Arrays; Line 49: for (String scheme: allSchemes) { Line 50: if (scheme.equals("Basic")) { Line 51: if (realmInfo != null) { Line 52: scheme += realmInfo; Line 53: } why can't we have the entire string as init parameter? Basic realm="xxxx" Line 54: } Line 55: res.setHeader("WWW-Authenticate", scheme); Line 56: } Line 57: res.sendError(HttpServletResponse.SC_UNAUTHORIZED); Line 55: res.setHeader("WWW-Authenticate", scheme); Line 56: } Line 57: res.sendError(HttpServletResponse.SC_UNAUTHORIZED); Line 58: } else { Line 59: chain.doFilter(request, response); please revert the condition, trivial short statement first and prefer positive first. if (isAuthent) { } else { } Line 60: } Line 61: Line 62: Line 63: } http://gerrit.ovirt.org/#/c/28022/22/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 41: Line 42: @Override Line 43: public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, Line 44: ServletException { Line 45: boolean exceptionOccurred = false; exceptionOccured -> doFilter? doFilter = false Line 46: try { Line 47: HttpServletRequest req = (HttpServletRequest) request; Line 48: if (!FiltersHelper.isAuthenticated(req)) { Line 49: LoginUserParameters params = null; Line 52: ExtMap authRecord = (ExtMap) request.getAttribute(FiltersHelper.Constants.REQUEST_AUTH_RECORD_KEY); Line 53: if (authRecord != null) { Line 54: String engineSessionId = UUID.randomUUID().toString(); Line 55: params = new LoginUserParameters(profileName, authRecord); Line 56: params.setSessionId(engineSessionId); any reason this is not in constructor as well? Line 57: params.setActionType(loginAsAdmin ? VdcActionType.LoginAdminUser : VdcActionType.LoginUser); Line 58: InitialContext context = new InitialContext(); Line 59: try { Line 60: VdcReturnValueBase returnValue = FiltersHelper.getBackend(context).login(params); Line 64: FiltersHelper.Constants.SESSION_ENGINE_SESSION_ID_KEY, Line 65: engineSessionId Line 66: ); Line 67: req.setAttribute(FiltersHelper.Constants.REQUEST_USER_KEY, Line 68: returnValue.getActionReturnValue()); why do we need this? Line 69: VdcQueryReturnValue result = Line 70: FiltersHelper.getBackend(context) Line 71: .runPublicQuery(VdcQueryType.GetConfigurationValue, Line 72: new GetConfigurationValueParameters(ConfigurationValues.ApplicationMode, Line 72: new GetConfigurationValueParameters(ConfigurationValues.ApplicationMode, Line 73: ConfigCommon.defaultConfigurationVersion)); Line 74: if (result.getSucceeded()) { Line 75: SetDataOnSessionParameters setDataParams = new SetDataOnSessionParameters( Line 76: FiltersHelper.Constants.SESSION_APPLICATION_MODE_KEY, hmm... this is different session, use different prefix Line 77: ApplicationMode.from((Integer) result.getReturnValue() Line 78: )); Line 79: setDataParams.setSessionId(engineSessionId); Line 80: FiltersHelper.getBackend(context).runAction(VdcActionType.SetDataOnSession, Line 73: ConfigCommon.defaultConfigurationVersion)); Line 74: if (result.getSucceeded()) { Line 75: SetDataOnSessionParameters setDataParams = new SetDataOnSessionParameters( Line 76: FiltersHelper.Constants.SESSION_APPLICATION_MODE_KEY, Line 77: ApplicationMode.from((Integer) result.getReturnValue() why can't the login do that and store it in session? what's so special that we need to address this configuration in aaa module? Line 78: )); Line 79: setDataParams.setSessionId(engineSessionId); Line 80: FiltersHelper.getBackend(context).runAction(VdcActionType.SetDataOnSession, Line 81: setDataParams); Line 75: SetDataOnSessionParameters setDataParams = new SetDataOnSessionParameters( Line 76: FiltersHelper.Constants.SESSION_APPLICATION_MODE_KEY, Line 77: ApplicationMode.from((Integer) result.getReturnValue() Line 78: )); Line 79: setDataParams.setSessionId(engineSessionId); why isn't this in constructor as well? Line 80: FiltersHelper.getBackend(context).runAction(VdcActionType.SetDataOnSession, Line 81: setDataParams); Line 82: } Line 83: Line 83: Line 84: } Line 85: } finally { Line 86: context.close(); Line 87: } doFilter=true Line 88: } Line 89: } Line 90: } catch (Exception ex) { Line 91: exceptionOccurred = true; http://gerrit.ovirt.org/#/c/28022/22/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 50: } catch (NumberFormatException ex) { Line 51: log.error("Session-TTL header was not passed. Not setting TTL value"); Line 52: } Line 53: } Line 54: if ((req.getHeader("Authorization") != null || !containsJsessionId(req))) { I do not understand why we need this block. Line 55: // No need to pass credentials again - if passed, login should be called Line 56: if (session != null) { Line 57: session.removeAttribute(FiltersHelper.Constants.SESSION_ENGINE_SESSION_ID_KEY); Line 58: } Line 59: } Line 60: } Line 61: chain.doFilter(request, response); Line 62: if (FiltersHelper.isAuthenticated(req)) { Line 63: session = req.getSession(false); if (session != null) as you get with false, but if already authenticated we must have session... so why do you get with false? Line 64: String engineSessionId = Line 65: (String) session.getAttribute(FiltersHelper.Constants.SESSION_ENGINE_SESSION_ID_KEY); Line 66: if (!persistentAuth Line 67: && (req.getAttribute(FiltersHelper.Constants.REQUEST_ALREADY_LOGGED_OUT_KEY) == null || Line 68: !Boolean.valueOf((Boolean) req.getAttribute(FiltersHelper.Constants.REQUEST_ALREADY_LOGGED_OUT_KEY)))) { Line 69: InitialContext ctx = new InitialContext(); Line 70: try { Line 71: VdcActionParametersBase params = new VdcActionParametersBase(); Line 72: params.setSessionId(engineSessionId); why not in constructor? Line 73: FiltersHelper.getBackend(ctx).runAction(VdcActionType.LogoutBySession, params); Line 74: session.invalidate(); Line 75: } finally { Line 76: ctx.close(); Line 70: try { Line 71: VdcActionParametersBase params = new VdcActionParametersBase(); Line 72: params.setSessionId(engineSessionId); Line 73: FiltersHelper.getBackend(ctx).runAction(VdcActionType.LogoutBySession, params); Line 74: session.invalidate(); just invalidate session and logout, remove the attribute of session id, no need for this already logout key as far as I understand. Line 75: } finally { Line 76: ctx.close(); Line 77: } Line 78: } Line 84: } Line 85: } Line 86: } Line 87: Line 88: private boolean containsJsessionId(HttpServletRequest req) { why can't it be req.getSession(false) != null? but I do not understand why we need this. Line 89: boolean result = false; Line 90: for (Cookie cookie : req.getCookies()) { Line 91: if (cookie.getName().equals("JSESSIONID")) { Line 92: result = true; http://gerrit.ovirt.org/#/c/28022/22/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 30: Line 31: @Override Line 32: public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, Line 33: ServletException { Line 34: boolean exceptionOccurred = false; same... doFilter = false Line 35: try { Line 36: HttpSession httpSession = ((HttpServletRequest) request).getSession(false); Line 37: if (httpSession != null) { Line 38: String engineSession = (String) httpSession.getAttribute(FiltersHelper.Constants.SESSION_ENGINE_SESSION_ID_KEY); Line 40: if (engineSession != null) { Line 41: ctx = new InitialContext(); Line 42: try { Line 43: VdcQueryParametersBase parameters = new VdcQueryParametersBase(); Line 44: parameters.setSessionId(engineSession); why not in constructor? Line 45: Line 46: VdcQueryReturnValue returnValue = FiltersHelper.getBackend(ctx) Line 47: .runPublicQuery(VdcQueryType.ValidateSession, parameters); Line 48: if (!returnValue.getSucceeded()) { Line 48: if (!returnValue.getSucceeded()) { Line 49: httpSession.removeAttribute(FiltersHelper.Constants.SESSION_ENGINE_SESSION_ID_KEY); Line 50: } else { Line 51: request.setAttribute(FiltersHelper.Constants.REQUEST_USER_KEY, Line 52: returnValue.getReturnValue()); we do not need that, we should be able to obtain it whenever we want from engine session Line 53: } Line 54: } finally { Line 55: ctx.close(); Line 56: } Line 53: } Line 54: } finally { Line 55: ctx.close(); Line 56: } Line 57: } doFilter = true Line 58: } Line 59: } catch (Exception ex) { Line 60: exceptionOccurred = true; Line 61: String msg = http://gerrit.ovirt.org/#/c/28022/22/backend/manager/modules/restapi/webapp/src/main/webapp/WEB-INF/web.xml File backend/manager/modules/restapi/webapp/src/main/webapp/WEB-INF/web.xml: Line 21: <url-pattern>/*</url-pattern> Line 22: </filter-mapping> Line 23: Line 24: <filter> Line 25: <filter-name>RestApiSessionMgmtFilter</filter-name> I start to think that better to move this as first filter, and set the engine session in the j2ee session if available, this way the session validation will invalidate it if expired and we do not have duplicates. Line 26: <filter-class>org.ovirt.engine.core.aaa.filters.RestApiSessionMgmtFilter</filter-class> Line 27: </filter> Line 28: <filter-mapping> Line 29: <filter-name>RestApiSessionMgmtFilter</filter-name> -- 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: 22 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@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: 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
