Profile updates from providers

5 files changed, 307 insertions, 24 deletions

diff --git a/spec/controllers/profiles_controller_spec.rb b/spec/controllers/profiles_controller_spec.rb
index 9d60dab12d1..b52b63e05a4 100644
--- a/spec/controllers/profiles_controller_spec.rb
+++ b/spec/controllers/profiles_controller_spec.rb
@@ -16,7 +16,11 @@ describe ProfilesController do
     end
 
     it "ignores an email update from a user with an external email address" do
-      ldap_user = create(:omniauth_user, external_email: true)
+      stub_omniauth_setting(sync_profile_from_provider: ['ldap'])
+      stub_omniauth_setting(sync_profile_attributes: true)
+
+      ldap_user = create(:omniauth_user)
+      ldap_user.create_user_synced_attributes_metadata(provider: 'ldap', name_synced: true, email_synced: true)
       sign_in(ldap_user)
 
       put :update,
@@ -27,5 +31,24 @@ describe ProfilesController do
       expect(response.status).to eq(302)
       expect(ldap_user.unconfirmed_email).not_to eq('john@gmail.com')
     end
+
+    it "ignores an email and name update but allows a location update from a user with external email and name, but not external location" do
+      stub_omniauth_setting(sync_profile_from_provider: ['ldap'])
+      stub_omniauth_setting(sync_profile_attributes: true)
+
+      ldap_user = create(:omniauth_user, name: 'Alex')
+      ldap_user.create_user_synced_attributes_metadata(provider: 'ldap', name_synced: true, email_synced: true, location_synced: false)
+      sign_in(ldap_user)
+
+      put :update,
+          user: { email: "john@gmail.com", name: "John", location: "City, Country" }
+
+      ldap_user.reload
+
+      expect(response.status).to eq(302)
+      expect(ldap_user.unconfirmed_email).not_to eq('john@gmail.com')
+      expect(ldap_user.name).not_to eq('John')
+      expect(ldap_user.location).to eq('City, Country')
+    end
   end
 end
diff --git a/spec/helpers/profiles_helper_spec.rb b/spec/helpers/profiles_helper_spec.rb
index b33b3f3a228..c1d0614c79e 100644
--- a/spec/helpers/profiles_helper_spec.rb
+++ b/spec/helpers/profiles_helper_spec.rb
@@ -6,22 +6,41 @@ describe ProfilesHelper do
       user = create(:user)
       allow(helper).to receive(:current_user).and_return(user)
 
-      expect(helper.email_provider_label).to be_nil
+      expect(helper.attribute_provider_label(:email)).to be_nil
     end
 
-    it "returns omniauth provider label for users with external email" do
+    it "returns omniauth provider label for users with external attributes" do
+      stub_omniauth_setting(sync_profile_from_provider: ['cas3'])
+      stub_omniauth_setting(sync_profile_attributes: true)
       stub_cas_omniauth_provider
-      cas_user = create(:omniauth_user, provider: 'cas3', external_email: true, email_provider: 'cas3')
+      cas_user = create(:omniauth_user, provider: 'cas3')
+      cas_user.create_user_synced_attributes_metadata(provider: 'cas3', name_synced: true, email_synced: true, location_synced: true)
       allow(helper).to receive(:current_user).and_return(cas_user)
 
-      expect(helper.email_provider_label).to eq('CAS')
+      expect(helper.attribute_provider_label(:email)).to eq('CAS')
+      expect(helper.attribute_provider_label(:name)).to eq('CAS')
+      expect(helper.attribute_provider_label(:location)).to eq('CAS')
+    end
+
+    it "returns the correct omniauth provider label for users with some external attributes" do
+      stub_omniauth_setting(sync_profile_from_provider: ['cas3'])
+      stub_omniauth_setting(sync_profile_attributes: true)
+      stub_cas_omniauth_provider
+      cas_user = create(:omniauth_user, provider: 'cas3')
+      cas_user.create_user_synced_attributes_metadata(provider: 'cas3', name_synced: false, email_synced: true, location_synced: false)
+      allow(helper).to receive(:current_user).and_return(cas_user)
+
+      expect(helper.attribute_provider_label(:name)).to be_nil
+      expect(helper.attribute_provider_label(:email)).to eq('CAS')
+      expect(helper.attribute_provider_label(:location)).to be_nil
     end
 
     it "returns 'LDAP' for users with external email but no email provider" do
-      ldap_user = create(:omniauth_user, external_email: true)
+      ldap_user = create(:omniauth_user)
+      ldap_user.create_user_synced_attributes_metadata(email_synced: true)
       allow(helper).to receive(:current_user).and_return(ldap_user)
 
-      expect(helper.email_provider_label).to eq('LDAP')
+      expect(helper.attribute_provider_label(:email)).to eq('LDAP')
     end
   end
 
diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb
index 5100a5a609e..6a6e465cea2 100644
--- a/spec/lib/gitlab/ldap/user_spec.rb
+++ b/spec/lib/gitlab/ldap/user_spec.rb
@@ -37,7 +37,8 @@ describe Gitlab::LDAP::User do
     end
 
     it "does not mark existing ldap user as changed" do
-      create(:omniauth_user, email: 'john@example.com', extern_uid: 'my-uid', provider: 'ldapmain', external_email: true, email_provider: 'ldapmain')
+      create(:omniauth_user, email: 'john@example.com', extern_uid: 'my-uid', provider: 'ldapmain')
+      ldap_user.gl_user.user_synced_attributes_metadata(provider: 'ldapmain', email: true)
       expect(ldap_user.changed?).to be_falsey
     end
   end
@@ -141,12 +142,12 @@ describe Gitlab::LDAP::User do
         expect(ldap_user.gl_user.email).to eq(info[:email])
       end
 
-      it "has external_email set to true" do
-        expect(ldap_user.gl_user.external_email?).to be(true)
+      it "has user_synced_attributes_metadata email set to true" do
+        expect(ldap_user.gl_user.user_synced_attributes_metadata.email_synced).to be_truthy
       end
 
-      it "has email_provider set to provider" do
-        expect(ldap_user.gl_user.email_provider).to eql 'ldapmain'
+      it "has synced_attribute_provider set to ldapmain" do
+        expect(ldap_user.gl_user.user_synced_attributes_metadata.provider).to eql 'ldapmain'
       end
     end
 
@@ -156,11 +157,11 @@ describe Gitlab::LDAP::User do
       end
 
       it "has a temp email" do
-        expect(ldap_user.gl_user.temp_oauth_email?).to be(true)
+        expect(ldap_user.gl_user.temp_oauth_email?).to be_truthy
       end
 
-      it "has external_email set to false" do
-        expect(ldap_user.gl_user.external_email?).to be(false)
+      it "has synced attribute email set to false" do
+        expect(ldap_user.gl_user.user_synced_attributes_metadata.email_synced).to be_falsey
       end
     end
   end
@@ -168,7 +169,7 @@ describe Gitlab::LDAP::User do
   describe 'blocking' do
     def configure_block(value)
       allow_any_instance_of(Gitlab::LDAP::Config)
-        .to receive(:block_auto_created_users).and_return(value)
+          .to receive(:block_auto_created_users).and_return(value)
     end
 
     context 'signup' do
diff --git a/spec/lib/gitlab/o_auth/user_spec.rb b/spec/lib/gitlab/o_auth/user_spec.rb
index 2cf0f7516de..8aaf320cbf5 100644
--- a/spec/lib/gitlab/o_auth/user_spec.rb
+++ b/spec/lib/gitlab/o_auth/user_spec.rb
@@ -10,7 +10,11 @@ describe Gitlab::OAuth::User do
     {
       nickname: '-john+gitlab-ETC%.git@gmail.com',
       name: 'John',
-      email: 'john@mail.com'
+      email: 'john@mail.com',
+      address: {
+        locality: 'locality',
+        country: 'country'
+      }
     }
   end
   let(:ldap_user) { Gitlab::LDAP::Person.new(Net::LDAP::Entry.new, 'ldapmain') }
@@ -422,11 +426,12 @@ describe Gitlab::OAuth::User do
     end
   end
 
-  describe 'updating email' do
+  describe 'ensure backwards compatibility with with sync email from provider option' do
     let!(:existing_user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'my-provider') }
 
     before do
       stub_omniauth_config(sync_email_from_provider: 'my-provider')
+      stub_omniauth_config(sync_profile_from_provider: ['my-provider'])
     end
 
     context "when provider sets an email" do
@@ -434,12 +439,12 @@ describe Gitlab::OAuth::User do
         expect(gl_user.email).to eq(info_hash[:email])
       end
 
-      it "has external_email set to true" do
-        expect(gl_user.external_email?).to be(true)
+      it "has external_attributes set to true" do
+        expect(gl_user.user_synced_attributes_metadata).not_to be_nil
       end
 
-      it "has email_provider set to provider" do
-        expect(gl_user.email_provider).to eql 'my-provider'
+      it "has attributes_provider set to my-provider" do
+        expect(gl_user.user_synced_attributes_metadata.provider).to eql 'my-provider'
       end
     end
 
