From 20f679d620380b5b5e662b790c76caf256867b01 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 26 Jun 2017 07:20:30 +0000
Subject: Allow unauthenticated access to the `/api/v4/users` API.

- The issue filtering frontend code needs access to this API for non-logged-in
  users + public projects. It uses the API to fetch information for a user by
  username.

- We don't authenticate this API anymore, but instead - if the `current_user` is
  not present:

  - Verify that the `username` parameter has been passed. This disallows an
    unauthenticated user from grabbing a list of all users on the instance. The
    `UsersFinder` class performs an exact match on the `username`, so we are
    guaranteed to get 0 or 1 users.
  - Verify that the resulting user (if any) is accessible to be viewed publicly
    by calling `can?(current_user, :read_user, user)`
---
 app/finders/users_finder.rb     |  7 +++++--
 lib/api/helpers.rb              |  6 ++++++
 lib/api/users.rb                | 23 +++++++++++++++++------
 spec/requests/api/users_spec.rb | 35 +++++++++++++++++++++++++++++++++--
 4 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/app/finders/users_finder.rb b/app/finders/users_finder.rb
index dbd50d1db7c..0534317df8f 100644
--- a/app/finders/users_finder.rb
+++ b/app/finders/users_finder.rb
@@ -27,8 +27,11 @@ class UsersFinder
     users = by_search(users)
     users = by_blocked(users)
     users = by_active(users)
-    users = by_external_identity(users)
-    users = by_external(users)
+
+    if current_user
+      users = by_external_identity(users)
+      users = by_external(users)
+    end
 
     users
   end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 2c73a6fdc4e..1322afaa64f 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -407,5 +407,11 @@ module API
 
       exception.status == 500
     end
+
+    # Does the current route match the route identified by
+    # `description`?
+    def route_matches_description?(description)
+      options.dig(:route_options, :description) == description
+    end
   end
 end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index c10e3364382..34619c90d8b 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -4,7 +4,7 @@ module API
 
     before do
       allow_access_with_scope :read_user if request.get?
-      authenticate!
+      authenticate! unless route_matches_description?("Get the list of users")
     end
 
     resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
@@ -51,15 +51,26 @@ module API
         use :pagination
       end
       get do
-        unless can?(current_user, :read_users_list)
-          render_api_error!("Not authorized.", 403)
-        end
-
         authenticated_as_admin! if params[:external].present? || (params[:extern_uid].present? && params[:provider].present?)
 
         users = UsersFinder.new(current_user, params).execute
 
-        entity = current_user.admin? ? Entities::UserWithAdmin : Entities::UserBasic
+        authorized =
+          if current_user
+            can?(current_user, :read_users_list)
+          else
+            # When `current_user` is not present, require that the `username`
+            # parameter is passed, to prevent an unauthenticated user from accessing
+            # a list of all the users on the GitLab instance. `UsersFinder` performs
+            # an exact match on the `username` parameter, so we are guaranteed to
+            # get either 0 or 1 `users` here.
+            params[:username].present? &&
+              users.all? { |user| can?(current_user, :read_user, user) }
+          end
+
+        render_api_error!("Not authorized.", 403) unless authorized
+
+        entity = current_user.try(:admin?) ? Entities::UserWithAdmin : Entities::UserBasic
         present paginate(users), with: entity
       end
 
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 18000d91795..01541901330 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -13,9 +13,40 @@ describe API::Users do
 
   describe 'GET /users' do
     context "when unauthenticated" do
-      it "returns authentication error" do
+      it "returns authorization error when the `username` parameter is not passed" do
         get api("/users")
-        expect(response).to have_http_status(401)
+
+        expect(response).to have_http_status(403)
+      end
+
+      it "returns the user when a valid `username` parameter is passed" do
+        user = create(:user)
+
+        get api("/users"), username: user.username
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(json_response.size).to eq(1)
+        expect(json_response[0]['id']).to eq(user.id)
+        expect(json_response[0]['username']).to eq(user.username)
+      end
+
+      it "returns authorization error when the `username` parameter refers to an inaccessible user" do
+        user = create(:user)
+
+        expect(Ability).to receive(:allowed?).with(nil, :read_user, user).and_return(false)
+
+        get api("/users"), username: user.username
+
+        expect(response).to have_http_status(403)
+      end
+
+      it "returns an empty response when an invalid `username` parameter is passed" do
+        get api("/users"), username: 'invalid'
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(json_response.size).to eq(0)
       end
     end
 
