Commit d39d968b authored by Sean McGivern's avatar Sean McGivern

Merge branch 'dm-ldap-email-readonly' into 'master'

Make sure user email is read only when synced with LDAP

Closes #41033

See merge request gitlab-org/gitlab-ce!15915
parents 3a19e532 481b8a71
...@@ -14,11 +14,11 @@ class Identity < ActiveRecord::Base ...@@ -14,11 +14,11 @@ class Identity < ActiveRecord::Base
end end
def ldap? def ldap?
provider.starts_with?('ldap') Gitlab::OAuth::Provider.ldap_provider?(provider)
end end
def self.normalize_uid(provider, uid) def self.normalize_uid(provider, uid)
if provider.to_s.starts_with?('ldap') if Gitlab::OAuth::Provider.ldap_provider?(provider)
Gitlab::LDAP::Person.normalize_dn(uid) Gitlab::LDAP::Person.normalize_dn(uid)
else else
uid.to_s uid.to_s
......
...@@ -738,7 +738,7 @@ class User < ActiveRecord::Base ...@@ -738,7 +738,7 @@ class User < ActiveRecord::Base
def ldap_user? def ldap_user?
if identities.loaded? if identities.loaded?
identities.find { |identity| identity.provider.start_with?('ldap') && !identity.extern_uid.nil? } identities.find { |identity| Gitlab::OAuth::Provider.ldap_provider?(identity.provider) && !identity.extern_uid.nil? }
else else
identities.exists?(["provider LIKE ? AND extern_uid IS NOT NULL", "ldap%"]) identities.exists?(["provider LIKE ? AND extern_uid IS NOT NULL", "ldap%"])
end end
......
...@@ -6,11 +6,11 @@ class UserSyncedAttributesMetadata < ActiveRecord::Base ...@@ -6,11 +6,11 @@ class UserSyncedAttributesMetadata < ActiveRecord::Base
SYNCABLE_ATTRIBUTES = %i[name email location].freeze SYNCABLE_ATTRIBUTES = %i[name email location].freeze
def read_only?(attribute) def read_only?(attribute)
Gitlab.config.omniauth.sync_profile_from_provider && synced?(attribute) sync_profile_from_provider? && synced?(attribute)
end end
def read_only_attributes def read_only_attributes
return [] unless Gitlab.config.omniauth.sync_profile_from_provider return [] unless sync_profile_from_provider?
SYNCABLE_ATTRIBUTES.select { |key| synced?(key) } SYNCABLE_ATTRIBUTES.select { |key| synced?(key) }
end end
...@@ -22,4 +22,10 @@ class UserSyncedAttributesMetadata < ActiveRecord::Base ...@@ -22,4 +22,10 @@ class UserSyncedAttributesMetadata < ActiveRecord::Base
def set_attribute_synced(attribute, value) def set_attribute_synced(attribute, value)
write_attribute("#{attribute}_synced", value) write_attribute("#{attribute}_synced", value)
end end
private
def sync_profile_from_provider?
Gitlab::OAuth::Provider.sync_profile_from_provider?(provider)
end
end end
---
title: Make sure user email is read only when synced with LDAP
merge_request: 15915
author:
type: fixed
...@@ -383,6 +383,7 @@ production: &base ...@@ -383,6 +383,7 @@ production: &base
# Sync user's profile from the specified Omniauth providers every time the user logs in (default: empty). # 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"], # Define the allowed providers using an array, e.g. ["cas3", "saml", "twitter"],
# or as true/false to allow all providers or none. # or as true/false to allow all providers or none.
# When authenticating using LDAP, the user's email is always synced.
# sync_profile_from_provider: [] # sync_profile_from_provider: []
# Select which info to sync from the providers above. (default: email). # Select which info to sync from the providers above. (default: email).
......
...@@ -229,16 +229,18 @@ In order to enable/disable an OmniAuth provider, go to Admin Area -> Settings -> ...@@ -229,16 +229,18 @@ In order to enable/disable an OmniAuth provider, go to Admin Area -> Settings ->
## Keep OmniAuth user profiles up to date ## Keep OmniAuth user profiles up to date
You can enable profile syncing from selected OmniAuth providers and for all or for specific user information. You can enable profile syncing from selected OmniAuth providers and for all or for specific user information.
When authenticating using LDAP, the user's email is always synced.
```ruby ```ruby
gitlab_rails['sync_profile_from_provider'] = ['twitter', 'google_oauth2'] gitlab_rails['sync_profile_from_provider'] = ['twitter', 'google_oauth2']
gitlab_rails['sync_profile_attributes'] = ['name', 'email', 'location'] gitlab_rails['sync_profile_attributes'] = ['name', 'email', 'location']
``` ```
**For installations from source** **For installations from source**
```yaml ```yaml
omniauth: omniauth:
sync_profile_from_provider: ['twitter', 'google_oauth2'] sync_profile_from_provider: ['twitter', 'google_oauth2']
sync_profile_claims_from_provider: ['email', 'location'] sync_profile_attributes: ['email', 'location']
``` ```
\ No newline at end of file
...@@ -36,10 +36,6 @@ module Gitlab ...@@ -36,10 +36,6 @@ module Gitlab
ldap_config.block_auto_created_users ldap_config.block_auto_created_users
end end
def sync_profile_from_provider?
true
end
def allowed? def allowed?
Gitlab::LDAP::Access.allowed?(gl_user) Gitlab::LDAP::Access.allowed?(gl_user)
end end
......
...@@ -19,6 +19,18 @@ module Gitlab ...@@ -19,6 +19,18 @@ module Gitlab
name.to_s.start_with?('ldap') name.to_s.start_with?('ldap')
end end
def self.sync_profile_from_provider?(provider)
return true if ldap_provider?(provider)
providers = Gitlab.config.omniauth.sync_profile_from_provider
if providers.is_a?(Array)
providers.include?(provider)
else
providers
end
end
def self.config_for(name) def self.config_for(name)
name = name.to_s name = name.to_s
if ldap_provider?(name) if ldap_provider?(name)
......
...@@ -12,7 +12,7 @@ module Gitlab ...@@ -12,7 +12,7 @@ module Gitlab
def initialize(auth_hash) def initialize(auth_hash)
self.auth_hash = auth_hash self.auth_hash = auth_hash
update_profile if sync_profile_from_provider? update_profile
add_or_update_user_identities add_or_update_user_identities
end end
...@@ -195,29 +195,31 @@ module Gitlab ...@@ -195,29 +195,31 @@ module Gitlab
end end
def sync_profile_from_provider? def sync_profile_from_provider?
providers = Gitlab.config.omniauth.sync_profile_from_provider Gitlab::OAuth::Provider.sync_profile_from_provider?(auth_hash.provider)
if providers.is_a?(Array)
providers.include?(auth_hash.provider)
else
providers
end
end end
def update_profile def update_profile
user_synced_attributes_metadata = gl_user.user_synced_attributes_metadata || gl_user.build_user_synced_attributes_metadata return unless sync_profile_from_provider? || creating_linked_ldap_user?
UserSyncedAttributesMetadata::SYNCABLE_ATTRIBUTES.each do |key| metadata = gl_user.user_synced_attributes_metadata || gl_user.build_user_synced_attributes_metadata
if auth_hash.has_attribute?(key) && gl_user.sync_attribute?(key)
gl_user[key] = auth_hash.public_send(key) # rubocop:disable GitlabSecurity/PublicSend if sync_profile_from_provider?
user_synced_attributes_metadata.set_attribute_synced(key, true) UserSyncedAttributesMetadata::SYNCABLE_ATTRIBUTES.each do |key|
else if auth_hash.has_attribute?(key) && gl_user.sync_attribute?(key)
user_synced_attributes_metadata.set_attribute_synced(key, false) gl_user[key] = auth_hash.public_send(key) # rubocop:disable GitlabSecurity/PublicSend
metadata.set_attribute_synced(key, true)
else
metadata.set_attribute_synced(key, false)
end
end end
metadata.provider = auth_hash.provider
end end
user_synced_attributes_metadata.provider = auth_hash.provider if creating_linked_ldap_user? && gl_user.email == ldap_person.email.first
gl_user.user_synced_attributes_metadata = user_synced_attributes_metadata metadata.set_attribute_synced(:email, true)
metadata.provider = ldap_person.provider
end
end end
def log def log
......
...@@ -38,7 +38,6 @@ describe Gitlab::LDAP::User do ...@@ -38,7 +38,6 @@ describe Gitlab::LDAP::User do
it "does not mark existing ldap user as changed" do it "does not mark existing ldap user as changed" do
create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=john smith,ou=people,dc=example,dc=com', provider: 'ldapmain') create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=john smith,ou=people,dc=example,dc=com', provider: 'ldapmain')
ldap_user.gl_user.user_synced_attributes_metadata(provider: 'ldapmain', email: true)
expect(ldap_user.changed?).to be_falsey expect(ldap_user.changed?).to be_falsey
end end
end end
...@@ -144,11 +143,15 @@ describe Gitlab::LDAP::User do ...@@ -144,11 +143,15 @@ describe Gitlab::LDAP::User do
expect(ldap_user.gl_user.email).to eq(info[:email]) expect(ldap_user.gl_user.email).to eq(info[:email])
end end
it "has user_synced_attributes_metadata email set to true" do it "has email set as synced" do
expect(ldap_user.gl_user.user_synced_attributes_metadata.email_synced).to be_truthy expect(ldap_user.gl_user.user_synced_attributes_metadata.email_synced).to be_truthy
end end
it "has synced_attribute_provider set to ldapmain" do it "has email set as read-only" do
expect(ldap_user.gl_user.read_only_attribute?(:email)).to be_truthy
end
it "has synced attributes provider set to ldapmain" do
expect(ldap_user.gl_user.user_synced_attributes_metadata.provider).to eql 'ldapmain' expect(ldap_user.gl_user.user_synced_attributes_metadata.provider).to eql 'ldapmain'
end end
end end
...@@ -162,9 +165,13 @@ describe Gitlab::LDAP::User do ...@@ -162,9 +165,13 @@ describe Gitlab::LDAP::User do
expect(ldap_user.gl_user.temp_oauth_email?).to be_truthy expect(ldap_user.gl_user.temp_oauth_email?).to be_truthy
end end
it "has synced attribute email set to false" do it "has email set as not synced" do
expect(ldap_user.gl_user.user_synced_attributes_metadata.email_synced).to be_falsey expect(ldap_user.gl_user.user_synced_attributes_metadata.email_synced).to be_falsey
end end
it "does not have email set as read-only" do
expect(ldap_user.gl_user.read_only_attribute?(:email)).to be_falsey
end
end end
end end
......
...@@ -202,11 +202,13 @@ describe Gitlab::OAuth::User do ...@@ -202,11 +202,13 @@ describe Gitlab::OAuth::User do
end end
context "and no account for the LDAP user" do context "and no account for the LDAP user" do
it "creates a user with dual LDAP and omniauth identities" do before do
allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user) allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
oauth_user.save oauth_user.save
end
it "creates a user with dual LDAP and omniauth identities" do
expect(gl_user).to be_valid expect(gl_user).to be_valid
expect(gl_user.username).to eql uid expect(gl_user.username).to eql uid
expect(gl_user.email).to eql 'johndoe@example.com' expect(gl_user.email).to eql 'johndoe@example.com'
...@@ -219,6 +221,18 @@ describe Gitlab::OAuth::User do ...@@ -219,6 +221,18 @@ describe Gitlab::OAuth::User do
] ]
) )
end end
it "has email set as synced" do
expect(gl_user.user_synced_attributes_metadata.email_synced).to be_truthy
end
it "has email set as read-only" do
expect(gl_user.read_only_attribute?(:email)).to be_truthy
end
it "has synced attributes provider set to ldapmain" do
expect(gl_user.user_synced_attributes_metadata.provider).to eql 'ldapmain'
end
end end
context "and LDAP user has an account already" do context "and LDAP user has an account already" do
...@@ -440,11 +454,15 @@ describe Gitlab::OAuth::User do ...@@ -440,11 +454,15 @@ describe Gitlab::OAuth::User do
expect(gl_user.email).to eq(info_hash[:email]) expect(gl_user.email).to eq(info_hash[:email])
end end
it "has external_attributes set to true" do it "has email set as synced" do
expect(gl_user.user_synced_attributes_metadata).not_to be_nil expect(gl_user.user_synced_attributes_metadata.email_synced).to be_truthy
end
it "has email set as read-only" do
expect(gl_user.read_only_attribute?(:email)).to be_truthy
end end
it "has attributes_provider set to my-provider" do it "has synced attributes provider set to my-provider" do
expect(gl_user.user_synced_attributes_metadata.provider).to eql 'my-provider' expect(gl_user.user_synced_attributes_metadata.provider).to eql 'my-provider'
end end
end end
...@@ -458,10 +476,13 @@ describe Gitlab::OAuth::User do ...@@ -458,10 +476,13 @@ describe Gitlab::OAuth::User do
expect(gl_user.email).not_to eq(info_hash[:email]) expect(gl_user.email).not_to eq(info_hash[:email])
end end
it "has user_synced_attributes_metadata set to nil" do it "has email set as not synced" 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 expect(gl_user.user_synced_attributes_metadata.email_synced).to be_falsey
end end
it "does not have email set as read-only" do
expect(gl_user.read_only_attribute?(:email)).to be_falsey
end
end end
end end
...@@ -508,11 +529,15 @@ describe Gitlab::OAuth::User do ...@@ -508,11 +529,15 @@ describe Gitlab::OAuth::User do
expect(gl_user.email).to eq(info_hash[:email]) expect(gl_user.email).to eq(info_hash[:email])
end end
it "has email_synced_attribute set to true" do it "has email set as synced" do
expect(gl_user.user_synced_attributes_metadata.email_synced).to be(true) expect(gl_user.user_synced_attributes_metadata.email_synced).to be(true)
end end
it "has my-provider as attributes_provider" do it "has email set as read-only" do
expect(gl_user.read_only_attribute?(:email)).to be_truthy
end
it "has synced attributes provider set to my-provider" do
expect(gl_user.user_synced_attributes_metadata.provider).to eql 'my-provider' expect(gl_user.user_synced_attributes_metadata.provider).to eql 'my-provider'
end end
end end
...@@ -524,7 +549,14 @@ describe Gitlab::OAuth::User do ...@@ -524,7 +549,14 @@ describe Gitlab::OAuth::User do
it "does not update the user email" do it "does not update the user email" do
expect(gl_user.email).not_to eq(info_hash[:email]) expect(gl_user.email).not_to eq(info_hash[:email])
expect(gl_user.user_synced_attributes_metadata.email_synced).to be(false) end
it "has email set as not synced" do
expect(gl_user.user_synced_attributes_metadata.email_synced).to be_falsey
end
it "does not have email set as read-only" do
expect(gl_user.read_only_attribute?(:email)).to be_falsey
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment