Align authentication with pnpcore

[sebastianmattar]

I would like to refactor AuthenticationManager to use PnPCore.Auth for token generation.
I propose the following changes:

1. Extend PnPCore.Auth to support custom Azure endpoints
2. Update PnP.Framework to use PnPCore.Auth, keep old constructors to maintain backwards compatibility
3. Update PnPPowershell to use PnPCore Auth

Do you agree with my proposal?
If yes, is it okay if I break some method signatures? (e.g. tokenCacheCallback parameter makes no longer sense)
Thank you for the feedback!

[jansenbe]

Hi @sebastianmattar ,

Thanks for raising this idea. Currently PnP PowerShell, PnP Framework and PnP Core SDK all use Microsoft.Identity.Client as common auth platform, we don't see immediate value of trying to unify all of our auth into the PnP.Core.Auth package at this point, however the idea is good and this might be a future optimization.

We would however like to get a better understanding of what authentication approaches you're missing today, can you provide some more context?

[sebastianmattar]

Hi @jansenbe ,

Thank you for the quick reply!

We are users of the PnP Framework and PnP PowerShell. Due to organisational requirements our applications have to use ACS authentication. I found several issues when using PnP provisioning in this scenario and wanted to fix them. I spent some time understanding AuthenticationManager and found it to be a good candidate for refactoring before adding more complexity to it, it is more than 1100 loc and the ACS authentication is a bit tacked on.

I also discovered that PnP.Core.Auth has very nicely structured implementations of the required authentication scenarios - why not use it then? This would reduce code duplication. Currently PnP.Core.Auth does not have ACS support and some authentication parameters are missing (custom authorities etc.) but this will be easy to add.

I have a working prototype which significantly reduces complexity and code duplication, AuthenticationManager is down to 550 lines while keeping backwards compatibility. It needs more testing though.

[sebastianmattar]

Another design problem that I found is that TokenHelper.cs relies on global variables and therefore is not safe for multi-threading.

[jansenbe]

Hi @sebastianmattar ,

We've just rewritten and cleaned up the authentication implementations in PnP PowerShell and PnP Framework by standardizing on MSAL. You've created a PR in PnP Core SDK, and we'll evaluate the PR with regard to the missing Pnp.Core.Auth features except the ACS bit. Azure ACS is legacy tech, not supported by MSAL, which we don't want to implement OOB in PnP Core SDK given there's easy way's for you to plug in your specific implementation. There's an ACS implementation in PnP Framework, we're open to improving the ACS implementation in that one. Sorry for the late reply.

[sebastianmattar]

Thanks, will do that!
Is there a timeframe when granular permissions (e.g. not using "Sites.FullControl.All") will be available for MSAL?
Would be great to leave ACS behind.