[Engine-patches] Change in ovirt-engine[master]: core : Add engine sso

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: core : Add engine sso
......................................................................


Patch Set 15:

(2 comments)

http://gerrit.ovirt.org/#/c/36119/15/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOContextListener.java
File 
backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOContextListener.java:

Line 14:     @Override
Line 15:     public void contextInitialized(ServletContextEvent event) {
Line 16:         ServletContext ctx = event.getServletContext();
Line 17:         SSOLocalConfig localConfig = new SSOLocalConfig();
Line 18:         SSOExtensionsManager ssoExtensionManager = new 
SSOExtensionsManager(localConfig);
no reason to have temporary ssoExtensionManager variable.
Line 19:         ctx.setAttribute(SSOUtils.EXTENSION_MANAGER, 
ssoExtensionManager);
Line 20:         ctx.setAttribute(SSOUtils.USER_GROUP_MANAGER, new 
SSOUserGroupManager());
Line 21:         ctx.setAttribute(SSOUtils.SSO_LOCAL_CONFIG, localConfig);
Line 22:     }


http://gerrit.ovirt.org/#/c/36119/15/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSORedirectServlet.java
File 
backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSORedirectServlet.java:

Line 39:                 throw new RuntimeException(ex);
Line 40:             }
Line 41:         } else {
Line 42:             throw new RuntimeException("No module name found in 
request.");
Line 43:         }
again, pattern is wrong.

 if (fail) {
     throw exception
 }
 continue as usual

also this function is used one time while the service is useless, there is no 
advantage is having it.

 protected void service(...) {
      // first do validation
     if (StringUtils.isNotEmpty(module)) {
         // I think this should be servlet exception
         throw new RuntimeException("No module name found in request.");
     }
     // then trivial block first
     if (!SSOUtils.isUserAuthenticated(session)) {
         request.getRequestDispatcher("/WEB-INF/login.jsp").forward(request, 
response);
     } else {
          SSOUtils.redirectToModule();
     }
 }

Not sure populating SQLException out of redirectModule is wise... you can 
redirect it into RuntimException as the SQL or specific cause is not important.
Line 44:     }


-- 
To view, visit http://gerrit.ovirt.org/36119
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4894fc12653027271b6abd4dd5313b10593703fa
Gerrit-PatchSet: 15
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