Merge branch 'generalize-profile-updates' into 'master'

Profile updates from providers

See merge request !12968

app/controllers/profiles_controller.rb

@@ -9,8 +9,6 @@ class ProfilesController < Profiles::ApplicationController
   end
 
   def update
-    user_params.except!(:email) if @user.external_email?
-
     respond_to do |format|
       result = Users::UpdateService.new(@user, user_params).execute
 

app/helpers/profiles_helper.rb

 module ProfilesHelper
-  def email_provider_label
-    return unless current_user.external_email?
-
-    current_user.email_provider.present? ? Gitlab::OAuth::Provider.label_for(current_user.email_provider) : "LDAP"
+  def attribute_provider_label(attribute)
+    user_synced_attributes_metadata = current_user.user_synced_attributes_metadata
+    if user_synced_attributes_metadata&.synced?(attribute)
+      if user_synced_attributes_metadata.provider
+        Gitlab::OAuth::Provider.label_for(user_synced_attributes_metadata.provider)
+      else
+        'LDAP'
+      end
+    end
   end
 end

app/models/user.rb

@@ -15,10 +15,12 @@ class User < ActiveRecord::Base
   include IgnorableColumn
   include FeatureGate
   include CreatedAtFilterable
+  include IgnorableColumn
 
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
-  ignore_column :authorized_projects_populated
+  ignore_column :external_email
+  ignore_column :email_provider
 
   add_authentication_token_field :authentication_token
   add_authentication_token_field :incoming_email_token
@@ -85,6 +87,7 @@ class User < ActiveRecord::Base
   has_many :identities, dependent: :destroy, autosave: true # rubocop:disable Cop/ActiveRecordDependent
   has_many :u2f_registrations, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :chat_names, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+  has_one :user_synced_attributes_metadata, autosave: true
 
   # Groups
   has_many :members, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
@@ -161,6 +164,7 @@ class User < ActiveRecord::Base
   after_update :update_emails_with_primary_email, if: :email_changed?
   before_save :ensure_authentication_token, :ensure_incoming_email_token
   before_save :ensure_user_rights_and_limits, if: :external_changed?
+  before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) }
   after_save :ensure_namespace_correct
   after_commit :update_invalid_gpg_signatures, on: :update, if: -> { previous_changes.key?('email') }
   after_initialize :set_projects_limit
@@ -1045,6 +1049,22 @@ class User < ActiveRecord::Base
     self.email == email
   end
 
+  def sync_attribute?(attribute)
+    return true if ldap_user? && attribute == :email
+
+    attributes = Gitlab.config.omniauth.sync_profile_attributes
+
+    if attributes.is_a?(Array)
+      attributes.include?(attribute.to_s)
+    else
+      attributes
+    end
+  end
+
+  def read_only_attribute?(attribute)
+    user_synced_attributes_metadata&.read_only?(attribute)
+  end
+
   protected
 
   # override, from Devise::Validatable

app/models/user_synced_attributes_metadata.rb

+class UserSyncedAttributesMetadata < ActiveRecord::Base
+  belongs_to :user
+
+  validates :user, presence: true
+
+  SYNCABLE_ATTRIBUTES = %i[name email location].freeze
+
+  def read_only?(attribute)
+    Gitlab.config.omniauth.sync_profile_from_provider && synced?(attribute)
+  end
+
+  def read_only_attributes
+    return [] unless Gitlab.config.omniauth.sync_profile_from_provider
+
+    SYNCABLE_ATTRIBUTES.select { |key| synced?(key) }
+  end
+
+  def synced?(attribute)
+    read_attribute("#{attribute}_synced")
+  end
+
+  def set_attribute_synced(attribute, value)
+    write_attribute("#{attribute}_synced", value)
+  end
+end

app/services/users/update_service.rb

@@ -34,6 +34,10 @@ module Users
     private
 
     def assign_attributes(&block)
+      if @user.user_synced_attributes_metadata
+        params.except!(*@user.user_synced_attributes_metadata.read_only_attributes)
+      end
+
       @user.assign_attributes(params) if params.any?
     end
   end

app/views/profiles/show.html.haml

@@ -45,12 +45,15 @@
           Some options are unavailable for LDAP accounts
     .col-lg-8
       .row
-        = f.text_field :name,  required: true, wrapper: { class: 'col-md-9' },
-          help: 'Enter your name, so people you know can recognize you.'
+        - if @user.read_only_attribute?(:name)
+          = f.text_field :name, required: true, readonly: true, wrapper: { class: 'col-md-9' },
+          help: "Your name was automatically set based on your #{ attribute_provider_label(:name) } account, so people you know can recognize you."
+        - else
+          = f.text_field :name, required: true, wrapper: { class: 'col-md-9' }, help: "Enter your name, so people you know can recognize you."
         = f.text_field :id, readonly: true, label: 'User ID', wrapper: { class: 'col-md-3' }
 
