Alon Bar-Lev has posted comments on this change. Change subject: aaa: Modify webadmin and userportal to use enginesso for authentication ......................................................................
Patch Set 10: (8 comments) http://gerrit.ovirt.org/#/c/36619/10/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/servlet/SSOLoginServlet.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/servlet/SSOLoginServlet.java: Line 18: if (ssoUrl == null) { Line 19: throw new RuntimeException("No sso-url init parameter specified."); Line 20: } Line 21: Line 22: postLoginUrl = getServletConfig().getInitParameter("post-action-url"); postActionUrl? Line 23: if (postLoginUrl == null) { Line 24: throw new RuntimeException("No post-action-url init parameter specified for SSOLoginServlet."); Line 25: } Line 26: } Line 28: @Override Line 29: protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { Line 30: response.sendRedirect(String.format("%s&opaque=%s&post_action_url=%s", Line 31: ssoUrl, Line 32: response.encodeURL(request.getParameter("opaque")), I really do not want to sound bad... but... the opaque is for sso... as it does not care what the parameter we send means, but the parameter for this servlet should be something that is clear as we do treat it in special way... so here we should have return_url, application_url or anything valid. Line 33: response.encodeURL(postLoginUrl))); Line 34: } Line 35: http://gerrit.ovirt.org/#/c/36619/10/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/servlet/SSOLogoutServlet.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/servlet/SSOLogoutServlet.java: Line 18: if (ssoLogoutUrl == null) { Line 19: throw new RuntimeException("No sso-logout-url init parameter specified."); Line 20: } Line 21: Line 22: postLoginUrl = getServletConfig().getInitParameter("post-action-url"); we do not need this. Line 23: if (postLoginUrl == null) { Line 24: throw new RuntimeException("No post-action-url init parameter specified for SSOLoginServlet."); Line 25: } Line 26: } Line 28: @Override Line 29: protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { Line 30: response.sendRedirect(String.format("%s&opaque=%s&post_action_url=%s", Line 31: ssoLogoutUrl, Line 32: response.encodeURL(request.getParameter("opaque")), not sure I understand why we return from logout :) Line 33: response.encodeURL(postLoginUrl))); Line 34: } Line 35: http://gerrit.ovirt.org/#/c/36619/10/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/servlet/SSOPostLoginServlet.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/servlet/SSOPostLoginServlet.java: Line 68: } catch (Exception ex) { Line 69: log.error("Exception creating user session", ex.getMessage()); Line 70: throw new RuntimeException("Exception creating user session", ex); Line 71: } Line 72: response.sendRedirect(request.getParameter("opaque")); I still think better to have a single try/catch for the entire block, and have all logic within. Line 73: } Line 74: http://gerrit.ovirt.org/#/c/36619/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/AsyncTaskManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/AsyncTaskManager.java: Line 139: } Line 140: Line 141: @OnTimerMethodAnnotation("_timer_Elapsed") Line 142: public synchronized void _timer_Elapsed() { Line 143: if (thereAreTasksToPoll()) { ? Line 144: pollAndUpdateAsyncTasks(); Line 145: Line 146: if (thereAreTasksToPoll() && logChangedMap) { Line 147: log.info("Finished polling Tasks, will poll again in {} seconds.", http://gerrit.ovirt.org/#/c/36619/10/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/BaseApplicationInit.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/BaseApplicationInit.java: Line 139: } Line 140: if (moduleUrl.indexOf(".html") != -1) { //$NON-NLS-1$ Line 141: moduleUrl = moduleUrl.substring(0, moduleUrl.indexOf("/")); //$NON-NLS-1$ Line 142: } Line 143: return moduleUrl + "sso/logout?opaque=" + Window.Location.getHref(); //$NON-NLS-1$ probably the logout should not have return url at all. Line 144: } Line 145: Line 146: protected void performLogin(T loginModel) { Line 147: DbUser loggedUser = loginModel.getLoggedUser(); http://gerrit.ovirt.org/#/c/36619/10/frontend/webadmin/modules/userportal-gwtp/src/main/webapp/WEB-INF/web.xml File frontend/webadmin/modules/userportal-gwtp/src/main/webapp/WEB-INF/web.xml: Line 136: <param-name>sso-logout-url</param-name> Line 137: <param-value>/ovirt-engine/sso/logout?module=userportal</param-value> Line 138: </init-param> Line 139: <init-param> Line 140: <param-name>post-action-url</param-name> post login was ok... it is more meaningful and has specific logic. Line 141: <param-value>/ovirt-engine/userportal/sso/post-login?</param-value> Line 142: </init-param> Line 143: </servlet> Line 144: <servlet-mapping> -- To view, visit http://gerrit.ovirt.org/36619 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iff0aee9d0f5ee606ff7f397cab69017ca7d9df08 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@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