-- 
cgit v1.2.1


From c39e4ccfb7cb76b9bdb613399aba2c2467b77751 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 26 Jun 2017 08:43:12 +0000
Subject: Add CHANGELOG entry for CE MR 12445

---
 .../34141-allow-unauthenticated-access-to-the-users-api.yml           | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 changelogs/unreleased/34141-allow-unauthenticated-access-to-the-users-api.yml

diff --git a/changelogs/unreleased/34141-allow-unauthenticated-access-to-the-users-api.yml b/changelogs/unreleased/34141-allow-unauthenticated-access-to-the-users-api.yml
new file mode 100644
index 00000000000..a3ade8db214
--- /dev/null
+++ b/changelogs/unreleased/34141-allow-unauthenticated-access-to-the-users-api.yml
@@ -0,0 +1,4 @@
+---
+title: Allow unauthenticated access to the /api/v4/users API
+merge_request: 12445
+author:
-- 
cgit v1.2.1


From 3c88a7869b87693ba8c3fb9814d39437dd569a31 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 29 Jun 2017 07:43:41 +0000
Subject: Implement review comments for !12445 from @godfat and @rymai.

- Use `GlobalPolicy` to authorize the users that a non-authenticated user can
  fetch from `/api/v4/users`. We allow access if the `Gitlab::VisibilityLevel::PUBLIC`
  visibility level is not restricted.

- Further, as before, `/api/v4/users` is only accessible to unauthenticated users if
  the `username` parameter is passed.

- Turn off `authenticate!` for the `/api/v4/users` endpoint by matching on the actual
  route + method, rather than the description.

- Change the type of `current_user` check in `UsersFinder` to be more
  compatible with EE.
---
 app/finders/users_finder.rb     | 11 ++++-------
 app/policies/base_policy.rb     |  6 ++++++
 app/policies/global_policy.rb   |  3 ++-
 app/policies/user_policy.rb     |  6 ------
 lib/api/helpers.rb              |  4 ++--
 lib/api/users.rb                | 26 +++++++++++---------------
 spec/requests/api/users_spec.rb |  2 +-
 7 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/app/finders/users_finder.rb b/app/finders/users_finder.rb
index 0534317df8f..07deceb827b 100644
--- a/app/finders/users_finder.rb
+++ b/app/finders/users_finder.rb
@@ -27,11 +27,8 @@ class UsersFinder
     users = by_search(users)
     users = by_blocked(users)
     users = by_active(users)
-
-    if current_user
-      users = by_external_identity(users)
-      users = by_external(users)
-    end
+    users = by_external_identity(users)
+    users = by_external(users)
 
     users
   end
@@ -63,13 +60,13 @@ class UsersFinder
   end
 
   def by_external_identity(users)
-    return users unless current_user.admin? && params[:extern_uid] && params[:provider]
+    return users unless current_user&.admin? && params[:extern_uid] && params[:provider]
 
     users.joins(:identities).merge(Identity.with_extern_uid(params[:provider], params[:extern_uid]))
   end
 
   def by_external(users)
-    return users = users.where.not(external: true) unless current_user.admin?
+    return users = users.where.not(external: true) unless current_user&.admin?
     return users unless params[:external]
 
     users.external
diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb
index 623424c63e0..261a2e780c5 100644
--- a/app/policies/base_policy.rb
+++ b/app/policies/base_policy.rb
@@ -1,4 +1,6 @@
 class BasePolicy
+  include Gitlab::CurrentSettings
+
   class RuleSet
     attr_reader :can_set, :cannot_set
     def initialize(can_set, cannot_set)
@@ -124,4 +126,8 @@ class BasePolicy
     yield
     @rule_set
   end
+
+  def restricted_public_level?
+    current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC)
+  end
 end
diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb
index 2683aaad981..e9be43a5037 100644
--- a/app/policies/global_policy.rb
+++ b/app/policies/global_policy.rb
@@ -1,9 +1,10 @@
 class GlobalPolicy < BasePolicy
   def rules
+    can! :read_users_list unless restricted_public_level?
+
     return unless @user
 
     can! :create_group if @user.can_create_group
-    can! :read_users_list
 
     unless @user.blocked? || @user.internal?
       can! :log_in unless @user.access_locked?
diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb
index 229846e368c..265c56aba53 100644
--- a/app/policies/user_policy.rb
+++ b/app/policies/user_policy.rb
@@ -1,6 +1,4 @@
 class UserPolicy < BasePolicy
-  include Gitlab::CurrentSettings
-
   def rules
     can! :read_user if @user || !restricted_public_level?
 
@@ -12,8 +10,4 @@ class UserPolicy < BasePolicy
       cannot! :destroy_user if @subject.ghost?
     end
   end
-
-  def restricted_public_level?
-    current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC)
-  end
 end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 1322afaa64f..a3aec8889d7 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -410,8 +410,8 @@ module API
 
     # Does the current route match the route identified by
     # `description`?
-    def route_matches_description?(description)
-      options.dig(:route_options, :description) == description
+    def request_matches_route?(method, route)
+      request.request_method == method && request.path == route
     end
   end
 end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 34619c90d8b..18ce58299e7 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -4,7 +4,7 @@ module API
 
     before do
       allow_access_with_scope :read_user if request.get?
-      authenticate! unless route_matches_description?("Get the list of users")
+      authenticate! unless request_matches_route?('GET', '/api/v4/users')
     end
 
     resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
@@ -55,22 +55,18 @@ module API
 
         users = UsersFinder.new(current_user, params).execute
 
-        authorized =
-          if current_user
-            can?(current_user, :read_users_list)
-          else
-            # When `current_user` is not present, require that the `username`
-            # parameter is passed, to prevent an unauthenticated user from accessing
-            # a list of all the users on the GitLab instance. `UsersFinder` performs
-            # an exact match on the `username` parameter, so we are guaranteed to
-            # get either 0 or 1 `users` here.
-            params[:username].present? &&
-              users.all? { |user| can?(current_user, :read_user, user) }
-          end
+        authorized = can?(current_user, :read_users_list)
+
+        # When `current_user` is not present, require that the `username`
+        # parameter is passed, to prevent an unauthenticated user from accessing
+        # a list of all the users on the GitLab instance. `UsersFinder` performs
+        # an exact match on the `username` parameter, so we are guaranteed to
+        # get either 0 or 1 `users` here.
+        authorized &&= params[:username].present? if current_user.blank?
 
-        render_api_error!("Not authorized.", 403) unless authorized
+        forbidden!("Not authorized to access /api/v4/users") unless authorized
 
-        entity = current_user.try(:admin?) ? Entities::UserWithAdmin : Entities::UserBasic
+        entity = current_user&.admin? ? Entities::UserWithAdmin : Entities::UserBasic
         present paginate(users), with: entity
       end
 
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 01541901330..bf7ed2d3ad6 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -34,7 +34,7 @@ describe API::Users do
       it "returns authorization error when the `username` parameter refers to an inaccessible user" do
         user = create(:user)
 
-        expect(Ability).to receive(:allowed?).with(nil, :read_user, user).and_return(false)
+        stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
 
         get api("/users"), username: user.username
 
-- 
cgit v1.2.1


From 96e986327c4dad9248f9013f191119ffafe4a6d8 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 3 Jul 2017 05:14:00 +0000
Subject: Implement review comments for !12445 from @jneen.

- Fix duplicate `prevent` declaration
- Add spec for `GlobalPolicy`
---
 app/policies/global_policy.rb       |  1 -
 spec/policies/global_policy_spec.rb | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)
 create mode 100644 spec/policies/global_policy_spec.rb

diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb
index 7767d3cccd5..55eefa76d3f 100644
--- a/app/policies/global_policy.rb
+++ b/app/policies/global_policy.rb
@@ -18,7 +18,6 @@ class GlobalPolicy < BasePolicy
     prevent :receive_notifications
     prevent :use_quick_actions
     prevent :create_group
-    prevent :log_in
   end
 
   rule { default }.policy do
diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb
new file mode 100644
index 00000000000..bb0fa0c0e9c
--- /dev/null
+++ b/spec/policies/global_policy_spec.rb
@@ -0,0 +1,34 @@
+require 'spec_helper'
+
+describe GlobalPolicy, models: true do
+  let(:current_user) { create(:user) }
+  let(:user) { create(:user) }
+
+  subject { GlobalPolicy.new(current_user, [user]) }
+
+  describe "reading the list of users" do
+    context "for a logged in user" do
+      it { is_expected.to be_allowed(:read_users_list) }
+    end
+
+    context "for an anonymous user" do
+      let(:current_user) { nil }
+
+      context "when the public level is restricted" do
+        before do
+          stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
+        end
+
+        it { is_expected.not_to be_allowed(:read_users_list) }
+      end
+
+      context "when the public level is not restricted" do
+        before do
+          stub_application_setting(restricted_visibility_levels: [])
+        end
+
+        it { is_expected.to be_allowed(:read_users_list) }
+      end
+    end
+  end
+end
-- 
cgit v1.2.1


From d1488268b2e31b8f3549c6e1e46955619535cd98 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 4 Jul 2017 12:19:48 +0000
Subject: Simplify authentication logic in the v4 users API for !12445.

- Rather than using an explicit check to turn off authentication for the
  `/users` endpoint, simply call `authenticate_non_get!`.

- All `GET` endpoints we wish to restrict already call
  `authenticated_as_admin!`, and so remain inacessible to anonymous users.

- This _does_ open up the `/users/:id` endpoint to anonymous access. It contains
  the same access check that `/users` users, and so is safe for use here.

- More context: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12445#note_34031323
---
 lib/api/helpers.rb              |  6 ------
 lib/api/users.rb                |  9 ++++++++-
 spec/requests/api/users_spec.rb | 20 +++++++++++++++++---
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index a3aec8889d7..2c73a6fdc4e 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -407,11 +407,5 @@ module API
 
       exception.status == 500
     end
-
-    # Does the current route match the route identified by
-    # `description`?
-    def request_matches_route?(method, route)
-      request.request_method == method && request.path == route
-    end
   end
 end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index bad4d76b428..5b9d9a71be4 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -4,10 +4,13 @@ module API
 
     before do
       allow_access_with_scope :read_user if request.get?
-      authenticate! unless request_matches_route?('GET', '/api/v4/users')
     end
 
     resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
+      before do
+        authenticate_non_get!
+      end
+
       helpers do
         def find_user(params)
           id = params[:user_id] || params[:id]
@@ -405,6 +408,10 @@ module API
     end
 
     resource :user do
+      before do
+        authenticate!
+      end
+
       desc 'Get the currently authenticated user' do
         success Entities::UserPublic
       end
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index aa95ae8c7cc..8640c16203e 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -169,6 +169,7 @@ describe API::Users do
   describe "GET /users/:id" do
     it "returns a user by id" do
       get api("/users/#{user.id}", user)
+
       expect(response).to have_http_status(200)
       expect(json_response['username']).to eq(user.username)
     end
@@ -179,9 +180,22 @@ describe API::Users do
       expect(json_response['is_admin']).to be_nil
     end
 
-    it "returns a 401 if unauthenticated" do
-      get api("/users/9998")
-      expect(response).to have_http_status(401)
+    context 'for an anonymous user' do
+      it "returns a user by id" do
+        get api("/users/#{user.id}")
+
+        expect(response).to have_http_status(200)
+        expect(json_response['username']).to eq(user.username)
+      end
+
+      it "returns a 404 if the target user is present but inaccessible" do
+        allow(Ability).to receive(:allowed?).and_call_original
+        allow(Ability).to receive(:allowed?).with(nil, :read_user, user).and_return(false)
+
+        get api("/users/#{user.id}")
+
+        expect(response).to have_http_status(404)
+      end
     end
 
     it "returns a 404 error if user id not found" do
-- 
cgit v1.2.1