Display only verified emails on notifications page

Validate confirmation of emails for user

Add validation to notification settings

Add different condition to validation

Add possibility to pick only email in dropdown

Fix user specs file

Remove unused method

Add changelog entry

Add cr remarks

Add cr remarks

app/assets/javascripts/profile/profile.js

@@ -40,7 +40,9 @@ export default class Profile {
   bindEvents() {
     $('.js-preferences-form').on('change.preference', 'input[type=radio]', this.submitForm);
     $('.js-group-notification-email').on('change', this.submitForm);
-    $('#user_notification_email').on('change', this.submitForm);
+    $('#user_notification_email').on('select2-selecting', event => {
+      setTimeout(this.submitForm.bind(event.currentTarget));
+    });
     $('#user_notified_of_own_activity').on('change', this.submitForm);
     this.form.on('submit', this.onSubmitForm);
   }

app/models/notification_setting.rb

@@ -14,6 +14,7 @@ class NotificationSetting < ApplicationRecord
   validates :user_id, uniqueness: { scope: [:source_type, :source_id],
                                     message: "already exists in source",
                                     allow_nil: true }
+  validate :owns_notification_email, if: :notification_email_changed?
 
   scope :for_groups, -> { where(source_type: 'Namespace') }
 
@@ -97,6 +98,13 @@ class NotificationSetting < ApplicationRecord
   def event_enabled?(event)
     respond_to?(event) && !!public_send(event) # rubocop:disable GitlabSecurity/PublicSend
   end
+
+  def owns_notification_email
+    return if user.temp_oauth_email?
+    return if notification_email.empty?
+
+    errors.add(:notification_email, _("is not an email you own")) unless user.verified_emails.include?(notification_email)
+  end
 end
 
 NotificationSetting.prepend_if_ee('EE::NotificationSetting')

app/models/user.rb

@@ -754,15 +754,15 @@ class User < ApplicationRecord
   end
 
   def owns_notification_email
-    return if temp_oauth_email?
+    return if new_record? || temp_oauth_email?
 
-    errors.add(:notification_email, _("is not an email you own")) unless all_emails.include?(notification_email)
+    errors.add(:notification_email, _("is not an email you own")) unless verified_emails.include?(notification_email)
   end
 
   def owns_public_email
     return if public_email.blank?
 
-    errors.add(:public_email, _("is not an email you own")) unless all_emails.include?(public_email)
+    errors.add(:public_email, _("is not an email you own")) unless verified_emails.include?(public_email)
   end
 
   def owns_commit_email
@@ -1200,18 +1200,20 @@ class User < ApplicationRecord
     all_emails
   end
 
-  def all_public_emails
-    all_emails(include_private_email: false)
-  end
-
-  def verified_emails
+  def verified_emails(include_private_email: true)
     verified_emails = []
     verified_emails << email if primary_email_verified?
-    verified_emails << private_commit_email
+    verified_emails << private_commit_email if include_private_email
     verified_emails.concat(emails.confirmed.pluck(:email))
     verified_emails
   end
 
+  def public_verified_emails
+    emails = verified_emails(include_private_email: false)
+    emails << email unless temp_oauth_email?
+    emails.uniq
+  end
+
   def any_email?(check_email)
     downcased = check_email.downcase
 

app/views/profiles/_email_settings.html.haml

@@ -5,7 +5,7 @@
 - help_text = email_change_disabled ? s_("Your account uses dedicated credentials for the \"%{group_name}\" group and can only be updated through SSO.") % { group_name: @user.managing_group.name } : read_only_help_text
 
 = form.text_field :email, required: true, class: 'input-lg', value: (@user.email unless @user.temp_oauth_email?), help: help_text.html_safe, readonly: readonly || email_change_disabled
-= form.select :public_email, options_for_select(@user.all_public_emails, selected: @user.public_email),
+= form.select :public_email, options_for_select(@user.public_verified_emails, selected: @user.public_email),
   { help: s_("Profiles|This email will be displayed on your public profile"), include_blank: s_("Profiles|Do not show on profile") },
   control_class: 'select2 input-lg', disabled: email_change_disabled
 - commit_email_link_url = help_page_path('user/profile/index', anchor: 'commit-email', target: '_blank')

app/views/profiles/notifications/_email_settings.html.haml

 - form = local_assigns.fetch(:form)
 .form-group
   = form.label :notification_email, class: "label-bold"
-  = form.select :notification_email, @user.all_public_emails, { include_blank: false }, class: "select2", disabled: local_assigns.fetch(:email_change_disabled, nil)
+  = form.select :notification_email, @user.public_verified_emails, { include_blank: false }, class: "select2", disabled: local_assigns.fetch(:email_change_disabled, nil)
   .help-block
     = local_assigns.fetch(:help_text, nil)

app/views/profiles/notifications/_group_settings.html.haml

@@ -13,4 +13,4 @@
 
   .table-section.section-30
     = form_for setting, url: profile_notifications_group_path(group), method: :put, html: { class: 'update-notifications' } do |f|
-      = f.select :notification_email, @user.all_public_emails, { include_blank: 'Global notification email' }, class: 'select2 js-group-notification-email'
+      = f.select :notification_email, @user.public_verified_emails, { include_blank: 'Global notification email' }, class: 'select2 js-group-notification-email'

changelogs/unreleased/security-25994-unverified-email-mitigation.yml

+---
+title: Display only verified emails on notifications and profile page
+merge_request:
+author:
+type: security

ee/spec/lib/gitlab/code_owners/entry_spec.rb

@@ -92,7 +92,7 @@ describe Gitlab::CodeOwners::Entry do
 
     it 'only adds users mentioned in the owner line' do
       other_user = create(:user)
-      other_user.emails
+      other_user.emails.load
 
       entry.add_matching_users_from([other_user, user])
 
@@ -109,7 +109,7 @@ describe Gitlab::CodeOwners::Entry do
 
     it 'adds users by primary email, case-insensitively' do
       second_user = create(:user, email: 'JANE@GITLAB.ORG')
-      second_user.emails
+      second_user.emails.load
 
       entry.add_matching_users_from([second_user, user])
 
@@ -119,7 +119,7 @@ describe Gitlab::CodeOwners::Entry do
     it 'adds users by secondary email, case-insensitively' do
       second_user = create(:user)
       second_user.emails.create!(email: 'JaNe@GitLab.org')
-      second_user.emails
+      second_user.emails.load
 
       entry.add_matching_users_from([second_user, user])
 

spec/controllers/profiles/notifications_controller_spec.rb

@@ -5,8 +5,8 @@ require 'spec_helper'
 describe Profiles::NotificationsController do
   let(:user) do
     create(:user) do |user|
-      user.emails.create(email: 'original@example.com')
-      user.emails.create(email: 'new@example.com')
+      user.emails.create(email: 'original@example.com', confirmed_at: Time.current)
+      user.emails.create(email: 'new@example.com', confirmed_at: Time.current)
       user.notification_email = 'original@example.com'
       user.save!
     end

spec/models/group_spec.rb

@@ -110,6 +110,11 @@ describe Group do
       let(:group_notification_email) { 'user+group@example.com' }
       let(:subgroup_notification_email) { 'user+subgroup@example.com' }
 
+      before do
+        create(:email, :confirmed, user: user, email: group_notification_email)
+        create(:email, :confirmed, user: user, email: subgroup_notification_email)
+      end
+
       subject { subgroup.notification_email_for(user) }
 
       context 'when both group notification emails are set' do

spec/models/notification_setting_spec.rb

@@ -48,6 +48,33 @@ RSpec.describe NotificationSetting do
         expect(notification_setting.reopen_merge_request).to eq(false)
       end
     end
+
+    context 'notification_email' do
+      let_it_be(:user) { create(:user) }
+      subject { described_class.new(source_id: 1, source_type: 'Project', user_id: user.id) }
+
+      it 'allows to change email to verified one' do
+        email = create(:email, :confirmed, user: user)
+
+        subject.update(notification_email: email.email)
+
+        expect(subject).to be_valid
+      end
+
+      it 'does not allow to change email to not verified one' do
+        email = create(:email, user: user)
+
+        subject.update(notification_email: email.email)
+
+        expect(subject).to be_invalid
+      end
+
+      it 'allows to change email to empty one' do
+        subject.update(notification_email: '')
+
+        expect(subject).to be_valid
+      end
+    end
   end
 
   describe '#for_projects' do

spec/models/user_spec.rb

@@ -310,7 +310,7 @@ describe User do
     end
 
     it_behaves_like 'an object with RFC3696 compliant email-formated attributes', :public_email, :notification_email do
-      subject { build(:user).tap { |user| user.emails << build(:email, email: email_value) } }
+      subject { create(:user).tap { |user| user.emails << build(:email, email: email_value, confirmed_at: Time.current) } }
     end
 
     describe '#commit_email' do
@@ -567,6 +567,32 @@ describe User do
           user = build(:user, email: "temp-email-for-oauth@example.com")
           expect(user).to be_valid
         end
+
+        it 'does not accept not verified emails' do
+          email = create(:email)
+          user = email.user
+          user.update(notification_email: email.email)
+
+          expect(user).to be_invalid
+        end
+      end
+
+      context 'owns_public_email' do
+        it 'accepts verified emails' do
+          email = create(:email, :confirmed, email: 'test@test.com')
+          user = email.user
+          user.update(public_email: email.email)
+
+          expect(user).to be_valid
+        end
+
+        it 'does not accept not verified emails' do
+          email = create(:email)
+          user = email.user
+          user.update(public_email: email.email)
+
+          expect(user).to be_invalid
+        end
       end
 
       context 'set_commit_email' do
@@ -2069,6 +2095,31 @@ describe User do
     end
   end
 
+  describe '#public_verified_emails' do
+    let(:user) { create(:user) }
+
+    it 'returns only confirmed public emails' do
+      email_confirmed = create :email, user: user, confirmed_at: Time.current
+      create :email, user: user
+
+      expect(user.public_verified_emails).to contain_exactly(
+        user.email,
+        email_confirmed.email
+      )
+    end
+
+    it 'returns confirmed public emails plus main user email when user is not confirmed' do
+      user = create(:user, confirmed_at: nil)
+      email_confirmed = create :email, user: user, confirmed_at: Time.current
+      create :email, user: user
+
+      expect(user.public_verified_emails).to contain_exactly(
+        user.email,
+        email_confirmed.email
+      )
+    end
+  end
+
   describe '#verified_email?' do
     let(:user) { create(:user) }
 
@@ -4199,9 +4250,10 @@ describe User do
           context 'when an ancestor has a level other than Global' do
             let(:ancestor) { create(:group) }
             let(:group) { create(:group, parent: ancestor) }
+            let(:email) { create(:email, :confirmed, email: 'ancestor@example.com', user: user) }
 
             before do
-              create(:notification_setting, user: user, source: ancestor, level: 'participating', notification_email: 'ancestor@example.com')
+              create(:notification_setting, user: user, source: ancestor, level: 'participating', notification_email: email.email)
             end
 
             it 'has the same level set' do
@@ -4226,10 +4278,12 @@ describe User do
             let(:grand_ancestor) { create(:group) }
             let(:ancestor) { create(:group, parent: grand_ancestor) }
             let(:group) { create(:group, parent: ancestor) }
+            let(:ancestor_email) { create(:email, :confirmed, email: 'ancestor@example.com', user: user) }
+            let(:grand_email) { create(:email, :confirmed, email: 'grand@example.com', user: user) }
 
             before do
-              create(:notification_setting, user: user, source: grand_ancestor, level: 'participating', notification_email: 'grand@example.com')
-              create(:notification_setting, user: user, source: ancestor, level: 'global', notification_email: 'ancestor@example.com')
+              create(:notification_setting, user: user, source: grand_ancestor, level: 'participating', notification_email: grand_email.email)
+              create(:notification_setting, user: user, source: ancestor, level: 'global', notification_email: ancestor_email.email)
             end
 
             it 'has the same email set' do
@@ -4267,7 +4321,7 @@ describe User do
     context 'when group has notification email set' do
       it 'returns group notification email' do
         group_notification_email = 'user+group@example.com'
-
+        create(:email, :confirmed, user: user, email: group_notification_email)
         create(:notification_setting, user: user, source: group, notification_email: group_notification_email)
 
         is_expected.to eq(group_notification_email)

spec/requests/api/notification_settings_spec.rb

@@ -19,7 +19,7 @@ describe API::NotificationSettings do
   end
 
   describe "PUT /notification_settings" do
-    let(:email) { create(:email, user: user) }
+    let(:email) { create(:email, :confirmed, user: user) }
 
     it "updates global notification settings for the current user" do
       put api("/notification_settings", user), params: { level: 'watch', notification_email: email.email }

spec/requests/openid_connect_spec.rb

@@ -9,15 +9,11 @@ describe 'OpenID Connect requests' do
       name: 'Alice',
       username: 'alice',
       email: 'private@example.com',
-      emails: [public_email],
-      public_email: public_email.email,
       website_url: 'https://example.com',
       avatar: fixture_file_upload('spec/fixtures/dk.png')
     )
   end
 
