Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remove deprecated session methods #340

Merged
merged 9 commits into from
Aug 3, 2022

Conversation

kangmingtay
Copy link
Member

@kangmingtay kangmingtay commented Aug 2, 2022

What kind of change does this PR introduce?

  • Remove the following deprecated functions and properties

    • setAuth(accessToken)
      • Based on the original PR to add setAuth(), it seems like it was used to set a custom access token in the header, this can just be done by updating the access token directly in local storage. The use case here doesn't seem too clear as well, using a custom access token would be overwritten the moment gotrue initiates a refresh.
    • user(), session(), this.currentSession, this.currentUser
      • can be retrieved using getSession()
  • Updated the session type which guarantees that if an object is of type Session, it will always have an access_token, refresh_token and user.

Copy link
Member

@alaister alaister left a comment

Choose a reason for hiding this comment

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

Looks great! Still need to convert the Errors to the custom ones :)

@kangmingtay
Copy link
Member Author

@alaister custom error types are added in this PR (#341) to reduce the amount of code that needs to be reviewed

src/GoTrueClient.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@J0 J0 left a comment

Choose a reason for hiding this comment

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

Looks good to me

src/GoTrueClient.ts Outdated Show resolved Hide resolved
Copy link
Member

@alaister alaister left a comment

Choose a reason for hiding this comment

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

lgtm! nice!

@kangmingtay kangmingtay merged commit ac2b257 into km/refactor-sign-in Aug 3, 2022
@kangmingtay kangmingtay deleted the km/cleanup-session-type branch August 3, 2022 13:33
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

🎉 This PR is included in version 1.23.0-next.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@bnjmnt4n
Copy link

bnjmnt4n commented Aug 4, 2022

  • Based on the original PR to add setAuth(), it seems like it was used to set a custom access token in the header, this can just be done by updating the access token directly in local storage. The use case here doesn't seem too clear as well, using a custom access token would be overwritten the moment gotrue initiates a refresh.

Is there going to be a replacement/alternative to this function? I think the primary usecase would be in server-side environments, where the server has a user's JWT and wants to perform an operation using the user's credentials to enforce RLS policies. The supabase-js library uses gotrue-js' getSession().access_token to construct the auth header, which is why setAuth() is particularly handy. Otherwise, the server would have to manually construct the auth headers themselves when creating a new supabase client, which is also fine, but certainly not as convenient.

The local storage example given would definitely be more complicated, and also wouldn't be feasible on server-side environments.

@inian
Copy link
Member

inian commented Aug 5, 2022

I agree having setAuth is useful. Apart from @bnjmnt4n's point on server side code, our other libraries like Realtime also exposes this method - ideally we have all our libraries expose a more generic setHeader method but till we have that setAuth seems to work.

Won't this also break the code here?

@alaister
Copy link
Member

alaister commented Aug 5, 2022

@inian that code is no longer needed as getSession() reads from localStorage everytime.

Regarding the setAuth() method I think it's actually better not to have in gotrue-js for serverside use cases as there is a potential race condition if the developer sets up one supabase client outside of the request handler and calls setAuth() inside the request handler, which could lead to it appearing you're logged in as another user.

The way we should be recommending people setup their supabase client serverside if the want to send requests as the end user is creating a supabase client per request. For example:

app.get('/example', (req, res) => {
  const supabase = createClient(process.env.SUPABASE_URL, req.cookies['sb-access-token'])
  
  ...
})

@inian
Copy link
Member

inian commented Aug 5, 2022

That makes sense Alaister. The client libraries for our other products still need something like setAuth but since gotrue is responsible for managing its own session, it doesn't make sense to have the user set it manually.

@pixtron
Copy link
Contributor

pixtron commented Aug 5, 2022

I strongly agree to remove setAuth(). Beside the race condition it also
a.) violated the single source of truth principle and
b.) it might have lead to mixed up sessions (eg. access_token from user A, remainder of session from user B).

I support the idea of creating the client on a per request basis. Although i'm not quite sure if passing the AT instead of the anon key to createClient() would actually work. Isn't the anon key always needed in the apikey header?

Reusing a client in a multi user/session environment (eg. long running express process) should be avoided as it might lead to unwanted behavior like this:

1.) req from user A hits server
2.) server calls setAuth('AcessTokenUserA')
3.) req from user B hits server
4.) server calls setAuth('AcessTokenUserB')
5.) Server executes a query for the request of UserA
6.) Server sends a response for User B to User A as the access token is set to AcessTokenUserB

@larbish
Copy link

larbish commented Aug 17, 2022

Hello supabase team, I'm the maintainer of the supabase module for Nuxt and I'm currently working on the migration to your v2.

For the v1, I was using the setAuth() method to make use of RLS on server side. I tried your proposal @alaister but as @pixtron said, I can not create a client with the token instead of the anon key, it results on an error SupabaseKey is required.

What is the way to achieve this ? 🙏

@hf
Copy link
Contributor

hf commented Aug 17, 2022

Hey @larbish,

As @kangmingtay explains, you can use the setSession() method instead which takes in a Refresh Token (not a JWT!). Alternatively, you can manipulate the value in local storage directly. This does mean that on the server side, the library requires the availability of a window.localStorage type of equivalent.

Not sure if I'm helping... the SupabaseKey is required error sounds to me like you're not sending the API key header which is the anon key @alaister is probably talking about.

@pixtron
Copy link
Contributor

pixtron commented Aug 17, 2022

@larbish try to set the Authorization Header:

import { createClient } from '@supabase/supabase-js';

createClient(SUPABASE_URL, SUPABASE_ANON_KEY, {global: { headers: {
  Authorization: `Bearer ${access_token}`
}}})

You can find the type for the options here: https://github.com/supabase/supabase-js/blob/c487683bf9ba9368f502a497ec67a2231c053612/src/lib/types.ts#L10

If you directly use gotrue-js it would be:

new GoTrueClient({headers: {Authorization: 'Bearer ...'}})

@larbish
Copy link

larbish commented Aug 17, 2022

Thank you @pixtron, that is exactly what I was looking for.

@alaister
Copy link
Member

Hey @larbish,

The only way you'd get the SupabaseKey is required is if you didn't pass anything to the second argument of createClient.

Would you mind checking that you are passing a token in there?

@larbish
Copy link

larbish commented Aug 17, 2022

@alaister Indeed, the error is only thrown when my token value is null. But I also confirm you can't send the token instead of the anon key. What is working is to set the JWT in options like @pixtron mentioned.

@GaryAustin1
Copy link

GaryAustin1 commented Sep 1, 2022

Related to realtime-js. Currently a user can change the jwt with setAuth (today two users were doing so with custom jwts and hit an issue with jwt format in their realtime handlers but it is same for just changing the jwt) and setAuth generates a refresh token event which realtime uses to update it's user token.

It is not clear (at least to me without more research into code) how in v2 a change in the jwt header (custom or just to another Supabase jwt like, anon key to real user) gets communicated to realtime-js which is already running a subscription. Maybe when supabase client is "reset" with new jwt header, the currently running realtime channel gets update?

At a minimum needs to be documented how to do that as there is probably a realtime-js call that would also have to be made in addition to the header change. Or it may be that if you change the jwt you need to close and restart the subscription.

I'll look into it more when I get time, but just throwing it out there.

@magicseth
Copy link

It would be nice to have a slightly easier path for custom JWT:

What I was hoping for was a SignInWithToken(...) api
that would take the JWT, create a session, and call the authstatechanged callbacks
Does that make sense?

@jmedellinc
Copy link

A workaround can be found on:
supabase/supabase-js#553

Related to realtime-js. Currently a user can change the jwt with setAuth (today two users were doing so with custom jwts and hit an issue with jwt format in their realtime handlers but it is same for just changing the jwt) and setAuth generates a refresh token event which realtime uses to update it's user token.

It is not clear (at least to me without more research into code) how in v2 a change in the jwt header (custom or just to another Supabase jwt like, anon key to real user) gets communicated to realtime-js which is already running a subscription. Maybe when supabase client is "reset" with new jwt header, the currently running realtime channel gets update?

At a minimum needs to be documented how to do that as there is probably a realtime-js call that would also have to be made in addition to the header change. Or it may be that if you change the jwt you need to close and restart the subscription.

I'll look into it more when I get time, but just throwing it out there.

