Refactor FormsAuthenticationHandler and add tests

Refactor FormsAuthenticationHandler and add tests

[pjo, 2014-03-22 01:26:49]

Thank you.
Some initial feedback.

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
File
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:86:
log.log(Level.FINE, "Authentication Cookie refresh");
"About to refresh Authentication cookie"
or
"About to authenticate"

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:90:
String cookies = result.getCookie();
s/cookies/cookie/
?

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:112:
"Empty username / password. Using authentication mode as Windows");
What does "as Windows" mean?

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:112:
"Empty username / password. Using authentication mode as Windows");
s!/!or!

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:134:
interface AuthenticationHandler {
please make inner

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:139:
class AuthenticationResult {
please make inner

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:146:
this.cookie = cookie;
null is OK for all these?

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:170:
}
the above three comments don't give any additional info; delete them

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
File
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:1:
// Copyright 2012 Google Inc. All Rights Reserved.
2014

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:65:
"Possible SP2013 environment with windows authentication", ex);
"Check if we are running against SharePoint 2013 and are configured to use
Windows authentication. Using Windows authentication and SharePoint 2013 is not
supported"

Is that right?

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:78:
+ "authentication.", result.getErrorCode());
First sentence: "Forms authentication failed with error code {0}."

For 2nd sentence, I'm not 100% sure what we're trying to get across, is it:
"Check if we are running with multiple claims providers. Running with multiple
claims providers is not supported."

For 3rd sentence, I'm not 100% sure what we're trying to get across, is it:
"Check if we are running with windows authentication. Running with windows
authentication is not supported."

For the 3rd one, i think running with windows authentication is supported, but
I'm not sure what the message we're trying to get across here is.

https://codereview.appspot.com/79030043/diff/40001/test/com/google/enterprise...
File
test/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandlerTest.java
(right):

https://codereview.appspot.com/79030043/diff/40001/test/com/google/enterprise...
test/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandlerTest.java:1:
// Copyright 2012 Google Inc. All Rights Reserved.
2014

[Tanmay Vartak, 2014-03-24 17:04:32]

Thanks for review. I have added my response to some of your comments. I will
upload new patch for review.

Regards,
Tanmay

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
File
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:112:
"Empty username / password. Using authentication mode as Windows");
Windows means "Windows Integrated Authentication. This can be NTLM or Kerberos".

On 2014/03/22 01:26:50, pjo wrote:
> What does "as Windows" mean?

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:146:
this.cookie = cookie;
Yes null is fine. I will remove auto generated comments by IDE.
On 2014/03/22 01:26:50, pjo wrote:
> null is OK for all these?

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:170:
}
I will remove auto generated comments by netbeans.
On 2014/03/22 01:26:50, pjo wrote:
> the above three comments don't give any additional info; delete them

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
File
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:65:
"Possible SP2013 environment with windows authentication", ex);
No. This was an existing comment, just moved from FormAuthenticationHandler to
here.

On SharePoint 2013 environment, authentication mode returned is "Forms" even
though SharePoint is configured to use Integrated Windows Authentication.
Adaptor looks at Authentication mode and if it is Forms, then adaptor will try
to authenticate using available user name and password by calling
"Authenitcation.asmx" web service. In this case, SharePoint will throw
exception. So Adaptor decides to use "Integrated Windows Authentication" (or
just Windows Authentication is SharePoint and IIS terms.)

On 2014/03/22 01:26:50, pjo wrote:
> "Check if we are running against SharePoint 2013 and are configured to use
> Windows authentication. Using Windows authentication and SharePoint 2013 is
not
> supported"
> 
> Is that right?

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:78:
+ "authentication.", result.getErrorCode());
Again existing comment. But I agree we should fix first statement.

This is another scenario, where SharePoint is configured to support multiple
claims providers. In this case Authentication mode is forms. But it is possible
that Adaptor is configured to use "Integrated Windows Authentication". In that
scenario, Forms authentication will fail for provided user name and password.

While adding support for forms authentication, it was decided to always fallback
to "Integrated Windows Authentication" in case of login error.

On 2014/03/22 01:26:50, pjo wrote:
> First sentence: "Forms authentication failed with error code {0}."
> 
> For 2nd sentence, I'm not 100% sure what we're trying to get across, is it:
> "Check if we are running with multiple claims providers. Running with multiple
> claims providers is not supported."
> 
> For 3rd sentence, I'm not 100% sure what we're trying to get across, is it:
> "Check if we are running with windows authentication. Running with windows
> authentication is not supported."
> 
> For the 3rd one, i think running with windows authentication is supported, but
> I'm not sure what the message we're trying to get across here is.