-      - if @user.external_email?
-        = f.text_field :email, required: true, readonly: true, help: "Your email address was automatically set based on your #{email_provider_label} account."
+      - if @user.read_only_attribute?(:email)
+        = f.text_field :email, required: true, readonly: true, help: "Your email address was automatically set based on your #{ attribute_provider_label(:email) } account."
       - else
         = f.text_field :email, required: true, value: (@user.email unless @user.temp_oauth_email?),
           help: user_email_help_text(@user)
@@ -64,6 +67,9 @@
       = f.text_field :linkedin
       = f.text_field :twitter
       = f.text_field :website_url, label: 'Website'
+      - if @user.read_only_attribute?(:location)
+        = f.text_field :location, readonly: true, help: "Your location was automatically set based on your #{ attribute_provider_label(:location) } account."
+      - else
         = f.text_field :location
       = f.text_field :organization
       = f.text_area :bio, rows: 4, maxlength: 250, help: 'Tell us about yourself in fewer than 250 characters.'

changelogs/unreleased/12968-generalize-profile-updates.yml

+---
+title: Generalize profile updates from providers
+merge_request: 12968
+author: Alexandros Keramidas

config/gitlab.yml.example

@@ -372,9 +372,16 @@ production: &base
     # showing GitLab's sign-in page (default: show the GitLab sign-in page)
     # auto_sign_in_with_provider: saml
 
-    # Sync user's email address from the specified Omniauth provider every time the user logs
-    # in (default: nil). And consequently make this field read-only.
-    # sync_email_from_provider: cas3
+    # Sync user's profile from the specified Omniauth providers every time the user logs in (default: empty).
+    # Define the allowed providers using an array, e.g. ["cas3", "saml", "twitter"],
+    # or as true/false to allow all providers or none.
+    # sync_profile_from_provider: []
+
+    # Select which info to sync from the providers above. (default: email).
+    # Define the synced profile info using an array. Available options are "name", "email" and "location"
+    # e.g. ["name", "email", "location"] or as true to sync all available.
+    # This consequently will make the selected attributes read-only.
+    # sync_profile_attributes: true
 
     # CAUTION!
     # This allows users to login without having a user account first. Define the allowed providers

config/initializers/1_settings.rb

