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

[Adam Young]

Looks Good.  ACK


On 08/23/2011 11:39 AM, Ade Lee wrote:
> 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