pnp / pnpframework

PnP Framework is a .NET library targeting Microsoft 365 containing the PnP Provisioning engine and a ton of other useful extensions
https://pnp.github.io/pnpframework/
MIT License
206 stars 143 forks source link

Significant error in GetACSAppOnlyContext when passing in clientid and secret #878

Open tbithell opened 1 year ago

tbithell commented 1 year ago

This appears to be a carry over from the prior library, but we recently, with the help of MS, found a pretty significant error that was resulting in literally millions of dependency exceptions as well as exceptions that bubbled up into our code from time to time.

Note: we are in a multi tenant env and the application is a very large enterprise app.

Until we changed from calling GetACSAppOnlyContext without the tenantId to sending in the tenantId we'd see loads of dependency exceptions that we'd ignore since they never bubbled up. More recently some started bubbling up because, after digging through the code quite a bit, it became clear that it is possible for the code to return null for the host.

Passing in the tenant id resolved both issues.

adambu commented 1 year ago

I can confirm this. The strange thing is that it would often work without the tenant ID, most of the time, maybe 85%?

So to be specific, the overload with public ClientContext GetACSAppOnlyContext(string siteUrl, string realm, string appId, string appSecret, string acsHostUrl = "accesscontrol.windows.net", string globalEndPointPrefix = "accounts")

...worked consistently, but either of the other two overloads failed a significant percentage of the calls with 401 http response. When there are so many calls, this becomes a big issue. Users would expect it to either work or not work in any given tenant. If it works enough to get through Dev process, you can see why this could have serious impact on enterprise business applications.

It would be nice to at least have some understanding of what factors could cause this to fail so often. Otherwise, it seems the overload should be removed, or a bug investigated. Thanks for considering.

johnguilbert1970 commented 1 year ago

This code works fine in my App Service on Azure..... _authenticationManager.GetACSAppOnlyContext(_siteUrl, _clientId, clientSecret) However, when I run in Debug in VSS 2022 on WIndows 11 machine throws a Failed to Retrieve Token or IOException: Unable to read data from the transport connection: An existing connection was forcibly closed by the remote host... Have you any update. I did change to what you had above. It work, but was intermittent. I am wondering if there is something going on with SharePoint Online Token Request? Also, checked from Powershell v7 using Connect-PnPOnline https://dhplg.sharepoint.com/sites/administration -ClientId -ClientSecret and it connects, but flags a warning: Connecting with Client Secret uses legacy authentication and provides limited functionality. We can for instance not execute requests towards the Microsoft Graph, which limits cmdlets related to Microsoft Teams, Microsoft Planner, Microsoft Flow and Microsoft 365 Groups. You can hide this warning by using Connect-PnPOnline [your parameters] -WarningAction Ignore. so this did work, but not sure if warning is new?

johnguilbert1970 commented 1 year ago

So, I basically had an issue syncing code with Azure Devops even after successful login to repository. This is when I realised has to be my machine. Found this article which solved the problem. link. This will prove useful to a lot of people I'm sure where they are getting "Failed Token Request" when using SharePoint App-only Authentication with PnP.Framework 1.13 GetACSAppOnlyContext command. This does still work using Client Id and Client Secret as it stands today. Hopefully Microsoft will let us know if this changes. Not sure if this can be suitably closed? @tbithell