@miguel-flowalytics
Copy link

Is there something I'm missing from this new configuration to create a SupabaseClient per server-side route request?

I'm unable to get a session or user when setting an accessToken in global.headers
More details here: #450

@aruke
Copy link

aruke commented Sep 23, 2022

I'm having the same problem.
Creating supabase client with accessToken

createClient(Environment.Supabase.URL, Environment.Supabase.ANON_KEY, {
  ...clientOptions,
  global: {
    headers: {
      Authorization: `Bearer ${accessToken}`,
    },
  },
});

Getting session:

const authenticatedClient = // Create with the method above
const { data: sessionData, error: sessionError } =
    await authenticatedClient.auth.getSession();
console.log('Session data');
console.log(sessionData, sessionError);

Output is:

Session data
{ session: null } null

Getting user:

const authenticatedClient = // Create with the method above
const { data: userData, error: userError } = await authenticatedClient.auth.getUser();
console.log('User data');
console.log(userData, userError);

Output:

User data
{ user: null } AuthApiError: This endpoint requires a Bearer token
    at /Users/aruke/Workspace/cisc/node_modules/@supabase/gotrue-js/src/lib/fetch.ts:40:16
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  __isAuthError: true,
  status: 401
}

--- Update

Raw HTTP request to GoTrue server on path /auth/v1/user with two headers apiKey: ANNON_KEY and Authorization: Bearer TOKEN successfully return signed-in user.

@bhvngt
Copy link

bhvngt commented Sep 28, 2022

I have finally figured out the real issue. In v2, by default supabaseClient persists session access token to local storage of the browser. Since on server side, this is not available, subsequent calls to the client after passing global header will not have access to the access_token.

supabase-client does allow one to send an alternate storage. So for server-side code, I have created this simple in-memory store. With this, I was able to fetch session and user information.

Here's the code snippet

function inMemoryStorageProvider(): SupportedStorage {
  const items = new Map();
  return ({
    getItem: (key: string) => items.get(key),
    setItem: (key: string, value: string) => {
      items.set(key, value);
    },
    removeItem: (key: string) => {
      items.delete(key);
    }
  }) as SupportedStorage;
}

const client = createClient(supabaseUrl, anonKey, {
  auth: { storage: inMemoryStorageProvider() }, 
  global: {headers: {Authorization: `Bearer ${access_token}`}}
});
client.auth.onAuthStateChange((event) => {
  console.log(`client-2 event`, event); // ✅
})

const {data: data2} = await client.auth.getSession(); 

@galizar
Copy link

galizar commented Sep 28, 2022

@bhvngt If you set persistSession: false in the auth config object, an in-memory storage will be used by default. Even then using your custom in-memory storage doesn't solve the issue though. Both getSession and getUser still return null for me.

@miguelespinoza
Copy link

@bhvngt I'm new to SSR, but I'd be weary of that in-memory store implementation. What if you have a multi-region site?

I'm using a cookie implementation (bypassing supabase storage), and it's been working great so far (not yet in production, very soon). You should check out cookie support for your SSR framework. I'm using Remix, so if you have any questions let me know.

But yup, it still wouldn't fix the issue with session and user being null.

@bhvngt
Copy link

bhvngt commented Sep 28, 2022

@galizar @miguelespinoza I tried with persistSession: false and it returns null session. However, with InMemoryStorage implementation, I could see getSession() call returning session object. oAuthStateChange event gets triggered too.

Here's the full script that I ran as a nodejs script.

function inMemoryStorageProvider(): SupportedStorage {
  const items = new Map();
  return ({
    getItem: (key: string) => items.get(key),
    setItem: (key: string, value: string) => {
      items.set(key, value);
    },
    removeItem: (key: string) => {
      items.delete(key);
    }
  }) as SupportedStorage;
}

const supabaseUrl = process.env.PUBLIC_SUPABASE_URL ?? "";
const anonKey = process.env.PUBLIC_SUPABASE_ANON_KEY ?? "";

// const authOptions = {auth: { persistSession: false }}; // returns null session ❌

