Issue 83730043: ADFS Support for SharePoint Adaptor

JohnL

Just peeking, not looking at logic. ;-) John L https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.java File src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.java (right): https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.java#newcode36 src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.java:36: ...

10 years ago (2014-04-02 21:47:14 UTC) #2

ondrejnovak

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java (right): https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode50 src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:50: private final String login = "/_layouts/Authenticate.aspx"; I don't think ...

10 years ago (2014-04-03 08:56:21 UTC) #3

Tanmay Vartak

Hi Ondra, Thanks for review. I have added my reply to some of your comments. ...

10 years ago (2014-04-10 17:26:07 UTC) #4

Hi Ondra,

Thanks for review. I have added my reply to some of your comments. Most of them
I have implemented. I will upload a patch soon once we decide on approach to
specify various properties.

Regards,
Tanmay

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java
(right):

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:50:
private final String login = "/_layouts/Authenticate.aspx";
/_layouts/Authenticate.aspx is part of regular claims processing.
http://msdn.microsoft.com/en-us/library/hh147177(v=office.14).aspx#SPO_RA_Cla...

Do you think we can have this as a default value? which can be overridden by
either use of system.property or via adaptopr config.properties?
On 2014/04/03 08:56:22, ondrejnovak wrote:
> I don't think we should hardcode this

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:103:
stsendpoint = sts + "/adfs/services/trust/2005/usernamemixed";
For SharePoint Online, we need to use little different implementation (need to
refer different node to extract token).

