Commit 481b8a71 authored by Douwe Maan's avatar Douwe Maan

Make sure user email is read only when synced with LDAP

parent d673628d
...@@ -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).
......
...@@ -230,6 +230,8 @@ In order to enable/disable an OmniAuth provider, go to Admin Area -> Settings -> ...@@ -230,6 +230,8 @@ In order to enable/disable an OmniAuth provider, go to Admin Area -> Settings ->
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']
...@@ -240,5 +242,5 @@ You can enable profile syncing from selected OmniAuth providers and for all or f ...@@ -240,5 +242,5 @@ You can enable profile syncing from selected OmniAuth providers and for all or f
```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']
``` ```
...@@ -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?
metadata = gl_user.user_synced_attributes_metadata || gl_user.build_user_synced_attributes_metadata
if sync_profile_from_provider?
UserSyncedAttributesMetadata::SYNCABLE_ATTRIBUTES.each do |key| UserSyncedAttributesMetadata::SYNCABLE_ATTRIBUTES.each do |key|
if auth_hash.has_attribute?(key) && gl_user.sync_attribute?(key) if auth_hash.has_attribute?(key) && gl_user.sync_attribute?(key)
gl_user[key] = auth_hash.public_send(key) # rubocop:disable GitlabSecurity/PublicSend gl_user[key] = auth_hash.public_send(key) # rubocop:disable GitlabSecurity/PublicSend
user_synced_attributes_metadata.set_attribute_synced(key, true) metadata.set_attribute_synced(key, true)
else else
user_synced_attributes_metadata.set_attribute_synced(key, false) metadata.set_attribute_synced(key, false)
end end
end end
user_synced_attributes_metadata.provider = auth_hash.provider metadata.provider = auth_hash.provider
gl_user.user_synced_attributes_metadata = user_synced_attributes_metadata end
if creating_linked_ldap_user? && gl_user.email == ldap_person.email.first
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