@@ -173,7 +173,20 @@ Settings.omniauth['external_providers'] = [] if Settings.omniauth['external_prov
 Settings.omniauth['block_auto_created_users'] = true if Settings.omniauth['block_auto_created_users'].nil?
 Settings.omniauth['auto_link_ldap_user'] = false if Settings.omniauth['auto_link_ldap_user'].nil?
 Settings.omniauth['auto_link_saml_user'] = false if Settings.omniauth['auto_link_saml_user'].nil?
-Settings.omniauth['sync_email_from_provider'] ||= nil
+
+Settings.omniauth['sync_profile_from_provider'] = false if Settings.omniauth['sync_profile_from_provider'].nil?
+Settings.omniauth['sync_profile_attributes'] = ['email'] if Settings.omniauth['sync_profile_attributes'].nil?
+
+# Handle backwards compatibility with merge request 11268
+if Settings.omniauth['sync_email_from_provider']
+  if Settings.omniauth['sync_profile_from_provider'].is_a?(Array)
+    Settings.omniauth['sync_profile_from_provider'] |= [Settings.omniauth['sync_email_from_provider']]
+  elsif !Settings.omniauth['sync_profile_from_provider']
+    Settings.omniauth['sync_profile_from_provider'] = [Settings.omniauth['sync_email_from_provider']]
+  end
+
+  Settings.omniauth['sync_profile_attributes'] |= ['email'] unless Settings.omniauth['sync_profile_attributes'] == true
+end
 
 Settings.omniauth['providers'] ||= []
 Settings.omniauth['cas3'] ||= Settingslogic.new({})

db/migrate/20170820120108_create_user_synced_attributes_metadata.rb

+class CreateUserSyncedAttributesMetadata < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :user_synced_attributes_metadata do |t|
+      t.boolean :name_synced, default: false
+      t.boolean :email_synced, default: false
+      t.boolean :location_synced, default: false
+      t.references :user, null: false, index: { unique: true }, foreign_key: { on_delete: :cascade }
+      t.string :provider
+    end
+  end
+end

db/migrate/20170828135939_migrate_user_external_mail_data.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MigrateUserExternalMailData < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    self.table_name = 'users'
+
+    include EachBatch
+  end
+
+  class UserSyncedAttributesMetadata < ActiveRecord::Base
+    self.table_name = 'user_synced_attributes_metadata'
+
+    include EachBatch
+  end
+
+  def up
+    User.each_batch do |batch|
+      start_id, end_id = batch.pluck('MIN(id), MAX(id)').first
+
+      execute <<-EOF
+        INSERT INTO user_synced_attributes_metadata (user_id, provider, email_synced)
+        SELECT id, email_provider, external_email
+        FROM users
+        WHERE external_email = TRUE
+        AND NOT EXISTS (
+          SELECT true
+          FROM user_synced_attributes_metadata
+          WHERE user_id = users.id
+          AND provider = users.email_provider
+        )
+        AND id BETWEEN #{start_id} AND #{end_id}
+      EOF
+    end
+  end
+
+  def down
+    UserSyncedAttributesMetadata.each_batch do |batch|
+      start_id, end_id = batch.pluck('MIN(id), MAX(id)').first
+
+      execute <<-EOF
+        UPDATE users
+        SET users.email_provider = metadata.provider, users.external_email = metadata.email_synced
+        FROM user_synced_attributes_metadata as metadata, users
+        WHERE metadata.email_synced = TRUE
+        AND metadata.user_id = users.id
+        AND id BETWEEN #{start_id} AND #{end_id}
+      EOF
+    end
+  end
+end

db/post_migrate/20170828170502_post_deploy_migrate_user_external_mail_data.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class PostDeployMigrateUserExternalMailData < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    self.table_name = 'users'
+
+    include EachBatch
+  end
+
+  class UserSyncedAttributesMetadata < ActiveRecord::Base
+    self.table_name = 'user_synced_attributes_metadata'
+
+    include EachBatch
+  end
+
+  def up
+    User.each_batch do |batch|
+      start_id, end_id = batch.pluck('MIN(id), MAX(id)').first
+
+      execute <<-EOF
+        INSERT INTO user_synced_attributes_metadata (user_id, provider, email_synced)
+        SELECT id, email_provider, external_email
+        FROM users
+        WHERE external_email = TRUE
+        AND NOT EXISTS (
+          SELECT true
+          FROM user_synced_attributes_metadata
+          WHERE user_id = users.id
+          AND provider = users.email_provider
+        )
+        AND id BETWEEN #{start_id} AND #{end_id}
+      EOF
+    end
+  end
+
+  def down
+    UserSyncedAttributesMetadata.each_batch do |batch|
+      start_id, end_id = batch.pluck('MIN(id), MAX(id)').first
+
+      execute <<-EOF
+        UPDATE users
+        SET users.email_provider = metadata.provider, users.external_email = metadata.email_synced
+        FROM user_synced_attributes_metadata as metadata, users
+        WHERE metadata.email_synced = TRUE
+        AND metadata.user_id = users.id
+        AND id BETWEEN #{start_id} AND #{end_id}
+      EOF
+    end
+  end
+end

db/post_migrate/20170828170513_remove_user_email_provider_column.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveUserEmailProviderColumn < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    remove_column :users, :email_provider, :string
+  end
+end

db/post_migrate/20170828170516_remove_user_external_mail_columns.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveUserExternalMailColumns < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    remove_column :users, :external_email, :boolean
+  end
+end

db/schema.rb

@@ -1539,6 +1539,16 @@ ActiveRecord::Schema.define(version: 20170905112933) do
 
   add_index "user_agent_details", ["subject_id", "subject_type"], name: "index_user_agent_details_on_subject_id_and_subject_type", using: :btree
 
+  create_table "user_synced_attributes_metadata", force: :cascade do |t|
+    t.boolean "name_synced", default: false
+    t.boolean "email_synced", default: false
+    t.boolean "location_synced", default: false
+    t.integer "user_id", null: false
+    t.string "provider"
+  end
+
+  add_index "user_synced_attributes_metadata", ["user_id"], name: "index_user_synced_attributes_metadata_on_user_id", unique: true, using: :btree
+
   create_table "users", force: :cascade do |t|
     t.string "email", default: "", null: false
     t.string "encrypted_password", default: "", null: false
@@ -1604,8 +1614,6 @@ ActiveRecord::Schema.define(version: 20170905112933) do
     t.boolean "notified_of_own_activity"
     t.string "preferred_language"
     t.string "rss_token"
-    t.boolean "external_email", default: false, null: false
-    t.string "email_provider"
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
@@ -1756,6 +1764,7 @@ ActiveRecord::Schema.define(version: 20170905112933) do
   add_foreign_key "todos", "projects", name: "fk_45054f9c45", on_delete: :cascade
   add_foreign_key "trending_projects", "projects", on_delete: :cascade
   add_foreign_key "u2f_registrations", "users"
+  add_foreign_key "user_synced_attributes_metadata", "users", on_delete: :cascade
   add_foreign_key "users_star_projects", "projects", name: "fk_22cd27ddfc", on_delete: :cascade
   add_foreign_key "web_hook_logs", "web_hooks", on_delete: :cascade
   add_foreign_key "web_hooks", "projects", name: "fk_0c8ca6d9d1", on_delete: :cascade

doc/integration/omniauth.md

@@ -224,3 +224,21 @@ By default Sign In is enabled via all the OAuth Providers that have been configu
 In order to enable/disable an OmniAuth provider, go to Admin Area -> Settings -> Sign-in Restrictions section -> Enabled OAuth Sign-In sources and select the providers you want to enable or disable.
 
 ![Enabled OAuth Sign-In sources](img/enabled-oauth-sign-in-sources.png)
+
+
+## Keep OmniAuth user profiles up to date
+
+You can enable profile syncing from selected OmniAuth providers and for all or for specific user information.
+ 
+ ```ruby
+   gitlab_rails['sync_profile_from_provider'] = ['twitter', 'google_oauth2']
+   gitlab_rails['sync_profile_attributes'] = ['name', 'email', 'location']
+ ```
+ 
+ **For installations from source**
+ 
+ ```yaml
+   omniauth:
+     sync_profile_from_provider: ['twitter', 'google_oauth2']
+     sync_profile_claims_from_provider: ['email', 'location']
+ ```
\ No newline at end of file

lib/gitlab/ldap/user.rb

@@ -36,7 +36,7 @@ module Gitlab
       end
 
       def find_by_email
-        ::User.find_by(email: auth_hash.email.downcase) if auth_hash.has_email?
+        ::User.find_by(email: auth_hash.email.downcase) if auth_hash.has_attribute?(:email)
       end
 
       def update_user_attributes
@@ -60,7 +60,7 @@ module Gitlab
         ldap_config.block_auto_created_users
       end
 
-      def sync_email_from_provider?
+      def sync_profile_from_provider?
         true
       end
 

lib/gitlab/o_auth/auth_hash.rb

@@ -32,8 +32,21 @@ module Gitlab
         @password ||= Gitlab::Utils.force_utf8(Devise.friendly_token[0, 8].downcase)
       end
 
-      def has_email?
-        get_info(:email).present?
+      def location
+        location = get_info(:address)
+        if location.is_a?(Hash)
+          [location.locality.presence, location.country.presence].compact.join(', ')
+        else
+          location
+        end
+      end
+
+      def has_attribute?(attribute)
+        if attribute == :location
+          get_info(:address).present?
+        else
+          get_info(attribute).present?
+        end
       end
 
       private

lib/gitlab/o_auth/user.rb

@@ -12,7 +12,7 @@ module Gitlab
 
       def initialize(auth_hash)
         self.auth_hash = auth_hash
-        update_email
+        update_profile if sync_profile_from_provider?
       end
 
       def persisted?
@@ -184,22 +184,32 @@ module Gitlab
         }
       end
 
