Ravi Nori has posted comments on this change. Change subject: core : Engine IDP ......................................................................
Patch Set 1: (6 comments) http://gerrit.ovirt.org/#/c/34194/1/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/EnginePrincipal.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/EnginePrincipal.java: Line 8: private static final long serialVersionUID = 7701951188631723261L; Line 9: private final String name; Line 10: private String engineSessionId; Line 11: Line 12: public EnginePrincipal(String name) { > Can an object of EnginePrincipal exist with a sessionId? if not, can you ad EnginePrincipal is instantiated by AbstractServerLoginModule (jboss class) using an option passed in ovirt-engine.xml.in (security domain config). Once that is done I get the principal in EngineLoginModule and set the engineSessionId. Only way to change the constructor would be to override the method AbstractServerLoginModule.createIdentity Please let me know your thoughts Line 13: this.name = name; Line 14: } Line 15: Line 16: @Override http://gerrit.ovirt.org/#/c/34194/1/backend/manager/modules/idp/src/main/java/org/ovirt/engine/core/idp/EngineAttributeHandler.java File backend/manager/modules/idp/src/main/java/org/ovirt/engine/core/idp/EngineAttributeHandler.java: Line 32: if (getType() == HANDLER_TYPE.SP) Line 33: return; Line 34: Line 35: HTTPContext httpContext = (HTTPContext) request.getContext(); Line 36: HttpSession session = httpContext.getRequest().getSession(false); > Can a session here return null? Done Line 37: Line 38: Principal userPrincipal = (Principal) session.getAttribute(GeneralConstants.PRINCIPAL_ID); Line 39: Line 40: if (userPrincipal == null) http://gerrit.ovirt.org/#/c/34194/1/backend/manager/modules/idp/src/main/java/org/ovirt/engine/core/idp/EngineLoginModule.java File backend/manager/modules/idp/src/main/java/org/ovirt/engine/core/idp/EngineLoginModule.java: Line 202: String profileName = (String) authResult.get(FiltersHelper.Constants.REQUEST_PROFILE_KEY); Line 203: Line 204: ExtMap authRecord = (ExtMap) authResult.get(FiltersHelper.Constants.REQUEST_AUTH_RECORD_KEY); Line 205: if (authRecord != null) { Line 206: InitialContext context = new InitialContext(); > Double try because of new InitialContext? Removed double try. I found http://gerrit.ovirt.org/#/c/29617/2 (AutoClosableLock) but I dont think it works very well with AutoClosableContext as new InitialContext() can throw an exception. So I added a new method to BackendUtils, namely getBackendLocal and am using it here. Please let me know your thoughts Line 207: try { Line 208: VdcLoginReturnValueBase returnValue = (VdcLoginReturnValueBase) FiltersHelper.getBackend(context).login(new Line 209: LoginUserParameters( Line 210: profileName, http://gerrit.ovirt.org/#/c/34194/1/backend/manager/modules/idp/src/main/webapp/WEB-INF/login-error.jsp File backend/manager/modules/idp/src/main/webapp/WEB-INF/login-error.jsp: Line 28: Line 29: <div class="col-sm-12"> Line 30: <div style="width:250px;"> Line 31: <form id="login_form" name="login_form" method="post" Line 32: action="j_security_check" enctype="application/x-www-form-urlencoded"> > Lots of trailing whitespaces, please fix. Done Line 33: <center> Line 34: <p> Line 35: Welcome to the <b> Identity Provider</b> Line 36: </p> http://gerrit.ovirt.org/#/c/34194/1/backend/manager/modules/idp/src/main/webapp/WEB-INF/login.jsp File backend/manager/modules/idp/src/main/webapp/WEB-INF/login.jsp: Line 28: Line 29: <div class="col-sm-12"> Line 30: <div style="width:250px;"> Line 31: <form id="login_form" name="login_form" method="post" Line 32: action="j_security_check" enctype="application/x-www-form-urlencoded"> > same. Done Line 33: <center> Line 34: <p> Line 35: Welcome to the <b> Identity Provider</b> Line 36: </p> http://gerrit.ovirt.org/#/c/34194/1/backend/manager/modules/idpfilters/src/main/java/org/ovirt/engine/core/idp/filters/EngineIDPLoginFilter.java File backend/manager/modules/idpfilters/src/main/java/org/ovirt/engine/core/idp/filters/EngineIDPLoginFilter.java: Line 73: Line 74: private boolean runQuery(String sessionID) { Line 75: boolean returnValue = false; Line 76: Line 77: BackendInternal backend = (BackendInternal) EjbUtils.findBean(BeanType.BACKEND, BeanProxyType.LOCAL); > Didn't you add some FilterUtils.getBackend? This part of code is not needed, removing it Line 78: VdcQueryParametersBase params = new VdcQueryParametersBase(); Line 79: params.setSessionId(sessionID); Line 80: Line 81: VdcQueryReturnValue queryReturnValue = backend.runInternalQuery(VdcQueryType.ValidateSession, params); -- To view, visit http://gerrit.ovirt.org/34194 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib549b3b563081a37b1fae3f437a73a208dd86db5 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
