fablabbcn / smartcitizen-api

The Smart Citizen Engine
https://developer.smartcitizen.me
GNU Affero General Public License v3.0
10 stars 4 forks source link

Two users with same email? #294

Closed oscgonfer closed 3 months ago

oscgonfer commented 5 months ago

So I have been reported an issue in which the log-in of a user gets redirected to another one on the onboarding. The steps of the onboarding are:

  1. Input email
  2. Ask for password (with the corresponding username)
  3. ...

So in Step 2, a weird case happened, in which two users shared the same email, which obviously shouldn't happen. This hasn't happened with our admin intervention, so there should is some bug that allowed two users to share email...

timcowlishaw commented 5 months ago

it appears we have quite a lot of users with the same email, and some of them were created quite recently (2023-07).

So a few things we need to do:

oscgonfer commented 5 months ago

So one of the potential rootcauses for this is that we have no check on the web/profile/edit.

pral2a commented 5 months ago

In principle the standard user creation method should validate that on backend.

Also the onboarding app that should check that in a combination of front-end / backend. However, it might be something fails there? 🍝

timcowlishaw commented 5 months ago

I think we've tracked it down - there's validation in onboarding, but users can edit their email in the web app with no validation. It'll be simple enough to fix that for future edits, but sorting out the existing emails with multiple accounts might take a little bit of untangling (nothing impossible though)!

On Tue, 16 Jan 2024 at 17:40, Pral2a @.***> wrote:

In principle the standard user creation method should validate that on backend.

Also the onboarding app that should check that in a combination of front-end / backend. However, it might be something fails there? 🍝

— Reply to this email directly, view it on GitHub https://github.com/fablabbcn/smartcitizen-api/issues/294#issuecomment-1894211605, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABBN634INH34V7ELJYPOLYO23RBAVCNFSM6AAAAABBYSDQGCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJUGIYTCNRQGU . You are receiving this because you were assigned.Message ID: @.***>

oscgonfer commented 4 months ago

Since this issue is widespread:

  1. We will unify all accounts that have the same email onto a single one. All devices will be moved over to that account
  2. Users can use password recovery if they can't access it (we will announce it on the forum)
  3. Users with no email will be deleted, including those that have devices. However, the devices owned by those users with no email, will be moved to user: 6556 until reclaimed

Then, we won't allow for create or update operations where email == null or not-unique.

timcowlishaw commented 3 months ago

Done in 51a3a984fda! we'll need to run the two rake tasks after deploying to production.

timcowlishaw commented 3 months ago

Procedure for running this in live: Bring down everything apart from the db container temporarily

docker exec -t smartcitizen-api-db-1 pg_dumpall -c -U postgres > dump_pre_user_deduplication.sql
docker compose run --no-deps app bundle exec bin/rake users:remove_null_emails | tee remove_null_emails.log
docker compose run --no-deps app bundle exec bin/rake users:deduplicate_emails | tee deduplicate_emails.log
docker exec -t smartcitizen-api-db-1 pg_dumpall -c -U postgres > dump_post_user_deduplication.sql

After each rake task, verify that the provided counts match up!

timcowlishaw commented 3 months ago

Opening this until we run the tasks on production

timcowlishaw commented 3 months ago

Now run succesfully on production!