search

dotkom / monoweb

Future version of Online, NTNU's informatics student association's website
https://web.online.ntnu.no/
MIT License
19 stars 2 forks source link

Fix auth0 synchronization not working #846

Closed henrikskog closed 8 months ago

henrikskog commented 8 months ago

Fixes a really ugly bug caused by bad code. I typed the domain-map-function argument as unknown for the auth0 repository user fetch function, and passed in the raw response from the auth0 sdk to map it to our domain user type. However, the sdk returns the data on the .data of the response. This is not catched by typescript, as the parameter is typed unknown. Note to self that unknown is bad

This made the call to Auth0Repository in the synchronization call fail, which means you could not sign up as you would never be able to sync down the user data to the user table, which is the one the rest of the application uses.

I also add some comments here

henrikskog commented 8 months ago

Interesting take. When changing the type from unknown the previous code won't compile though. Not sure how to write a failing test here after fixing the type. The test should also make sense to keep in the code base right?

junlarsen commented 8 months ago

Agree with @henrikhorluck that tests should be written

henrikskog commented 8 months ago

I have now written a e2e test for the sync service.

I have had quite some trouble mocking the auth0 sdk NGL. I attempted to mock just the ManagementClient from the auth0 module using vi.mock like the first example in the docs, but I ran into some weird issues. I could verify that client was being mocked, however inside the createServiceLayer call, the mocked users.get on the mock only returned undefined, even though the mock was being applied and it returned my mocked value outside the createServiceLayer call. Super weird and super annoying, but I won't be surprised if this is just a skill issue.

I think it may have been caused by this but I'm not sure.

I ended up using https://github.com/eratio08/vitest-mock-extended after reading this issue which was asking for just what I wanted.

Ideally this would work without vitest-mock-extended, and this is probably something that should be taken a look at. If one of you wants to take a look at it I would greatly appreciate it.

Seeing that I did not manage to just mock the export from auth0 I had to recreate the serviceLayer, so the test looks quite different that other e2e-tests that just use the normal createServiceLayer from core.

Note that I also cleaned up the handleUserSync function. Some of the sync logic was being applied in packages/auth/src/auth-options.ts, so I moved everything to the service.

vercel[bot] commented 8 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
invoicification ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 20, 2024 4:58pm
henrikskog commented 8 months ago

I'm going to merge this as it is fixing a breaking bug that blocks other devs. If you find anything you want me to change, LMK and I'll open a new PR for it @junlarsen @henrikhorluck