One question I have is, if we allow user to specify different endpoint here,
that might not work with reqXML we have here.
On 2014/04/03 08:56:22, ondrejnovak wrote:
> I don't think you should hardcode this, what if I want different STS URL (e.g.
> SharePoint online)

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:115:
requestProperties.put("Expect", "100-continue");
removed. These settings were required if I want to use CookieManager object to
grab cookie. But I changed implementation.
On 2014/04/03 08:56:22, ondrejnovak wrote:
> I guess you put it here because .NET does send this header by default, I would
> just drop it.

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:116:
requestProperties.put("Connection", "Keep-Alive");
Done.
On 2014/04/03 08:56:22, ondrejnovak wrote:
> I would drop this

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:117:
requestProperties.put("Content-Length", Integer.toString(saml.length()));
Done
On 2014/04/03 08:56:22, ondrejnovak wrote:
> I would move this to the SAMLAuthenticationHandler class

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:130:
String url = endpoint + "/_trust";
Again this was specific to ADFS implementation. But we can override it.
On 2014/04/03 08:56:22, ondrejnovak wrote:
> not fan of the hardcoding again

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:144:
requestProperties.put("User-Agent", "Mozilla/5.0 (compatible; "
Again due to CookieManager object. Removed.
On 2014/04/03 08:56:22, ondrejnovak wrote:
> why do you think we need this - in my tests, this is not necessary

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:153:
private String generateSAMLRequest() {
I have updated reqXML to use CDATA to escape any special characters in username
and password.
On 2014/04/03 08:56:22, ondrejnovak wrote:
> At least & should be handled to avoid XML issues

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:166: =
document.getElementsByTagName("t:RequestSecurityTokenResponse");
Done.
On 2014/04/03 08:56:22, ondrejnovak wrote:
> You are relying on the namespace alias to be always "t". You should use
> getElementsByTagNameNS()

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:170:
return null;
Done.
On 2014/04/03 08:56:22, ondrejnovak wrote:
> you should also throw an exception here and not return null

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:173:
String token = getOuterXml(responseToken);
getTextContent() gets the text value for XML node. So it is skipping all XML
tags. Also we need entire "RequestSecurityTokenResponse" XML including
"<t:RequestSecurityTokenResponse>" and "</t:RequestSecurityTokenResponse>"

Do we have anything in JAVA like in .NET XmlNode.OuterXml?
On 2014/04/03 08:56:22, ondrejnovak wrote:
> I don't understand what this is for? Why don't you just run
> nodex.item(0).getTextContent()

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.java
(right):

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.java:36:
// Wrapper class to provide basic SAML finctionality which can be used by
On 2014/04/02 21:47:14, John L wrote:
> Use javadoc, "finctionality" => "functionality", "diffrent" => "different",
add
> a period.

Done.

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.java:38:
public class SAMLAuthenticationHandler implements AuthenticationHandler {
On 2014/04/02 21:47:14, John L wrote:
> "SamlAuthenticationHandler"

Done.

https://codereview.appspot.com/83730043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.java:121:
connection.addRequestProperty(key, connectionProperties.get(key));
Done
On 2014/04/03 08:56:22, ondrejnovak wrote:
> I think content-length should be set here by default and not passed by the
user.

https://codereview.appspot.com/83730043/diff/1/test/com/google/enterprise/ada...
File
test/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandlerTest.java
(right):

https://codereview.appspot.com/83730043/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandlerTest.java:30:
public class SAMLAuthenticationHandlerTest {
On 2014/04/02 21:47:14, John L wrote:
> "SamlAuthenticationHandlerTest"

Done.

Sign in to reply to this message.

ondrejnovak

I think keeping these as default with the option to override it in adaptor-config.properties would ...

10 years ago (2014-04-11 11:25:24 UTC) #5

I think keeping these as default with the option to override it in
adaptor-config.properties would be best.

Thanks

Ondra


On Thu, Apr 10, 2014 at 7:26 PM, <tvartak@google.com> wrote:

> Hi Ondra,
>
> Thanks for review. I have added my reply to some of your comments. Most
> of them I have implemented. I will upload a patch soon once we decide on
> approach to specify various properties.
>
> Regards,
> Tanmay
>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java
> File
> src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java
> (right):
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode50
> src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:50:
> private final String login = "/_layouts/Authenticate.aspx";
> /_layouts/Authenticate.aspx is part of regular claims processing.
> http://msdn.microsoft.com/en-us/library/hh147177(v=office.
> 14).aspx#SPO_RA_ClaimsSequence
>
> Do you think we can have this as a default value? which can be
> overridden by either use of system.property or via adaptopr
> config.properties?
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> I don't think we should hardcode this
>>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode103
> src/com/google/enterprise/adaptor/sharepoint/
> AdfsHandshakeManager.java:103:
> stsendpoint = sts + "/adfs/services/trust/2005/usernamemixed";
> For SharePoint Online, we need to use little different implementation
> (need to refer different node to extract token).
>
> One question I have is, if we allow user to specify different endpoint
> here, that might not work with reqXML we have here.
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> I don't think you should hardcode this, what if I want different STS
>>
> URL (e.g.
>
>> SharePoint online)
>>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode115
> src/com/google/enterprise/adaptor/sharepoint/
> AdfsHandshakeManager.java:115:
> requestProperties.put("Expect", "100-continue");
> removed. These settings were required if I want to use CookieManager
> object to grab cookie. But I changed implementation.
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> I guess you put it here because .NET does send this header by default,
>>
> I would
>
>> just drop it.
>>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode116
> src/com/google/enterprise/adaptor/sharepoint/
> AdfsHandshakeManager.java:116:
> requestProperties.put("Connection", "Keep-Alive");
> Done.
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> I would drop this
>>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode117
> src/com/google/enterprise/adaptor/sharepoint/
> AdfsHandshakeManager.java:117:
> requestProperties.put("Content-Length",
> Integer.toString(saml.length()));
> Done
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> I would move this to the SAMLAuthenticationHandler class
>>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode130
> src/com/google/enterprise/adaptor/sharepoint/
> AdfsHandshakeManager.java:130:
> String url = endpoint + "/_trust";
> Again this was specific to ADFS implementation. But we can override it.
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> not fan of the hardcoding again
>>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode144
> src/com/google/enterprise/adaptor/sharepoint/
> AdfsHandshakeManager.java:144:
> requestProperties.put("User-Agent", "Mozilla/5.0 (compatible; "
> Again due to CookieManager object. Removed.
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> why do you think we need this - in my tests, this is not necessary
>>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode153
> src/com/google/enterprise/adaptor/sharepoint/
> AdfsHandshakeManager.java:153:
> private String generateSAMLRequest() {
> I have updated reqXML to use CDATA to escape any special characters in
> username and password.
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> At least & should be handled to avoid XML issues
>>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode166
> src/com/google/enterprise/adaptor/sharepoint/
> AdfsHandshakeManager.java:166:
> = document.getElementsByTagName("t:RequestSecurityTokenResponse");
> Done.
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> You are relying on the namespace alias to be always "t". You should
>>
> use
>
>> getElementsByTagNameNS()
>>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode170
> src/com/google/enterprise/adaptor/sharepoint/
> AdfsHandshakeManager.java:170:
> return null;
> Done.
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> you should also throw an exception here and not return null
>>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode173
> src/com/google/enterprise/adaptor/sharepoint/
> AdfsHandshakeManager.java:173:
> String token = getOuterXml(responseToken);
> getTextContent() gets the text value for XML node. So it is skipping all
> XML tags. Also we need entire "RequestSecurityTokenResponse" XML
> including "<t:RequestSecurityTokenResponse>" and
> "</t:RequestSecurityTokenResponse>"
>
> Do we have anything in JAVA like in .NET XmlNode.OuterXml?
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> I don't understand what this is for? Why don't you just run
>> nodex.item(0).getTextContent()
>>
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.java
> File
> src/com/google/enterprise/adaptor/sharepoint/
> SAMLAuthenticationHandler.java
> (right):
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
> java#newcode36
> src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
> java:36:
> // Wrapper class to provide basic SAML finctionality which can be used
> by
> On 2014/04/02 21:47:14, John L wrote:
>
>> Use javadoc, "finctionality" => "functionality", "diffrent" =>
>>
> "different", add
>
>> a period.
>>
>
> Done.
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
> java#newcode38
> src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
> java:38:
> public class SAMLAuthenticationHandler implements AuthenticationHandler
> {
> On 2014/04/02 21:47:14, John L wrote:
>
>> "SamlAuthenticationHandler"
>>
>
> Done.
>
> https://codereview.appspot.com/83730043/diff/1/src/com/
> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
> java#newcode121
> src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
> java:121:
> connection.addRequestProperty(key, connectionProperties.get(key));
> Done
> On 2014/04/03 08:56:22, ondrejnovak wrote:
>
>> I think content-length should be set here by default and not passed by
>>
> the user.
>
> https://codereview.appspot.com/83730043/diff/1/test/com/
> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandlerTest.java
> File
> test/com/google/enterprise/adaptor/sharepoint/
> SAMLAuthenticationHandlerTest.java
> (right):
>
> https://codereview.appspot.com/83730043/diff/1/test/com/
> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandlerTest.
> java#newcode30
> test/com/google/enterprise/adaptor/sharepoint/
> SAMLAuthenticationHandlerTest.java:30:
> public class SAMLAuthenticationHandlerTest {
> On 2014/04/02 21:47:14, John L wrote:
>
>> "SamlAuthenticationHandlerTest"
>>
>
> Done.
>
> https://codereview.appspot.com/83730043/
>

Sign in to reply to this message.

pjo

+1 - technology's compounding interest - On Fri, Apr 11, 2014 at 4:25 AM, Ondrej ...

10 years ago (2014-04-11 18:42:50 UTC) #6

+1


- technology's compounding interest
-


On Fri, Apr 11, 2014 at 4:25 AM, Ondrej Novak <ondrejnovak@google.com>wrote:

> I think keeping these as default with the option to override it in
> adaptor-config.properties would be best.
>
> Thanks
>
> Ondra
>
>
> On Thu, Apr 10, 2014 at 7:26 PM, <tvartak@google.com> wrote:
>
>> Hi Ondra,
>>
>> Thanks for review. I have added my reply to some of your comments. Most
>> of them I have implemented. I will upload a patch soon once we decide on
>> approach to specify various properties.
>>
>> Regards,
>> Tanmay
>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java
>> File
>> src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java
>> (right):
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode50
>> src/com/google/enterprise/adaptor/sharepoint/
>> AdfsHandshakeManager.java:50:
>> private final String login = "/_layouts/Authenticate.aspx";
>> /_layouts/Authenticate.aspx is part of regular claims processing.
>> http://msdn.microsoft.com/en-us/library/hh147177(v=office.
>> 14).aspx#SPO_RA_ClaimsSequence
>>
>> Do you think we can have this as a default value? which can be
>> overridden by either use of system.property or via adaptopr
>> config.properties?
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> I don't think we should hardcode this
>>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode103
>> src/com/google/enterprise/adaptor/sharepoint/
>> AdfsHandshakeManager.java:103:
>> stsendpoint = sts + "/adfs/services/trust/2005/usernamemixed";
>> For SharePoint Online, we need to use little different implementation
>> (need to refer different node to extract token).
>>
>> One question I have is, if we allow user to specify different endpoint
>> here, that might not work with reqXML we have here.
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> I don't think you should hardcode this, what if I want different STS
>>>
>> URL (e.g.
>>
>>> SharePoint online)
>>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode115
>> src/com/google/enterprise/adaptor/sharepoint/
>> AdfsHandshakeManager.java:115:
>> requestProperties.put("Expect", "100-continue");
>> removed. These settings were required if I want to use CookieManager
>> object to grab cookie. But I changed implementation.
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> I guess you put it here because .NET does send this header by default,
>>>
>> I would
>>
>>> just drop it.
>>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode116
>> src/com/google/enterprise/adaptor/sharepoint/
>> AdfsHandshakeManager.java:116:
>> requestProperties.put("Connection", "Keep-Alive");
>> Done.
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> I would drop this
>>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode117
>> src/com/google/enterprise/adaptor/sharepoint/
>> AdfsHandshakeManager.java:117:
>> requestProperties.put("Content-Length",
>> Integer.toString(saml.length()));
>> Done
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> I would move this to the SAMLAuthenticationHandler class
>>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode130
>> src/com/google/enterprise/adaptor/sharepoint/
>> AdfsHandshakeManager.java:130:
>> String url = endpoint + "/_trust";
>> Again this was specific to ADFS implementation. But we can override it.
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> not fan of the hardcoding again
>>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode144
>> src/com/google/enterprise/adaptor/sharepoint/
>> AdfsHandshakeManager.java:144:
>> requestProperties.put("User-Agent", "Mozilla/5.0 (compatible; "
>> Again due to CookieManager object. Removed.
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> why do you think we need this - in my tests, this is not necessary
>>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode153
>> src/com/google/enterprise/adaptor/sharepoint/
>> AdfsHandshakeManager.java:153:
>> private String generateSAMLRequest() {
>> I have updated reqXML to use CDATA to escape any special characters in
>> username and password.
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> At least & should be handled to avoid XML issues
>>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode166
>> src/com/google/enterprise/adaptor/sharepoint/
>> AdfsHandshakeManager.java:166:
>> = document.getElementsByTagName("t:RequestSecurityTokenResponse");
>> Done.
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> You are relying on the namespace alias to be always "t". You should
>>>
>> use
>>
>>> getElementsByTagNameNS()
>>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode170
>> src/com/google/enterprise/adaptor/sharepoint/
>> AdfsHandshakeManager.java:170:
>> return null;
>> Done.
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> you should also throw an exception here and not return null
>>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode173
>> src/com/google/enterprise/adaptor/sharepoint/
>> AdfsHandshakeManager.java:173:
>> String token = getOuterXml(responseToken);
>> getTextContent() gets the text value for XML node. So it is skipping all
>> XML tags. Also we need entire "RequestSecurityTokenResponse" XML
>> including "<t:RequestSecurityTokenResponse>" and
>> "</t:RequestSecurityTokenResponse>"
>>
>> Do we have anything in JAVA like in .NET XmlNode.OuterXml?
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> I don't understand what this is for? Why don't you just run
>>> nodex.item(0).getTextContent()
>>>
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.java
>> File
>> src/com/google/enterprise/adaptor/sharepoint/
>> SAMLAuthenticationHandler.java
>> (right):
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>> java#newcode36
>> src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>> java:36:
>> // Wrapper class to provide basic SAML finctionality which can be used
>> by
>> On 2014/04/02 21:47:14, John L wrote:
>>
>>> Use javadoc, "finctionality" => "functionality", "diffrent" =>
>>>
>> "different", add
>>
>>> a period.
>>>
>>
>> Done.
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>> java#newcode38
>> src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>> java:38:
>> public class SAMLAuthenticationHandler implements AuthenticationHandler
>> {
>> On 2014/04/02 21:47:14, John L wrote:
>>
>>> "SamlAuthenticationHandler"
>>>
>>
>> Done.
>>
>> https://codereview.appspot.com/83730043/diff/1/src/com/
>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>> java#newcode121
>> src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>> java:121:
>> connection.addRequestProperty(key, connectionProperties.get(key));
>> Done
>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>
>>> I think content-length should be set here by default and not passed by
>>>
>> the user.
>>
>> https://codereview.appspot.com/83730043/diff/1/test/com/
>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandlerTest.java
>> File
>> test/com/google/enterprise/adaptor/sharepoint/
>> SAMLAuthenticationHandlerTest.java
>> (right):
>>
>> https://codereview.appspot.com/83730043/diff/1/test/com/
>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandlerTest.
>> java#newcode30
>> test/com/google/enterprise/adaptor/sharepoint/
>> SAMLAuthenticationHandlerTest.java:30:
>> public class SAMLAuthenticationHandlerTest {
>> On 2014/04/02 21:47:14, John L wrote:
>>
>>> "SamlAuthenticationHandlerTest"
>>>
>>
>> Done.
>>
>> https://codereview.appspot.com/83730043/
>>
>
>

Sign in to reply to this message.

Tanmay Vartak

Yes I have implemented same. New patch will be out by EOD On Fri, Apr ...

10 years ago (2014-04-11 18:48:52 UTC) #7

Yes I have implemented same. New patch will be out by EOD


On Fri, Apr 11, 2014 at 11:42 AM, PJ <pjo@google.com> wrote:

> +1
>
>
> - technology's compounding interest
> -
>
>
> On Fri, Apr 11, 2014 at 4:25 AM, Ondrej Novak <ondrejnovak@google.com>wrote:
>
>> I think keeping these as default with the option to override it in
>> adaptor-config.properties would be best.
>>
>> Thanks
>>
>> Ondra
>>
>>
>> On Thu, Apr 10, 2014 at 7:26 PM, <tvartak@google.com> wrote:
>>
>>> Hi Ondra,
>>>
>>> Thanks for review. I have added my reply to some of your comments. Most
>>> of them I have implemented. I will upload a patch soon once we decide on
>>> approach to specify various properties.
>>>
>>> Regards,
>>> Tanmay
>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java
>>> File
>>> src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java
>>> (right):
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode50
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> AdfsHandshakeManager.java:50:
>>> private final String login = "/_layouts/Authenticate.aspx";
>>> /_layouts/Authenticate.aspx is part of regular claims processing.
>>> http://msdn.microsoft.com/en-us/library/hh147177(v=office.
>>> 14).aspx#SPO_RA_ClaimsSequence
>>>
>>> Do you think we can have this as a default value? which can be
>>> overridden by either use of system.property or via adaptopr
>>> config.properties?
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> I don't think we should hardcode this
>>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#
>>> newcode103
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> AdfsHandshakeManager.java:103:
>>> stsendpoint = sts + "/adfs/services/trust/2005/usernamemixed";
>>> For SharePoint Online, we need to use little different implementation
>>> (need to refer different node to extract token).
>>>
>>> One question I have is, if we allow user to specify different endpoint
>>> here, that might not work with reqXML we have here.
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> I don't think you should hardcode this, what if I want different STS
>>>>
>>> URL (e.g.
>>>
>>>> SharePoint online)
>>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#
>>> newcode115
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> AdfsHandshakeManager.java:115:
>>> requestProperties.put("Expect", "100-continue");
>>> removed. These settings were required if I want to use CookieManager
>>> object to grab cookie. But I changed implementation.
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> I guess you put it here because .NET does send this header by default,
>>>>
>>> I would
>>>
>>>> just drop it.
>>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#
>>> newcode116
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> AdfsHandshakeManager.java:116:
>>> requestProperties.put("Connection", "Keep-Alive");
>>> Done.
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> I would drop this
>>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#
>>> newcode117
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> AdfsHandshakeManager.java:117:
>>> requestProperties.put("Content-Length",
>>> Integer.toString(saml.length()));
>>> Done
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> I would move this to the SAMLAuthenticationHandler class
>>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#
>>> newcode130
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> AdfsHandshakeManager.java:130:
>>> String url = endpoint + "/_trust";
>>> Again this was specific to ADFS implementation. But we can override it.
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> not fan of the hardcoding again
>>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#
>>> newcode144
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> AdfsHandshakeManager.java:144:
>>> requestProperties.put("User-Agent", "Mozilla/5.0 (compatible; "
>>> Again due to CookieManager object. Removed.
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> why do you think we need this - in my tests, this is not necessary
>>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#
>>> newcode153
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> AdfsHandshakeManager.java:153:
>>> private String generateSAMLRequest() {
>>> I have updated reqXML to use CDATA to escape any special characters in
>>> username and password.
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> At least & should be handled to avoid XML issues
>>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#
>>> newcode166
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> AdfsHandshakeManager.java:166:
>>> = document.getElementsByTagName("t:RequestSecurityTokenResponse");
>>> Done.
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> You are relying on the namespace alias to be always "t". You should
>>>>
>>> use
>>>
>>>> getElementsByTagNameNS()
>>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#
>>> newcode170
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> AdfsHandshakeManager.java:170:
>>> return null;
>>> Done.
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> you should also throw an exception here and not return null
>>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#
>>> newcode173
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> AdfsHandshakeManager.java:173:
>>> String token = getOuterXml(responseToken);
>>> getTextContent() gets the text value for XML node. So it is skipping all
>>> XML tags. Also we need entire "RequestSecurityTokenResponse" XML
>>> including "<t:RequestSecurityTokenResponse>" and
>>> "</t:RequestSecurityTokenResponse>"
>>>
>>> Do we have anything in JAVA like in .NET XmlNode.OuterXml?
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> I don't understand what this is for? Why don't you just run
>>>> nodex.item(0).getTextContent()
>>>>
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.java
>>> File
>>> src/com/google/enterprise/adaptor/sharepoint/
>>> SAMLAuthenticationHandler.java
>>> (right):
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>>> java#newcode36
>>> src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>>> java:36:
>>> // Wrapper class to provide basic SAML finctionality which can be used
>>> by
>>> On 2014/04/02 21:47:14, John L wrote:
>>>
>>>> Use javadoc, "finctionality" => "functionality", "diffrent" =>
>>>>
>>> "different", add
>>>
>>>> a period.
>>>>
>>>
>>> Done.
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>>> java#newcode38
>>> src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>>> java:38:
>>> public class SAMLAuthenticationHandler implements AuthenticationHandler
>>> {
>>> On 2014/04/02 21:47:14, John L wrote:
>>>
>>>> "SamlAuthenticationHandler"
>>>>
>>>
>>> Done.
>>>
>>> https://codereview.appspot.com/83730043/diff/1/src/com/
>>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>>> java#newcode121
>>> src/com/google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandler.
>>> java:121:
>>> connection.addRequestProperty(key, connectionProperties.get(key));
>>> Done
>>> On 2014/04/03 08:56:22, ondrejnovak wrote:
>>>
>>>> I think content-length should be set here by default and not passed by
>>>>
>>> the user.
>>>
>>> https://codereview.appspot.com/83730043/diff/1/test/com/
>>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandlerTest.java
>>> File
>>> test/com/google/enterprise/adaptor/sharepoint/
>>> SAMLAuthenticationHandlerTest.java
>>> (right):
>>>
>>> https://codereview.appspot.com/83730043/diff/1/test/com/
>>> google/enterprise/adaptor/sharepoint/SAMLAuthenticationHandlerTest.
>>> java#newcode30
>>> test/com/google/enterprise/adaptor/sharepoint/
>>> SAMLAuthenticationHandlerTest.java:30:
>>> public class SAMLAuthenticationHandlerTest {
>>> On 2014/04/02 21:47:14, John L wrote:
>>>
>>>> "SamlAuthenticationHandlerTest"
>>>>
>>>
>>> Done.
>>>
>>> https://codereview.appspot.com/83730043/
>>>
>>
>>
>

Sign in to reply to this message.

pjo

Thank you. First quick pass for me. https://codereview.appspot.com/83730043/diff/20001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java (right): https://codereview.appspot.com/83730043/diff/20001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode49 src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:49: public class ...

10 years ago (2014-04-12 00:45:34 UTC) #9

Tanmay Vartak

Implemented most of code review comments. https://codereview.appspot.com/83730043/diff/20001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java (right): https://codereview.appspot.com/83730043/diff/20001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode49 src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:49: public class AdfsHandshakeManager ...

10 years ago (2014-04-14 21:09:34 UTC) #11

Tanmay Vartak

Hi PJ / Ondra, I want to introduce AuthenticationFactory object get appropriate authentication client, so ...

10 years ago (2014-04-15 22:35:18 UTC) #12

pjo

Thank you. https://codereview.appspot.com/83730043/diff/40001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java (right): https://codereview.appspot.com/83730043/diff/40001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode117 src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:117: public Builder (String sharePointUrl, String username, s/r ...

10 years ago (2014-04-16 20:13:35 UTC) #13

Tanmay Vartak

essage=With code review comments and Authentication handler factory

10 years ago (2014-04-18 20:51:24 UTC) #14

Tanmay Vartak

On 2014/04/18 20:51:24, Tanmay Vartak wrote: > essage=With code review comments and Authentication handler factory ...

10 years ago (2014-04-18 20:52:12 UTC) #15

Tanmay Vartak

With code review comments and Authentication handler factory

10 years ago (2014-04-18 20:52:49 UTC) #16

Tanmay Vartak

Implemented most of the code review comments https://codereview.appspot.com/83730043/diff/40001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java (right): https://codereview.appspot.com/83730043/diff/40001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode117 src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:117: public Builder ...

10 years ago (2014-04-18 20:54:01 UTC) #17

mifern

This CL seems to add support for Live authentication and ADFS authentication. Can you also ...

10 years ago (2014-04-21 18:12:20 UTC) #20

Tanmay Vartak

Hi Miguel, Thanks for your code review. For your reference ADFS flow. (also applicable for ...

10 years ago (2014-04-21 21:05:32 UTC) #21

Hi Miguel,

Thanks for your code review.
For your reference ADFS flow. (also applicable for Live authentication with
minor variations).
1. ADFS is a SAML implentation by Microsoft. SharePoint can be configured to
trust ADFS to authenticate a User and generating associated claims for User.
2. When User tries to access SharePoint secured with ADFS, user gets redirected
to ADFS endpoint (STSENDPOINT).
3. ADFS will validate user credentials and if credentials are valid, it will
return SAML authentication token which can be used to establish user identity by
SharePoint.
4. For Adaptor, AdfsHandshakeManager will request SAML token from ADFS using
HTTP post to configured STSENDPOINT.
5. In response, ADFS will return SAML Token. AdfsHandshakeManager will extract
this token from response.
5. AdfsHandshakeManager will do another HTTP Post to trust location using
extarcted token.
6. In response, SharePoint will include authentication cookies.
7. Authentication cookies will be used to make subsequent requests.
8. FormsAuthenticationHandler provides base framework to refresh authentication
cookies on regular interval.
9. Process flow for Live authentication is almost same with variation in "token
value"  and "HTTP Post" for obtaining cookie.

https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java
(right):

https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:197:
return String.format(reqXML, escapeXml(stsendpoint),
We can call this wrapXml or wrapCdata is thats sounds OK.

With CDATA we don't need to worry about any special chars such as & or <. Only
thing that we need to escape is ']]>'.

On 2014/04/21 18:12:20, mifern wrote:
> escapeXml on escapes cdata. Can username and password contain "<" or "&" that
> require cdata escaping?

https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java
(right):

https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:489: String
stsendpoint = config.getValue("sts.endpoint");

Initially when I started with User profile adaptor, myself and Eric discussed
about having common base class but we decided to keep them separate for
simplicity with side effect of duplicating code.

We can still revisit that decision but I want to avoid it in this CL.

On 2014/04/21 18:12:20, mifern wrote:
> Same comments as SharePointUserProfileAdaptor.java.
> 
> Also, there is a log of code duplication between here and
> SharePointUserProfileAdaptor.java. It seems like using a base class would be
> prudent.

Sign in to reply to this message.

Tanmay Vartak

On 2014/04/21 21:05:32, Tanmay Vartak wrote: > Hi Miguel, > > Thanks for your code ...

10 years ago (2014-04-21 21:11:43 UTC) #22

On 2014/04/21 21:05:32, Tanmay Vartak wrote:
> Hi Miguel,
> 
> Thanks for your code review.
> For your reference ADFS flow. (also applicable for Live authentication with
> minor variations).
> 1. ADFS is a SAML implentation by Microsoft. SharePoint can be configured to
> trust ADFS to authenticate a User and generating associated claims for User.
> 2. When User tries to access SharePoint secured with ADFS, user gets
redirected
> to ADFS endpoint (STSENDPOINT).
> 3. ADFS will validate user credentials and if credentials are valid, it will
> return SAML authentication token which can be used to establish user identity
by
> SharePoint.
> 4. For Adaptor, AdfsHandshakeManager will request SAML token from ADFS using
> HTTP post to configured STSENDPOINT.
> 5. In response, ADFS will return SAML Token. AdfsHandshakeManager will extract
> this token from response.
> 5. AdfsHandshakeManager will do another HTTP Post to trust location using
> extarcted token.
> 6. In response, SharePoint will include authentication cookies.
> 7. Authentication cookies will be used to make subsequent requests.
> 8. FormsAuthenticationHandler provides base framework to refresh
authentication
> cookies on regular interval.
> 9. Process flow for Live authentication is almost same with variation in
"token
> value"  and "HTTP Post" for obtaining cookie.
> 
>
https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/...
> File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java
> (right):
> 
>
https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/...
> src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:197:
> return String.format(reqXML, escapeXml(stsendpoint),
> We can call this wrapXml or wrapCdata is thats sounds OK.
> 
> With CDATA we don't need to worry about any special chars such as & or <. Only
> thing that we need to escape is ']]>'.
> 
> On 2014/04/21 18:12:20, mifern wrote:
> > escapeXml on escapes cdata. Can username and password contain "<" or "&"
that
> > require cdata escaping?
> 
>
https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/...
> File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java
> (right):
> 
>
https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/...
> src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:489:
String
> stsendpoint = config.getValue("sts.endpoint");
> 
> Initially when I started with User profile adaptor, myself and Eric discussed
> about having common base class but we decided to keep them separate for
> simplicity with side effect of duplicating code.
> 
> We can still revisit that decision but I want to avoid it in this CL.
> 
> On 2014/04/21 18:12:20, mifern wrote:
> > Same comments as SharePointUserProfileAdaptor.java.
> > 
> > Also, there is a log of code duplication between here and
> > SharePointUserProfileAdaptor.java. It seems like using a base class would be
> > prudent.

STSRELAM : This is an identifier used by ADFS to identify relying party (in this
case particular SharePoint application).

Sign in to reply to this message.

mifern

https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java (right): https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode197 src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:197: return String.format(reqXML, escapeXml(stsendpoint), escapeCdata sounds like a good compromise. ...

10 years ago (2014-04-21 21:42:19 UTC) #23

JohnL

https://codereview.appspot.com/83730043/diff/80001/test/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptorTest.java File test/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptorTest.java (right): https://codereview.appspot.com/83730043/diff/80001/test/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptorTest.java#newcode80 test/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptorTest.java:80: private AuthenticationHandlerFactory authenticatioFactory "authenticatioFactory" => "authenticationFactory"

10 years ago (2014-04-21 22:21:19 UTC) #24

Tanmay Vartak

https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java (right): https://codereview.appspot.com/83730043/diff/80001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode197 src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:197: return String.format(reqXML, escapeXml(stsendpoint), On 2014/04/21 21:42:20, mifern wrote: > ...

10 years ago (2014-04-22 18:25:05 UTC) #26

pjo

A few comments. Thank you https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java (right): https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode247 src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:247: return input; Does returning ...

10 years ago (2014-04-24 00:20:23 UTC) #27

mifern

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/AuthenticationHandlerFactoryImpl.java File src/com/google/enterprise/adaptor/sharepoint/AuthenticationHandlerFactoryImpl.java (right): https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/AuthenticationHandlerFactoryImpl.java#newcode54 src/com/google/enterprise/adaptor/sharepoint/AuthenticationHandlerFactoryImpl.java:54: return URI.create(endpoint).toASCIIString(); So UNICODE characters will be replaced by ...

10 years ago (2014-04-24 04:00:37 UTC) #28

pjo

Thank you https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java File src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java (right): https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java#newcode67 src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java:67: if (sharePointUrl == null || username == ...

10 years ago (2014-04-24 17:15:56 UTC) #29

ondrejnovak

Some small nit picks from me, otherwise looks good https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java (right): https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode142 src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:142: ...

10 years ago (2014-04-25 09:19:32 UTC) #30

ejona

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java#newcode451 src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:451: config.addKey("sts.endpoint", ""); Should these new configuration values be prefixed ...

10 years ago (2014-04-25 20:58:43 UTC) #31

Tanmay Vartak

On 2014/04/25 20:58:43, ejona wrote: > https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java > File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java > (right): > > https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java#newcode451 ...

10 years ago (2014-04-25 21:14:02 UTC) #32

Tanmay Vartak

On 2014/04/25 09:19:32, ondrejnovak wrote: > Some small nit picks from me, otherwise looks good ...

10 years ago (2014-04-25 21:20:07 UTC) #33

ejona

I may find the time to give it a high-level look-over. Based on the email ...

10 years ago (2014-04-25 21:57:29 UTC) #34

pjo

After looking at body of FormsAuthenticationHandler it looks close to template method <http://en.wikipedia.org/wiki/Template_method_pattern> (without the ...

10 years ago (2014-04-26 02:21:07 UTC) #35

After looking at body of FormsAuthenticationHandler it looks close to template
method <http://en.wikipedia.org/wiki/Template_method_pattern> (without the
subclass part).

So, here is a proposal:
  # Keep FormsAuthenticationHandler   # Remove
FormsAuthenticationHandler.AuthenticationHandler
  # Add abstract `AuthenticationResult authenticate()'
to FormsAuthenticationHandler
  # Make SamlAuthenticationHandler and
        SharePointFormsAuthenticationHandler subclass
        FormsAuthenticationHandler   # Remove boolean isFormsAuthentication
()because
     the name makes refers to instance, not to SP server. Instead
     have start return or throw fact that server doens't support
     forms-authn and keep state in adaptor.

Alternate proposal:
  # Rename AuthenticationHandler interface to AuthenticationClient
  # Continue to have FormsAuthencationHandler and have it accept
     instance of AuthencationClient
  # Remove boolean isFormsAuthentication()because
     the name makes refers to instance, not to SP server. Instead
     have start return or throw fact that server doens't support
     forms-authn and keep state in adaptor.


The reason I'd like to make some changes is:
  # AuthnHandler and FormsAuthnHandler as names don't communicate
      their relationship
  # isFormsAuthentcation() makes it seem like it's about handler,
       but it really is about the SP server, and that's confusing

Thank you very much,
PJ





- technology's compounding interest
-


On Fri, Apr 25, 2014 at 2:57 PM, <ejona@google.com> wrote:

> I may find the time to give it a high-level look-over. Based on the
> email conversation, it seemed I would need to look at the code more to
> comment.
>
>
>
> https://codereview.appspot.com/83730043/diff/100001/src/
> com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java
> File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java
> (right):
>
> https://codereview.appspot.com/83730043/diff/100001/src/
> com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java#newcode451
> src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:451:
> config.addKey("sts.endpoint", "");
> On 2014/04/25 20:58:44, ejona wrote:
>
>> Should these new configuration values be prefixed with "sharepoint."?
>>
>
> I was meaning "sharepoint.sts.endpoint". "sharepoint" would be there
> because that is the adaptor you are providing configuration for. I'm not
> certain you want the sharepoint there, but I would lean toward having
> it. Just something to consider.
>
> https://codereview.appspot.com/83730043/
>

Sign in to reply to this message.

Tanmay Vartak

With code review comments and restructuring Authentication Handler

9 years, 12 months ago (2014-04-29 18:42:55 UTC) #36

Tanmay Vartak

I have implemented most of the code review comments. https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java File src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java (right): https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java#newcode142 src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:142: ...

9 years, 12 months ago (2014-04-29 18:43:31 UTC) #37

I have implemented most of the code review comments.

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

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:142:
this.login = sharePointUrl + DEFAULT_LOGIN;
This works
On 2014/04/25 09:19:33, ondrejnovak wrote:
> Can you check, if this will this work when sharePointUrl ends with trailing
> slash?
> http://sp.example.com//_layouts/Authenticate.aspx%22

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:170:
Map<String, String> requestProperties = new HashMap<String, String>();
On 2014/04/25 09:19:33, ondrejnovak wrote:
> s/requestProperties/requestHeaders

Done.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:188:
Map<String, String> requestProperties = new HashMap<String, String>();
On 2014/04/25 09:19:33, ondrejnovak wrote:
> headers

Done.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:212: =
document.getElementsByTagNameNS("*","RequestSecurityTokenResponse");
Done. Should we fallback to * if default namespace
"http://schemas.xmlsoap.org/ws/2005/02/trust" is not working?
On 2014/04/25 09:19:33, ondrejnovak wrote:
> why are you using * - you should use the exact namespace name

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:247:
return input;
Modified to return empty string.
On 2014/04/24 00:20:23, pjo wrote:
> Does returning null make sense? How is that going to go into the string? 
Should
> it be fatal?

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

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AuthenticationHandlerFactoryImpl.java:54:
return URI.create(endpoint).toASCIIString();
This piece of code is borrowed from existing SharePoint Adaptor code and works
fine with Unicode characters.
On 2014/04/24 04:00:38, mifern wrote:
> So UNICODE characters will be replaced by ? and lost. That would mean the
> endpoint would be invalid then.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AuthenticationHandlerFactoryImpl.java:57:
private static URI spUrlToUri(String url) throws IOException {
Again this one taken from SharePoint Adaptor code.
This works with https:// as well. In fact to use ADFS, SharePoint needs to be
https://

Why are we doing this ?
Consider following url, "https://sharepoint.example.com/sites/My Site
Collection". In this url host "https://sharepoint.example.com" is properly
encoded but path "/sites/My Site Collection" is not. So this method is creating
host uri "https://sharepoint.example.com" and path uri
"/sites/My%20Site%Collection" and 
get combine output as
"https://sharepoint.example.com/sites/My%20Site%Collection".

This conversion is more applicable for urls returned by SharePoint where host is
already encode but path is not. See comment by Eric about SharePoint being silly
here. :)

I moved existing code from SharePoint Adaptor class to here so that it can be
reused between SharePoint adaptor and user profile adaptor.

We might need to revisit this code and some other places in Adaptor for Unicode
handling as part of another CL.
On 2014/04/24 04:00:38, mifern wrote:
> So you're assuming here that the url does not start with "http://"?

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AuthenticationHandlerFactoryImpl.java:87:
+ "/_vti_bin/Authentication.asmx").toString();
On 2014/04/24 00:20:23, pjo wrote:
> one too much indent

Done.

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

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java:82:
this.login = login;
As mentioned by PJ, keeping it consistent with existing code.
On 2014/04/24 04:00:38, mifern wrote:
> If login can't be null, then you should have a precondition check.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java:87:
this.stsendpoint = stsendpoint;
As mentioned by PJ, keeping it consistent with existing code.
On 2014/04/24 04:00:38, mifern wrote:
> If stsendpoint can't be null, then you should have a precondition check.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java:92:
if (Strings.isNullOrEmpty(stsendpoint)
On 2014/04/24 04:00:38, mifern wrote:
> These check are not needed if handled above.

Done.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java:121:
= document.getElementsByTagNameNS("*","BinarySecurityToken");
On 2014/04/25 09:19:33, ondrejnovak wrote:
> Why the asterisk? Why don't you use the exact namespace name.

Done. Same connect applicable as in ADFS

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java:141:
String url = login;
On 2014/04/24 04:00:38, mifern wrote:
> url local variable is not needed.

Done.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java:147:
String cookie = postResponse.getPostResponseHeaderField("Set-Cookie");
On 2014/04/24 04:00:38, mifern wrote:
> Just return, no need for cookie local variable.

Done.

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

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java:60:
log.log(Level.WARNING, "Empty SAML Token.");
As per discussion with PJ, I am changing code to throw IO exception that will
not fallback to Windows Integrated Authentication. We are here because user
wants to use Live authentication.
On 2014/04/24 04:00:38, mifern wrote:
> What would cause an empty token? Is there an error code? What action can the
> user take?

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java:67:
log.log(Level.FINE, "Authentication Cookie {0}", cookie);

Logging @ FINE. Should be OK.
On 2014/04/24 04:00:38, mifern wrote:
> So it's OK to log the cookie but not the token?

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java:107:
url = new URL(url.toURI().toASCIIString());

Again existing code.
On 2014/04/24 04:00:38, mifern wrote:
> I don't think this is right. You could lose some characters. Maybe you to
encode
> them using UTF-8.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java:139:
if (inputStream != null) {
As per documentation Error stream can be null if there is an error but server
dont have any data to report about it.
On 2014/04/24 04:00:38, mifern wrote:
> Can either the stream be null?

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

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:450: // When
running against ADFS authentication, set this to ADFS endpoint.
On 2014/04/25 09:19:33, ondrejnovak wrote:
> Can you add the defaults as comment here

Done.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:451:
config.addKey("sts.endpoint", "");
On 2014/04/25 21:57:29, ejona wrote:
> On 2014/04/25 20:58:44, ejona wrote:
> > Should these new configuration values be prefixed with "sharepoint."?
> 
> I was meaning "sharepoint.sts.endpoint". "sharepoint" would be there because
> that is the adaptor you are providing configuration for. I'm not certain you
> want the sharepoint there, but I would lean toward having it. Just something
to
> consider.

Done.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:452: // When
running against ADFS authentication, set this to relam value.
On 2014/04/24 00:20:23, pjo wrote:
> relam? is it a realm?

Done.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:453:
config.addKey("sts.relam", "");
On 2014/04/24 00:20:23, pjo wrote:
> relam?

Done.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:457:
config.addKey("sharepoint.useLiveAuthentication", "false");
Noted down.
On 2014/04/24 00:20:23, pjo wrote:
> The sequence of attempted authentications, the sequence of
> conditions and steps, will need to be carefully spelled
> out in documentation.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:479: String
stsrelam = config.getValue("sts.relam");
On 2014/04/24 00:20:23, pjo wrote:
> if it is a realm then please change throughout

Done.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:503: } else
if (!Strings.isNullOrEmpty(stsendpoint)
On 2014/04/25 20:58:44, ejona wrote:
> stsendpoint and stsrelam will never be null.

Done.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:504: &&
!Strings.isNullOrEmpty(stsrelam)) {
yes.
On 2014/04/24 04:00:38, mifern wrote:
> Just stsendpoint and stsrelam being non-empty is enough for ADFS?

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

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:198:
// When running against ADFS authentication, set this to ADFS endpoint.
On 2014/04/25 09:19:33, ondrejnovak wrote:
> Can you put here the defaults as comments

Done.

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:200:
config.addKey("sts.relam", "");
On 2014/04/24 00:20:23, pjo wrote:
> realm?  throughout

Done.

Sign in to reply to this message.

Tanmay Vartak

As per recommendation by PJ, we can remove isFormsAuthentication method. Even though actual code changes ...

9 years, 12 months ago (2014-04-29 18:53:13 UTC) #38

ejona

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java File src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java (right): https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java#newcode107 src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java:107: url = new URL(url.toURI().toASCIIString()); On 2014/04/24 04:00:38, mifern wrote: ...

9 years, 12 months ago (2014-04-29 19:55:34 UTC) #39

pjo

9 years, 12 months ago (2014-04-30 20:46:02 UTC) #40

LGTM.  Thank you.

Please:
(1) fix these nits
(2) double check that not logging sensitive info
(3) double check no "relam" anywhere

Say if you'd like another look.

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

https://codereview.appspot.com/83730043/diff/100001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:201:
String extractToken(String tokenResponse) throws IOException {
private or Visible for testing?

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

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:52:
public class AdfsHandshakeManager implements SamlHandshakeManager {
annotate with javadoc

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:217:
throw new IOException("ADFS token not available in response");
Consider only throwing.

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:219: Node
responseToken = nodes.item(0);
we know it's the first one?

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManager.java:221:
log.log(Level.FINER, "ADFS Authentication Token {0}", token);
Is this OK to log?

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

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/FormsAuthenticationHandler.java:65:
abstract boolean isFormsAuthentication() throws IOException;
Put TODO to remove it.

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

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java:94:
|| Strings.isNullOrEmpty(login)) {
i think fits on one line

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManager.java:132:
log.log(Level.FINER, "Live Authentication Token {0} namespace = {1} ", new
Object[] {token, n.getNamespaceURI()});
split this line

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

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java:72:
authenticationHandler.samlClient = samlClient;
pass as ctor parameter

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java:80: 
remove blank line

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java:81:
log.log(Level.FINE, "Fetching SAML Token using {0}", samlClient);
Does samlClient have a good toString()?

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SamlAuthenticationHandler.java:89:
log.log(Level.FINER, "Authentication Cookie {0}", cookie);
Should we log the cookie?

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

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:454: // When
running against ADFS authentication, set this to relam value.
s/relam/realm/

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

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointFormsAuthenticationHandler.java:71:
authenticationHandler.authenticationClient = authenticationClient;
pass to ctor

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

https://codereview.appspot.com/83730043/diff/120001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:200:
// When running against ADFS authentication, set this to relam value.
s/relam/realm/

https://codereview.appspot.com/83730043/diff/120001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManagerTest.java
(right):

https://codereview.appspot.com/83730043/diff/120001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/sharepoint/AdfsHandshakeManagerTest.java:257:
username, password, new ScheduledThreadPoolExecutor(1), manager).build();
over80

https://codereview.appspot.com/83730043/diff/120001/test/com/google/enterpris...
File
test/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManagerTest.java
(right):

https://codereview.appspot.com/83730043/diff/120001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/sharepoint/LiveAuthenticationHandshakeManagerTest.java:182:
"username@domain", "password&123", new ScheduledThreadPoolExecutor(1),
manager).build();
over80

https://codereview.appspot.com/83730043/diff/120001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java
(right):

https://codereview.appspot.com/83730043/diff/120001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:2093:
public <T extends EndpointReference> T getEndpointReference(Class<T> clazz) {
over80

Sign in to reply to this message.

Issue 83730043: ADFS Support for SharePoint Adaptor (Closed)

Description

Patch Set 1 #

Patch Set 2 : With code review comments and live authentication support #

Patch Set 3 : With code review comments on patch 2 #

Patch Set 4 : Discard this patch #

Patch Set 5 : With code review comments and Authentication handler factory #

Patch Set 6 : With code review comments implemented #

Patch Set 7 : With code review comments and restructuring Authentication Handler #

Messages