const authOptions = {auth: { storage: inMemoryStorageProvider() }};
const signInClient = createClient(supabaseUrl, anonKey, {...authOptions});
signInClient.auth.onAuthStateChange(async (event) => {
  console.log(`signInClient event`, event); // prints SIGN_IN ✅
})
const {data: {session}} = await signInClient.auth.signInWithPassword({email, password});
console.log("signInClient session access_token - " , session?.access_token) // prints access_token ✅

const directSessionClient = createClient(supabaseUrl, anonKey, {...authOptions, global: {headers: {Authorization: `Bearer ${session?.access_token}`}}});
directSessionClient.auth.onAuthStateChange((event) => {
  console.log(`directSessionClient event`, event); // gets called ✅
})

const {data: getSessionResult} = await directSessionClient.auth.getSession();
console.log(`getSessionResult session`, getSessionResult.session); // prints session object ✅

Output is as follows

signInClient event SIGNED_IN
signInClient session access_token - ******

directSessionClient event SIGNED_IN
getSessionResult session {
  access_token: '******,
  token_type: 'bearer',
  expires_in: 3600,
  refresh_token: '****',
  user: {
    id: '******,
    aud: '',
    role: 'authenticated',
    email: 'test@example.app',
    email_confirmed_at: '2022-09-27T13:58:31.726441Z',
    phone: '*******',
    phone_confirmed_at: '2022-09-27T13:58:31.72668Z',
    confirmed_at: '2022-09-27T13:58:31.726441Z',
    last_sign_in_at: '2022-09-28T17:59:43.381682678Z',
    app_metadata: { provider: 'email', providers: [Array] },
    user_metadata: {},
    identities: [],
    created_at: '2022-09-27T13:58:31.725704Z',
    updated_at: '2022-09-28T17:59:43.382583Z'
  },
  expires_at: 1664391583
}

I'm using a cookie implementation (bypassing supabase storage), and it's been working great so far (not yet in production, > very soon). You should check out cookie support for your SSR framework. I'm using Remix, so if you have any questions let > me know.

@miguelespinoza Thanks for offering help on the cookie implementation. I am using svelte-kit which gives me access to cookie. Would love to see a snippet of your implementation, if thats fine with you.

@pixtron
Copy link
Contributor

pixtron commented Sep 28, 2022

@bhvngt do you still get a session with directSessionClient if you remove those two lines from your script?

const {data: {session}} = await signInClient.auth.signInWithPassword({email, password});
console.log("signInClient session access_token - " , session?.access_token)

My assumption is that signInClient.auth.signInWithPassword({email, password}) fills your InMemoryStorage with a valid session. As signInClient and directSessionClient share the same in memory store, you can access the session generated from signInClient in directSessionClient. Whereas if you set {auth: { persistSession: false }} each of the two clients have their own internal in memory store.

Setting the option {global: {headers: {Authorization: ''}}} works for the rest client, but it won't initialize a session in supabase.auth (aka gotrue-js).
The only two ways i know, to initialize a session from outside in supabase.auth is to:

a.) call supabase.auth.setSession(refreshToken), with a valid refresh token. As there is no globalThis.localStorage on server side, you will either have to set {auth: { persistSession: false }} to use the internal in memory store, or pass a custom storage implementation.

b.) cretate a custom storage (see example above inMemoryStorageProvider()), prefill the session in the in memory storage (wherever you get the session data from) and pass it to supabase.auth with {auth: { storage: myCustomStorage };

Be careful to not share a single supabase client between different requests in a single process & multi user environment like a long running express server.

Edit: there is also a method to get the user (but no session initialization) with an access token: supbase.auth.getUser(accessToken).
https://github.com/supabase/gotrue-js/blob/rc/src/GoTrueClient.ts#L472

@bhvngt
Copy link

bhvngt commented Sep 29, 2022

@pixtron Thanks for the info. You are right. Commenting out signIn code and passing the access_token explicitly return null session.

b.) cretate a custom storage (see example above inMemoryStorageProvider()), prefill the session in the in memory storage > (wherever you get the session data from) and pass it to supabase.auth with {auth: { storage: myCustomStorage };

I can confirm that this approach works.

a.) call supabase.auth.setSession(refreshToken), with a valid refresh token. As there is no globalThis.localStorage on server side, you will either have to set {auth: { persistSession: false }} to use the internal in memory store, or pass a custom storage implementation.

