Commit fd9c17ba authored by Magdalena Frankiewicz's avatar Magdalena Frankiewicz Committed by Mayra Cabrera

Add primary email to emails upon confirmation

parent 9ecf5834
......@@ -29,7 +29,7 @@ class Email < ApplicationRecord
end
def unique_email
self.errors.add(:email, 'has already been taken') if User.exists?(email: self.email)
self.errors.add(:email, 'has already been taken') if primary_email_of_another_user?
end
def validate_email_format
......@@ -40,4 +40,14 @@ class Email < ApplicationRecord
def update_invalid_gpg_signatures
user.update_invalid_gpg_signatures if confirmed?
end
def user_primary_email?
email.casecmp?(user.email)
end
private
def primary_email_of_another_user?
User.where(email: email).where.not(id: user_id).exists?
end
end
......@@ -274,14 +274,21 @@ class User < ApplicationRecord
after_update :username_changed_hook, if: :saved_change_to_username?
after_destroy :post_destroy_hook
after_destroy :remove_key_cache
after_create :add_primary_email_to_emails!, if: :confirmed?
after_commit(on: :update) do
if previous_changes.key?('email')
# Grab previous_email here since previous_changes changes after
# #update_emails_with_primary_email and #update_notification_email are called
# Add the old primary email to Emails if not added already - this should be removed
# after the background migration for MR https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70872/ has completed,
# as the primary email is now added to Emails upon confirmation
# Issue to remove that: https://gitlab.com/gitlab-org/gitlab/-/issues/344134
previous_confirmed_at = previous_changes.key?('confirmed_at') ? previous_changes['confirmed_at'][0] : confirmed_at
previous_email = previous_changes[:email][0]
if previous_confirmed_at && !emails.exists?(email: previous_email)
# rubocop: disable CodeReuse/ServiceClass
Emails::CreateService.new(self, user: self, email: previous_email).execute(confirmed_at: previous_confirmed_at)
# rubocop: enable CodeReuse/ServiceClass
end
update_emails_with_primary_email(previous_confirmed_at, previous_email)
update_invalid_gpg_signatures
end
end
......@@ -935,6 +942,8 @@ class User < ApplicationRecord
end
def unique_email
return if errors.added?(:email, _('has already been taken'))
if !emails.exists?(email: email) && Email.exists?(email: email)
errors.add(:email, _('has already been taken'))
end
......@@ -963,24 +972,6 @@ class User < ApplicationRecord
skip_reconfirmation! if emails.confirmed.where(email: self.email).any?
end
# Note: the use of the Emails services will cause `saves` on the user object, running
# through the callbacks again and can have side effects, such as the `previous_changes`
# hash and `_was` variables getting munged.
# By using an `after_commit` instead of `after_update`, we avoid the recursive callback
# scenario, though it then requires us to use the `previous_changes` hash
# rubocop: disable CodeReuse/ServiceClass
def update_emails_with_primary_email(previous_confirmed_at, previous_email)
primary_email_record = emails.find_by(email: email)
Emails::DestroyService.new(self, user: self).execute(primary_email_record) if primary_email_record
# the original primary email was confirmed, and we want that to carry over. We don't
# have access to the original confirmation values at this point, so just set confirmed_at
Emails::CreateService.new(self, user: self, email: previous_email).execute(confirmed_at: previous_confirmed_at)
update_columns(confirmed_at: primary_email_record.confirmed_at) if primary_email_record&.confirmed_at
end
# rubocop: enable CodeReuse/ServiceClass
def update_invalid_gpg_signatures
gpg_keys.each(&:update_invalid_gpg_signatures)
end
......@@ -1389,7 +1380,7 @@ class User < ApplicationRecord
all_emails << email unless temp_oauth_email?
all_emails << private_commit_email if include_private_email
all_emails.concat(emails.map(&:email))
all_emails
all_emails.uniq
end
def verified_emails(include_private_email: true)
......@@ -1397,7 +1388,7 @@ class User < ApplicationRecord
verified_emails << email if primary_email_verified?
verified_emails << private_commit_email if include_private_email
verified_emails.concat(emails.confirmed.pluck(:email))
verified_emails
verified_emails.uniq
end
def public_verified_emails
......@@ -1978,6 +1969,25 @@ class User < ApplicationRecord
ci_job_token_scope.present?
end
# override from Devise::Confirmable
#
# Add the primary email to user.emails (or confirm it if it was already
# present) when the primary email is confirmed.
def confirm(*args)
saved = super(*args)
return false unless saved
email_to_confirm = self.emails.find_by(email: self.email)
if email_to_confirm.present?
email_to_confirm.confirm(*args)
else
add_primary_email_to_emails!
end
saved
end
protected
# override, from Devise::Validatable
......@@ -2018,6 +2028,12 @@ class User < ApplicationRecord
'en'
end
# rubocop: disable CodeReuse/ServiceClass
def add_primary_email_to_emails!
Emails::CreateService.new(self, user: self, email: self.email).execute(confirmed_at: self.confirmed_at)
end
# rubocop: enable CodeReuse/ServiceClass
def notification_email_verified
return if notification_email.blank? || temp_oauth_email?
......
......@@ -3,6 +3,8 @@
module Emails
class DestroyService < ::Emails::BaseService
def execute(email)
raise StandardError, 'Cannot delete primary email' if email.user_primary_email?
email.destroy && update_secondary_emails!(email.email)
end
......
......@@ -44,7 +44,7 @@
%span.badge.badge-muted.badge-pill.gl-badge.badge-info= s_('Profiles|Public email')
- if @primary_email === current_user.notification_email_or_default
%span.badge.badge-muted.badge-pill.gl-badge.badge-info= s_('Profiles|Default notification email')
- @emails.each do |email|
- @emails.reject(&:user_primary_email?).each do |email|
%li{ data: { qa_selector: 'email_row_content' } }
= render partial: 'shared/email_with_badge', locals: { email: email.email, verified: email.confirmed? }
%span.float-right
......
# frozen_string_literal: true
class ScheduleAddPrimaryEmailToEmailsIfUserConfirmed < Gitlab::Database::Migration[1.0]
INTERVAL = 2.minutes.to_i
BATCH_SIZE = 10_000
MIGRATION = 'AddPrimaryEmailToEmailsIfUserConfirmed'
disable_ddl_transaction!
class User < ActiveRecord::Base
include ::EachBatch
self.table_name = 'users'
self.inheritance_column = :_type_disabled
end
def up
queue_background_migration_jobs_by_range_at_intervals(
User,
MIGRATION,
INTERVAL,
batch_size: BATCH_SIZE,
track_jobs: true
)
end
def down
# intentionally blank
end
end
9cefd32c003a68752f257973a983f77215b02011b7ca792de06c0e92c2462745
\ No newline at end of file
......@@ -102,6 +102,8 @@ RSpec.describe Admin::UsersController do
end
describe "POST #impersonate" do
let_it_be(:user) { create(:user) }
before do
stub_licensed_features(extended_audit_events: true)
end
......
......@@ -96,7 +96,7 @@ RSpec.describe SessionsController, :geo do
context 'when OTP authentication fails' do
it_behaves_like 'an auditable failed authentication' do
let(:user) { create(:user, :two_factor) }
let_it_be(:user) { create(:user, :two_factor) }
let(:operation) { authenticate_2fa(otp_attempt: 'invalid', otp_user_id: user.id) }
let(:method) { 'OTP' }
end
......@@ -108,7 +108,7 @@ RSpec.describe SessionsController, :geo do
end
it_behaves_like 'an auditable failed authentication' do
let(:user) { create(:user, :two_factor_via_u2f) }
let_it_be(:user) { create(:user, :two_factor_via_u2f) }
let(:operation) { authenticate_2fa(device_response: 'invalid', otp_user_id: user.id) }
let(:method) { 'U2F' }
end
......@@ -122,7 +122,7 @@ RSpec.describe SessionsController, :geo do
end
it_behaves_like 'an auditable failed authentication' do
let(:user) { create(:user, :two_factor_via_webauthn) }
let_it_be(:user) { create(:user, :two_factor_via_webauthn) }
let(:operation) { authenticate_2fa(device_response: 'invalid', otp_user_id: user.id) }
let(:method) { 'WebAuthn' }
end
......
......@@ -9,7 +9,7 @@ RSpec.describe PasswordsController do
stub_licensed_features(extended_audit_events: true)
end
let(:user) { create(:user) }
let_it_be(:user) { create(:user) }
subject { post :create, params: { user: { email: user.email } } }
......
......@@ -7,7 +7,7 @@ RSpec.describe Audit::Changes do
describe '.audit_changes' do
let(:current_user) { create(:user, name: 'Mickey Mouse') }
let(:user) { create(:user, name: 'Donald Duck') }
let!(:user) { create(:user, name: 'Donald Duck') }
let(:options) { { model: user } }
subject(:audit!) { foo_instance.audit_changes(:name, options) }
......
......@@ -4,7 +4,8 @@ require 'spec_helper'
RSpec.describe EE::Audit::ProjectChangesAuditor do
describe '.audit_changes' do
let(:user) { create(:user) }
let_it_be(:user) { create(:user) }
let(:project) do
create(
:project,
......
......@@ -15,7 +15,7 @@ RSpec.describe Gitlab::UsageDataNonSqlMetrics do
described_class.uncached_data
end
expect(recorder.count).to eq(55)
expect(recorder.count).to eq(57)
end
end
end
......@@ -52,7 +52,7 @@ RSpec.describe API::AuditEvents do
end
context 'when authenticated, as an admin' do
let(:admin) { create(:admin) }
let_it_be(:admin) { create(:admin) }
context 'audit events feature is not available' do
it_behaves_like '403 response' do
......
......@@ -28,7 +28,7 @@ RSpec.describe API::Repositories do
context 'when unauthenticated', 'and project is public' do
it_behaves_like 'an auditable and successful request' do
let(:project) { create(:project, :public, :repository) }
let_it_be(:project) { create(:project, :public, :repository) }
let(:current_user) { nil }
end
end
......
......@@ -62,6 +62,9 @@ RSpec.describe API::Users do
end
context 'extended audit events' do
let_it_be(:user) { create(:user) }
let_it_be(:admin) { create(:admin) }
before do
stub_licensed_features(extended_audit_events: true)
end
......
......@@ -164,9 +164,14 @@ RSpec.describe SmartcardController, type: :request do
end
it 'logs audit event' do
audit_event_service = instance_double(AuditEventService)
audit_event_service = instance_double(::AuditEventService)
expect(AuditEventService).to(
# Creating a confirmed user also creates an email corresponding to the user primary email
expect(::AuditEventService).to(
receive(:new)
.with(instance_of(User), instance_of(User), action: :create).and_call_original)
expect(::AuditEventService).to(
receive(:new)
.with(instance_of(User), instance_of(User), with: auth_method, ip_address: '127.0.0.1')
.and_return(audit_event_service))
......
......@@ -5,7 +5,8 @@ require 'spec_helper'
RSpec.describe ::Applications::CreateService do
include TestRequestHelpers
let(:user) { create(:user) }
let_it_be(:user) { create(:user) }
let(:group) { create(:group) }
let(:params) { attributes_for(:application) }
......
......@@ -3,12 +3,12 @@
require 'spec_helper'
RSpec.describe Users::ApproveService do
let(:current_user) { create(:admin) }
let_it_be(:current_user) { create(:admin) }
subject(:service) { described_class.new(current_user) }
describe '#execute', :enable_admin_mode do
let(:user) { create(:user, :blocked_pending_approval) }
let!(:user) { create(:user, :blocked_pending_approval) }
subject(:operation) { service.execute(user) }
......
......@@ -3,12 +3,12 @@
require 'spec_helper'
RSpec.describe Users::BlockService do
let(:current_user) { create(:admin) }
let_it_be(:current_user) { create(:admin) }
subject(:service) { described_class.new(current_user) }
describe '#execute' do
let(:user) { create(:user) }
let!(:user) { create(:user) }
subject(:operation) { service.execute(user) }
......
......@@ -3,7 +3,8 @@
require 'spec_helper'
RSpec.describe Users::CreateService do
let(:current_user) { create(:admin) }
let_it_be(:current_user) { create(:admin) }
let(:params) do
{
name: 'John Doe',
......
......@@ -3,12 +3,12 @@
require 'spec_helper'
RSpec.describe Users::DestroyService do
let(:current_user) { create(:admin) }
let_it_be(:current_user) { create(:admin) }
subject(:service) { described_class.new(current_user) }
describe '#execute' do
let(:user) { create(:user) }
let!(:user) { create(:user) }
subject(:operation) { service.execute(user) }
......
......@@ -32,7 +32,7 @@ RSpec.describe Users::RejectService do
end
context 'when user does not have permission to reject another user' do
let(:current_user) { create(:user) }
let_it_be(:current_user) { create(:user) }
it 'does not log any audit event' do
expect { reject_user }.not_to change { AuditEvent.count }
......
......@@ -97,12 +97,14 @@ RSpec.describe Users::UpdateService do
context 'audit events' do
context 'licensed' do
let_it_be_with_reload(:user) { create(:user) }
before do
stub_licensed_features(admin_audit_log: true)
end
context 'updating administrator status' do
let_it_be(:admin_user) { create(:admin) }
let_it_be_with_reload(:admin_user) { create(:admin) }
it 'logs making a user an administrator' do
expect do
......@@ -113,8 +115,9 @@ RSpec.describe Users::UpdateService do
end
it 'logs making an administrator a user' do
admin = create(:admin)
expect do
update_user_as(admin_user, create(:admin), admin: false)
update_user_as(admin_user, admin, admin: false)
end.to change { AuditEvent.count }.by(1)
expect(AuditEvent.last.present.action).to eq('Changed admin status from true to false')
......@@ -170,7 +173,8 @@ RSpec.describe Users::UpdateService do
end
context 'with an admin user' do
let!(:admin_user) { create(:admin) }
let_it_be_with_reload(:admin_user) { create(:admin) }
let(:service) { described_class.new(admin_user, ActionController::Parameters.new(params).permit!) }
let(:params) do
{ name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass' }
......
......@@ -3,7 +3,8 @@
require 'spec_helper'
RSpec.describe Emails::CreateService do
let(:user) { create(:user) }
let_it_be(:user) { create(:user) }
let(:opts) { { email: 'new@email.com', user: user } }
subject(:service) { described_class.new(user, opts) }
......
......@@ -3,8 +3,9 @@
require 'spec_helper'
RSpec.describe Keys::CreateService do
let(:admin) { create(:admin) }
let(:user) { create(:user) }
let_it_be(:admin) { create(:admin) }
let_it_be(:user) { create(:user) }
let(:params) { attributes_for(:key).merge(user: user) }
subject { described_class.new(admin, params) }
......
......@@ -335,6 +335,7 @@ RSpec.describe Projects::CreateService, '#execute' do
context 'audit events' do
include_examples 'audit event logging' do
let_it_be(:user) { create(:user) }
let(:operation) { create_project(user, opts) }
let(:fail_condition!) do
allow(Gitlab::VisibilityLevel).to receive(:allowed_for?).and_return(false)
......
......@@ -17,6 +17,8 @@ RSpec.describe Projects::GroupLinks::CreateService, '#execute' do
context 'audit events' do
include_examples 'audit event logging' do
let_it_be(:user) { create :user }
let_it_be(:project) { create :project }
let(:operation) { create_group_link(user, project, group, opts) }
let(:fail_condition!) do
create(:project_group_link, project: project, group: group)
......
......@@ -123,6 +123,8 @@ RSpec.describe Projects::UpdateService, '#execute' do
end
context 'audit events' do
let_it_be(:user) { create(:user) }
let(:audit_event_params) do
{
author_id: user.id,
......
# frozen_string_literal: true
module Gitlab
module BackgroundMigration
# Add user primary email to emails table if confirmed
class AddPrimaryEmailToEmailsIfUserConfirmed
INNER_BATCH_SIZE = 1_000
# Stubbed class to access the User table
class User < ActiveRecord::Base
include ::EachBatch
self.table_name = 'users'
self.inheritance_column = :_type_disabled
scope :confirmed, -> { where.not(confirmed_at: nil) }
has_many :emails
end
# Stubbed class to access the Emails table
class Email < ActiveRecord::Base
self.table_name = 'emails'
self.inheritance_column = :_type_disabled
belongs_to :user
end
def perform(start_id, end_id)
User.confirmed.where(id: start_id..end_id).select(:id, :email, :confirmed_at).each_batch(of: INNER_BATCH_SIZE) do |users|
current_time = Time.now.utc
attributes = users.map do |user|
{
user_id: user.id,
email: user.email,
confirmed_at: user.confirmed_at,
created_at: current_time,
updated_at: current_time
}
end
Email.insert_all(attributes)
end
mark_job_as_succeeded(start_id, end_id)
end
private
def mark_job_as_succeeded(*arguments)
Gitlab::Database::BackgroundMigrationJob.mark_all_as_succeeded(
'AddPrimaryEmailToEmailsIfUserConfirmed',
arguments
)
end
end
end
end
......@@ -69,6 +69,8 @@ module Gitlab
user_pb.emails << build_email(user.email, user.confirmed?)
user.emails.each do |email|
next if email.user_primary_email?
user_pb.emails << build_email(email.email, email.confirmed?)
end
......
......@@ -3,8 +3,7 @@
require 'spec_helper'
RSpec.describe Oauth::AuthorizationsController do
let(:user) { create(:user, confirmed_at: confirmed_at) }
let(:confirmed_at) { 1.hour.ago }
let(:user) { create(:user) }
let!(:application) { create(:oauth_application, scopes: 'api read_user', redirect_uri: 'http://example.com') }
let(:params) do
{
......@@ -40,7 +39,7 @@ RSpec.describe Oauth::AuthorizationsController do
end
context 'when the user is unconfirmed' do
let(:confirmed_at) { nil }
let(:user) { create(:user, :unconfirmed) }
it 'returns 200 and renders error view' do
subject
......@@ -73,8 +72,6 @@ RSpec.describe Oauth::AuthorizationsController do
include_examples "Implicit grant can't be used in confidential application"
context 'when the user is confirmed' do
let(:confirmed_at) { 1.hour.ago }
context 'when there is already an access token for the application with a matching scope' do
before do
scopes = Doorkeeper::OAuth::Scopes.from_string('api')
......
......@@ -4,6 +4,7 @@ require 'spec_helper'
RSpec.describe 'Profile > Emails' do
let(:user) { create(:user) }
let(:other_user) { create(:user) }
before do
sign_in(user)
......@@ -23,15 +24,25 @@ RSpec.describe 'Profile > Emails' do
expect(page).to have_content('Resend confirmation email')
end
it 'does not add a duplicate email' do
fill_in('Email', with: user.email)
it 'does not add an email that is the primary email of another user' do
fill_in('Email', with: other_user.email)
click_button('Add email address')
email = user.emails.find_by(email: user.email)
email = user.emails.find_by(email: other_user.email)
expect(email).to be_nil
expect(page).to have_content('Email has already been taken')
end
it 'adds an email that is the primary email of the same user' do
fill_in('Email', with: user.email)
click_button('Add email address')
email = user.emails.find_by(email: user.email)
expect(email).to be_present
expect(page).to have_content("#{user.email} Verified")
expect(page).not_to have_content("#{user.email} Unverified")
end
it 'does not add an invalid email' do
fill_in('Email', with: 'test.@example.com')
click_button('Add email address')
......
......@@ -4,6 +4,7 @@ require 'spec_helper'
RSpec.describe 'User manages emails' do
let(:user) { create(:user) }
let(:other_user) { create(:user) }
before do
sign_in(user)
......@@ -11,7 +12,7 @@ RSpec.describe 'User manages emails' do
visit(profile_emails_path)
end
it "shows user's emails" do
it "shows user's emails", :aggregate_failures do
expect(page).to have_content(user.email)
user.emails.each do |email|
......@@ -19,7 +20,7 @@ RSpec.describe 'User manages emails' do
end
end
it 'adds an email' do
it 'adds an email', :aggregate_failures do
fill_in('email_email', with: 'my@email.com')
click_button('Add')
......@@ -34,21 +35,21 @@ RSpec.describe 'User manages emails' do
end
end
it 'does not add a duplicate email' do
fill_in('email_email', with: user.email)
it 'does not add an email that is the primary email of another user', :aggregate_failures do
fill_in('email_email', with: other_user.email)
click_button('Add')
email = user.emails.find_by(email: user.email)
email = user.emails.find_by(email: other_user.email)
expect(email).to be_nil
expect(page).to have_content(user.email)
expect(page).to have_content('Email has already been taken')
user.emails.each do |email|
expect(page).to have_content(email.email)
end
end
it 'removes an email' do
it 'removes an email', :aggregate_failures do
fill_in('email_email', with: 'my@email.com')
click_button('Add')
......
......@@ -11,6 +11,7 @@ RSpec.describe 'GPG signed commits' do
perform_enqueued_jobs do
create :gpg_key, key: GpgHelpers::User1.public_key, user: user
user.reload # necessary to reload the association with gpg_keys
end
visit project_commit_path(project, ref)
......
......@@ -383,7 +383,7 @@ RSpec.describe UsersHelper do
end
context 'when `user.unconfirmed_email` is set' do
let(:user) { create(:user, unconfirmed_email: 'foo@bar.com') }
let(:user) { create(:user, :unconfirmed, unconfirmed_email: 'foo@bar.com') }
it 'sets `modal_attributes.messageHtml` correctly' do
expect(Gitlab::Json.parse(confirm_user_data[:modal_attributes])['messageHtml']).to eq('This user has an unconfirmed email address (foo@bar.com). You may force a confirmation.')
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::BackgroundMigration::AddPrimaryEmailToEmailsIfUserConfirmed do
let(:users) { table(:users) }
let(:emails) { table(:emails) }
let!(:unconfirmed_user) { users.create!(name: 'unconfirmed', email: 'unconfirmed@example.com', confirmed_at: nil, projects_limit: 100) }
let!(:confirmed_user_1) { users.create!(name: 'confirmed-1', email: 'confirmed-1@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
let!(:confirmed_user_2) { users.create!(name: 'confirmed-2', email: 'confirmed-2@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
let!(:email) { emails.create!(user_id: confirmed_user_1.id, email: 'confirmed-1@example.com', confirmed_at: 1.day.ago) }
let(:perform) { described_class.new.perform(users.first.id, users.last.id) }
it 'adds the primary email of confirmed users to Emails, unless already added', :aggregate_failures do
expect(emails.where(email: [unconfirmed_user.email, confirmed_user_2.email])).to be_empty
expect { perform }.not_to raise_error
expect(emails.where(email: unconfirmed_user.email).count).to eq(0)
expect(emails.where(email: confirmed_user_1.email, user_id: confirmed_user_1.id).count).to eq(1)
expect(emails.where(email: confirmed_user_2.email, user_id: confirmed_user_2.id).count).to eq(1)
email_2 = emails.find_by(email: confirmed_user_2.email, user_id: confirmed_user_2.id)
expect(email_2.confirmed_at).to eq(confirmed_user_2.reload.confirmed_at)
end
it 'sets timestamps on the created Emails' do
perform
email_2 = emails.find_by(email: confirmed_user_2.email, user_id: confirmed_user_2.id)
expect(email_2.created_at).not_to be_nil
expect(email_2.updated_at).not_to be_nil
end
context 'when a range of IDs is specified' do
let!(:confirmed_user_3) { users.create!(name: 'confirmed-3', email: 'confirmed-3@example.com', confirmed_at: 1.hour.ago, projects_limit: 100) }
let!(:confirmed_user_4) { users.create!(name: 'confirmed-4', email: 'confirmed-4@example.com', confirmed_at: 1.hour.ago, projects_limit: 100) }
it 'only acts on the specified range of IDs', :aggregate_failures do
expect do
described_class.new.perform(confirmed_user_2.id, confirmed_user_3.id)
end.to change { Email.count }.by(2)
expect(emails.where(email: confirmed_user_4.email).count).to eq(0)
end
end
end
......@@ -140,6 +140,8 @@ RSpec.describe Gitlab::Gpg::InvalidGpgSignatureUpdater do
key: GpgHelpers::User1.public_key,
user: user
user.reload # necessary to reload the association with gpg_keys
expect(invalid_gpg_signature.reload.verification_status).to eq 'unverified_key'
# InvalidGpgSignatureUpdater is called by the after_update hook
......
......@@ -82,7 +82,7 @@ RSpec.describe Gitlab::Spamcheck::Client do
end
end
describe '#build_user_proto_buf', :aggregate_failures do
describe '#build_user_protobuf', :aggregate_failures do
it 'builds the expected protobuf object' do
user_pb = described_class.new.send(:build_user_protobuf, user)
expect(user_pb.username).to eq user.username
......
......@@ -12,7 +12,7 @@ RSpec.describe Gitlab::X509::Signature do
end
shared_examples "a verified signature" do
let_it_be(:user) { create(:user, email: X509Helpers::User1.certificate_email) }
let!(:user) { create(:user, email: X509Helpers::User1.certificate_email) }
subject(:signature) do
described_class.new(
......@@ -30,10 +30,12 @@ RSpec.describe Gitlab::X509::Signature do
expect(signature.verification_status).to eq(:verified)
end
it "returns an unverified signature if the email matches but isn't confirmed" do
user.update!(confirmed_at: nil)
context "if the email matches but isn't confirmed" do
let!(:user) { create(:user, :unconfirmed, email: X509Helpers::User1.certificate_email) }
expect(signature.verification_status).to eq(:unverified)
it "returns an unverified signature" do
expect(signature.verification_status).to eq(:unverified)
end
end
it 'returns an unverified signature if email does not match' do
......@@ -297,7 +299,7 @@ RSpec.describe Gitlab::X509::Signature do
end
context 'verified signature' do
let_it_be(:user) { create(:user, email: X509Helpers::User1.certificate_email) }
let_it_be(:user) { create(:user, :unconfirmed, email: X509Helpers::User1.certificate_email) }
subject(:signature) do
described_class.new(
......@@ -316,52 +318,56 @@ RSpec.describe Gitlab::X509::Signature do
allow(OpenSSL::X509::Store).to receive(:new).and_return(store)
end
it 'returns a verified signature if email does match' do
expect(signature.x509_certificate).to have_attributes(certificate_attributes)
expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
expect(signature.verified_signature).to be_truthy
expect(signature.verification_status).to eq(:verified)
end
context 'when user email is confirmed' do
before_all do
user.confirm
end
it "returns an unverified signature if the email matches but isn't confirmed" do
user.update!(confirmed_at: nil)
it 'returns a verified signature if email does match', :ggregate_failures do
expect(signature.x509_certificate).to have_attributes(certificate_attributes)
expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
expect(signature.verified_signature).to be_truthy
expect(signature.verification_status).to eq(:verified)
end
expect(signature.verification_status).to eq(:unverified)
end
it 'returns an unverified signature if email does not match', :aggregate_failures do
signature = described_class.new(
X509Helpers::User1.signed_tag_signature,
X509Helpers::User1.signed_tag_base_data,
"gitlab@example.com",
X509Helpers::User1.signed_commit_time
)
expect(signature.x509_certificate).to have_attributes(certificate_attributes)
expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
expect(signature.verified_signature).to be_truthy
expect(signature.verification_status).to eq(:unverified)
end
it 'returns an unverified signature if email does not match' do
signature = described_class.new(
X509Helpers::User1.signed_tag_signature,
X509Helpers::User1.signed_tag_base_data,
"gitlab@example.com",
X509Helpers::User1.signed_commit_time
)
it 'returns an unverified signature if email does match and time is wrong', :aggregate_failures do
signature = described_class.new(
X509Helpers::User1.signed_tag_signature,
X509Helpers::User1.signed_tag_base_data,
X509Helpers::User1.certificate_email,
Time.new(2020, 2, 22)
)
expect(signature.x509_certificate).to have_attributes(certificate_attributes)
expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
expect(signature.verified_signature).to be_falsey
expect(signature.verification_status).to eq(:unverified)
end
expect(signature.x509_certificate).to have_attributes(certificate_attributes)
expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
expect(signature.verified_signature).to be_truthy
expect(signature.verification_status).to eq(:unverified)
end
it 'returns an unverified signature if certificate is revoked' do
expect(signature.verification_status).to eq(:verified)
it 'returns an unverified signature if email does match and time is wrong' do
signature = described_class.new(
X509Helpers::User1.signed_tag_signature,
X509Helpers::User1.signed_tag_base_data,
X509Helpers::User1.certificate_email,
Time.new(2020, 2, 22)
)
signature.x509_certificate.revoked!
expect(signature.x509_certificate).to have_attributes(certificate_attributes)
expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes)
expect(signature.verified_signature).to be_falsey
expect(signature.verification_status).to eq(:unverified)
expect(signature.verification_status).to eq(:unverified)
end
end
it 'returns an unverified signature if certificate is revoked' do
expect(signature.verification_status).to eq(:verified)
signature.x509_certificate.revoked!
it 'returns an unverified signature if the email matches but is not confirmed' do
expect(signature.verification_status).to eq(:unverified)
end
end
......
# frozen_string_literal: true
require 'spec_helper'
require_migration!
RSpec.describe ScheduleAddPrimaryEmailToEmailsIfUserConfirmed, :sidekiq do
let(:migration) { described_class.new }
let(:users) { table(:users) }
let!(:user_1) { users.create!(name: 'confirmed-user-1', email: 'confirmed-1@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
let!(:user_2) { users.create!(name: 'confirmed-user-2', email: 'confirmed-2@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
let!(:user_3) { users.create!(name: 'confirmed-user-3', email: 'confirmed-3@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
let!(:user_4) { users.create!(name: 'confirmed-user-4', email: 'confirmed-4@example.com', confirmed_at: 1.day.ago, projects_limit: 100) }
before do
stub_const("#{described_class.name}::BATCH_SIZE", 2)
stub_const("#{described_class.name}::INTERVAL", 2.minutes.to_i)
end
it 'schedules addition of primary email to emails in delayed batches' do
Sidekiq::Testing.fake! do
freeze_time do
migration.up
expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, user_1.id, user_2.id)
expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, user_3.id, user_4.id)
expect(BackgroundMigrationWorker.jobs.size).to eq(2)
end
end
end
end
......@@ -13,6 +13,15 @@ RSpec.describe Email do
it_behaves_like 'an object with RFC3696 compliant email-formatted attributes', :email do
subject { build(:email) }
end
context 'when the email conflicts with the primary email of a different user' do
let(:user) { create(:user) }
let(:email) { build(:email, email: user.email) }
it 'is invalid' do
expect(email).to be_invalid
end
end
end
it 'normalize email value' do
......@@ -33,7 +42,7 @@ RSpec.describe Email do
end
describe 'scopes' do
let(:user) { create(:user) }
let(:user) { create(:user, :unconfirmed) }
it 'scopes confirmed emails' do
create(:email, :confirmed, user: user)
......
......@@ -1123,7 +1123,7 @@ RSpec.describe User do
end
describe 'after commit hook' do
describe '#update_emails_with_primary_email' do
describe 'when the primary email is updated' do
before do
@user = create(:user, email: 'primary@example.com').tap do |user|
user.skip_reconfirmation!
......@@ -1132,13 +1132,7 @@ RSpec.describe User do
@user.reload
end
it 'gets called when email updated' do
expect(@user).to receive(:update_emails_with_primary_email)
@user.update!(email: 'new_primary@example.com')
end
it 'adds old primary to secondary emails when secondary is a new email' do
it 'keeps old primary to secondary emails when secondary is a new email' do
@user.update!(email: 'new_primary@example.com')
@user.reload
......@@ -1146,22 +1140,6 @@ RSpec.describe User do
expect(@user.emails.pluck(:email)).to match_array([@secondary.email, 'primary@example.com'])
end
it 'adds old primary to secondary emails if secondary is becoming a primary' do
@user.update!(email: @secondary.email)
@user.reload
expect(@user.emails.count).to eq 1
expect(@user.emails.first.email).to eq 'primary@example.com'
end
it 'transfers old confirmation values into new secondary' do
@user.update!(email: @secondary.email)
@user.reload
expect(@user.emails.count).to eq 1
expect(@user.emails.first.confirmed_at).not_to eq nil
end
context 'when the first email was unconfirmed and the second email gets confirmed' do
let(:user) { create(:user, :unconfirmed, email: 'should-be-unconfirmed@test.com') }
......@@ -1178,11 +1156,8 @@ RSpec.describe User do
expect(user).to be_confirmed
end
it 'keeps the unconfirmed email unconfirmed' do
email = user.emails.first
expect(email.email).to eq('should-be-unconfirmed@test.com')
expect(email).not_to be_confirmed
it 'does not add unconfirmed email to secondary' do
expect(user.emails.map(&:email)).not_to include('should-be-unconfirmed@test.com')
end
it 'has only one email association' do
......@@ -1244,7 +1219,7 @@ RSpec.describe User do
expect(user.email).to eq(confirmed_email)
end
it 'moves the old email' do
it 'keeps the old email' do
email = user.reload.emails.first
expect(email.email).to eq(old_confirmed_email)
......@@ -1499,7 +1474,7 @@ RSpec.describe User do
allow_any_instance_of(ApplicationSetting).to receive(:send_user_confirmation_email).and_return(true)
end
let(:user) { create(:user, confirmed_at: nil, unconfirmed_email: 'test@gitlab.com') }
let(:user) { create(:user, :unconfirmed, unconfirmed_email: 'test@gitlab.com') }
it 'returns unconfirmed' do
expect(user.confirmed?).to be_falsey
......@@ -1509,6 +1484,22 @@ RSpec.describe User do
user.confirm
expect(user.confirmed?).to be_truthy
end
it 'adds the confirmed primary email to emails' do
expect(user.emails.confirmed.map(&:email)).not_to include(user.email)
user.confirm
expect(user.emails.confirmed.map(&:email)).to include(user.email)
end
end
context 'if the user is created with confirmed_at set to a time' do
let!(:user) { create(:user, email: 'test@gitlab.com', confirmed_at: Time.now.utc) }
it 'adds the confirmed primary email to emails upon creation' do
expect(user.emails.confirmed.map(&:email)).to include(user.email)
end
end
describe '#to_reference' do
......@@ -2216,7 +2207,7 @@ RSpec.describe User do
end
context 'primary email not confirmed' do
let(:user) { create(:user, confirmed_at: nil) }
let(:user) { create(:user, :unconfirmed) }
let!(:email) { create(:email, :confirmed, user: user, email: 'foo@example.com') }
it 'finds user respecting the confirmed flag' do
......@@ -2231,7 +2222,7 @@ RSpec.describe User do
end
it 'returns nil when user is not confirmed' do
user = create(:user, email: 'foo@example.com', confirmed_at: nil)
user = create(:user, :unconfirmed, email: 'foo@example.com')
expect(described_class.find_by_any_email(user.email, confirmed: false)).to eq(user)
expect(described_class.find_by_any_email(user.email, confirmed: true)).to be_nil
......@@ -6011,7 +6002,7 @@ RSpec.describe User do
subject { user.confirmation_required_on_sign_in? }
context 'when user is confirmed' do
let(:user) { build_stubbed(:user) }
let(:user) { create(:user) }
it 'is falsey' do
expect(user.confirmed?).to be_truthy
......
......@@ -1906,7 +1906,8 @@ RSpec.describe API::Users do
expect(response).to have_gitlab_http_status(:ok)
expect(response).to include_pagination_headers
expect(json_response).to be_an Array
expect(json_response.first['email']).to eq(email.email)
expect(json_response.first['email']).to eq(user.email)
expect(json_response.second['email']).to eq(email.email)
end
it "returns a 404 for invalid ID" do
......@@ -2488,7 +2489,8 @@ RSpec.describe API::Users do
expect(response).to have_gitlab_http_status(:ok)
expect(response).to include_pagination_headers
expect(json_response).to be_an Array
expect(json_response.first["email"]).to eq(email.email)
expect(json_response.first['email']).to eq(user.email)
expect(json_response.second['email']).to eq(email.email)
end
context "scopes" do
......
......@@ -305,7 +305,7 @@ RSpec.describe UsersController do
context 'user with keys' do
let!(:gpg_key) { create(:gpg_key, user: user) }
let!(:another_gpg_key) { create(:another_gpg_key, user: user) }
let!(:another_gpg_key) { create(:another_gpg_key, user: user.reload) }
shared_examples_for 'renders all verified GPG keys' do
it 'renders all verified keys separated with a new line with text/plain content type' do
......
......@@ -3,7 +3,8 @@
require 'spec_helper'
RSpec.describe Emails::CreateService do
let(:user) { create(:user) }
let_it_be(:user) { create(:user) }
let(:opts) { { email: 'new@email.com', user: user } }
subject(:service) { described_class.new(user, opts) }
......@@ -22,7 +23,7 @@ RSpec.describe Emails::CreateService do
it 'has the right user association' do
service.execute
expect(user.emails).to eq(Email.where(opts))
expect(user.emails).to include(Email.find_by(opts))
end
end
end
......@@ -15,5 +15,15 @@ RSpec.describe Emails::DestroyService do
expect(user.emails).not_to include(email)
expect(response).to be true
end
context 'when it corresponds to the user primary email' do
let(:email) { user.emails.find_by!(email: user.email) }
it 'does not remove the email and raises an exception' do
expect { service.execute(email) }.to raise_error(StandardError, 'Cannot delete primary email')
expect(user.emails).to include(email)
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment