----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4994/#review7554 -----------------------------------------------------------
I am still trying to figure out how to test this patch. I requested a free certificate form startssl.com but still waiting for email from them. /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16720> Can you please add comment with explanation of this field? /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16721> Line too long. /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16731> Empty line. /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16733> It seems like we duplicate here logic from UserRegistrationServlet. Maybe we can extract the common logic to some abstract base class and inherit or to some util class? /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16722> Does dn stand for "domainName"? If so can we change the name to be more descriptive? /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16723> Same here. Can we make the ldapDN be more descriptive? Also, please avoid consecutive capital letters in variable names. /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16724> Please put comments on new line. Comments should start with capital letter and end with full stop. Also, can we ensure that our assumptions are met by using Preconditions.checkState()? /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16734> Why ascii? /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16725> Please remove commented line. /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16726> Add space before and after "+" /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16727> I think it is better to use Preconditions.checkState() here instead of assertion. We want this check be performed also in production code. /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16728> Line too long. /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16729> Do we really need this comment? /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16730> Can we make the exception message more descriptive? /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment16732> Can we reuse the existing user registration logic from UserRegistrationServlet or from RobotAgentUtil.createUser()? Maybe it would be better to create RegistrationUtil class and move the method there... - Yuri On 2012-05-03 18:04:29, Ali Lown wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4994/ > ----------------------------------------------------------- > > (Updated 2012-05-03 18:04:29) > > > Review request for wave, Michael MacFadden, Yuri Zelikov, and vjrj. > > > Summary > ------- > > Adds ability to login with X.509 client certificates instead of a username > and password. > Relies on the wave userid being the same as the username of the email for the > domain listed in the certificate. > > Patch adds 3 new config values: > ENABLE_CLIENTAUTH - fairly explanatory > CLIENTAUTH_CERT_DOMAIN - required if enabled. Allows the domain the > certificate was issued for to differ (e.g. subdomain) from the wave server > DISABLE_LOGINPAGE - allows password-based authentication to be disabled > forcing the use of client certificates only. > > Patch is a compilation between myself and Thomas Leonard > ([email protected]). > The patch is tidied and rebased version of the original patches from the > mailing list/github from February. > > Known issue: > _Sometimes_ it is has been observed that after a session has expired, the > login screen is presented without the user being automatically logged in. > Entering a username and hitting enter then uses the certificate and the user > is logged in. Reproducing this bug locally has been impossible. (Someone else > can try to narrow down the cause if they want :) ) > > > Diffs > ----- > > /README 1332795 > /server-config.xml 1332795 > /server.config.example 1332795 > /src/org/waveprotocol/box/server/CoreSettings.java 1332795 > /src/org/waveprotocol/box/server/gxp/AuthenticationPage.gxp 1332795 > /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java 1332795 > /src/org/waveprotocol/box/server/rpc/ServerRpcProvider.java 1332795 > > Diff: https://reviews.apache.org/r/4994/diff > > > Testing > ------- > > Compiled and run locally without issue. > Been deployed to my server and client certificates were issued for all users. > Has been operating fine since February. > > > Thanks, > > Ali > >
