incubrain / astrotribe

A global network of astronomers helping to inspire and educate the next generation.
https://astrotribe.vercel.app
6 stars 2 forks source link

building/testing auth flow #142

Open aayu5hgit opened 1 year ago

aayu5hgit commented 1 year ago

Here we're building/testing/discussing the supabase auth flow.

This will be developed further in isolation here using a raw implementation of supabase-js because the nuxt module hides all of the logic and auth is a key part of our app that we need to understand.

Flows handled:

aayu5hgit commented 1 year ago

@Drew-Macgibbon

(bug): Improper login page reload

At the time, when auth is OFF, everything works normal. But when auth is ON and login is clicked, there are times when it doesn't showcase the changes such as

A forced page reload has to be made to see the changes.

aayu5hgit commented 1 year ago

@Drew-Macgibbon

(bug): Update Many returns null

When the auth and SMTP are toggled ON and Update Many is clicked, console shows up the following error

image

I couldn't understand the actual working of this functionality, so can you explain me just the rough flow of things taking place within this functionality?

Drew-Macgibbon commented 1 year ago

@Drew-Macgibbon

(bug): Improper login page reload

At the time, when auth is OFF, everything works normal. But when auth is ON and login is clicked, there are times when it doesn't showcase the changes such as

  • The navbar still shows login and not logged in
  • Page doesn't gets updated

A forced page reload has to be made to see the changes.

I think I fixed this in develop, but we lost power so I couldn't merge before I left.

I'll send you the updated env tomorrow so you can work on the dev branch.

Drew-Macgibbon commented 1 year ago

@Drew-Macgibbon

(bug): Update Many returns null

When the auth and SMTP are toggled ON and Update Many is clicked, console shows up the following error

image

I couldn't understand the actual working of this functionality, so can you explain me just the rough flow of things taking place within this functionality?

This is expected behaviour, but a shoddy implementation by me. I was using data from private-data which is in the git ignore since it includes emails of users etc.

I just commented out the code and set users to an empty array since the code is only for use locally. And errors if the import isn't valid.

Although it is an admin route, so should return an error that you are not authorised. As I'm pretty sure I set isAdmin to false in the server/middleware/auth

aayu5hgit commented 1 year ago

@Drew-Macgibbon

(bug): Login creds & supabase auth

While logging in on the live website, some errors are being encountered:

Drew-Macgibbon commented 1 year ago

@Drew-Macgibbon

(bug): Login creds & supabase auth

While logging in on the live website, some errors are being encountered:

  • Post (400); there is something going wrong with supabase auth image

  • After clicking submit button, the toast (which is used in profile updation page) is raising up. So we need to make another component which only handles the submission.

Ah yes the feedback needs to be updated.

Check the supabase auth logs for more info on the error. You can also resend magic links from the auth tab in supabase.

If the createError is an object convert it to a template literal for logging the error.message

Drew-Macgibbon commented 1 year ago

@aayu5hgit list all the bugs you're facing so I can help give you direction.

aayu5hgit commented 1 year ago

@Drew-Macgibbon

(bug): Auth API Error

When I tried to login locally, it wasn't happening, then I registered again and the verification email didn't arrive and the following error is being displayed in the console (if I try to enter credentials)

image

NOTE: Scenario was tested when

Drew-Macgibbon commented 1 year ago

@Drew-Macgibbon

(bug): Auth API Error

When I tried to login locally, it wasn't happening, then I registered again and the verification email didn't arrive and the following error is being displayed in the console (if I try to enter credentials)

image

NOTE: Scenario was tested when

  • [x] Auth was ON
  • [x] SMTP was ON

OK, just had a quick look at the supabase logs and am seeing permission denied for schema public

Which means we have a grant_permission error. The authenticator is the Postgres user without the permission. As far as I am aware the authenticator should only be accessing the auth schema. So I think the issue stems from using the auth.users.id as the primary key for public.users.id. This is likely confusing the authenticator

