Unconfirm wrongfully verified email records
Related issues:
- Original MR: https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/639#note_367554344
- https://gitlab.com/gitlab-org/gitlab/-/issues/218446
This MR takes care of cleaning up wrongfully confirmed email records.
The MR does the following:
- Iterates over all Email records in a background migration.
- For each batch, look for wrongfully verified email addresses.
- Unconfirm the email address and the primary email address (since you can easily switch primary emails).
- Send the confirmation email for both emails.
- Send an extra explanation email.
Notes:
- Note 1: We only do this for active users, we skip bots.
- Note 2: We might send the informing emails multiple times to the same user. In case they have more than one email records affected.
- Note 3: I had to modify the secondary email confirm template to avoid confusion and not just handle the case where user adds an email. See the change in app/views/devise/mailer/_confirmation_instructions_secondary.html.haml
Explanation email:
The BG migration updates the confirmed_at
and the confirmation_token
attributes in batches in order to avoid large volume of record updates.
I verified in devise that a custom-generated confirmation_token
can be confirmed (there is no extra validation).
email = Email.find(1) # where confirmation_token is set by us
Email.confirm_by_token(email.confirmation_token)
email.reload.confirmed? # returns true
Generating confirmation tokens:
Tokens need to be unique since there is a unique index on the column. For generating the confirmation token, we'll use the md5
helper method in PG. We add several attributes in order to prevent users "guessing" the token.
md5(emails.id::varchar || emails.created_at || users.encrypted_password || '#{Integer(Time.now.to_i)}') as md5_string
- id - unique
- emails.created_at
- encrypted_password - column is not accessible from the app at all
- current unix epoch - in case there is an md5 collision (very unlikely), the next retry will have different input string
Queries
We iterate over all the emails when triggering a BG job which will create 400+ jobs.
- Batch size: 1K
- Interval: 5 minutes
- Records to be updated: 100K * 2 (100K email records and about 100K user records)
- Total runtime: Iterating 236714 email records, ~20 hours
We use low batch size and high(er) interval because we're sending emails.
Query for finding affected users (batch that contains large volume of items to be migrated):
SELECT "emails".* FROM "emails"
INNER JOIN "users" ON "users"."id" = "emails"."user_id"
WHERE
"users"."state" = 'active' AND
"users"."user_type" IS NULL AND
"emails"."id" BETWEEN 425000 AND 426000
AND (emails.confirmed_at IS NOT NULL)
AND (emails.confirmed_at = users.confirmed_at)
AND (emails.email <> users.email)
Note: I tried to add a specialized index on users, but it seems that the planner prefers the pkey.
I used the following query to find the "heaviest" batch:
SELECT emails.id / 1000, count(*)
FROM users
INNER JOIN emails ON emails.user_id=users.id
WHERE
users.state IN ('active')
AND (users.user_type IS NULL OR users.user_type IN (NULL, 6, 4))
AND users.confirmed_at IS NOT NULL
AND emails.confirmed_at IS NOT NULL
AND emails.confirmed_at=users.confirmed_at
AND users.email <> emails.email
group by emails.id / 1000
order by 2 desc
limit 10;
Email update query:
WITH md5_strings as (
SELECT emails.id as email_id, md5(emails.id::varchar || emails.created_at || users.encrypted_password || '1592547886') as md5_string
FROM "emails"
INNER JOIN "users" ON "users"."id" = "emails"."user_id"
WHERE
"users"."state" = 'active' AND
"users"."user_type" IS NULL AND
"emails"."id" BETWEEN 425000 AND 426000 AND
(emails.confirmed_at IS NOT NULL) AND
(emails.confirmed_at = users.confirmed_at) AND
(emails.email <> users.email)
)
UPDATE "emails"
SET confirmed_at = NULL, confirmation_token=md5_strings.md5_string
FROM md5_strings
WHERE id = md5_strings.email_id
User update query
UPDATE "users"
SET confirmed_at = NULL,
confirmation_token=md5(users.id::varchar || users.created_at || users.encrypted_password || '1592547886')
WHERE "users"."id" IN (worst case: 1K user ids)
Worst case: updating 1k user records. Finds records by primary key.
Verification
Snippet to generate "bad" data in your local environment:
user1 = FactoryBot.create(:user, username: 'emailverificationtest1', email: 'emailverificationtest1@test.com', confirmed_at: Time.now)
user2 = FactoryBot.create(:user, username: 'emailverificationtest2', email: 'emailverificationtest2@test.com', confirmed_at: Time.now)
[user1, user2].each do |user|
email = user.emails.create!(email: "#{user.username}.secondemail@test.com")
email.update(confirmed_at: user.confirmed_at)
end
After executing the snippet, you can run rake db:migrate
. The mailer job output should show up in the development.log
file (assuming that sidekiq is running).
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
I tested the email sending locally and verified that the emails we send confirmation links.
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team