JonathanHolvey / sharepy

Simple SharePoint authentication for Python
GNU General Public License v3.0
175 stars 52 forks source link

Possible fix for #20 ADFS based authentication #22

Closed joemeneses closed 5 years ago

joemeneses commented 6 years ago

20

This library originally did not work for my SharePoint environment either, but I wrote a fix for it in a custom SharePoint Token Service (STS) + Microsoft Online (MSO) Authentication configuration to replicate this diagram Created the SharePointAPI class to help deal with interactions with sharepoint lists. Far as I can tell will work with non ADFS SPO implementations as well as my own ADFS version. Use is the same as any other connection except to set adfsAuth = True

Added _spoAdfsAuth member function to SharePointSession class to handle authenticating with custom STS based ADFS SPO implementations. Documentation for that is here.

To handle this new authentication method, also created customStsSamlm and msoSaml xml templates.

Here is a test drive

import sharepy as session
from sharepy import spListsAPI

connect_url = "https://tenant.sharepoint.com/sites/subsite/"
s = session.connect(connect_url, username="user@domain.com", password="pass", adfsAuth=True)
shareLists = spListsAPI(s)
print(shareLists.GetListCollection())
print(shareLists.GetListItems('Case-insensitive List Name'))
JonathanHolvey commented 6 years ago

This is interesting. I had been planning to add support for ADFS authentication to fix issue #10. However, that discussion has gone quiet, and the user that created the issue hasn't been able to confirm if what I've done so far in the auth-classes branch is working.

I'll take a look at this PR in detail when I get a chance and see if we're trying to do the same thing. The error messages reported in #10 and #20 are different though, so there might be something else going on.

My aim for a version 2 release of SharePy is to use Requests authentication classes, with one each for SharePoint Online and ADFS (and maybe others). Unfortunately, I don't have an ADFS account to test with. Perhaps we could work on this together?

joemeneses commented 6 years ago

Hmm. I can't speak to the error that #10 had, as in my environment I was not receiving the same error. I did however originally get the error as referenced in #20 and #19 for namespace issues and invalid credentials. I imagine these were both solved when I started to reference the custom STS that is on-premises with credentials instead of the /extSTS.srf link.

Taking a quick look through the auth_classes branch, I see that you tried the same thing I did, but the SharepointADFS class was a work in progress. The _spoAdfsAuth in this PR would complete it through getting the SPOIDCRL cookie to carry around with subsequent HTTP requests.

I haven't mucked around with py Requests auth classes before so it'd be a good learning opportunity.

kkurup3 commented 6 years ago

Sorry for the late response. Thank you very much! This did fix the issue. The authentication was successful. However, I did receive another error message. I was going to mess around with it to see whats up. I'm not too familiar with stuff. I will paste the error message here just in case you guys may know why this may occur.

image

joemeneses commented 6 years ago

@kkurup3 Nice find, this one is on me. I updated the PR with a change to how the API uses the link supplied to the SharePoint site. This standardization of how to use the link required a document change:

Required: the URL supplied to the sharepy class on initialization should be in the
below formats, and MUST end with /_vti_bin . This ending is the best way to standardize
this code across varying SharePoint site installation use cases.

 https://<tenant>.sharepoint.com/sites/<subsite>/_vti_bin/ or
 https://<tenant>.sharepoint.com/<top site>/_vti_bin/

If you know how to find the WSDL files for your SharePoint site, and your use case doesn't
fall within the above examples you can set the vti_bin_url flag to your specific URL.

If you know how to find the WSDL files for your SharePoint site, and your use case doesn't
fall within the above examples you can set the vti_bin_url flag to your specific URL.

Your driver code should still work the same as before, but the url will now need to be in one of the above formats.

annhessl commented 5 years ago

having the same issue also. worked a couple of times and then I got locked out of creating new sessions with 'Authentication Failure'. Any idea if this fix will work for LDAP authentication also?

wappler99 commented 5 years ago

Jonathan, any idea when you'll do release 2 of SharePy with the above ADFS fix? I've got an ADFS account and I'd be happy to help with tests!

JonathanHolvey commented 5 years ago

