Alon Bar-Lev has posted comments on this change. Change subject: aaa: Intorduce filters ......................................................................
Patch Set 27: (16 comments) http://gerrit.ovirt.org/#/c/28022/27/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 36: this.userName = user; Line 37: this.profile = profile; Line 38: } Line 39: Line 40: public UserProfile() { you do not need default constructor Line 41: Line 42: } Line 43: Line 44: } Line 84: result = translateUserRestApiSpecific(translateFrom); Line 85: } Line 86: if (result == null) { Line 87: result = new UserProfile(translateFrom, null); Line 88: } if you check for valid profile, there is no way we accept null profile. this is a failure. Line 89: return result; Line 90: } Line 91: Line 92: private UserProfile translateUserProfileUpn(String translateFrom) { Line 97: result = AuthenticationProfileRepository.getInstance().getProfile(profileName) != null ? new UserProfile(translateFrom.substring(0, separator), profileName) : null; Line 98: } Line 99: return result; Line 100: } Line 101: - Line 102: private UserProfile translateUserRestApiSpecific(String translateFrom) { Line 103: UserProfile result = null; Line 104: int separator = translateFrom.indexOf("\\"); Line 105: if (separator != -1) { Line 102: private UserProfile translateUserRestApiSpecific(String translateFrom) { Line 103: UserProfile result = null; Line 104: int separator = translateFrom.indexOf("\\"); Line 105: if (separator != -1) { Line 106: - Line 107: String profileName = translateFrom.substring(0, separator); Line 108: result = AuthenticationProfileRepository.getInstance().getProfile(profileName) != null ? new UserProfile(translateFrom.substring(separator+1), profileName) : null; Line 109: } Line 110: return result; Line 112: Line 113: Line 114: private void handleCredentials(HttpServletRequest request, String user, String password) { Line 115: UserProfile userProfile = translateUser(user); Line 116: AuthenticationProfile profile = AuthenticationProfileRepository.getInstance().getProfile(userProfile.profile); you can put the profile within the UserProfile class... you already had it no? and fail if no (return null from translateUser). Line 117: if (profile == null) { Line 118: log.error(String.format("Error in obtaining profile %1$s", userProfile.profile)); Line 119: } else { Line 120: ExtMap outputMap = profile.getAuthn().invoke(new ExtMap().mput( http://gerrit.ovirt.org/#/c/28022/27/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/EnforceAuthFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/EnforceAuthFilter.java: Line 21: private List<String> additionalSchemes; Line 22: Line 23: @Override Line 24: public void init(FilterConfig filterConfig) throws ServletException { Line 25: String schemes = filterConfig.getInitParameter("schemes"); I am thinking if not better to get init parameter of form: schemes.* instead of split... do getInitParameters() and enumerate extract what is prefix schemes.x sorted. not critical, but will allow comma within name... Line 26: if (schemes == null) { Line 27: schemes = ""; Line 28: } Line 29: additionalSchemes = Arrays.asList(schemes.split(",")); http://gerrit.ovirt.org/#/c/28022/27/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 HTTP_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 final static String REQUEST_PERSISTENT_AUTH = "presist_auth"; again... please add ovirt_aaa_ prefix to all values, so there will be no conflict with others. not sure I understand why we need the last one. Line 18: } Line 19: Line 20: public static BackendLocal getBackend(Context context) { Line 21: http://gerrit.ovirt.org/#/c/28022/27/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 42: String profileName = (String) request.getAttribute(FiltersHelper.Constants.REQUEST_PROFILE_KEY); Line 43: Line 44: ExtMap authRecord = (ExtMap) request.getAttribute(FiltersHelper.Constants.REQUEST_AUTH_RECORD_KEY); Line 45: if (authRecord != null) { Line 46: String engineSessionId = UUID.randomUUID().toString(); move this to within try? Line 47: InitialContext context = new InitialContext(); Line 48: try { Line 49: VdcReturnValueBase returnValue = FiltersHelper.getBackend(context).login(new Line 50: LoginUserParameters( Line 56: if (returnValue.getSucceeded()) { Line 57: HttpSession session = req.getSession(true); Line 58: session.setAttribute( Line 59: FiltersHelper.Constants.HTTP_SESSION_ENGINE_SESSION_ID_KEY, Line 60: engineSessionId I expect to write here returnValue.getSessionId() and get rid of me trying to allocate it above... Line 61: ); Line 62: } Line 63: } finally { Line 64: context.close(); http://gerrit.ovirt.org/#/c/28022/27/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 43: /** Line 44: * In order to support several alternative authentication extension we store their associated profiles in a stack inside the HTTP session, Line 45: * this is the key for that stack. Line 46: */ Line 47: private static final String STACK_ATTR = NegotiationFilter.class.getName() + ".stack"; this should go into the helper, no? Line 48: Line 49: Line 50: private static final String CAPABILITIES_PARAMETER = "capabilities"; Line 51: http://gerrit.ovirt.org/#/c/28022/27/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/RestApiSessionMgmtEpilogueFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/RestApiSessionMgmtEpilogueFilter.java: Line 17: import org.ovirt.engine.core.common.action.VdcActionParametersBase; Line 18: import org.ovirt.engine.core.common.action.VdcActionType; Line 19: Line 20: Line 21: public class RestApiSessionMgmtEpilogueFilter implements Filter { this is the session management.... no need for Epilogue :) Line 22: Line 23: private static Log log = LogFactory.getLog(RestApiSessionMgmtEpilogueFilter.class); Line 24: Line 25: private static final int MINIMAL_SESSION_TTL = 1; Line 36: Line 37: chain.doFilter(request, response); Line 38: HttpServletRequest req = (HttpServletRequest)request; Line 39: Line 40: if (request.getAttribute(FiltersHelper.Constants.REQUEST_PERSISTENT_AUTH) != null) { why do you need this? extract the prefer header here, do not assume anything. Line 41: HttpSession session = req.getSession(); Line 42: try { Line 43: int ttlValue = Integer.parseInt(req.getHeader("Session-TTL")) * SECONDS_IN_MINUTE; Line 44: if (ttlValue >= MINIMAL_SESSION_TTL) { Line 41: HttpSession session = req.getSession(); Line 42: try { Line 43: int ttlValue = Integer.parseInt(req.getHeader("Session-TTL")) * SECONDS_IN_MINUTE; Line 44: if (ttlValue >= MINIMAL_SESSION_TTL) { Line 45: session = req.getSession(true); you already have session, see above... why get it twice? Line 46: session.setMaxInactiveInterval(ttlValue); Line 47: } Line 48: } catch (NumberFormatException ex) { Line 49: log.error("Session-TTL header was not passed. Not setting TTL value"); http://gerrit.ovirt.org/#/c/28022/27/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/RestApiSessionMgmtPrologueFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/RestApiSessionMgmtPrologueFilter.java: Line 11: import javax.servlet.ServletResponse; Line 12: import javax.servlet.http.HttpServletRequest; Line 13: import javax.servlet.http.HttpSession; Line 14: Line 15: public class RestApiSessionMgmtPrologueFilter implements Filter { this is plain InvalidateSessionIfAuthorizationHeaderFilter filter... :) Line 16: Line 17: @Override Line 18: public void init(FilterConfig filterConfig) throws ServletException { Line 19: } Line 25: Enumeration<String> preferValues = req.getHeaders("Prefer"); Line 26: while (preferValues.hasMoreElements()) { Line 27: if ("persistent-auth".equals(preferValues.nextElement())) { Line 28: request.setAttribute(FiltersHelper.Constants.REQUEST_PERSISTENT_AUTH, true); Line 29: HttpSession session = req.getSession(false); the above belongs to the previous filter Line 30: if ((req.getHeader("Authorization") != null)) { Line 31: // No need to pass credentials again - if passed, login should be called Line 32: if (session != null) { Line 33: session.invalidate(); Line 32: if (session != null) { Line 33: session.invalidate(); Line 34: session = null; Line 35: } Line 36: } the above statement is the only statement that should be within this filter as far as I understand. Line 37: break; Line 38: } Line 39: } Line 40: chain.doFilter(request, response); -- 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: 27 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
