[CIS-1185] Token provider

[evsaev]

🔗 Issue Links

CIS-1185

🎯 Goal

Make it possible to pass a tokenProvider when connecting the chat-client.

📝 Summary

1. chat client has connect method that takes a token provider as an argument (the same as on other platforms)
2. chat client still has connect method that requires a token but with the note that a token must not expire
3. chat client no longer have tokenProvider exposed

⚠️ The 3rd change is a breaking one but it might be OK to ship it:

• customers who is using non-expiring tokens will not be affected by this change
• customers who is using expiring and a tokenProvider that was removed would be glad to hear that the whole flow is simplified:

Before:

// 1. Get a user
let user = 

// 2. Create a token provider
let tokenProvider: TokenProvider = { completion in
   authService.fetchToken(for: user, completion: completion)
}

// 3. Call the token provider to receive the 1st token
tokenProvider { tokenResult in
 guard case .success(let token) = tokenResult else { return }

 // 4. Create and connect the chat-client
 let chatClient: ChatClient = ...
 chatClient.tokenProvider = tokenProvider

 OR

 let chatClient = ChatClient(config, tokenProvider: tokenProvider)

 // 5. Connect the chat client with received token
 chatClient.connect(userInfo: user, token: token, completion: { /*handle connection error*/ })
}

After:

// 1. Get a user
let user: UserInfo = 

// 2. Create a token provider
let tokenProvider: TokenProvider = { completion in
   authService.fetchToken(for: user, completion: completion)
}

// 3. Create and connect chat-client with a token provider
let chatClient: ChatClient = ...
chatClient.connect(userInfo: user, tokenProvider: tokenProvider, completion: { /*handle connection error*/ })

🧪 Manual Testing Notes

Sign in to the DemoApp as a normal/guest/anon user and make sure the connection flow works as before.

☑️ Contributor Checklist

• I have signed the Stream CLA (required)
• Changelog is updated with client-facing changes
• New code is covered by unit tests
• This change follows zero ⚠️ policy (required)
• Comparison screenshots added for visual changes
• Affected documentation updated (docusaurus, tutorial, CMS)

🎁 Meme

[bielikb]

This is much needed change! The approach is basically the same I took. Great minds think alike :)

[nuno-vieira]

The change LGTM ✅ I would prefer to deprecate the old API instead of removing it though. Is this possible?

[polqf]

If I were to be the client, I would not be happy with either the previous nor the proposed approaches. Chances are that I want to configure all the things in an initial point in the execution of the app, and only call connect whenever needed, without the need of knowing more about it.

Is this something we should maybe consider? connect should know nothing of tokens IMO

[evsaev]

If I were to be the client, I would not be happy with either the previous nor the proposed approaches. Chances are that I want to configure all the things in an initial point in the execution of the app, and only call connect whenever needed, without the need of knowing more about it.

Is this something we should maybe consider? connect should know nothing of tokens IMO

If I were to be the client, I would not be happy with either the previous nor the proposed approaches. Chances are that I want to configure all the things in an initial point in the execution of the app, and only call connect whenever needed, without the need of knowing more about it.

Is this something we should maybe consider? connect should know nothing of tokens IMO

@polqf yeah, was also thinking about that.

The tokenProvider could be passed once via init when ChatClient is created but in that case it needs a user to fetch a token for:

(userID: UserId, completion: @escaping (Result<Token, Error>) -> Void) -> Void

Tho it can be done this way, I see 2 potential issues:

1. it is not aligned with other SDKs
2. it can be misleading because for guest/anonymous users the tokenProvider won't be triggered but has to be provided

The 2nd is more or less fine and expected but 1st contradicts with API alignment we're aiming for.

[polqf]

@polqf yeah, was also thinking about that.

The tokenProvider could be passed once via init when ChatClient is created but in that case it needs a user to fetch a token for:

(userID: UserId, completion: @escaping (Result<Token, Error>) -> Void) -> Void
Tho it can be done this way, I see 2 potential issues:

it is not aligned with other SDKs
it can be misleading because for guest/anonymous users the tokenProvider won't be triggered but has to be provided
The 2nd is more or less fine and expected but 1st contradicts with API alignment we're aiming for.

Yeah. I do agree with 1, but tbh I don't thing this should be something we base our decisions off. Otherwise there's always a blocking point towards our ideal "excellence" direction
On the init approach, I always thought it is better to have a configure method that does exactly that. The same way we were discussing before that connect and init could happen at different places/moments, I believe this is the case too.

