diff options
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 a8ae0b92334..a127e98d8e8 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.  + + +## 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 |