Jericho / ZoomNet

.NET client library for the Zoom.us REST API v2
MIT License
69 stars 46 forks source link

Allow multiple instances of my app to share a Server-to-Server OAuth token #269

Closed KyMback closed 1 year ago

KyMback commented 1 year ago

Please take a look at https://devforum.zoom.us/t/randomly-receiving-invalid-access-token-for-server-to-server-oauth/74432 issue at zoom.

I have multiple ZoomNet client apps which are sharing the same credentials to Zoom Server-to-Server application. And each time when one client receives new access token the previous ones are invalidated. Currently this library have no way to working with this case (no token_index, no ability to manually refresh access token), so I'm forced to request new access token each time before new API call.

Could you advice smth or add support of token_index and ability to manually refresh access tokens (in order to do this at RetryCoordinator)?

Jericho commented 1 year ago

Thanks for the link. This is very interesting, I wasn't aware of this token_index and, correct me if I'm wrong, I don't think this is documented in Zoom's API documentation.

I think adding an Index parameter to the OAuthConnectionInfo in ZoomNet would be a reasonable solution. What do you think?

Jericho commented 1 year ago

My account is not setup with multiple token_index, therefore I can't test this new functionality. Are you in a position to test it for me? If I publish a beta package to my MyGet feed, would you be able to try it and give me some feedback?

Jericho commented 1 year ago

I published the beta package here. Let me know if you can test it.

By the way, you'll notice that the syntax to instantiate your OAuthConnectionInfo is changing slightly in this beta package. That's because I am working on #270

KyMback commented 1 year ago

I don't think this is documented in Zoom's API documentation.

You are right, I also didn't find anything in documentation. I was surprised to find this behavior. I mean when new access token invalidates the previous one.

My account is not setup with multiple token_index, therefore I can't test this new functionality

My account also doesn't support it right now. At least I have checked that zoom returns me the "Maximum group number limit exceeded" error. I found on a ticket above that this is the correct behavior...

Could you also add ability to manually refresh access token? Mb by providing OAuthTokenHandler? Because now this is internal class and I have no way to reuse RefreshTokenIfNecessary functionality

Jericho commented 1 year ago

I have checked that zoom returns me the "Maximum group number limit exceeded" error.

Thanks for validating.

Could you also add ability to manually refresh access token?

To be honest with you, I did not intend to make it public. My goal is to shield developers from the intricacies of requesting and refreshing authentication tokens but I did make sure to include a "OnRefresh" delegate on the OAuthConnectionInfo class which allows you to be notified whenever a token is requested. I'm wondering if that's good enough for your scenario?

As always though, I'm keeping an open mind. Maybe you could expand on your scenario and explain why you need to be able to manually request/refresh authentication token? Maybe you have a scenario that I never envisioned?

KyMback commented 1 year ago

Maybe you could expand on your scenario and explain why you need to be able to manually request/refresh authentication token?

To try get access token again without instantiating a new instance of ZoomClient, when I get error "Invalid access token", otherwise I have no chance to do this. So I just want to avoid extra auth call on each ZoomClient call => I'm trying to use singleton OAuthConnectionInfo.

ZoomClient doesn't handle this case, so it's impossible to refetch access token if you have singleton OAuthConnectionInfo and receive "Invalid access token"

Jericho commented 1 year ago

Even if ZoomNet provided you a way to manually get a new token, that would not solve the problem of having to instantiate a new instance of ZoomClient. There's currently no way for you to update an instantiated ZoomClient to use a token you manually requested. So I think we need to invest a little time to come up with a better design.

By the way, I did some testing about this scenarios and I have a few observations:

Observation number 1: it's really easy to reproduce the problem

var connectionInfo1 = OAuthConnectionInfo.ForServerToServer(clientId, clientSecret, accountId);
var connectionInfo2 = OAuthConnectionInfo.ForServerToServer(clientId, clientSecret, accountId);

// Instantiate two clients, each with their own connectionInfo.
var client1 = new ZoomClient(connectionInfo1);
var client2 = new ZoomClient(connectionInfo2);

// Connection number 1 requests a new token prior to the API call and User.GetAsync call subsequently completes successfully 
var myUser1 = await client1.Users.GetCurrentAsync(CancellationToken.None).ConfigureAwait(false);

// Connection number 2 requests a new token (thereby invalidating the token in connection number 1) and the User.GetAsync call completes successfully 
var myUser2 = await client2.Users.GetCurrentAsync(CancellationToken.None).ConfigureAwait(false);

// The call to User.GetAsync fails because the token in connection number 1 has been invalidated
myUser1 = await client1.Users.GetCurrentAsync(CancellationToken.None).ConfigureAwait(false);

