ably / ably-flutter

A wrapper around our Cocoa and Java client library SDKs, providing iOS and Android support for those using Flutter and Dart.
https://ably.com/download
Apache License 2.0
60 stars 16 forks source link

ErrorInfo is not a subtype of type TokenDetails? #517

Closed ryzr closed 7 months ago

ryzr commented 8 months ago

View Sentry Stacktrace

I currently override the authCallback, to match functionality similar to Laravel Echo. Basically:

authCallback: (tokenParams) async {
  final jwt = await api.authorizeBroadcaster(channelName: currentChannelName, token: currentToken?.token);
  final parsed = parseJwt(jwt);

  currentToken = TokenDetails(
    jwt,
    capability: parsed['x-ably-capability'],
    clientId: parsed['x-ably-clientId'],
    expires: parsed['exp'] * 1000 as int,
    issued: parsed['iat'] * 1000 as int,
  );

  return currentToken;
}

where currentChannelName and currentToken are properties of a class that wraps the Realtime client.

Somewhere through the internals of the package, an ErrorInfo object is being returned, rather than a TokenDetails. This could be an error with our API call? Either way, since it is being returned, and not thrown, I don't have a way of catching and handling this error. I haven't been able to recreate this particular error, so I'm not sure what impact this error has on users (if any at all).

So far, I've only seen this error (as well as a couple of other type-mismatch errors related to channel subscriptions) on iOS.

My guess:

This method is asserting the type TokenDetails https://github.com/ably/ably-flutter/blob/9769ccebc4a2c1b3462f09f0402728ad6bb29d6c/lib/src/rest/src/rest_auth.dart#L12-L17

This method is returning an error instead https://github.com/ably/ably-flutter/blob/9769ccebc4a2c1b3462f09f0402728ad6bb29d6c/ios/Classes/handlers/AuthHandlers.swift#L55-L62

Though I don't feel particularly confident with iOS/native development, so I could be wrong here!

I hope the Sentry share link is helpful.

┆Issue is synchronized with this Jira Task by Unito

sacOO7 commented 8 months ago

@ryzr laravel-echo authorization works completely different than that of other SDK's. Are you using laravel-broadcaster at server side and flutter at client side ( instead of laravel-echo ) ?

sacOO7 commented 8 months ago

If there is a strong need to get this working using ably-flutter, lots of tweaks will be needed in the auth implmentation. It will be mostly same as how we deal with auth here => https://github.com/ably-forks/laravel-echo/blob/master/src/channel/ably/auth.ts

ryzr commented 8 months ago

Sorry, to clarify - yes I am using Laravel broadcasting on the server-side, laravel-echo in the web app, and now ably-flutter in our mobile app. Our Flutter implementation was in fact inspired by the source in the ably-forks/laravel-echo package, and does seem to work well 99% of the time, except for a few edge-cases which we're working on. I only wanted to provide that context so you understood why our authCallback might look a little different to most implementations, and to make sense of some of the details in the Sentry stack trace.

Despite it working the majority of the time, the issue I linked is something that pops up VERY occasionally in Sentry, I'd estimate it's affecting 1 in 10,000 users, or less. It's quite possible its just an internet connectivity issue.

I only wanted to report this issue as I currently have no way of catching what ErrorInfo is, to understand further as to what is going on.

sacOO7 commented 8 months ago

@ryzr good to hear that ably-flutter sdk is working well for you πŸ‘. Though I am skeptical about workings of ably-flutter sdk API's where explicit channel auth is required, specifically for private and presence channels. In laravel-echo. we have modified code to perform explicit channel auth ( for private and presence channels) before each channel is attached. Assuming, we have a authenticated connection with ably with token as basetoken not containing any of the channel claims ->

  1. For channel1 attach, it will implicitly send token auth request to upgrade existing basetoken to include channel1. So request will be made to server as specified by your code =>
    token1 = await api.authorizeBroadcaster(channelName: channel1, token: basetoken);
  2. For channel2 attach, it will implicitly send token auth request to upgrade existing token1 to include channel2. So request will be made to server as specified your code =>
    token2 = await api.authorizeBroadcaster(channelName: channel2, token: token1);

    and so on ..., so the final token will have all of the channel claims starting from channel1.

Since flutter auth api is not modified/available same as laravel-echo. You need to tweak authCallback. Having a list of all channels that needs to be authenticated beforehand is a must in this case.

var channels = ['private:channel1', 'private:channel2', 'presence:channel3', 'presence:channel4'];
var activeJwt = null;
authCallback: (tokenParams) async {

// authorize/include claims for one channel at a time
for (var channel in channels) {
  activeJwt= await api.authorizeBroadcaster(channelName: channel, token: activeJwt);
}
final parsed = parseJwt(activeJwt);
  finalToken = TokenDetails(
    activeJwt,
    capability: parsed['x-ably-capability'],
    clientId: parsed['x-ably-clientId'],
    expires: parsed['exp'] * 1000 as int,
    issued: parsed['iat'] * 1000 as int,
  );

  return finalToken;
}

Note -

  1. I am assuming TokenDetails function is a custom dart function same as https://github.com/ably-forks/laravel-echo/blob/18529c2c8a9656b432e4947212e7cc55abc878ac/src/channel/ably/utils.ts#L24
  2. api.authorizeBroadcaster method also sends user auth token as request header/param ( obtained during app signup/login ), so that laravel-broadcaster detects the right user and permissions are given to the channel accordingly.
sacOO7 commented 8 months ago

@ryzr did you try above solution or any other questions you want to ask ?

