Use trusted clients for inference auth #2278
Conversation
AbdBarho
force-pushed
the
trusted-client-auth
branch
from
07acb22
to
d54e453
Compare
AbdBarho
force-pushed
the
trusted-client-auth
branch
from
d54e453
to
0f4eb38
Compare
| // TODO: use api key from env | ||
| api_key: "0000", | ||
| client: "website", | ||
| user_id: jwt.sub, |
There was a problem hiding this comment.
Is this the correct uuid in the data collection backend?
There was a problem hiding this comment.
No, this is the id of the user in the frontend db.
After thinking about this, using the data backend id would introduce a lot of coupling between the services, we can still aggregate information in the website if needed
There was a problem hiding this comment.
No, this is the id of the user in the frontend db.
After thinking about this, using the data backend id would introduce a lot of coupling between the services, we can still aggregate information in the website if needed
Sure it's up to you. But I think having the data backend would allow the inference to communicate the data backend directly if needed Also using frontend id is kind of useless I think. It only avoids we have to call the data backend to get the id before we call the inference (which we can avoid by attaching it to the session or the JWT). Or maybe we can store both frontend and data backend id. IMO we have made a mistake by having the frontend DB. We use it because we are too lazy to reimplement all the NextAuth stuff but the trade-off is our infas is very complicated and fragmented. Every app has a custom user management system.
Also after this, I think we can deprecate all other sign methods of the inference (or just disable them for now) and use the frontend auth for other methods.
There was a problem hiding this comment.
updated to use the backend user id!
AbdBarho
requested review from
andreaskoepf,
melvinebenezer,
yk and
olliestanley
as code owners
| } catch (err) { | ||
| // if we get an error here, the user might not yet exist in the inference database, which is why we try to create | ||
| // user once (it won't do anything if the user already exists) and then retry the chat creation again. | ||
| // this is maybe not the cleanest solution, but otherwise we would have to sign up all users of the website | ||
| // to inference automatically, which is maybe an overkill | ||
| await this.inference_login(); | ||
| return create(); | ||
| } |
There was a problem hiding this comment.
Can we check for the http status here?
| @@ -6,7 +6,7 @@ const handler = withoutRole("banned", async (req, res, token) => { | |||
| const user = await getBackendUserCore(token.sub); | |||
| const oasstApiClient = createApiClientFromUser(user); | |||
| if (req.method === "GET") { | |||
| const tos_acceptance_date = await oasstApiClient.fetch_tos_acceptance(user); | |||
| const tos_acceptance_date = (await oasstApiClient.fetch_frontend_user(user)).tos_acceptance_date; | |||
There was a problem hiding this comment.
You can deconstruct the result here
Fixes #2285
Changes in the PR:
Changes here conflict with #2221, although not significantly, only the urls, maybe this could be addressed as part of #2286