Observation number 2: this problem can be mitigated by sharing the connection info rather than setting up a connection for each client. This works well if you need multiple clients in the same .NET process but it obviously doesn't help at all if you have multiple machines or multiple processes that are not able to share memory objects:

var connectionInfo = OAuthConnectionInfo.ForServerToServer(clientId, clientSecret, accountId);

// Instantiate two clients sharing the same connectionInfo (this is important!)
var client1 = new ZoomClient(connectionInfo);
var client2 = new ZoomClient(connectionInfo);

// Connection number 1 requests a new token and the User.GetAsync call completes successfully 
var myUser1 = await client1.Users.GetCurrentAsync(CancellationToken.None).ConfigureAwait(false);

// Connection number 2 does not need to request a new token and the User.GetAsync call completes successfully 
var myUser2 = await client2.Users.GetCurrentAsync(CancellationToken.None).ConfigureAwait(false);

// Both of the following calls succeed because the token in the shared connection is still valid
myUser1 = await client1.Users.GetCurrentAsync(CancellationToken.None).ConfigureAwait(false);
myUser2 = await client2.Users.GetCurrentAsync(CancellationToken.None).ConfigureAwait(false);

Observation number 3: I can't test because my account is not setup for multiple "group numbers" but I'm fairly certain the addition of the token index solves this problem:

var connectionInfo1 = OAuthConnectionInfo.ForServerToServer(clientId, clientSecret, accountId, null, 0); // <-- token index zero
var connectionInfo2 = OAuthConnectionInfo.ForServerToServer(clientId, clientSecret, accountId, null, 1); // <-- token index one

// Instantiate two clients, each with their own connectionInfo.
var client1 = new ZoomClient(connectionInfo1);
var client2 = new ZoomClient(connectionInfo2);

// Connection number 1 requests a new token for group number zero and the User.GetAsync call completes successfully 
var myUser1 = await client1.Users.GetCurrentAsync(CancellationToken.None).ConfigureAwait(false);

// Connection number 2 requests a new token for group number one and the User.GetAsync call completes successfully 
var myUser2 = await client2.Users.GetCurrentAsync(CancellationToken.None).ConfigureAwait(false);

// Both of the following calls succeed because the token in their respective connections are still valid
myUser1 = await client1.Users.GetCurrentAsync(CancellationToken.None).ConfigureAwait(false);
myUser2 = await client2.Users.GetCurrentAsync(CancellationToken.None).ConfigureAwait(false);

Observation number 4: Configuring your account for multiple "group numbers" is great but not scalable. My understanding is that Zoom is willing to increase your group number to a relatively low number such as 2 or 3 or maybe 5 (I could be wrong though. I base my opinion on the comments made by Zoom employees in the forum discussion you previously linked). This is great if you need an equally low number of ZoomClients. But what happens when you have a cloud solution with ten, twelve or ever more instances of your application? We are back to square one.

Observation number 5: This whole issue can be bypassed by creating multiple Server-to-Server OAuth apps in your Zoom account. A Zoom employee said the following:

There is no limit to the number of server to server OAuth apps that you can have in your account.

What this means is that you can create a large number of OAuth apps to match the number of instance of your application. Each OAuth app will have their own secret info (account id, client id and client secret) and therefore requesting/refreshing access token for one app does not interfere with the tokens for the other apps. This is great but I don't know how realistic this is. Will developers really create large number of apps (10, 20, 50, etc.) and manage three secret values for each app?

Jericho commented 1 year ago

Observation number 5: Server-to-Server connection should accept a previously issued token. This would be an improvement to the current connection info. ZoomNet should allow developers to instantiate a S2S connection info with a previously issued token (in addition to the necessary clientId, client secret and account id, of course). This would help in scenarios where you have multiple instances of your app and you want them to share the same token token (hence reducing the likelihood of one instance of your app requesting a new token which invalidates previously issued tokens).

This would not completely solve the problem but it's a step in the right direction.

Something along these lines:

// Notice the 'access token' parameter.
var connectionInfo = OAuthConnectionInfo.ForServerToServer(clientId, clientSecret, accountId, accessToken);
KyMback commented 1 year ago

Even if ZoomNet provided you a way to manually get a new token, that would not solve the problem of having to instantiate a new instance of ZoomClient

This helps to add some retry policy to refetch access token if it for some reasons isn't active

This is great but I don't know how realistic this is. Will developers really create large number of apps (10, 20, 50, etc.) and manage three secret values for each app?

I also see that this is not really scalable solution

Observation number 5: Server-to-Server connection should accept a previously issued token.

Yes it should get ability to reuse same access token between multiple instances, but still if you receive "Invalid access token" you should either create new instance of OAuthConnectionInfo to refetch access token either we can have ability to refetch access token in this cases (or do it manually). I just want to have not tricky way to create smth like "retry policy" for such cases

