From 93daeee16428707fc348f8c45215854aed6e117a Mon Sep 17 00:00:00 2001
From: Markus Koller <markus-koller@gmx.ch>
Date: Wed, 18 Jan 2017 11:23:25 +0100
Subject: [PATCH] Don't allow blocked users to authenticate through other means

Gitlab::Auth.find_with_user_password is currently used in these places:

- resource_owner_from_credentials in config/initializers/doorkeeper.rb,
  which is used for the OAuth Resource Owner Password Credentials flow

- the /session API call in lib/api/session.rb, which is used to reveal
  the user's current authentication_token

In both cases users should only be authenticated if they're in the
active state.
---
 lib/gitlab/auth.rb                          |  2 +-
 spec/lib/gitlab/auth_spec.rb                | 12 +++++++++++
 spec/requests/api/doorkeeper_access_spec.rb | 18 +++++++++++++++++
 spec/requests/api/oauth_tokens_spec.rb      | 22 +++++++++++++++++++++
 spec/requests/api/session_spec.rb           | 18 +++++++++++++++++
 spec/requests/git_http_spec.rb              | 12 +++++++++--
 6 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 6166f8d0dd9..c6f9d0d7b82 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -40,7 +40,7 @@ module Gitlab
 
             Gitlab::LDAP::Authentication.login(login, password)
           else
-            user if user.valid_password?(password)
+            user if user.active? && user.valid_password?(password)
           end
         end
       end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index daf8f5c1d6c..8726ca569ca 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -210,6 +210,18 @@ describe Gitlab::Auth, lib: true do
       end
     end
 
+    it "does not find user in blocked state" do
+      user.block
+
+      expect( gl_auth.find_with_user_password(username, password) ).not_to eql user
+    end
+
+    it "does not find user in ldap_blocked state" do
+      user.ldap_block
+
+      expect( gl_auth.find_with_user_password(username, password) ).not_to eql user
+    end
+
     context "with ldap enabled" do
       before do
         allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true)
diff --git a/spec/requests/api/doorkeeper_access_spec.rb b/spec/requests/api/doorkeeper_access_spec.rb
index 2974875510a..f6fd567eca5 100644
--- a/spec/requests/api/doorkeeper_access_spec.rb
+++ b/spec/requests/api/doorkeeper_access_spec.rb
@@ -39,4 +39,22 @@ describe API::API, api: true do
       end
     end
   end
+
+  describe "when user is blocked" do
+    it "returns authentication error" do
+      user.block
+      get api("/user"), access_token: token.token
+
+      expect(response).to have_http_status(401)
+    end
+  end
+
+  describe "when user is ldap_blocked" do
+    it "returns authentication error" do
+      user.ldap_block
+      get api("/user"), access_token: token.token
+
+      expect(response).to have_http_status(401)
+    end
+  end
 end
diff --git a/spec/requests/api/oauth_tokens_spec.rb b/spec/requests/api/oauth_tokens_spec.rb
index 7e2cc50e591..367225df717 100644
--- a/spec/requests/api/oauth_tokens_spec.rb
+++ b/spec/requests/api/oauth_tokens_spec.rb
@@ -29,5 +29,27 @@ describe API::API, api: true  do
         expect(json_response['access_token']).not_to be_nil
       end
     end
+
+    context "when user is blocked" do
+      it "does not create an access token" do
+        user = create(:user)
+        user.block
+
+        request_oauth_token(user)
+
+        expect(response).to have_http_status(401)
+      end
+    end
+
+    context "when user is ldap_blocked" do
+      it "does not create an access token" do
+        user = create(:user)
+        user.ldap_block
+
+        request_oauth_token(user)
+
+        expect(response).to have_http_status(401)
+      end
+    end
   end
 end
diff --git a/spec/requests/api/session_spec.rb b/spec/requests/api/session_spec.rb
index 794e2b5c04d..28fab2011a5 100644
--- a/spec/requests/api/session_spec.rb
+++ b/spec/requests/api/session_spec.rb
@@ -87,5 +87,23 @@ describe API::Session, api: true  do
         expect(response).to have_http_status(400)
       end
     end
+
+    context "when user is blocked" do
+      it "returns authentication error" do
+        user.block
+        post api("/session"), email: user.username, password: user.password
+
+        expect(response).to have_http_status(401)
+      end
+    end
+
+    context "when user is ldap_blocked" do
+      it "returns authentication error" do
+        user.ldap_block
+        post api("/session"), email: user.username, password: user.password
+
+        expect(response).to have_http_status(401)
+      end
+    end
   end
 end
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 87786e85621..006d6a6af1c 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -221,12 +221,20 @@ describe 'Git HTTP requests', lib: true do
               end
 
               context "when the user is blocked" do
-                it "responds with status 404" do
+                it "responds with status 401" do
                   user.block
                   project.team << [user, :master]
 
                   download(path, env) do |response|
-                    expect(response).to have_http_status(404)
+                    expect(response).to have_http_status(401)
+                  end
+                end
+
+                it "responds with status 401 for unknown projects (no project existence information leak)" do
+                  user.block
+
+                  download('doesnt/exist.git', env) do |response|
+                    expect(response).to have_http_status(401)
                   end
                 end
               end