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

Add frontend code to merge users #3488

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add frontend code to merge users #3488

wants to merge 2 commits into from

Conversation

AbdBarho
Copy link
Collaborator

@AbdBarho AbdBarho commented Jun 17, 2023

Adds the ability to merge users in the frontend. Refs #3246

The code is ready (and I tested it locally) but is currently not used anywhere.

We need to update the way we communicate with the backend before we can use this. I am also unsure what would be the best place to run this code.

Should we allow the users to execute this? or should we just manually trigger it for all users and then forget about it. What about when we generate the JWT?

Maybe as a final step, I want to add some stuff to the account page to show the different accounts, and maybe make it easier to link/unlink accounts. #3073

logger.info({ message: "Merging user accounts", frontendId, accounts, backendIds });
let remainingIds: string[];

[backendId, ...remainingIds] = backendIds;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here I am just taking the first, maybe there is a better idea.

@AbdBarho AbdBarho marked this pull request as ready for review June 17, 2023 18:23
@notmd
Copy link
Collaborator

notmd commented Jun 17, 2023

Thanks but I am really against this decision. The backend should depend on the frontend, just like the inference is doing now.

@AbdBarho
Copy link
Collaborator Author

AbdBarho commented Jun 17, 2023

I do see the value of using the frontend id, since the frontend for the time being is the central service where all accounts are stored (without duplicates)

If we want to add discord bot support to the inference, we would still have the duplication problem, but now in the inference not in the data backend.

As long as any given user could have multiple accounts, we need a way to identify them across all services in our stack, which means we need a central service for auth.

In our current state, this service could be the frontend, but could also be the data backend, which does not make a big difference from an architectural point of view.

I could see us separating the auth part from the frontend to its own service, which is used by any component we want.
Or, of course, we can say the data backend is our auth service, and delegate the entire auth to there, and then change the frontend to basically map all auth calls directly to the backend.

I am very interested in your ideas.

@notmd
Copy link
Collaborator

notmd commented Jun 17, 2023

I agree with @AbdBarho to make the frontend as an auth service since it is the entry point of our app and we also can reuse the next auth logic.

Sorry, but I don’t understand what you mean by „the backend should depend on the frontend“. Could you please explain how you propose to solve the duplicate accounts problem?

I'm calling it the backend depend on the frontend because the frontend doesn't need to talk to the backend to authenticate the user. And the backend just simply trusts whatever the frontend sends since it already authenticated.

Here is my idea for the migration

  • Add a frontend_id column (unique and nullable) to the backend user table. The frontend will send a frontend_id header and backend will use that to identify user. The backend still can use the user.id for it internal usage . We still can use auth_method:id as a fallback while we migrate data. This approach will prevent creating duplicate account.
  • Write a script to dedup accounts and run it.
  • After migrate all data, make the frontend_id column is required. Enfore the frontend always send the frontend_id, remove the auth_method:id header, and remove the auth_method, username, and display_name from user table.

These steps can be simplified if we use the frontend_id as the id column in user table.

BTW you seem to imply that it is good that the inference server now depends on the frontend but IMO this is a big architectural mistake which should be fixed, e.g. to allow discord bots etc to also use the inference api..

The discord bot needs to communicate with the frontend for authentication. There is no way you can avoid this behavior in microservice.

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

Successfully merging this pull request may close these issues.

2 participants