Jericho commented 1 year ago

Observation number 6: ambiguous error code when a token has been invalidated. It's very unfortunate that Zoom returns { "code": 124, "message": Invalid access token." } when a token has been invalidated because it's the same error code when developer attempts to use an invalid token. I wish the message had been something clearer like "access token is expired", or "access token has been invalidated" or something similar.

Therefore, I can't think of a way to accurately detect when a token has been invalidated.

Jericho commented 1 year ago

Thinking out loud (feel free to challenge my ideas, let me know if you agree/disagree, etc.):

For a scenario where you have multiple instances of your app, the ideal solution would be:

  1. Only one of the instances can request a token at a time. The other instances are blocked and wait for the token to be issued.
  2. When the token is issued, it gets saved in a repository where it's accessible by all your instances. For example, a Redis database, Azure blob storage, or any other central location.
  3. All instances use the token from the central repository until it's about to expire
  4. When the token is about to expire, go back to step 1

To accomplish this, what if I provided an interface called ITokenManagementStrategy and you would be responsible for implementing it in your app? This would allow you to save the token in whatever central repository you choose. Maybe over time I could provide a few implementations for well-known services such as Azure, AWS, etc. Maybe the community could help providing these implementations.

Here's what I think this interface could look like:

public interface ITokenManagementStrategy
{
    /// <summary>
    /// Retrieve token information from your central repository.
    /// </summary>
    Task<(string AccessToken, string RefreshToken, DateTime TokenExpiration, int TokenIndex)> GetTokenInfoAsync();

    /// <summary>
    /// Acquire a lease prior to renewing a token to prevent other instances from renewing the token at the same time.
    /// </summary>
    Task AcquireLeaseAsync();

    /// <summary>
    /// Persist token information to your central repository.
    /// </summary>
    Task SaveTokenInfoAsync(string accessToken, string refreshToken, DateTime tokenExpiration, int tokenIndex);

    /// <summary>
    /// Release the lock that was previously acquired.
    /// </summary>
    Task ReleaseLeaseAsync();
}

which you would use like this:


public class MyTokenManagementStrategy: ITokenManagementStrategy
{
   ... here you would implement the interface ...
}

var connectionInfo = OAuthConnectionInfo.ForServerToServer(clientId, clientSecret, accountId, new MyTokenManagementStrategy());
var client = new ZoomClient(connectionInfo);
KyMback commented 1 year ago

Seems good, It will be nice to see that!

Jericho commented 1 year ago

I published another beta release which contains the following enhancements:

  1. As previously mentioned, the syntax to instantiate a OAuth connection is slightly different.
    
    // Old syntax
    var connectionInfo = new OAuthConnection...);

// New syntax var connectionInfo = OAuthConnection.ForServerToServer(...);

2. You can specify a token index (if you don't, we assume the default index which is 0). Here's an example: 
```csharp
var connectionInfo = OAuthConnectionInfo.ForServerToServer(clientId, clientSecret, accountId, 1);
  1. ZoomNet has a new concept of a "token repository". You implement the ITokenRepository which allows you to save and retrieve token information to a custom repository of your choice (SQL Server, Redis, Azure blob storage, etc.)
    var tokenIndex = 0;
    var tokenRepository = new MyTokenRepository(); // <-- this is your custom implementation of the interface
    var connectionInfo = OAuthConnectionInfo.ForServerToServer(clientId, clientSecret, accountId, tokenIndex, tokenRepository);
  2. Additionally, you can specify an array of indices rather than a single value. If you do so, ZoomNet will automatically cycle through the indices when the token needs to be refreshed
    var tokenIndices = new[] { 0, 1, 2 };
    var tokenRepository = new MyTokenRepository();
    var connectionInfo = OAuthConnectionInfo.ForServerToServer(clientId, clientSecret, accountId, tokenIndices, tokenRepository);
  3. I am currently working on a sample implementation of the ITokenRepository interface that will save the token information to Azure blob storage. This sample implementation can be use as-is or as a model for a custom implementation. Stay tuned, I'll let you know when this is ready.
  4. I have updated ZoomNet's Readme file (see here) to explain these new options. I would really appreciate feedback on this. Let me know if the new text is clear.
Jericho commented 1 year ago

I have published a beta of a new nuget package that contains a class that saves token information to Azure Blog storage here.

Let me know if you have a chance to try the nuget package or to use it's C# source as a model to create your own token repository class.

Jericho commented 1 year ago

I raised a separate issue for the "token_index" feature. This issue will be used to track the "token repository" feature.

Jericho commented 1 year ago

Looks like Zoom released a solution to this scenario. This means that we will need neither the "token index" nor the "token repository".