From 10444f61f85219eb6b2c10586996717d3b0afa8b Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Tue, 28 Jun 2016 18:19:04 -0500 Subject: [PATCH 1/2] Fixed 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. --- doc/integration/omniauth.md | 12 +++++++++--- lib/gitlab/o_auth/user.rb | 2 -- spec/lib/gitlab/o_auth/user_spec.rb | 17 +++++++++++++++-- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/doc/integration/omniauth.md b/doc/integration/omniauth.md index 820f40f81a..46b260e703 100644 --- a/doc/integration/omniauth.md +++ b/doc/integration/omniauth.md @@ -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 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 -projects. You will need to use the full name of the provider, like `google_oauth2` -for Google. Refer to the examples for the full names of the supported providers. +**creating accounts, or logging in via these providers** will not be able to have +access to internal projects. You will need to use the full name of the provider, +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** diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 7af75a9cc4..0a91d3918d 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -56,8 +56,6 @@ module Gitlab if external_provider? && @user @user.external = true - elsif @user - @user.external = false end @user diff --git a/spec/lib/gitlab/o_auth/user_spec.rb b/spec/lib/gitlab/o_auth/user_spec.rb index 6727a83e58..fbb5895c2e 100644 --- a/spec/lib/gitlab/o_auth/user_spec.rb +++ b/spec/lib/gitlab/o_auth/user_spec.rb @@ -51,12 +51,25 @@ describe Gitlab::OAuth::User, lib: true do end 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) stub_omniauth_config(allow_single_sign_on: ['twitter'], external_providers: ['facebook']) oauth_user.save 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 From db0d3fc3e96e5f2b0f642ea3240d5265c3ee659c Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 27 Jun 2016 13:24:08 +0100 Subject: [PATCH 2/2] Ensure logged-out users can't see private refs --- CHANGELOG | 3 ++ app/models/concerns/mentionable.rb | 2 +- app/services/todo_service.rb | 2 +- spec/models/concerns/mentionable_spec.rb | 37 ++++++++++++++++++++++++ 4 files changed, 42 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 775ea60681..eb5a5f7fcf 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -23,6 +23,9 @@ v 8.10.0 (unreleased) - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w) - Add basic system information like memory and disk usage to the admin panel +v 8.9.4 (unreleased) + - Ensure references to private repos aren't shown to logged-out users + v 8.9.3 - Fix encrypted data backwards compatibility after upgrading attr_encrypted gem. !4963 - Fix rendering of commit notes. !4953 diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index f00b5b8497..8cac47246d 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -45,7 +45,7 @@ module Mentionable def all_references(current_user = nil, text = nil, extractor: nil) extractor ||= Gitlab::ReferenceExtractor. - new(project, current_user || author) + new(project, current_user) if text extractor.analyze(text, author: author) diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 239bd17a03..6bb0a72d30 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -237,7 +237,7 @@ class TodoService end def filter_mentioned_users(project, target, author) - mentioned_users = target.mentioned_users + mentioned_users = target.mentioned_users(author) mentioned_users = reject_users_without_access(mentioned_users, project, target) mentioned_users.delete(author) mentioned_users.uniq diff --git a/spec/models/concerns/mentionable_spec.rb b/spec/models/concerns/mentionable_spec.rb index cb33edde82..0344dae8b5 100644 --- a/spec/models/concerns/mentionable_spec.rb +++ b/spec/models/concerns/mentionable_spec.rb @@ -29,6 +29,43 @@ describe Issue, "Mentionable" do it { is_expected.not_to include(user2) } end + describe '#referenced_mentionables' do + context 'with an issue on a private project' do + let(:project) { create(:empty_project, :public) } + let(:issue) { create(:issue, project: project) } + let(:public_issue) { create(:issue, project: project) } + let(:private_project) { create(:empty_project, :private) } + let(:private_issue) { create(:issue, project: private_project) } + let(:user) { create(:user) } + + def referenced_issues(current_user) + text = "#{private_issue.to_reference(project)} and #{public_issue.to_reference}" + + issue.referenced_mentionables(current_user, text) + end + + context 'when the current user can see the issue' do + before { private_project.team << [user, Gitlab::Access::DEVELOPER] } + + it 'includes the reference' do + expect(referenced_issues(user)).to contain_exactly(private_issue, public_issue) + end + end + + context 'when the current user cannot see the issue' do + it 'does not include the reference' do + expect(referenced_issues(user)).to contain_exactly(public_issue) + end + end + + context 'when there is no current user' do + it 'does not include the reference' do + expect(referenced_issues(nil)).to contain_exactly(public_issue) + end + end + end + end + describe '#create_cross_references!' do let(:project) { create(:project) } let(:author) { double('author') }