PowerPlatform-DataverseServiceClient
PowerPlatform-DataverseServiceClient copied to clipboard
microsoft
Do not refresh the access token in the CurrentAccessToken property
The property is documented to "return the current access token" but actually refreshes the token if it's expired, with sync over async code, which is bad.
This commit simply returns the current access token if it's available.
@0xced Its necessary to refresh this token on request when expired.
The underlying refresh mechanism is fully Async as driven by MSAL, which we cannot alter, nor would we want to alter given the way in which MSAL deals with this. The External Authentication pattern is also setup as Async to support the downstream
That said, we would be willing to explore a sync return where the token management is onboard and the token is not expired and only rolling to the Async mechanism when a refresh is needed. The downside of that is that our client then has to make the decision as to whether a token is expired, which is what its doing right now. This has been called out by our internal security team as a problem based on updated token security protocols and we are discussing removing that feature in the client. This is a Pref vs Security conversation right now.
Are you seeing an issue currently that is driving this?
CurrentAccessToken is used only during clone today. Or are you trying to directly access it?
Its necessary to refresh this token on request when expired.
Refreshing is necessary when performing HTTP requests that must include a valid bearer token, but when accessing a property I'm expecting that zero network calls are made under the hood. A GetAccessTokenAsync(CancellationToken cancellationToken) would be more appropriate.
Are you seeing an issue currently that is driving this? CurrentAccessToken is used only during clone today. Or are you trying to directly access it?
I'm accessing it for debugging purpose only but to reiterate, sync over async code is bad and should be avoided.
Also, all the ServiceClient constructors are performing plenty of network requests and this is bad design in my opinion. But I should probably open another issue to discuss the constructors specifically.
Happy to chat through it, I would suggest doing it on the discussion board.
As we said in the release notes are are introducing a new, more di/delayed invocation friendly, constructor that will eventually subsume the existing constructors. This will allow for the deferred execution of the client setup leg.
Actually I think we could have both:
- public string CurrentAccessToken { get; }
- public Task
RefreshAccessTokenAsync(CancellationToken cancellationToken);
Did you get a chance to discuss the matter?
We accepted a variation of this change as we dropped .net core 3.1 support.. this has been updated into 1.1.9's release