Yair Zaslavsky has posted comments on this change. Change subject: aaa: Intorduce filters ......................................................................
Patch Set 27: (12 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 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. thi what about the case of "user" in user name format? (no domain, no profile). 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: > - Done 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: > - Done 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 Done 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: where do we need comma within the 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 co done. at the rest-api session mgmt filter i set it to be used by the last rest-api filter. 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? move what ?the new InitialContext? it is already wrapped by a try... i have done so in other places as well Line 47: InitialContext context = new InitialContext(); Line 48: try { Line 49: VdcReturnValueBase returnValue = FiltersHelper.getBackend(context).login(new Line 50: LoginUserParameters( 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? not sure... i use them only here. 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 :) please pay attention that this is the 2nd part... the original code of RestApiSessionMgmtFilter had this part at end. 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 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? Done 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 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 which previous filter? this is first in chain... 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 see my above comment, not sure 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
