From 6d45909f03f6cc32f72135ce7ca7b4fd62132c15 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 15 May 2014 09:57:21 +0200 Subject: [PATCH 1/5] Add test for current behavior of current_user --- spec/requests/api/api_helpers_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/requests/api/api_helpers_spec.rb b/spec/requests/api/api_helpers_spec.rb index 6f961d321b..2dcbce09b2 100644 --- a/spec/requests/api/api_helpers_spec.rb +++ b/spec/requests/api/api_helpers_spec.rb @@ -39,6 +39,11 @@ describe API, api: true do end describe ".current_user" do + it "should return nil for an invalid token" do + env[API::APIHelpers::PRIVATE_TOKEN_HEADER] = 'invalid token' + current_user.should be_nil + end + it "should leave user as is when sudo not specified" do env[API::APIHelpers::PRIVATE_TOKEN_HEADER] = user.private_token current_user.should == user From 34fd557055e027b6423241e73b7cf26c741c0b6b Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 15 May 2014 10:17:13 +0200 Subject: [PATCH 2/5] Move user access check to Gitlab::UserAccess --- lib/gitlab/git_access.rb | 13 +------------ lib/gitlab/user_access.rb | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 12 deletions(-) create mode 100644 lib/gitlab/user_access.rb diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index f3e781ac4e..4f49ca4189 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -61,18 +61,7 @@ module Gitlab private def user_allowed?(user) - return false if user.blocked? - - if Gitlab.config.ldap.enabled - if user.ldap_user? - # Check if LDAP user exists and match LDAP user_filter - Gitlab::LDAP::Access.open do |adapter| - return false unless adapter.allowed?(user) - end - end - end - - true + Gitlab::UserAccess.allowed?(user) end end end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb new file mode 100644 index 0000000000..16df21b49b --- /dev/null +++ b/lib/gitlab/user_access.rb @@ -0,0 +1,18 @@ +module Gitlab + module UserAccess + def self.allowed?(user) + return false if user.blocked? + + if Gitlab.config.ldap.enabled + if user.ldap_user? + # Check if LDAP user exists and match LDAP user_filter + Gitlab::LDAP::Access.open do |adapter| + return false unless adapter.allowed?(user) + end + end + end + + true + end + end +end From 02b85fd2366bc6c0d3194ab68e13eb6291733c26 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 15 May 2014 10:03:26 +0200 Subject: [PATCH 3/5] Check user access status in API for current_user --- lib/api/helpers.rb | 5 +++++ spec/requests/api/api_helpers_spec.rb | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 7ee4b9d138..654c1f62c6 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -8,6 +8,11 @@ module API def current_user private_token = (params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]).to_s @current_user ||= User.find_by(authentication_token: private_token) + + unless @current_user && Gitlab::UserAccess.allowed?(@current_user) + return nil + end + identifier = sudo_identifier() # If the sudo is the current user do nothing diff --git a/spec/requests/api/api_helpers_spec.rb b/spec/requests/api/api_helpers_spec.rb index 2dcbce09b2..a486947657 100644 --- a/spec/requests/api/api_helpers_spec.rb +++ b/spec/requests/api/api_helpers_spec.rb @@ -44,6 +44,11 @@ describe API, api: true do current_user.should be_nil end + it "should return nil for a user without access" do + Gitlab::UserAccess.stub(allowed?: false) + current_user.should be_nil + end + it "should leave user as is when sudo not specified" do env[API::APIHelpers::PRIVATE_TOKEN_HEADER] = user.private_token current_user.should == user From 2d8c310f11f6340e043da97dfc10f268a78c2d9e Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 15 May 2014 10:30:50 +0200 Subject: [PATCH 4/5] Make user access test pass for the right reason If we do not set a private token during the test, current_user will be nil because the user is not found, not due to the access check. --- spec/requests/api/api_helpers_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/requests/api/api_helpers_spec.rb b/spec/requests/api/api_helpers_spec.rb index a486947657..e2f222c0d3 100644 --- a/spec/requests/api/api_helpers_spec.rb +++ b/spec/requests/api/api_helpers_spec.rb @@ -45,6 +45,7 @@ describe API, api: true do end it "should return nil for a user without access" do + env[API::APIHelpers::PRIVATE_TOKEN_HEADER] = user.private_token Gitlab::UserAccess.stub(allowed?: false) current_user.should be_nil end From 223a8695be207aa1725d9ae3755e4d0396dfe9f0 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 15 May 2014 10:32:20 +0200 Subject: [PATCH 5/5] Add API access checks to CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index be5da8bebf..b1a7c3effb 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -16,6 +16,7 @@ v 6.9.0 - Two Step MR creation process - Remove unwanted files from satellite working directory with git clean -fdx - Accept merge request via API (sponsored by O'Reilly Media) + - Add more access checks during API calls v 6.8.0 - Ability to at mention users that are participating in issue and merge req. discussion