Skip to content

Conversation

Vinzent03
Copy link
Collaborator

@Vinzent03 Vinzent03 commented Aug 26, 2023

Changes

Switches from Hive to Shared Preferences for storing the access token

This is done by still including the Hive package:

  1. reading the access token
  2. writing it to shared prefs
  3. removing it from Hive
  4. Removing the file and the parent directory

@Vinzent03 Vinzent03 changed the title feat: use SharedPreferences for access token feat(supabase_flutter): use SharedPreferences for access token Aug 26, 2023
@Vinzent03 Vinzent03 marked this pull request as ready for review August 26, 2023 22:26
@Vinzent03
Copy link
Collaborator Author

SharedPreferences uses a prefix, so it's not possible to use the package to use the js localstorage entry as well. I've now accessed the localStorage directly to use the same entry. Is a bit more complicated, because you can't import dart:html on non web platform.

@dshukertjr
Copy link
Member

@Vinzent03

Setting the prefix to an empty string '' will allow access to all preferences created by any non-flutter versions of the app (for migrating from a native app to flutter).

Can we do this and use the same storage key as the JS SDK?

@Vinzent03
Copy link
Collaborator Author

No, because that's not per instance, but for the global Shared preference and would therefore conflict with the other users use of this.

Copy link
Member

@dshukertjr dshukertjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we still match the key excluding the prefix so that developers that want to use Flutter web in combination with the JS client library can just add SharedPreferences. setPrefix('') to their code baase?

@Vinzent03
Copy link
Collaborator Author

Vinzent03 commented Aug 31, 2023

I wouldn't do that, because you would need to use an allowList to prevent SharedPreferences from reading values it doesn't support. This is much more effort for the user. I think the current implementation is fine. Or don't you want the compatibility to be enabled by default?

If the prefix is set to a value such as '' that causes it to read values that were not originally stored by the SharedPreferences, initializing SharedPreferences may fail if any of the values are of types that are not supported by SharedPreferences. In this case, you can set an allowList that contains only preferences of supported types.

@dshukertjr
Copy link
Member

It seems like there are discussions around exposing the underlying storage for shared preferences. If this lands, we might be able to access the session object stored by supabase-js from this library.
flutter/flutter#123078

It's not going to make it compatible with supabase-js just yet because of the flutter. prefix and shared_preference's lack of object support, but let's modify our new storage key to match what the js client library has instead of our current SUPABASE_PERSIST_SESSION_KEY just of parity then.
https://github.com/supabase/supabase-js/blob/master/src/SupabaseClient.ts#L99

@Vinzent03
Copy link
Collaborator Author

Ah supabase-js uses a different storage key, okay. Will fix that. I don't get your first point, though. I've already implemented the access to the window.localStorage, which works fine. But I see now that the session object in js is differently structured than what we store in Dart

So do we want both to work together or not?

@dshukertjr
Copy link
Member

dshukertjr commented Sep 3, 2023

@Vinzent03

I don't get your first point, though. I've already implemented the access to the window.localStorage

I didn't realize that for web, you are access the local storage directly, instead of accessing it though shared preferences. That's great! I didn't have to worry about all the prefix issues and stuff that I had mentioned previously 😂

You might be aware of it, but the JS SDK doesn't stringify the session object when storing it in the local storage, so that is something we have to take into account when storing and retrieving session on the web with local storage.

But I see now that the session object in js is differently structured than what we store in Dart

Yeah, didn't think about this one, but good catch! Let's do it!

So do we want both to work together or not?

Yes, I think it would be great to have compatibility between the JS SDK and supabase_flutter!

Copy link
Member

@dshukertjr dshukertjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Just one final check, and I think this PR is good to be merged!

@Vinzent03 Vinzent03 requested a review from dshukertjr September 4, 2023 15:21
Copy link
Member

@dshukertjr dshukertjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome! Making supabase_flutter compatible with the JS client is huge 💪

@Vinzent03 Vinzent03 merged commit 9d72a59 into next Sep 5, 2023
@Vinzent03 Vinzent03 deleted the fix/hive branch September 5, 2023 13:57
@tomekit
Copy link

tomekit commented Oct 18, 2023

Just my two cents. Have you guys considered using: https://pub.dev/packages/flutter_secure_storage instead of SharedPreferences?

@Vinzent03
Copy link
Collaborator Author

It's not really necessary, as the access token is accessible on the web via localstorage as well. And it's easier to maintain as it's supported on all platforms and means a dependency less. If you want to, you can still use it by providing your own LocalStorage implementation.

@tomekit
Copy link

tomekit commented Oct 18, 2023

I've assumed that SharedPreferences replaces Hive on all platforms. For Web SharedPreferences is more than fine, browsers have built-in security, so no other website can steal the token (on desktop that's not the case).

@tomekit
Copy link

tomekit commented Oct 19, 2023

To follow up on my previous comment, what I really meant is where the session data ends on the user's device and whether it is actually secure to use SharedPreferences (that is whether other apps could potentially access Supabase session). It's still way better than a previously used Hive.

Please find below comparison tables:

flutter_secure_storage

Platform | Location
Android | EncryptedSharedPreferences
iOS/macOS | Keychain
Linux | libsecret
Web | ?
Windows | ?

shared_preferences

Platform | Location
Android | SharedPreferences
iOS/macOS | NSUserDefaults
Linux | In the XDG_DATA_HOME directory
Web | LocalStorage
Windows | In the roaming AppData directory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants