refactor: combine the storage interface for storage and pkce in gotrue_client - #1087
refactor: combine the storage interface for storage and pkce in gotrue_client#1087Vinzent03 wants to merge 19 commits into
Conversation
c006dd1 to
0ae2b71
Compare
|
Thanks for this PR, but honestly, I am not a huge fan of these refactorings. I feel like the library is deviating more and more from supabase-js, which makes it hard to maintain. Unless there is something broken, I'd say we keep it the way it is, and maybe we can revisit it when a major version release comes. |
|
I'm quite confused, because the whole goal of this pr is to align with supabase-js .
So this pr makes it easier to maintain the similarity to the js sdk. |
|
It's a refactoring that doesn't really solve any issues that users are asking for. We can probably wait for a major version bump for changes like this. |
|
This pull request has been inactive for 90 days. If you plan to continue working on this PR, please leave a comment to keep it open. |
|
This pull request was automatically closed due to inactivity. If you'd like to continue this work, please reopen the PR or create a new one. |
| /// Dispose the instance to free up resources. | ||
| Future<void> dispose() async { | ||
| _targetLifecycleState = null; | ||
| await _restoreSessionCancellableOperation.cancel(); |
There was a problem hiding this comment.
This code was actually not functional before as one cannot really cancel a future. Canceling this future would just call the onCancel function, but that is not specified here. So it would have just been fine to call recoverSession() directly.
|
@Vinzent03 just fyi, I changed the base of this from the v3 branch to main so that I can remove the v3 branch. |
|
@Vinzent03 this was superseded by #1804 and #1805, I'd love your review on those ones! |
…sted (#1804) Credit to @Vinzent03: gating the broadcast channel on `persistSession` was part of #1087, which also diagnosed #1085 as a consequence of the unconditional channel back in 2024. This PR carries that part over to the v3 code base. ## What kind of change does this PR introduce? Bug fix with a breaking default for standalone clients on web. ## What is the current behavior? `AuthClient` opens the multi-tab `BroadcastChannel` unconditionally on web, keyed only by the project URL. Every client for the same project on a page therefore shares one channel. When the app's client signs a user in, the broadcast handler in a standalone `SupabaseClient` created with the service role key saves that session too, and the service client starts sending the user's JWT. This is #1085. `persistSession` only exists on `FlutterAuthClientOptions`, where it selects between shared preferences and the in-memory `EmptyLocalStorage`. ## What is the new behavior? - `AuthClient` and `AuthClientOptions` gain `persistSession`, defaulting to `false`. - `FlutterAuthClientOptions.persistSession` now lives on the parent (still defaulting to `true`), so the same flag selects the Flutter session storage and reaches the auth client. - `_mayStartBroadcastChannel` returns early when the session is not persisted. A client that does not persist neither sends nor receives cross-tab auth events, matching auth-js which only creates the channel when `persistSession` is set. ## Breaking changes A `SupabaseClient` or `AuthClient` constructed directly no longer takes part in cross-tab session sync on web unless `persistSession: true` is passed. `Supabase.initialize` keeps syncing by default. ## Tests - `packages/supabase_auth/test/src/persist_session_broadcast_test.dart` (browser only): persisting clients receive each other's events, a non-persisting client is neither written to nor broadcasts. - `packages/supabase_flutter/test/persist_session_broadcast_test.dart` (browser only, runs in the `flutter test --platform chrome` job): a sign-in through `Supabase.instance.client` does not reach a standalone service role client, and does reach a second client with `persistSession: true`. ## Compliance matrix `client.session_management.persist_session` now lists `AuthClientOptions.persistSession` instead of the removed `FlutterAuthClientOptions.persistSession` field. Symbol and drift checks pass locally. Fixes #1085 Closes SDK-1749 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added configurable session persistence for authentication clients. - Standalone clients can opt in to cross-tab session synchronization. - Flutter clients enable persistence by default when using non-empty storage. - Added support for forwarding retry options through Flutter authentication configuration. - **Bug Fixes** - Clients using custom access tokens or in-memory storage no longer open cross-tab synchronization channels. - **Documentation** - Expanded migration guidance for session persistence and cross-tab synchronization. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ge (#1805) > [!NOTE] > Stacked on #1804, which adds `persistSession` to `AuthClient`. The base flips to `main` once that merges. ## What kind of change does this PR introduce? Breaking refactor of session persistence, tracked in SDK-1750. Credit to @Vinzent03: the design here is the one from #1087, opened in November 2024. A single storage interface for the session and the pkce verifiers, the client owning persistence like auth-js, `persistSession` and `storageKey` on the auth options, and dropping the no-op `CancelableOperation` around the recovery all come from that PR. It could not be rebased after the package rename and the v3 cleanups, so this is a fresh implementation of the same idea. ## What is the current behavior? Session persistence lives in `supabase_flutter`: `SupabaseAuth` listens to `onAuthStateChange` and writes the session to a `LocalStorage`, while the pkce code verifiers go to a separate `AuthAsyncStorage` passed as `pkceAsyncStorage`. Customizing storage means implementing two interfaces, and the plain `supabase` package has no session persistence at all. ## What is the new behavior? `AuthClient` owns persistence, as in auth-js: - One `AuthAsyncStorage` (`AuthClientOptions.asyncStorage`, renamed from `pkceAsyncStorage`) holds the session and the code verifiers. Its methods take positional parameters. - With `persistSession` the client writes the session on every change and restores it on construction. Writes are queued in order and a failed write is logged rather than thrown. `AuthClient.initialized` completes once the restore is done; `Supabase.initialize` awaits it, so `currentSession` is set when it returns, as before. An expired session is refreshed after `initialized` completes, so the wait never touches the network. - `storageKey` names the session key, defaulting to `defaultPersistSessionKey(url)`. It also prefixes the pkce verifier keys and names the broadcast channel. `PKCEVerifierStore` still reads and cleans up verifiers under the old `supabase.auth.token` prefix so an in-flight flow completes across the upgrade. - `LocalStorage`, `EmptyLocalStorage`, `SharedPreferencesLocalStorage` and `FlutterAuthClientOptions.localStorage` are gone. `persistSession: false` replaces `EmptyLocalStorage`. - `SharedPreferencesAuthAsyncStorage` stays the Flutter default. On web it writes to `window.localStorage` so the session is shared with supabase-js, and it decodes a verifier that `SharedPreferencesAsync` JSON-encoded there before. On other platforms a value written by v2 through the legacy `SharedPreferences` API is moved over on first read, once per key, and a removed value retires its legacy entry so a signed-out session cannot come back. - `SupabaseAuth` in `supabase_flutter` loses the storage handling, and the no-op `CancelableOperation` around the old recovery goes with it. `package:async` is no longer a dependency of `supabase_flutter`. - `initialSession` is emitted to every new subscriber of `onAuthStateChange` as its first event, with the session at that moment, the way auth-js and supabase-swift do. It waits for the restore, and events that fire in the meantime are held back so the initial one stays first. The stream no longer replays its latest event or error to late subscribers; a listener attached after a sign-in gets `initialSession` with that session instead of a replayed `signedIn`. `AuthClient.initialized` remains the await point for the restore, since the event now describes the moment of subscription rather than startup. The ticket asked for the storage write to be awaited before notifying subscribers. The write is queued instead, so `_saveSession` stays synchronous and the session version checks around refreshes keep their meaning. The write still happens at the same time, only the event no longer waits for it. ## Breaking changes See the two new sections in `MIGRATION.md`. In short: `LocalStorage` and its implementations are removed, `pkceAsyncStorage` is `asyncStorage`, `AuthAsyncStorage` methods are positional, and the pkce verifier keys move under `storageKey`. ## Tests - `packages/supabase_auth/test/session_persistence_test.dart`: write on sign-in, removal on sign-out, restore in a new client, custom key, non-persisting client, expired session refresh and sign-out, corrupt values, failing storage. - `packages/supabase_auth/test/pkce_flow_test.dart`: legacy prefix fallback and cleanup. - `packages/supabase_flutter/test/storage_migration_test.dart`, `storage_test.dart`, `storage_web_test.dart` (browser): the unified storage on VM and web, including the v2 migrations. - The remaining `supabase_flutter` tests are ported to `asyncStorage`. The full `supabase_auth`, `supabase` and `supabase_flutter` suites pass locally, the browser ones on Chrome. ## Compliance matrix `client.session_management.custom_storage` and `persist_session` are reconciled. Symbol and drift checks pass locally. Closes SDK-1750 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Sessions and PKCE verifiers now use shared, configurable asynchronous storage. - Added customizable storage keys and an initialization signal for session restoration. - Flutter uses asynchronous shared-preferences storage by default, with legacy-value migration. - Expired restored sessions can refresh automatically, and PKCE flows continue to support legacy stored values. - **Breaking Changes** - Replaced `localStorage` and `pkceAsyncStorage` with `asyncStorage`. - Updated custom storage methods to use positional parameters. - Removed legacy local-storage APIs and related configuration options. - **Documentation** - Added migration guidance and updated custom storage examples. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What kind of change does this PR introduce?
refactor with breaking changes and fixes
What is the current behavior?
The session gets stored by the supabase_flutter package by listening to auth events. The session is stored by a different class/interface than the pkce token. This adds unnecessary complexity and is a difference to the supabase-js sdk.
What is the new behavior?
The gotrue package manages the session storage itself (like in supabase-js) and uses the same interface as the pkce token.
Reasons for this pr:
persistSessionfield, which is now used to explicitly turn on the session broadcasting. fixes Version 2.7.0 prevents reading data on web under certain conditions #1085I deprecated both
localStorage(used for session storage bysupabase_flutter) andpkceAsyncStorage(used bysupabaseto store the pkce token) and replace them with a unified storage interfaceasyncStorage, which uses theGotrueAsyncStorageclass, which was previously used for pkce only. Though I refactored the arguments of the methods to use positional arguments instead of named arguments. This is much more idiomatic to other storage packages and common for methods with just one or two arguments. To support existing configuration as best as possible I created a new class, which combines both deprecated config options (localStorageandpkceAsyncStorage) into one and decides by the suffix of the storage key to use the session storage or pkce storage. Note that an implementation of theGotrueAsyncStorageclass still needs to be refactored, but that only affects the method signature and not the implementation itself.Additionally, I noticed that many document comments were outdated, because of the 'new' options classes for each package, so I updated them.
Breaking Changes
GotrueAsyncStorageclassAdditional context
I think this is a great refactoring that does not affect many people, because I don't think many flutter user customize the storage. But it makes it easier and more streamlined for the ones that do so and opens new easier usage ways outside of flutter with auth session storage now built-in.