-  let(:public_email) { build :email, email: 'public@example.com' }
-
   let(:access_grant) { create :oauth_access_grant, application: application, resource_owner_id: user.id }
   let(:access_token) { create :oauth_access_token, application: application, resource_owner_id: user.id }
 
@@ -37,7 +33,7 @@ describe 'OpenID Connect requests' do
       'name'           => 'Alice',
       'nickname'       => 'alice',
       'email'          => 'public@example.com',
-      'email_verified' => false,
+      'email_verified' => true,
       'website'        => 'https://example.com',
       'profile'        => 'http://localhost/alice',
       'picture'        => "http://localhost/uploads/-/system/user/avatar/#{user.id}/dk.png",
@@ -62,6 +58,11 @@ describe 'OpenID Connect requests' do
     get '/oauth/userinfo', params: {}, headers: { 'Authorization' => "Bearer #{access_token.token}" }
   end
 
+  before do
+    email = create(:email, :confirmed, email: 'public@example.com', user: user)
+    user.update!(public_email: email.email)
+  end
+
   context 'Application without OpenID scope' do
     let(:application) { create :oauth_application, scopes: 'api' }
 
@@ -123,7 +124,7 @@ describe 'OpenID Connect requests' do
       end
 
       it 'has false in email_verified claim' do