[Tanmay Vartak, 2014-03-24 18:47:07]

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
File
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:134:
interface AuthenticationHandler {
On 2014/03/22 01:26:50, pjo wrote:
> please make inner 

Done.

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:134:
interface AuthenticationHandler {
On 2014/03/22 01:26:50, pjo wrote:
> please make inner 

Done.

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:139:
class AuthenticationResult {
On 2014/03/22 01:26:50, pjo wrote:
> please make inner

Done.

[pjo, 2014-03-24 22:46:48]

Thank you.

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
File
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/40001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:146:
this.cookie = cookie;
On 2014/03/24 17:04:32, Tanmay Vartak wrote:
> Yes null is fine. I will remove auto generated comments by IDE.
> On 2014/03/22 01:26:50, pjo wrote:
> > null is OK for all these?
> 

Done.

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
File
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:112:
"Empty username or password. Using authentication mode as Windows");
OK, can we put:
"Empty username or password. Using Windows Integrated Authentication."

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:145:
int cookieTimeOut, String errorCode) {
cookieTimeout cannot be null cause it is an int :)

There is a special meaning for cookieTimeout also,
right? -1 or any negative? If so, can there be a method
like `boolean hasCookieTimeout() {'

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
File
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:67:
"Possible SP2013 environment with windows authentication", ex);
Even if this comment existed before still please
let's make it clearer and direct toward actions
that can be taken.

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:79:
+ "authentication.", result.getErrorCode());
ibid

[Tanmay Vartak, 2014-03-24 23:48:34]

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
File
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:112:
"Empty username or password. Using authentication mode as Windows");
On 2014/03/24 22:46:49, pjo wrote:
> OK, can we put:
> "Empty username or password. Using Windows Integrated Authentication."

Done.

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:145:
int cookieTimeOut, String errorCode) {
On 2014/03/24 22:46:49, pjo wrote:
> cookieTimeout cannot be null cause it is an int :)
> 
> There is a special meaning for cookieTimeout also,
> right? -1 or any negative? If so, can there be a method
> like `boolean hasCookieTimeout() {'

Done.

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
File
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:67:
"Possible SP2013 environment with windows authentication", ex);
On 2014/03/24 22:46:49, pjo wrote:
> Even if this comment existed before still please
> let's make it clearer and direct toward actions
> that can be taken.

Done.

https://codereview.appspot.com/79030043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:79:
+ "authentication.", result.getErrorCode());
On 2014/03/24 22:46:49, pjo wrote:
> ibid

Done.

[pjo, 2014-03-25 21:46:52]

Minor items.  Thank you.

https://codereview.appspot.com/79030043/diff/100001/src/com/google/enterprise...
File
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:70:
//authenticationMode = AuthenticationMode.WINDOWS;
annotate? remove?

https://codereview.appspot.com/79030043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:79:
+ "Possible SharePoint environment with multiple claims provider. "
s/provider/providers/

https://codereview.appspot.com/79030043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:97:
if(cookies == null || cookies.isEmpty()) {
s/if/if /

https://codereview.appspot.com/79030043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:102:
if (result.getTimeoutSeconds() == null) {
you've seen this actually be null?  correct?

how do you feel about correcting negative values to DEFAULT also?

https://codereview.appspot.com/79030043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:122:
return authenticationMode == AuthenticationMode.FORMS;
why store it, it is not used anywhere else?

[Tanmay Vartak, 2014-03-25 23:03:39]

https://codereview.appspot.com/79030043/diff/100001/src/com/google/enterprise...
File
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:70:
//authenticationMode = AuthenticationMode.WINDOWS;
On 2014/03/25 21:46:52, pjo wrote:
> annotate? remove?

Done.

https://codereview.appspot.com/79030043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:79:
+ "Possible SharePoint environment with multiple claims provider. "
On 2014/03/25 21:46:52, pjo wrote:
> s/provider/providers/

Done.

https://codereview.appspot.com/79030043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:97:
if(cookies == null || cookies.isEmpty()) {
On 2014/03/25 21:46:52, pjo wrote:
> s/if/if /

Done.

https://codereview.appspot.com/79030043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:102:
if (result.getTimeoutSeconds() == null) {
On 2014/03/25 21:46:52, pjo wrote:
> you've seen this actually be null?  correct?
> 
> how do you feel about correcting negative values to DEFAULT also?

Done.

[pjo, 2014-03-25 23:10:56]

LGTM.  Thank you.

https://codereview.appspot.com/79030043/diff/120001/src/com/google/enterprise...
File
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java
(right):

https://codereview.appspot.com/79030043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:120:
// Save authentication mode value as class level variable to


s/as class level variable //

class level makes me think it is static.