This approach has one down side. When session gets initiated with refresh_token, as the name suggest, it gets invalidated and instead a new refresh_token becomes active. If the old token is shared by another client (say browser) then it can cause issues for those clients.

Be careful to not share a single supabase client between different requests in a single process & multi user environment like a long running express server.

Thanks for the input. In my app, my server is stateless for supabase client. So no sharing of client is happening.

Edit: there is also a method to get the user (but no session initialization) with an access token: supbase.auth.getUser(accessToken).

Interesting. Thanks for sharing this info.

For initiating server based user authenticated calls, here are two approaches that seems to be working with their respective trade-offs

With Global Authorization header

  • send access_token from the browser via cookie or any other means
  • use global authorization header to instantiate authenticated client
  • call getUser(access_token) to access user object.
  • call any other methods to access private data.

Here's the code snippet that follows the above approach

const access_token = "*********";
const globalOptions = {global: { headers: { Authorization: `Bearer ${access_token}` } }};
const sbClient = createClient(supabaseUrl, anonKey, { ...globalOptions});
sbClient.auth.onAuthStateChange((event) => {
  console.log(`sbClient event`, event); // does not get called ❌
});

const { data: getUserResult } = await sbClient.auth.getUser(access_token);
console.log(`getUserResult user`, getUserResult.user); // prints user object ✅
console.log(`sbClient.from("private_table")`, await sbClient.from("private_table").select("private_col")); // fetches private data for authenticated cllient ✅

One downside is that onAuthStateChange event does not gets triggered with this approach.

With In memory storage

In order to trigger onAuthStateChange trigger, I had to fill session information inside InMemoryStorage and pass that.
Here both access_token and refresh_token along with expires_at are required. I could skip passing access_token via global authorization header since it is already available in the storage. I could also skip passing access_token to getUser().

Here's the code snippet that takes this approach. Here, I could skip using global authentication header

function inMemoryStorageProvider(): SupportedStorage {
  const items = new Map();
  return ({
    getItem: (key: string) => items.get(key),
    setItem: (key: string, value: string) => {
      items.set(key, value);
    },
    removeItem: (key: string) => {
      items.delete(key);
    },
  }) as SupportedStorage;
}

const session = {
  access_token: "*******",
  refresh_token: "******",
  expires_at: 1664427836
};
const storage = inMemoryStorageProvider();
storage.setItem(`sb-${host}-auth-token`, JSON.stringify(session)); // host is either localhost or supabase subdomain depending on the env
const authOptions = {auth: { storage }}; 

const sbClient = createClient(supabaseUrl, anonKey, { ...authOptions, });
sbClient.auth.onAuthStateChange((event) => {
  console.log(`sbClient event`, event); // gets called ✅
});

const { data: getUserResult } = await sbClient.auth.getUser();
console.log(`getUserResult user`, getUserResult.user); // prints session object ✅
console.log(`sbClient.from("private_table")`, await sbClient.from("private_table").select("private_col")); 

Would have preferred a simpler or at least well documented method to make authenticated calls on the server side.

@aruke
Copy link

aruke commented Oct 1, 2022

@galizar @miguelespinoza I tried with persistSession: false and it returns null session. However, with InMemoryStorage implementation, I could see getSession() call returning session object. oAuthStateChange event gets triggered too.

I tried this on localhost, and when I try to log in with a user, the key used is sb-localhost-auth-token. This may be a very dumb question, but does this works with two or more users?

@silentworks
Copy link
Contributor

@galizar @miguelespinoza I tried with persistSession: false and it returns null session. However, with InMemoryStorage implementation, I could see getSession() call returning session object. oAuthStateChange event gets triggered too.

I tried this on localhost, and when I try to log in with a user, the key used is sb-localhost-auth-token. This may be a very dumb question, but does this works with two or more users?

@aruke As in two or more users on the same computer or on different computers? if the former then no as the token key would be the same for both user's and one would have to log out before the other could log in, but generally this is how sessions work in any application. If the latter then yes this will work for two or more users.

@aruke
Copy link

aruke commented Oct 12, 2022

@silentworks Thanks for the answer.

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

Successfully merging this pull request may close these issues.

None yet