ryzr commented 8 months ago

@ryzr did you try above solution or any other questions you want to ask ?

Sorry - I believe the Laravel Echo stuff was a distraction from the actual issue I'm having.

If you see our Sentry Stacktrace an ErrorInfo object is being passed up from the iOS libraries, where a TokenDetails is expected.

This is where TokenDetails is expected: https://github.com/ably/ably-flutter/blob/9769ccebc4a2c1b3462f09f0402728ad6bb29d6c/lib/src/rest/src/rest_auth.dart#L12-L17

Line 57 of this method returns an instance of ErrorInfo. Is it possibly meant to be wrapped in a FlutterError() ? I'm not familiar with how any of the PlatformMethod stuff works. https://github.com/ably/ably-flutter/blob/9769ccebc4a2c1b3462f09f0402728ad6bb29d6c/ios/Classes/handlers/AuthHandlers.swift#L43-L62

sacOO7 commented 8 months ago

Okay, @ryzr few questions I want to ask

  1. Does your app support signup/login?
  2. Are you using private/presence channels?
  3. Where are you exactly calling authorize method?
  4. What does api.authorizeBroadcaster method do? Having an example code snippet will be super useful here
ryzr commented 8 months ago
  1. Does your app support signup/login?

Yes, it does.

  1. Are you using private/presence channels?

Both private and presence

  1. Where are you exactly calling authorize method?

Just before connecting to a channel: https://github.com/ryzr/flutter-ably-laravel-example/blob/main/lib/ably.dart#L54-L61

  1. What does api.authorizeBroadcaster method do?

Just an API call to the laravel /broadcasting/auth route: https://github.com/ryzr/flutter-ably-laravel-example/blob/main/lib/ably.dart#L125-L135

The entire AblyService class linked it similar to what we're running in the app - the code is comparable to what is in the Sentry Stacktrace (though I've removed all the auth logic and just replaced with environment variables for simplicity).

Again, I've found it hard to reproduce the errors - it could be token expiry, network timeouts or something like that?

sacOO7 commented 8 months ago

@ryzr thanks for creating the example app. I will go through it and get back to you πŸ‘

sacOO7 commented 8 months ago

There's a code smell here. You are using root api key https://github.com/ryzr/flutter-ably-laravel-example/blob/88262ddbdf0e79da26963b3df262c0a1a87b3765/lib/ably.dart#L24 at client side. If you see WARNING section under laravel-broadcaster-setup. It says not to use key at client side. If you still want to use it ( assuming it is hashed/not exposed in the release build), then you don't need other forms of authentication at all. You don't need authcallback and channel auth. You can just use ably-flutter sdk as is!

sacOO7 commented 8 months ago

Note If you don't want to store key as such, you should explicitly request it from laravel-server with custom-endpoint. The custom-endpoint should only be available for authenticated users. Main aim is not to expose API_KEY to the user. There are tools in the market that can extract APK / .APP file, so we can see the hardcoded key as is ( if not hashed. i.e. using 1. https://docs.flutter.dev/deployment/obfuscate, 2.https://codewithandrea.com/articles/flutter-api-keys-dart-define-env-files/#what-about-obfuscation)

sacOO7 commented 8 months ago

@ryzr is your issue resolved? or do you have more questions related to this

sacOO7 commented 8 months ago

Hey @ryzr any updates on above?

ryzr commented 7 months ago

@sacOO7 sorry I couldn't get back to you sooner!

Thank you for all of the advice with the api key logic. I feel silly having missed that - it makes a lot of sense! We have since released an app update with these changes, but still encounter this error.

I suppose what I'm trying to suggest, is why

https://github.com/ably/ably-flutter/blob/9769ccebc4a2c1b3462f09f0402728ad6bb29d6c/lib/src/rest/src/rest_auth.dart#L12-L17

is trying to return ErrorInfo. Should it not instead be throwing a PlatformException?

EDIT: to be clear - the Ably integration (including auth) does work in our app. This is an edge-case bug that has been popping up infrequently in Sentry, and it bothers me that I'm unable to catch it and understand the underlying cause.

sacOO7 commented 7 months ago

Hi @ryzr sorry, I didn't get your response. Are you still using ABLY_KEY in your app? Can you please update the code here https://github.com/ryzr/flutter-ably-laravel-example/

sacOO7 commented 7 months ago

@sacOO7 sorry I couldn't get back to you sooner!

Thank you for all of the advice with the api key logic. I feel silly having missed that - it makes a lot of sense! We have since released an app update with these changes, but still encounter this error.

I suppose what I'm trying to suggest, is why

https://github.com/ably/ably-flutter/blob/9769ccebc4a2c1b3462f09f0402728ad6bb29d6c/lib/src/rest/src/rest_auth.dart#L12-L17

is trying to return ErrorInfo. Should it not instead be throwing a PlatformException?

EDIT: to be clear - the Ably integration (including auth) does work in our app. This is an edge-case bug that has been popping up infrequently in Sentry, and it bothers me that I'm unable to catch it and understand the underlying cause.

This problem is only on ios device right? or does it also occur on android device

sacOO7 commented 7 months ago

Any updates on above @ryzr

sacOO7 commented 7 months ago

@ryzr let us know if the issue is resolved πŸ‘

sacOO7 commented 7 months ago

@ryzr closing the issue for now. My recommendation for now is not to use authcallback and channel auth. You can just use ably-flutter sdk as is! So, there's no need for custom implementation as such. SDK should work fine just with given API_KEY. You can open a new issue if needed. Thanks again !