I haven't actually had a chance to review this PR yet, but I hope to get a chance to in the coming weeks. Once I've done this and finished a bit of a refactor (see #9, #13, #14, #15) a v2 release shouldn't be too far away.

It's great to hear you're willing to help with testing. Firstly, can you confirm that the code in this PR solves the issue and allows login using ADFS? Do you see any other issues that need addressing?

wappler99 commented 5 years ago

I haven't actually had a chance to review this PR yet, but I hope to get a chance to in the coming weeks. Once I've done this and finished a bit of a refactor (see #9, #13, #14, #15) a v2 release shouldn't be too far away.

It's great to hear you're willing to help with testing. Firstly, can you confirm that the code in this PR solves the issue and allows login using ADFS? Do you see any other issues that need addressing?

I can confirm that the code in the PR works with the ADFS login. Don’t know about any other issues at this stage, but as part of the test I’m happy to see if basic functions like downloading and uploading a document etc work. Whenever you need my help with the testing, just let me know.

joemeneses commented 5 years ago

Hi all, Busy week on my end, I will be free of a lot of my current load in the new few days so I expect to work on this in the coming week. See updates in the above PR changes requested thread. @wappler99 glad to hear the ADFS login worked for you. I was hoping that what had started as a hack to resolve authentication in our environment for my current project is useful to others 😛

joemeneses commented 5 years ago

I've merged the auth-classes branch into this PR and tested out the adfs fix - it work without issue. The sharepoint API wrapper I wrote has been yoinked from the PR as well.

jko404 commented 5 years ago

Hi, I am new to this. I tried using auth-classes branch and use this code but I didnt manage to make it work:

auth = sharepy.auth.SharePointOnline(username="userid@domain.com") auth.login(site="domain.sharepoint.com") r = requests.get("https://domain.sharepoint.com", auth=auth)

im getting this error: Authentication Failure: AADSTS70002: Error validating credentials. AADSTS50126: Invalid username or password Digest request failed

Someone know how to fix this?

joemeneses commented 5 years ago

Pushing latest change requests then switching branches to develop

@KnocturnalAtGit It is possible that one of the fixes that @JonathanHolvey suggested above fixes your issue. For certain, and I have no idea how it was working for me before without, I had forgotten to include the self.digest variable to the constructor so it's possible that's what was breaking. Not entirely sure, especially since your error says

Authentication Failure: AADSTS70002: Error validating credentials. AADSTS50126: Invalid username or password

But it's possible. Give it another whirl after these changes go up.

JonathanHolvey commented 5 years ago

I've just merged this manually. Can you please check everything is still functioning @joemeneses ?

joemeneses commented 5 years ago

yes, the merge works fine

tschroedertlc commented 5 years ago

@JonathanHolvey @joemeneses

Great job on introducing the ADFS authentication method. The library works for me again, except I had to use a few hacks to your code to get it to work. I'd send a pull request but embarrassingly, I'm not super familiar with pull requests and such on github, so apologies for this form of feedback.

On line 218 of auth.py of the develop branch, I had to change that line to:

samlAssertion = re.search(r'<saml.?:Assertion.*saml.?:Assertion>', response.text, flags=re.DOTALL).group(0)

My response.text xml had the namespace saml1 instead of saml, so your code was not picking up the extra 1. Not sure if other numbers or longer strings would ever be in the same position, in which case that might need to be modified further.

Also, until I added the re.DOTALL flag, it was not finding that group. I tried searching for various strings and realized that it must think that there is a newline in there, so it was coming up with an empty search without that flag.

Lastly, not sure what is going on with this, but the print functions were not printing for me (in jupyter lab) with the carriage returns and end input such as: print("BinarySecurityToken request failed. The server did not send a valid response\r", end="") It printed correctly when I just simplified it to: print("BinarySecurityToken request failed. The server did not send a valid response")

Thanks again for your work on this! If you can have a bit of patience with me, I could work on sending a pull request, but if not, I hope the feedback is helpful.

keathmilligan commented 5 years ago

This branch worked for me. No hacks needed so far...

foadgr commented 5 years ago

I recently switched to Okta authentication for Office 365 and haven't been able to connect correctly. I've tried the ADFS auth shown in the sharepydevelop branch, which returns Token requested, but response did not include the STS SAML Assertion (this suggests the STSAuthUrl is invalid). Any idea if this an issue specific to Okta or O365?

foadgr commented 5 years ago

I recently switched to Okta authentication for Office 365 and haven't been able to connect correctly. I've tried the ADFS auth shown in the sharepydevelop branch, which returns Token requested, but response did not include the STS SAML Assertion (this suggests the STSAuthUrl is invalid). Any idea if this an issue specific to Okta or O365?

Resolved - this issue is specific to whether the user authenticates with WS-Federation. The XML response from https://login.microsoftonline.com/GetUserRealm.srf?login={username}&xml=1 should return the below (de-identified for security):

<RealmInfo Success="true">
<State>4</State>
<UserState>1</UserState>
<Login>{username}</Login>
<NameSpaceType>Managed</NameSpaceType>
<DomainName>XX_DOMAIN_NAME</DomainName>
<IsFederatedNS>false</IsFederatedNS>
<FederationBrandName>XX_FEDERATION_BRAND_NAME</FederationBrandName>
<CloudInstanceName>microsoftonline.com</CloudInstanceName>
</RealmInfo>
JonathanHolvey commented 5 years ago

@joemeneses @tschroedertlc I'm just trying to clean up some of the regular expressions used in the ADFS authentication. Ideally I'd like to replace them with calls to the ElementTree library, so that they are parsed properly as XML. Can you give me an example for the SAML assertion and BinarySecurityToken responses?

Edit: I found examples for those two, and now all I'm missing is the digest XML response.

I'm working on these changes in my branch auth-tidy, and should be able to merge this into a new release after a little testing. Please take a look if you're interested.

tschroedertlc commented 5 years ago

One issue I've had is that the the XML namespaces in my server response are not 'saml', but instead are 'saml1'. I imagine it could be possible that it is different for other people as well. In case it is helpful, I used the xml.dom.minidom module to generically grab the assertion from the XML like so:

dom = minidom.parseString(response.text)
def find_in_node(node, strings):
    for child in node.childNodes[::-1]:
        node_name = child.nodeName.lower()
        if all([s in node_name for s in strings]):
            return child.toxml()
        else:
            if child.hasChildNodes():
                next_node = find_in_node(child, strings)
                if next_node is not None:
                    return next_node
    return None

samlAssertion = find_in_node(dom, ('assertion', 'saml'))

That way the namespace is irrelevant.

Also, from what I saw, you needed a sample xml digest? Hopefully I interpreted that correctly, so if it is helpful, my (modified) xml digest reponse looks like:

<?xml version="1.0" ?>
<soap:Envelope xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <soap:Body>
    <GetUpdatedFormDigestInformationResponse xmlns="http://schemas.microsoft.com/sharepoint/soap/">
      <GetUpdatedFormDigestInformationResult>
        <DigestValue>0x74D78E901B8C5FD530E6F92E84267AE51725F3B9E22460D18CB45157485C08AC66A71C06101D7A994C77E552899C525102F7A36A9EA2B5F840B977470B4D1048,04 Jul 2019 19:50:10 -0000</DigestValue>
        <TimeoutSeconds>1800</TimeoutSeconds>
        <WebFullUrl>https://mycompany.sharepoint.com</WebFullUrl>
        <LibraryVersion>16.0.9019.1216</LibraryVersion>
        <SupportedSchemaVersions>14.0.0.0,15.0.0.0</SupportedSchemaVersions>
      </GetUpdatedFormDigestInformationResult>
    </GetUpdatedFormDigestInformationResponse>
  </soap:Body>
</soap:Envelope>
JonathanHolvey commented 5 years ago

@tschroedertlc thanks for that.

Parsing the XML properly should solve the problem of the namespace differing from server to server. This is because the prefix before the colon is just an alias for the namespace, and the actual namespace is mapped to the alias in an xmlns:<alias>="urn:oasis:names:tc:SAML:1.0:assertion" attribute further up the tree. The namespace alias can be saml or saml1 (or anything else), but actual namespace used when parsing is urn:oasis:names:tc:SAML:1.0:assertion which will never change.

JonathanHolvey commented 4 years ago

@tschroedertlc I'm trying to work out where I got to with this. I added one several commits 58ae53d..7d06cc3 since I last commented, which appear to deal with some of the XML namespace issues at least. Are you able to try again and see what errors you get?

I'm keen to release this fix to (hopefully) resolve some of the recent issues that have been reported.