search

purtuga / SPWidgets

Sharepoint Custom UI Widgets
74 stars 34 forks source link

pickSPUser - Claims Based Authentication with a Custom Claims Provider #42

Closed seanmarthur closed 9 years ago

seanmarthur commented 9 years ago

Hi - amazing suite of tools here. I've been making the transition from full aspx/server side stuff to more client side development for my 2010 projects in preparation for our organization's migration to 2013, and this is really great stuff.

I noticed an issue on the pickSPUser control in our environment, which is Claims Based with a Custom Claims Provider - People.asmx webservice SearchPrincipals will return account names in the form of DOMAIN\USER, but when we call ResolvePrincipals, they get shot through the custom claims provider, which spits them back out as an encoded claims string, ie: i:0e.t|stsprovider|user@domain.com, which causes an issue in this branch of the code, to resolve the principal (my own comments in-line):

peoplePicker.js lines 549 - 566:

else {
resolvePrincipals({
    principalKeys: u.item.accountName // DOMAIN\USER at this point
})
.then(function(xmlDoc/*, status*/){
    // TODO: handle error conditions? (low risk of ocuring)
    u.item.accountId = $(xmlDoc)
    .find("AccountName:contains('" + u.item.accountName + "')") //AccountName in the returned xml is encoded claims, while u.item.accountName is still DOMAIN\USER - this is never a match
    .parent()
    .find("UserInfoID") // so we never get the user's ID, and we fail.
    .text();
    addToSelectionList();
}

I've fixed and tested this locally by doing this:

u.item.accountId = $(xmlDoc)
    //We are only searching for one user by their DOMAIN\USER - our claim provider guarantees only one result from this operation - maybe there is some validation that can take place here but for my purposes we can assume the result is the actual user we searched for.
    //.find("AccountName:contains('" + u.item.accountName + "')")
    .find("PrincipalInfo")
    .find("UserInfoID")
    .text();
// Set the account name to the SAML claims encoded id - not domain\user that we searched with
u.item.accountName = $(xmlDoc)
    .find("PrincipalInfo")
    .find("UserInfoID")
    .text();
    addToSelectionList();

This may not be worth incorporating into the code base in the current state without some sort of validation in place, but I wanted to bring this up.

purtuga commented 9 years ago

Thank you for posting this.  And yes, I think this would be worth a fix to be able to accommodate both types of authentication schemes.  Could you send me or post the response (XML) that show the claims based user being returned?  I would like to compare it to the other type of resolvePrincipals response and see if the solution you propose will handle both.  I'm also going to be creating a unit test suite for this project and that XML will help with it.  BTW: if you already know it handles both cases, let me know. Or send through a pull request and I'll merge it in.  :) Thanks again.  Paul -- sent from mobile

purtuga commented 9 years ago

I'm looking closer at the code and I like your proposed fix above. I think the only validation to add would be if the resolvePrinciplas for some reason returns more than one value... in that case, i think will handle it as a error condition and not attempt to set the user in the picker... What do you think? Also, if still possible, could you send me a sample of the xml request and response so I can build a test case? /Paul

seanmarthur commented 9 years ago

I'm very new to git and github, so I'll pass on the pull request for now :). I'd like to get more familiar with all this and contribute in the future.

Here is an xml snip of the ResolvePrincipals request/response (from fiddler)- Request (personal info modified and line breaks inserted):

<soap:Envelope xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/">
<soap:Body>
<ResolvePrincipals xmlns="http://schemas.microsoft.com/sharepoint/soap/">
<principalKeys>
<string>DOMAIN\USER</string>
</principalKeys>
<principalType>All</principalType>
<addToUserInfoList>true</addToUserInfoList>
</ResolvePrincipals>
</soap:Body>
</soap:Envelope>

Response:

<?xml version="1.0" encoding="utf-8"?>
<soap:Envelope xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<soap:Body>
<ResolvePrincipalsResponse xmlns="http://schemas.microsoft.com/sharepoint/soap/">
<ResolvePrincipalsResult>
<PrincipalInfo>
<AccountName>i:0e.t|claimprovider|user@domain.company.com</AccountName>
<UserInfoID>1</UserInfoID>
<DisplayName>First, Last M</DisplayName>
<Email>user@company.com</Email>
<Department>11111</Department>
<Title>Software Engineer</Title>
<IsResolved>true</IsResolved>
<PrincipalType>User</PrincipalType>
</PrincipalInfo>
</ResolvePrincipalsResult>
</ResolvePrincipalsResponse>
</soap:Body>
</soap:Envelope>

I have identified an exception case - when the user id portion of domain\user is contained within another user, for example 'domain\user' and 'domain\user1' - resolving 'domain\user' returns multiple results. This is an exceptionally rare case in my organization due to naming conventions for accounts, but it could be more prevalent in other organizations.

Here is the response XML for a multiple match on ResolvePrincipals (scrubbed again):

<?xml version="1.0" encoding="utf-8"?><soap:Envelope xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<soap:Body>
<ResolvePrincipalsResponse xmlns="http://schemas.microsoft.com/sharepoint/soap/">
<ResolvePrincipalsResult>
<PrincipalInfo>
<AccountName>DOMAIN\USER</AccountName>
<UserInfoID>-1</UserInfoID>
<IsResolved>false</IsResolved>
<MoreMatches>
<PrincipalInfo>
<AccountName>i:0e.t|claimprovider|USER1@domain.company.com</AccountName>
<UserInfoID>16</UserInfoID>
<DisplayName>Last, First M</DisplayName>
<Email>user1@company.com</Email>
<Department>111111</Department>
<Title>Technician</Title>
<IsResolved>true</IsResolved>
<PrincipalType>User</PrincipalType>
</PrincipalInfo>
<PrincipalInfo>
<AccountName>i:0e.t|claimprovider|USER@domain.company.com</AccountName>
<UserInfoID>17</UserInfoID>
<DisplayName>Last, First M</DisplayName>
<Email>user@company.com</Email>
<Department>111111</Department>
<Title>Inspector</Title>
<IsResolved>true</IsResolved>
<PrincipalType>User</PrincipalType>
</PrincipalInfo>
</MoreMatches>
<PrincipalType>All</PrincipalType>
</PrincipalInfo>
</ResolvePrincipalsResult>
</ResolvePrincipalsResponse>
</soap:Body>
</soap:Envelope>

From this, we can see we resolved on DOMAIN\USER, but returned two results - user@domain.com and user1@domain.com. My fix for assuming a single result fails in this case. Failing because two results were returned isn't fair either - we should just be able to match that second result and be happy. In my specific case the transformation from DOMAIN\USER to UPN format is well understood, so I can do some simple string manipulation to validate for my application. But I do not think that fix would be universal. A simple fix might be to just display the 'please choose...' dialog again and have the user pick from the resolved list, but that is not very friendly since they already picked the name they wanted.

Maybe we could keep some of the fields from the SearchPrincipalsResult - for my specific instance at least, I'm getting back DisplayName, Email, Department and Title - these could be used to match against the ResolvePrincipalsResult.

purtuga commented 9 years ago

This is great stuff... Thank you. And no problem on the contribution.

I'm going to use the XML to build test cases, so thanks again for that.

The condition you described was the one I was afraid of... I actually made changes over the weekend that no longer look for the login name (https://github.com/purtuga/SPWidgets/blob/master/src/peoplePickerWidget/peoplePicker.js#L564)... but as you can see from the change I made, I did include code that checks for multiples being returned... Based on the output you provided I agree with your suggestion: I will use some of the data available from SearchPrincipals to see if I can find the user in the ResolvePrincipals result set... Thanks again... I'll work on this a little more when I get a chance.