-        expect(json_response['email_verified']).to eq(false)
+        expect(json_response['email_verified']).to eq(true)
       end
     end
 

spec/requests/profiles/notifications_controller_spec.rb

@@ -5,8 +5,8 @@ require 'spec_helper'
 describe 'view user notifications' do
   let(:user) do
     create(:user) do |user|
-      user.emails.create(email: 'original@example.com')
-      user.emails.create(email: 'new@example.com')
+      user.emails.create(email: 'original@example.com', confirmed_at: Time.current)
+      user.emails.create(email: 'new@example.com', confirmed_at: Time.current)
       user.notification_email = 'original@example.com'
       user.save!
     end

spec/services/notification_service_spec.rb

@@ -2457,6 +2457,8 @@ describe NotificationService, :mailer do
               group = create(:group)
 
               project.update(group: group)
+
+              create(:email, :confirmed, user: u_custom_notification_enabled, email: group_notification_email)
               create(:notification_setting, user: u_custom_notification_enabled, source: group, notification_email: group_notification_email)
             end
 
@@ -2491,6 +2493,7 @@ describe NotificationService, :mailer do
               group = create(:group)
 
               project.update(group: group)
+              create(:email, :confirmed, user: u_member, email: group_notification_email)
               create(:notification_setting, user: u_member, source: group, notification_email: group_notification_email)
             end
 
@@ -2584,6 +2587,7 @@ describe NotificationService, :mailer do
               group = create(:group)
 
               project.update(group: group)
+              create(:email, :confirmed, user: u_member, email: group_notification_email)
               create(:notification_setting, user: u_member, source: group, notification_email: group_notification_email)
             end
 

spec/support/shared_examples/mailers/notify_shared_examples.rb

@@ -28,6 +28,7 @@ RSpec.shared_examples 'an email sent to a user' do
     it 'is sent to user\'s group notification email' do
       group_notification_email = 'user+group@example.com'
 
+      create(:email, :confirmed, user: recipient, email: group_notification_email)
       create(:notification_setting, user: recipient, source: group, notification_email: group_notification_email)
 
       expect(subject).to deliver_to(group_notification_email)