Maybe I am proposing something that it is too big for this PR, but I think it worths a discussion, at least.

[evsaev]

Yeah. I do agree with 1, but tbh I don't thing this should be something we base our decisions off. Otherwise there's always a blocking point towards our ideal "excellence" direction On the init approach, I always thought it is better to have a configure method that does exactly that. The same way we were discussing before that connect and init could happen at different places/moments, I believe this is the case too.

Maybe I am proposing something that it is too big for this PR, but I think it worths a discussion, at least.

Ah, I've got you now. Yes, initializing and configuring chat-client in different places is a good point 👍 But if we go with configure, someone can ask why we provide ChatClientConfig via ChatClient.init and not via dedicated configure method? And changing it gonna time because we won't longer be able to create database/apiClient/wsClient without having a config at the client initialization point OR will need to recreate them after configureis called.

I don't thing this should be something we base our decisions off. Otherwise there's always a blocking point towards our ideal "excellence" direction

As mentioned, the approach with configure method is also a good one but since it's connection API we can't immediately introduce it on iOS. This way, we'll never get APIs aligned. But what we can do is to suggest the new approach to other teams and if they like it - plan migration to the new connect API for the next major. And for now, go with what other SDKs have.

@polqf WDYT?

[polqf]

@evsaev yeah, as long as there is a discussion to improve this, I am happy to let this through for now 😄 Having it aligned with other SDKs is always better than having it as we have it now.

[bielikb]

Great stuff!

[nuno-vieira]

LGTM! ✅

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

90.0% Coverage
0.0% Duplication

[bradleyrzeller]

Hi -

If I understand this change correctly, there is now no way for the token provider to differentiate between a token needed for the "initial" connection and one that is needed because the current one expired (a refresh). This distinction is critical for the token provider to determine whether or not a cached token can be attempted or not.

Before, you could provide the initial token in the connect() method and the token provider could be set afterwards. In my testing with this setup, the token provider was only ever invoked upon token invalidation / refreshes. So, I could provide a cached token on the initial connect() call and the token provider was safe to assume it was called as a result of a refresh, and it would always fetch a new one from our app server

There are some workarounds to the new API here but I'm curious what the stream team recommends for this situation.

[nuno-vieira]

Hi @bradleyrzeller!

You are correct. For now, you can do the workarounds, and maybe add a boolean flag to check if the method is the first time being called. Currently, we don't have plans to change this, especially because it is aligned with the rest of the platforms. But we will discuss this internally and will let you know if we have updates on this in the future 👍

Best,
Nuno

[bradleyrzeller]

Ok, thanks for the reply @nuno-vieira.

I've been testing my workaround here and I am actually not seeing the token provider be called automatically after the token has expired.

I am generating a token using your token generator with 1 minute expiry. The token works initially as expected. I can send messages up until it expires ... and then the client disconnects. However, as I mentioned, the client is not invoking my token provider when it clearly knows that the token has expired (api calls return 401s etc.).

Can you provide some additional insight here? Thanks!

[nuno-vieira]

Hi, @bradleyrzeller can you provide a snippet of your workaround?

[bradleyrzeller]

It's essentially this:

        client.connectUser(
            userInfo: .init(id: "foobar"),
            tokenProvider: { callback in
                if self.allowCachedCredentials {
                    self.allowCachedCredentials = false
                    callback(.success(cachedToken))
                } else {
                    fetchNewToken { tokenResult in
                        callback(tokenResult)
                    }
                }
        )

The token provider is only ever invoked once, upon initial connection. I cannot get it to be invoked again even after my token has expired and the client disconnects.

[bradleyrzeller]

@nuno-vieira it seems to be because the RequestDecoder is failing to decode an error payload into a ErrorPayload on this line. The actual payload I am seeing is

{
  "code" : 40,
  "message" : "SendEvent failed with error: \"JWTAuth error: token is expired (exp)\"",
  "more_info" : "https:\/\/getstream.io\/chat\/docs\/api_errors_response",
  "StatusCode" : 401,
  "duration" : ""
}

but the ErrorPayload expects a details key.

This throws off downstream logic that detects token expiration and triggers a refresh...

[bradleyrzeller]

Just filed an issue for visibility.