-      def sync_email_from_provider?
-        auth_hash.provider.to_s == Gitlab.config.omniauth.sync_email_from_provider.to_s
-      end
+      def sync_profile_from_provider?
+        providers = Gitlab.config.omniauth.sync_profile_from_provider
 
-      def update_email
-        if auth_hash.has_email? && sync_email_from_provider?
-          if persisted?
-            gl_user.skip_reconfirmation!
-            gl_user.email = auth_hash.email
+        if providers.is_a?(Array)
+          providers.include?(auth_hash.provider)
+        else
+          providers
         end
+      end
+
+      def update_profile
+        user_synced_attributes_metadata = gl_user.user_synced_attributes_metadata || gl_user.build_user_synced_attributes_metadata
 
-          gl_user.external_email = true
-          gl_user.email_provider = auth_hash.provider
+        UserSyncedAttributesMetadata::SYNCABLE_ATTRIBUTES.each do |key|
+          if auth_hash.has_attribute?(key) && gl_user.sync_attribute?(key)
+            gl_user[key] = auth_hash.public_send(key) # rubocop:disable GitlabSecurity/PublicSend
+            user_synced_attributes_metadata.set_attribute_synced(key, true)
+          else
+            user_synced_attributes_metadata.set_attribute_synced(key, false)
           end
         end
 
+        user_synced_attributes_metadata.provider = auth_hash.provider
+        gl_user.user_synced_attributes_metadata = user_synced_attributes_metadata
+      end
+
       def log
         Gitlab::AppLogger
       end

lib/gitlab/saml/user.rb

@@ -40,7 +40,7 @@ module Gitlab
       end
 
       def find_by_email
-        if auth_hash.has_email?
+        if auth_hash.has_attribute?(:email)
           user = ::User.find_by(email: auth_hash.email.downcase)
           user.identities.new(extern_uid: auth_hash.uid, provider: auth_hash.provider) if user
           user

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

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
 

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

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

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