to test this theory I will add a new primary key and move the public.users.id to auth_id. Although I've just discovered that the auth.users.id is NOT immutable, which will require some further thought. I guess a trigger that updates auth_id if it's changed should work...

Drew-Macgibbon commented 1 year ago

@aayu5hgit ok I have implemented the changes. I also added a trigger to watch the auth.users.id and update the row accordingly. This needed to be handled because if the user updates their email (obviously something we want to allow) it will change their auth.id.

You will need to update the prisma endpoints to check the auth_id instead of the id

hopefully this resolves the errors, I'll leave the testing in your capable hands.

aayu5hgit commented 1 year ago

@Drew-Macgibbon okay, so do I need to update the whole ‘prisma schema‘ too on the codebase?

Drew-Macgibbon commented 1 year ago

thought of an edge case, if the user updates their email, the id and email will not match anything in the public.users table, so a new row would be created. fixed by using the OLD SQL selector:

CREATE OR REPLACE FUNCTION public.create_user_in_public() RETURNS TRIGGER AS $$
BEGIN
    -- Update the row with NEW values if a corresponding row exists with OLD.id
    UPDATE public.users SET auth_id = NEW.id, email = NEW.email WHERE auth_id = OLD.id;

    -- Upsert logic (handles updates where the OLD.id wasn't found)
    INSERT INTO public.users (auth_id, email)
    VALUES (NEW.id, NEW.email)
    ON CONFLICT (auth_id)
    DO UPDATE SET email = excluded.email, auth_id = excluded.auth_id;

    RETURN NEW;
END;
$$ LANGUAGE plpgsql;
aayu5hgit commented 1 year ago

@Drew-Macgibbon after going through the above snippet, I think that in the ON CONFLICT clause of the UPSERT logic, we don't need to explicitly set auth_id = excluded.auth_id because it's the same value as the one in the conflict target (param).

    INSERT INTO public.users (auth_id, email)
    VALUES (NEW.id, NEW.email)
    ON CONFLICT (auth_id)
    DO UPDATE SET email = excluded.email;

I might not be correct, but thought of discussing with you.😅

Drew-Macgibbon commented 1 year ago

@aayu5hgit you're correct. Will update it.

aayu5hgit commented 1 year ago

Drew-Macgibbon I tried updating the prisma endpoints to check the auth_id instead of id, but it isn't resolving the error. I updated the code as:

const user = await client.users.findFirst({
    where: {
      auth_id: id
    },
    include: {
      roles: true
    }
  })
// Do I need to change this 
const { id } = event.context.params

// To this
const authId = parseInt(req.params.auth_id)

// Or this
const { authId } = event.context.params
Drew-Macgibbon commented 1 year ago

@aayu5hgit it depends on where the id param is coming from. If it's coming from the auth.user then it will be id if it's coming from public.user then it's auth_id. Both are strings.

I'll take a quick look at the code now and see if I can spot anything. Also, if you want help with errors make sure to post the error.

I also reccomend copy pasting the error into chatGPT with some context for immediate help.

aayu5hgit commented 1 year ago

@Drew-Macgibbon yes, I'll do the required and post things to maintain a record.

Also, I did change the public.user part's id with auth_id but I don't see any change in the error.

Drew-Macgibbon commented 1 year ago

@aayu5hgit what is the error?

I've tried to perform a clean npm i but the network keeps dropping, so not much I can do from my end.

Maybe update the zod schemas if you haven't already.

UserBasicSchema should have these:

id: z.number(),
auth_id: z.string().optional(),
aayu5hgit commented 1 year ago

@Drew-Macgibbon not working, can you look into it? I think there's something wrong with Supabase.

aayu5hgit commented 1 year ago

@Drew-Macgibbon Finally working, wohoo! Tested with a new email and it worked. Check out the recording for ref.

https://github.com/incubrain/astrotribe/assets/86314754/ed261363-8a02-49c3-ab84-ef1031acd4f0

I think there is a problem where the email that has already been verified is giving invalid credential error when passed the same in the env as testing email.