@@ -452,8 +457,9 @@ describe Gitlab::OAuth::User do
         expect(gl_user.email).not_to eq(info_hash[:email])
       end
 
-      it "has external_email set to false" do
-        expect(gl_user.external_email?).to be(false)
+      it "has user_synced_attributes_metadata set to nil" do
+        expect(gl_user.user_synced_attributes_metadata.provider).to eql 'my-provider'
+        expect(gl_user.user_synced_attributes_metadata.email_synced).to be_falsey
       end
     end
   end
@@ -487,4 +493,172 @@ describe Gitlab::OAuth::User do
       end
     end
   end
+
+  describe 'updating email with sync profile' do
+    let!(:existing_user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'my-provider') }
+
+    before do
+      stub_omniauth_config(sync_profile_from_provider: ['my-provider'])
+      stub_omniauth_config(sync_profile_attributes: true)
+    end
+
+    context "when provider sets an email" do
+      it "updates the user email" do
+        expect(gl_user.email).to eq(info_hash[:email])
+      end
+
+      it "has email_synced_attribute set to true" do
+        expect(gl_user.user_synced_attributes_metadata.email_synced).to be(true)
+      end
+
+      it "has my-provider as attributes_provider" do
+        expect(gl_user.user_synced_attributes_metadata.provider).to eql 'my-provider'
+      end
+    end
+
+    context "when provider doesn't set an email" do
+      before do
+        info_hash.delete(:email)
+      end
+
+      it "does not update the user email" do
+        expect(gl_user.email).not_to eq(info_hash[:email])
+        expect(gl_user.user_synced_attributes_metadata.email_synced).to be(false)
+      end
+    end
+  end
+
+  describe 'updating name' do
+    let!(:existing_user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'my-provider') }
+
+    before do
+      stub_omniauth_setting(sync_profile_from_provider: ['my-provider'])
+      stub_omniauth_setting(sync_profile_attributes: true)
+    end
+
+    context "when provider sets a name" do
+      it "updates the user name" do
+        expect(gl_user.name).to eq(info_hash[:name])
+      end
+    end
+
+    context "when provider doesn't set a name" do
+      before do
+        info_hash.delete(:name)
+      end
+
+      it "does not update the user name" do
+        expect(gl_user.name).not_to eq(info_hash[:name])
+        expect(gl_user.user_synced_attributes_metadata.name_synced).to be(false)
+      end
+    end
+  end
+
+  describe 'updating location' do
+    let!(:existing_user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'my-provider') }
+
+    before do
+      stub_omniauth_setting(sync_profile_from_provider: ['my-provider'])
+      stub_omniauth_setting(sync_profile_attributes: true)
+    end
+
+    context "when provider sets a location" do
+      it "updates the user location" do
+        expect(gl_user.location).to eq(info_hash[:address][:locality] + ', ' + info_hash[:address][:country])
+        expect(gl_user.user_synced_attributes_metadata.location_synced).to be(true)
+      end
+    end
+
+    context "when provider doesn't set a location" do
+      before do
+        info_hash[:address].delete(:country)
+        info_hash[:address].delete(:locality)
+      end
+
+      it "does not update the user location" do
+        expect(gl_user.location).to be_nil
+        expect(gl_user.user_synced_attributes_metadata.location_synced).to be(false)
+      end
+    end
+  end
+
+  describe 'updating user info' do
+    let!(:existing_user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'my-provider') }
+
+    context "update all info" do
+      before do
+        stub_omniauth_setting(sync_profile_from_provider: ['my-provider'])
+        stub_omniauth_setting(sync_profile_attributes: true)
+      end
+
+      it "updates the user email" do
+        expect(gl_user.email).to eq(info_hash[:email])
+        expect(gl_user.user_synced_attributes_metadata.email_synced).to be(true)
+      end
+
+      it "updates the user name" do
+        expect(gl_user.name).to eq(info_hash[:name])
+        expect(gl_user.user_synced_attributes_metadata.name_synced).to be(true)
+      end
+
+      it "updates the user location" do
+        expect(gl_user.location).to eq(info_hash[:address][:locality] + ', ' + info_hash[:address][:country])
+        expect(gl_user.user_synced_attributes_metadata.location_synced).to be(true)
+      end
+
+      it "sets my-provider as the attributes provider" do
+        expect(gl_user.user_synced_attributes_metadata.provider).to eql('my-provider')
+      end
+    end
+
+    context "update only requested info" do
+      before do
+        stub_omniauth_setting(sync_profile_from_provider: ['my-provider'])
+        stub_omniauth_setting(sync_profile_attributes: %w(name location))
+      end
+
+      it "updates the user name" do
+        expect(gl_user.name).to eq(info_hash[:name])
+        expect(gl_user.user_synced_attributes_metadata.name_synced).to be(true)
+      end
+
+      it "updates the user location" do
+        expect(gl_user.location).to eq(info_hash[:address][:locality] + ', ' + info_hash[:address][:country])
+        expect(gl_user.user_synced_attributes_metadata.location_synced).to be(true)
+      end
+
+      it "does not update the user email" do
+        expect(gl_user.user_synced_attributes_metadata.email_synced).to be(false)
+      end
+    end
+
+    context "update default_scope" do
+      before do
+        stub_omniauth_setting(sync_profile_from_provider: ['my-provider'])
+      end
+
+      it "updates the user email" do
+        expect(gl_user.email).to eq(info_hash[:email])
+        expect(gl_user.user_synced_attributes_metadata.email_synced).to be(true)
+      end
+    end
+
+    context "update no info when profile sync is nil" do
+      it "does not have sync_attribute" do
+        expect(gl_user.user_synced_attributes_metadata).to be(nil)
+      end
+
+      it "does not update the user email" do
+        expect(gl_user.email).not_to eq(info_hash[:email])
+      end
+
+      it "does not update the user name" do
+        expect(gl_user.name).not_to eq(info_hash[:name])
+      end
+
+      it "does not update the user location" do
+        expect(gl_user.location).not_to eq(info_hash[:address][:country])
+      end
+    end
+  end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index fd83a58ed9f..abf732e60bf 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -2116,4 +2116,70 @@ describe User do
       expect(user.verified_email?('other_email@example.com')).to be false
     end
   end
