summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2017-09-06 15:34:07 +0000
committerDouwe Maan <douwe@gitlab.com>2017-09-06 15:34:07 +0000
commit58e367fda0ea8301cab912f7b8ed0b79b24f410e (patch)
treec6f0641040060c74d5e49706d744a8a0a61c3147
parentcdd8f2f345aeb3fb05bbe8f567e72b717f388636 (diff)
parent4df54f260751a832ebf0b8c18524020d6604994b (diff)
downloadgitlab-ce-58e367fda0ea8301cab912f7b8ed0b79b24f410e.tar.gz
Merge branch 'generalize-profile-updates' into 'master'
Profile updates from providers See merge request !12968
-rw-r--r--app/controllers/profiles_controller.rb2
-rw-r--r--app/helpers/profiles_helper.rb13
-rw-r--r--app/models/user.rb22
-rw-r--r--app/models/user_synced_attributes_metadata.rb25
-rw-r--r--app/services/users/update_service.rb4
-rw-r--r--app/views/profiles/show.html.haml16
-rw-r--r--changelogs/unreleased/12968-generalize-profile-updates.yml4
-rw-r--r--config/gitlab.yml.example13
-rw-r--r--config/initializers/1_settings.rb15
-rw-r--r--db/migrate/20170820120108_create_user_synced_attributes_metadata.rb15
-rw-r--r--db/migrate/20170828135939_migrate_user_external_mail_data.rb57
-rw-r--r--db/post_migrate/20170828170502_post_deploy_migrate_user_external_mail_data.rb57
-rw-r--r--db/post_migrate/20170828170513_remove_user_email_provider_column.rb12
-rw-r--r--db/post_migrate/20170828170516_remove_user_external_mail_columns.rb12
-rw-r--r--db/schema.rb13
-rw-r--r--doc/integration/omniauth.md18
-rw-r--r--lib/gitlab/ldap/user.rb4
-rw-r--r--lib/gitlab/o_auth/auth_hash.rb17
-rw-r--r--lib/gitlab/o_auth/user.rb32
-rw-r--r--lib/gitlab/saml/user.rb2
-rw-r--r--spec/controllers/profiles_controller_spec.rb25
-rw-r--r--spec/helpers/profiles_helper_spec.rb31
-rw-r--r--spec/lib/gitlab/ldap/user_spec.rb19
-rw-r--r--spec/lib/gitlab/o_auth/user_spec.rb190
-rw-r--r--spec/models/user_spec.rb66
25 files changed, 626 insertions, 58 deletions
diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index 076076fd1b3..d83824fef06 100644
--- a/app/controllers/profiles_controller.rb
+++ b/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
diff --git a/app/helpers/profiles_helper.rb b/app/helpers/profiles_helper.rb
index 45238f12ac7..5a4fda0724c 100644
--- a/app/helpers/profiles_helper.rb
+++ b/app/helpers/profiles_helper.rb
@@ -1,7 +1,12 @@
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
diff --git a/app/models/user.rb b/app/models/user.rb
index c5b5f09722f..105eb62f1fa 100644
--- a/app/models/user.rb
+++ b/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
diff --git a/app/models/user_synced_attributes_metadata.rb b/app/models/user_synced_attributes_metadata.rb
new file mode 100644
index 00000000000..9f374304164
--- /dev/null
+++ b/app/models/user_synced_attributes_metadata.rb
@@ -0,0 +1,25 @@
+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
diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index 2f9855273dc..6188b8a4349 100644
--- a/app/services/users/update_service.rb
+++ b/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
diff --git a/app/views/profiles/show.html.haml b/app/views/profiles/show.html.haml
index aa8d5a8bc1a..35ad280b037 100644
--- a/app/views/profiles/show.html.haml
+++ b/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,7 +67,10 @@
= f.text_field :linkedin
= f.text_field :twitter
= f.text_field :website_url, label: 'Website'
- = f.text_field :location
+ - 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.'
.prepend-top-default.append-bottom-default
diff --git a/changelogs/unreleased/12968-generalize-profile-updates.yml b/changelogs/unreleased/12968-generalize-profile-updates.yml
new file mode 100644
index 00000000000..d09793512c1
--- /dev/null
+++ b/changelogs/unreleased/12968-generalize-profile-updates.yml
@@ -0,0 +1,4 @@
+---
+title: Generalize profile updates from providers
+merge_request: 12968
+author: Alexandros Keramidas
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index c5704ac5857..e9661090844 100644
--- a/config/gitlab.yml.example
+++ b/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
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 360b72cdea3..7c1ca05a57b 100644
--- a/config/initializers/1_settings.rb
+++ b/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({})
diff --git a/db/migrate/20170820120108_create_user_synced_attributes_metadata.rb b/db/migrate/20170820120108_create_user_synced_attributes_metadata.rb
new file mode 100644
index 00000000000..79028e34987
--- /dev/null
+++ b/db/migrate/20170820120108_create_user_synced_attributes_metadata.rb
@@ -0,0 +1,15 @@
+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
diff --git a/db/migrate/20170828135939_migrate_user_external_mail_data.rb b/db/migrate/20170828135939_migrate_user_external_mail_data.rb
new file mode 100644
index 00000000000..592e141b7e6
--- /dev/null
+++ b/db/migrate/20170828135939_migrate_user_external_mail_data.rb
@@ -0,0 +1,57 @@
+# 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
diff --git a/db/post_migrate/20170828170502_post_deploy_migrate_user_external_mail_data.rb b/db/post_migrate/20170828170502_post_deploy_migrate_user_external_mail_data.rb
new file mode 100644
index 00000000000..fefd931e5d2
--- /dev/null
+++ b/db/post_migrate/20170828170502_post_deploy_migrate_user_external_mail_data.rb
@@ -0,0 +1,57 @@
+# 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
diff --git a/db/post_migrate/20170828170513_remove_user_email_provider_column.rb b/db/post_migrate/20170828170513_remove_user_email_provider_column.rb
new file mode 100644
index 00000000000..570f2b3772a
--- /dev/null
+++ b/db/post_migrate/20170828170513_remove_user_email_provider_column.rb
@@ -0,0 +1,12 @@
+# 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
diff --git a/db/post_migrate/20170828170516_remove_user_external_mail_columns.rb b/db/post_migrate/20170828170516_remove_user_external_mail_columns.rb
new file mode 100644
index 00000000000..bb81dc682b3
--- /dev/null
+++ b/db/post_migrate/20170828170516_remove_user_external_mail_columns.rb
@@ -0,0 +1,12 @@
+# 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
diff --git a/db/schema.rb b/db/schema.rb
index 69911ce7107..1c1a5e63bc4 100644
--- a/db/schema.rb
+++ b/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
diff --git a/doc/integration/omniauth.md b/doc/integration/omniauth.md
index 6c11f46a70a..0e20b8096e9 100644
--- a/doc/integration/omniauth.md
+++ b/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
diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb
index 39180dc17d9..3bf27b37ae6 100644
--- a/lib/gitlab/ldap/user.rb
+++ b/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
diff --git a/lib/gitlab/o_auth/auth_hash.rb b/lib/gitlab/o_auth/auth_hash.rb
index 7d6911a1ab3..1f331b1e91d 100644
--- a/lib/gitlab/o_auth/auth_hash.rb
+++ b/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
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index e8330917e91..7704bf715e4 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/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,20 +184,30 @@ module Gitlab
}
end
- def sync_email_from_provider?
- auth_hash.provider.to_s == Gitlab.config.omniauth.sync_email_from_provider.to_s
+ def sync_profile_from_provider?
+ providers = Gitlab.config.omniauth.sync_profile_from_provider
+
+ if providers.is_a?(Array)
+ providers.include?(auth_hash.provider)
+ else
+ providers
+ end
end
- def update_email
- if auth_hash.has_email? && sync_email_from_provider?
- if persisted?
- gl_user.skip_reconfirmation!
- gl_user.email = auth_hash.email
- 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
diff --git a/lib/gitlab/saml/user.rb b/lib/gitlab/saml/user.rb
index 8a7cc690046..0f323a9e8b2 100644
--- a/lib/gitlab/saml/user.rb
+++ b/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
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