GitLab links unverified secondary emails to user accounts. It leads to security issues and allows users to pretend to be owners of some emails that do not belong to them. This should be fixed by #356665 (closed).
This issue is a follow-up.
GitLab reserves unverified secondary emails. So for such emails, it is not possible to
create a new account
add it as a secondary email and verify for another user account
A malicious user could add emails of potential customer (admin|john|etc)@xyz.org as unverified secondary emails to their account. When such customers start using GitLab.com, they will not be able to create an account for their employees.
And bug related to the mentioned security issue directly: To accept an invite sent to an email, which is an unverified secondary email, either creating a GitLab account with this email or adding it to an existing account as a secondary verified email is needed. Currently, it is not possible to do so.
This is a significant change and there might be some difficulties due to the complexity of the current implementation. My optimistic weight is 3 based on the implementation details mentioned in the description.
This is closely related to #352514 (closed), and there might be other issues that I can't find right now. I think this issue has been known for quite some time but we haven't managed to resolve it yet.
Thank you, Drew. Yes, it is closely related but should be addressed separately. I suggested a potential solution to that issue there: #352514 (comment 1027488682)
The customer was trying to create a new user with a specific email address but they kept seeing a Email is already being taken error. When searching the Users admin UI, they were unable to locate any users with that email address. I've supplied them with the workaround.
@sabinecarpenter We just finished our planning last week and it looks like this one did not make Deliverable so it likely won't make it. I'll proactively move it to the next milestone.
@sbouly The issues are picked up for assignment by any engineer on the team as they wrap up existing items. There is a risk that this work may not be completed in 16.9 but it's still in the queue to be addressed next.
I believe this issue proposes that we don't reserve unverified secondary emails.
@chantalskye and I were considering this in light of our new delete unverified user account implementation...
Would it be easier to:
On secondary email verification have a message that says something like if the email is unverified after 3 days, it will be deleted.
Delete the unverified secondary email after 3 days if it is not verified.
This would serve to reduce the impact of allowing multiple unverified secondary emails as in the implementation detail since it would tidy up non-verified emails pretty quickly... (Scenario this deals with is what if 2 users with unverified secondary email are somehow able to verify their emails at very different times).
@alvin That is interesting suggestion. But I would consider this as a plan B if the initial proposed solution for this issue is not feasible from implementation perspective or by some other reason.
Why? The suggestion to delete unverified secondary emails after 3 days does not protect if malicious user created automation that would re-create the unverified secondary email right after it is auto-deleted. That way malicious user would still keep email they don't own as reserved in the GitLab system.
I was thinking about what happens after this GitLab should not reserve unverified secondary emails till they are verified. What happens to the duplicates when one user verifies the secondary email?
What happens to the duplicates when one user verifies the secondary email?
We could keep unverified duplicates, but the system should not allow confirming any those emails while confirmed secondary email exists - that is why it is important to add this constraint on database layer to guarantee data integrity. Otherwise we should go with your suggestion.
The problem is that GitLab stores emails in two places users.email and emails table. Easing the existing database constraint database for emails table as suggested might be unsafe for the existing implementation
Example: User adds secondary email to their account as unverified. Then they create account for that email - user account with unconfirmed primary email. Then user go back to the account where they added unverified secondary email and confirm it. Then user confirm newly created account. We would get the same email confirmed twice one as a secondary email and another one as a primary email. That would lead to data integrity issue as per our implementation.
@alvin I updated the issue with your proposed solution.
We have emails table where we store users' emails. Currently when user adds email to their account, the email becomes reserved. Malicious users could reserve email in the gitlab system they don't own. We want to prevent that.
There is CREATE UNIQUE INDEX index_emails_on_email ON emails USING btree (email); Is it possible to limit this UNIQUE constraint to confirmed emails only - WHERE "emails"."confirmed_at" IS NOT NULL?
Alternative solution is discussed in #367823 (comment 1792224022), to delete the unverified secondary email after 3 days if it is not verified.
@adil.farrukh@bdenkovych@alvin Do you think this is release post worthy? This is a typebug but the MR is a typefeature - I could see how customers would want to know about this, but only the admin/group owner persona I'm torn.
@hsutor I think we can include a release post even though this is a typebug because it'd be a good quality of life update for admins and support team (and informs users about how secondary emails are now handled).
@hsutor Thanks for asking this . I was just about to ask the same . I agree with Adil, I think it is worth mentioning this change since we fix the "bug" by providing new application behavior.
The MR that implements this feature is still on review. I hope it will get into %17.0, but there is a risk that it might not since the due date of %17.0 is very soon.