Yair Zaslavsky has posted comments on this change. Change subject: aaa: Intorduce filters ......................................................................
Patch Set 17: (13 comments) http://gerrit.ovirt.org/#/c/28022/17/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 38: @Override Line 39: public void init(FilterConfig filterConfig) throws ServletException { Line 40: try { Line 41: userNameFormat = UserNameFormat.valueOf(filterConfig.getInitParameter("user-name-format")); Line 42: } catch (Exception ex) { > please log error Done Line 43: userNameFormat = UserNameFormat.UPN; Line 44: } Line 45: Line 46: } Line 69: Line 70: private UserProfile translateUser(String translateFrom) { Line 71: int separator = -1; Line 72: UserProfile result = new UserProfile(); Line 73: if (userNameFormat == UserNameFormat.UPN && translateFrom.indexOf("\\") == -1) { > if you are using UPM notation you should not look for '\' Done Line 74: separator = translateFrom.lastIndexOf("@"); Line 75: result.userName = translateFrom.substring(0, separator); Line 76: result.profile = translateFrom.substring(separator+1); Line 77: } else if (userNameFormat == UserNameFormat.RESTAPI_SPECIFIC && translateFrom.indexOf("@") == -1) { Line 73: if (userNameFormat == UserNameFormat.UPN && translateFrom.indexOf("\\") == -1) { Line 74: separator = translateFrom.lastIndexOf("@"); Line 75: result.userName = translateFrom.substring(0, separator); Line 76: result.profile = translateFrom.substring(separator+1); Line 77: } else if (userNameFormat == UserNameFormat.RESTAPI_SPECIFIC && translateFrom.indexOf("@") == -1) { > hmmm... why are you looking for '@' in this case? i prefer your 2nd suggestion. Line 78: separator = translateFrom.indexOf("\\"); Line 79: result.profile = translateFrom.substring(0, separator); Line 80: result.userName = translateFrom.substring(separator+1); Line 81: } Line 84: Line 85: private void handleCredentials(ServletRequest request, String user, String password) { Line 86: UserProfile userProfile = translateUser(user); Line 87: user = userProfile.userName; Line 88: String profileName = userProfile.profile; > you can really use userProfile and not these temp variables... Done Line 89: AuthenticationProfile profile = AuthenticationProfileRepository.getInstance().getProfile(profileName); Line 90: if (profile == null) { Line 91: String msg = String.format("Error in obtaining profile %1$s", profileName); Line 92: log.error(msg); http://gerrit.ovirt.org/#/c/28022/17/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 static class Constants { Line 14: public static final String AUTHENTICATED_KEY = "authenticated"; Line 15: public final static String ENGINE_SESSION_ID_KEY = "engineSessionId"; Line 16: public final static String AUTH_RECORD_KEY = "auth_record"; Line 17: public final static String UNAUTHORIZED_KEY = "unauthorized"; > so why do we need this key? Done Line 18: public final static String SCHEMES_KEY = "schemes"; Line 19: public final static String PROFILE_KEY = "profile"; Line 20: } Line 21: Line 15: public final static String ENGINE_SESSION_ID_KEY = "engineSessionId"; Line 16: public final static String AUTH_RECORD_KEY = "auth_record"; Line 17: public final static String UNAUTHORIZED_KEY = "unauthorized"; Line 18: public final static String SCHEMES_KEY = "schemes"; Line 19: public final static String PROFILE_KEY = "profile"; > please separate clearly between request properties and session properties, done. Line 20: } Line 21: Line 22: public static BackendLocal getBackend(Context context) { Line 23: http://gerrit.ovirt.org/#/c/28022/17/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 73: * stacks of profiles later when processing requests. Line 74: */ Line 75: private void findNegotiatingProfiles(ServletRequest req) { Line 76: List<String> schemes = new ArrayList<String>(); Line 77: if (profiles == null) { > the profiles and the scheme both are static, no need to calculate it every please notice the double check pattern here, only one calculation. I don't mind moving this to the init method. Line 78: synchronized (this) { Line 79: if (profiles == null) { Line 80: schemes = new ArrayList<>(); Line 81: profiles = new ArrayList<AuthenticationProfile>(); Line 101: // If there are no authentication profiles supporting negotiation then we don't do anything, as there is no Line 102: // authentication to perform: Line 103: findNegotiatingProfiles(req); Line 104: if (profiles.isEmpty()) { Line 105: chain.doFilter(req, rsp); > you should do the doFilter anyway.... remove the doFilter from doAuth Done Line 106: } else { Line 107: // Perform the authentication: Line 108: doAuth((HttpServletRequest) req, (HttpServletResponse) rsp, chain); Line 109: } http://gerrit.ovirt.org/#/c/28022/17/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: VdcActionParametersBase params = new VdcActionParametersBase(); Line 51: params.setSessionId(engineSessionId); Line 52: FiltersHelper.getBackend(ctx).runAction(VdcActionType.LogoutBySession, params); Line 53: session.invalidate(); Line 54: FiltersHelper.closeContext(ctx); > -->finally Done Line 55: } catch (Exception ex) { Line 56: log.error(String.format("Error in logout. Message is: %1$s", ex.getMessage())); Line 57: if (log.isDebugEnabled()) { Line 58: log.debug("", ex); Line 69: Line 70: private boolean containsJessionId(HttpServletRequest req) { Line 71: boolean result = false; Line 72: for (Cookie cookie : req.getCookies()) { Line 73: if (cookie.equals("JESSIONID")) { > replace all JESSIONID->JSESSOINID Done Line 74: break; Line 75: } Line 76: } Line 77: return result; Line 73: if (cookie.equals("JESSIONID")) { Line 74: break; Line 75: } Line 76: } Line 77: return result; > you are not doing anything with result! already fixed (in later patch)... Line 78: } Line 79: Line 80: @Override Line 81: public void destroy() { http://gerrit.ovirt.org/#/c/28022/17/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 26: public void init(FilterConfig filterConfig) throws ServletException { Line 27: } Line 28: Line 29: @Override Line 30: public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, > why can't we put this logic within the other filter? I was asking this yesterday... i had a feeling something is wrong here.... Line 31: ServletException { Line 32: HttpSession httpSession = ((HttpServletRequest) request).getSession(false); Line 33: if (httpSession != null) { Line 34: String engineSession = (String) httpSession.getAttribute(FiltersHelper.Constants.ENGINE_SESSION_ID_KEY); Line 40: httpSession.setAttribute( Line 41: FiltersHelper.Constants.AUTHENTICATED_KEY, Line 42: FiltersHelper.getBackend(ctx).runPublicQuery(VdcQueryType.ValidateSession, parameters).getSucceeded() Line 43: ); Line 44: FiltersHelper.closeContext(ctx); > -> finally Done Line 45: } catch (Exception ex) { Line 46: log.error(String.format("An error has occurred while session validation. Message is %1$s", ex.getMessage())); Line 47: if (log.isDebugEnabled()) { Line 48: log.debug("", ex); -- 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: 17 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
