----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4994/#review7855 -----------------------------------------------------------
Great Job! Thanks for doing this. Just few more comments. /README <https://reviews.apache.org/r/4994/#comment17185> Great! Thanks for adding the explanation. Can you maybe also add explanation on how to acquire the CA? /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment17190> I think it would be better to put the code from the "else" block into a separate method with proper name and javadoc. /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment17188> Can we extract these 3 lines into a separate method with descriptive name? we can put the comments into the method javadoc. /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment17187> WaveIdentifiers.isValidIdentifier(email) == true -> WaveIdentifiers.isValidIdentifier(email) No need to test the equality here. /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java <https://reviews.apache.org/r/4994/#comment17191> Same here, IMO the code in the "else" block should be refactored into its own method. /src/org/waveprotocol/box/server/util/RegistrationUtil.java <https://reviews.apache.org/r/4994/#comment17186> Please add the default constructor and make it private to prevent instanciation. - Yuri On 2012-05-05 17:13:23, Ali Lown wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4994/ > ----------------------------------------------------------- > > (Updated 2012-05-05 17:13:23) > > > 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 > ----- > > /test/org/waveprotocol/box/server/rpc/AuthenticationServletTest.java > 1332795 > /src/org/waveprotocol/box/server/util/RegistrationUtil.java PRE-CREATION > /src/org/waveprotocol/box/server/rpc/ServerRpcProvider.java 1332795 > /src/org/waveprotocol/box/server/rpc/UserRegistrationServlet.java 1332795 > /src/org/waveprotocol/box/server/gxp/AuthenticationPage.gxp 1332795 > /src/org/waveprotocol/box/server/rpc/AuthenticationServlet.java 1332795 > /README 1332795 > /server-config.xml 1332795 > /server.config.example 1332795 > /src/org/waveprotocol/box/server/CoreSettings.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 > >
