Yair Zaslavsky has posted comments on this change. Change subject: aaa: Intorduce filters ......................................................................
Patch Set 22: (17 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 what do you mean ? if it gets the variable from init param, why not 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 ok, so the init variable should be diffrerent - not what format is supported, but if both formats are supported or only UPN. but in this case, wont we have some ambiguity when parsing ? You said profile\u...@example.com is valid 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? Done 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 Done 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? to avoid subsequent call to the get application mode query on 2nd call , 3rd... calls. 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 re I'll check. 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: 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? ok 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 posit i'll do that, but don't we prefer to have the code of chain.doFilter as later as possible in the code? looks more natural to me. 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? sure, i'll change. 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? not in any ctor of our parameter classes, can be chaged. 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? required by rest-api. Line 69: VdcQueryReturnValue result = Line 70: FiltersHelper.getBackend(context) Line 71: .runPublicQuery(VdcQueryType.GetConfigurationValue, Line 72: new GetConfigurationValueParameters(ConfigurationValues.ApplicationMode, 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. to force us perform authentication. 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 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? answered before, i'll fix. Line 73: FiltersHelper.getBackend(ctx).runAction(VdcActionType.LogoutBySession, params); Line 74: session.invalidate(); Line 75: } finally { Line 76: ctx.close(); Line 84: } Line 85: } Line 86: } Line 87: Line 88: private boolean containsJsessionId(HttpServletRequest req) { > why can't it be req.getSession(false) != null? i'll fix. 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 ok 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 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 e i disagree. if not placing the user, then at least placing the UUID. but why having redundant db call? i always have a user due to invocation of the session validation query, why put its id on session and later on query again the db for it? Line 53: } Line 54: } finally { Line 55: ctx.close(); Line 56: } 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 engi please do not use the term j2ee session :) it's confusing :) there are http sessions, j2ee sessions (which we dont use) and engine sessions :) and in addition, what about all the rest-api specific code, why have it in first filter? 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
