[Pki-devel] patch for review : https://bugzilla.redhat.com/show_bug.cgi?id=712931

[Ade Lee]

On Mon, 2011-08-22 at 12:22 -0400, Adam Young wrote: 
> I'm strill tracking down why the install failed.  So far, I can just 
> determine that the CS DirSrv is not running when the ldap modify command 
> comes through, but I am not sure why.
> 
> 
> base/ca/shared/conf/proxy.conf
> 
> while it is a proxy to tomcat, from the apache perspective, tomcat is a 
> subordinate.  It might not be the clearest name.  Perhaps 
> proxy-tomcat.conf  or proxy-dogtag.conf,  although I thought 
> dogtag.conf  was a fairly descriptive name.
> 
As discussed in review, we will leave this name as is. 
> 
> All of the stanzas now appear identical with the exception of the 
> LocationMatch and one with NSSVerifyClient require.   Can the 3 
> NSSVerifyClient stanzas be collapsed   be collapsed into one?
> 
> 
They could - but I suggest we leave that for a later patch where we
clean up the URL structure.  For now, it makes more sense to me to keep
them separate because the separate stanzas accurately describe what is
on each of the admin, agent and ee ports. 

Another thing to consider.  In the non-IPA product, it is possible to
configure the console (which uses admin URLs) to use client auth.  If we
were to move some of the admin functions to http in future, I could see
a need to make them client-auth. (and therefore, there may be a need to
keep them separated).

> 
> ImportCAChainPanel.java:display
> 
> if this line fails:
> context.put("machineName", cs.getString("machineName"));
> 
> The processing continue on, but the next two lines:
>              context.put("https_port", 
> cs.getString("pkicreate.ee_secure_port"));
>              context.put("http_port", 
> cs.getString("pkicreate.unsecure_port"));
> 
> Will never get executed.  THat doesn't seem like the intention we want.  
> These two lines should probable be outside the catch block, and trigger 
> an exception that stops processing, as it seems like things are 
> significantly broken at this point.
> 

The correct thing to do here if any of these lines are not executed is
to log an error and display an error message to the user.  Which I have
now done.

Note that this does not affect IPA as this is in the display of an
installation panel. pkisilent does not use these values.


> AdminRequestFilter.java
> 
> Drop the bad_port boolean and move the conditional code up to where 
> bad_port is set.
> 
>      if (bad_port) {
>                      CMS.debug( filterName + ":  " + msg );
>                      CMS.debug( filterName + ": uri is " + uri);
>                      if ((param_active != null) 
> &&(param_active.equals("false"))) {
>                          CMS.debug("Filter is disabled .. continuing");
>                      } else {
>                          resp.sendError( 
> HttpServletResponse.SC_NOT_FOUND, msg );
>                          return;
>                      }
>                  }
> 

Actually, I disagree with this one.  There are two places where bad_port
is set - so not using the boolean causes unnecessary duplication of
code.  We might be able to do without the boolean, but not without
compromising the readability of the code.

> 
> AgentRequestFilter.java
> EEClientAuthRequestFilter.java
> EERequestFilter.java
> 
> Code is duplicate of the code in the AdminRequest\Filter.  refactor to a 
> common method.  Comment from above applies here as well re:  bad_port
> 
Yes -- I agree with this.  Actually, as all the code in each of the
filters is pretty much duplicated - I think we can refactor this to a
single filter class which can be instantiated with a couple of
parameters.

As we want to get this out for 6.1 and in as much testing as possible, I
suggest we open a bug to do this refactoring in the 8.2 timeframe.

> fixProxyPorts: Why catch the exception and continue.  Wouldn't it be 
> better to throw the exception?  Do we really want to continue if this 
> code does not succeed?
> 

Done 

> 
> Relative paths in html files:  seesm like we should specify a place for 
> the helper files to live, instead of assuming they will be in 
> ../cms-funcs.js  as that is a little on the fragile side. I am guessing 
> that it can no longer be just /ee/  since we might be coming from 
> /eeca/    Probably OK as is for this commit, but lets figure out what 
> we'd ideally want to do.    I'm guessing that the eeca approach is 
> really not what we want, and this is a work around for it, too.
> 
OK

> 
> I didn't give the perl code a thorough a look as the Java code, but it 
> looked OK.
> 
We walked through it in the code review.
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel