Commit 5e6342b7 authored by Douwe Maan's avatar Douwe Maan

Merge branch '19312-confidential-issue' into 'master'

Fix privilege escalation issue with OAuth external users

Related to https://gitlab.com/gitlab-org/gitlab-ce/issues/19312

This MR fixes a privilege escalation issue, where manually set external users would be reverted back to internal users if they logged in via OAuth and that provider was not in the `external_providers` list.

/cc @douwe 

See merge request !1975
parents 293cf090 10444f61
...@@ -127,9 +127,15 @@ The chosen OmniAuth provider is now active and can be used to sign in to GitLab ...@@ -127,9 +127,15 @@ The chosen OmniAuth provider is now active and can be used to sign in to GitLab
This setting was introduced with version 8.7 of GitLab This setting was introduced with version 8.7 of GitLab
You can define which OmniAuth providers you want to be `external` so that all users You can define which OmniAuth providers you want to be `external` so that all users
creating accounts via these providers will not be able to have access to internal **creating accounts, or logging in via these providers** will not be able to have
projects. You will need to use the full name of the provider, like `google_oauth2` access to internal projects. You will need to use the full name of the provider,
for Google. Refer to the examples for the full names of the supported providers. like `google_oauth2` for Google. Refer to the examples for the full names of the
supported providers.
>**Note:**
If you decide to remove an OmniAuth provider from the external providers list
you will need to manually update the users that use this method to login, if you
want their accounts to be upgraded to full internal accounts.
**For Omnibus installations** **For Omnibus installations**
......
...@@ -56,8 +56,6 @@ module Gitlab ...@@ -56,8 +56,6 @@ module Gitlab
if external_provider? && @user if external_provider? && @user
@user.external = true @user.external = true
elsif @user
@user.external = false
end end
@user @user
......
...@@ -51,12 +51,25 @@ describe Gitlab::OAuth::User, lib: true do ...@@ -51,12 +51,25 @@ describe Gitlab::OAuth::User, lib: true do
end end
context 'provider was external, now has been removed' do context 'provider was external, now has been removed' do
it 'should mark existing user internal' do it 'should not mark external user as internal' do
create(:omniauth_user, extern_uid: 'my-uid', provider: 'twitter', external: true) create(:omniauth_user, extern_uid: 'my-uid', provider: 'twitter', external: true)
stub_omniauth_config(allow_single_sign_on: ['twitter'], external_providers: ['facebook']) stub_omniauth_config(allow_single_sign_on: ['twitter'], external_providers: ['facebook'])
oauth_user.save oauth_user.save
expect(gl_user).to be_valid expect(gl_user).to be_valid
expect(gl_user.external).to be_falsey expect(gl_user.external).to be_truthy
end
end
context 'provider is not external' do
context 'when adding a new OAuth identity' do
it 'should not promote an external user to internal' do
user = create(:user, email: 'john@mail.com', external: true)
user.identities.create(provider: provider, extern_uid: uid)
oauth_user.save
expect(gl_user).to be_valid
expect(gl_user.external).to be_truthy
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