search

NangoHQ / nango

A single API for all your integrations.
https://www.nango.dev
Other
4.82k stars 439 forks source link

fix(connect-ui): enforce session token integrations #3032

Closed bodinsamuel closed 4 days ago

bodinsamuel commented 4 days ago

Describe your changes

Fixes https://linear.app/nango/issue/NAN-1945/enforce-allowed-integrations-in-each-auth-endpoint

linear[bot] commented 4 days ago

NAN-1945 Enforce allowed_integrations in each auth endpoint

bodinsamuel commented 4 days ago

Would it make sense to factor this shared code out so future tweaks only change one file?

Yes and no, I am all for not repeating myself but sometimes it's handy. In that case factorizing means a lot of ifs and possible branching depending on the auth strategy. There are only a few lines that are strictly similar between all of them (e.g: this change, getting the provider/integrations). Basically, we could wrap the bootstrap of each auth and maybe the error handling.

TBonnin commented 4 days ago

sorry I fat fingered the close button. Meant to comment

TBonnin commented 4 days ago

This change looks very much like it belongs in a middleware. We would have to move the logCtx in RequestLocals but I am not sure it is possible.

bodinsamuel commented 4 days ago

This change looks very much like it belongs in a middleware. We would have to move the logCtx in RequestLocals but I am not sure it is possible.

This might, but requires a massive change to all auth controllers creating the logCtx beforehand and this makes the logic hard to follow (less self-contained) imo. I'll create a helper since both of you advocate for it

bodinsamuel commented 4 days ago

Cleaned with an helper and added some integrations tests 😇