+
+  describe '#sync_attribute?' do
+    let(:user) { described_class.new }
+
+    context 'oauth user' do
+      it 'returns true if name can be synced' do
+        stub_omniauth_setting(sync_profile_attributes: %w(name location))
+        expect(user.sync_attribute?(:name)).to be_truthy
+      end
+
+      it 'returns true if email can be synced' do
+        stub_omniauth_setting(sync_profile_attributes: %w(name email))
+        expect(user.sync_attribute?(:email)).to be_truthy
+      end
+
+      it 'returns true if location can be synced' do
+        stub_omniauth_setting(sync_profile_attributes: %w(location email))
+        expect(user.sync_attribute?(:email)).to be_truthy
+      end
+
+      it 'returns false if name can not be synced' do
+        stub_omniauth_setting(sync_profile_attributes: %w(location email))
+        expect(user.sync_attribute?(:name)).to be_falsey
+      end
+
+      it 'returns false if email can not be synced' do
+        stub_omniauth_setting(sync_profile_attributes: %w(location email))
+        expect(user.sync_attribute?(:name)).to be_falsey
+      end
+
+      it 'returns false if location can not be synced' do
+        stub_omniauth_setting(sync_profile_attributes: %w(location email))
+        expect(user.sync_attribute?(:name)).to be_falsey
+      end
+
+      it 'returns true for all syncable attributes if all syncable attributes can be synced' do
+        stub_omniauth_setting(sync_profile_attributes: true)
+        expect(user.sync_attribute?(:name)).to be_truthy
+        expect(user.sync_attribute?(:email)).to be_truthy
+        expect(user.sync_attribute?(:location)).to be_truthy
+      end
+
+      it 'returns false for all syncable attributes but email if no syncable attributes are declared' do
+        expect(user.sync_attribute?(:name)).to be_falsey
+        expect(user.sync_attribute?(:email)).to be_truthy
+        expect(user.sync_attribute?(:location)).to be_falsey
+      end
+    end
+
+    context 'ldap user' do
+      it 'returns true for email if ldap user' do
+        allow(user).to receive(:ldap_user?).and_return(true)
+        expect(user.sync_attribute?(:name)).to be_falsey
+        expect(user.sync_attribute?(:email)).to be_truthy
+        expect(user.sync_attribute?(:location)).to be_falsey
+      end
+
+      it 'returns true for email and location if ldap user and location declared as syncable' do
+        allow(user).to receive(:ldap_user?).and_return(true)
+        stub_omniauth_setting(sync_profile_attributes: %w(location))
+        expect(user.sync_attribute?(:name)).to be_falsey
+        expect(user.sync_attribute?(:email)).to be_truthy
+        expect(user.sync_attribute?(:location)).to be_truthy
+      end
+    end
+  end
 end