From 294fa6fcdcfa7d76bc97b754d2930f3686f54997 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@selenight.nl>
Date: Thu, 12 Oct 2017 11:01:12 +0200
Subject: [PATCH] Remove authentication using user.private_token

---
 app/controllers/application_controller.rb     |  7 +-
 doc/topics/autodevops/index.md                |  2 +-
 lib/api/api_guard.rb                          | 22 ++---
 .../application_controller_spec.rb            | 86 ++++++-------------
 spec/features/atom/dashboard_issues_spec.rb   |  6 +-
 spec/features/atom/dashboard_spec.rb          |  6 +-
 spec/features/atom/issues_spec.rb             |  6 +-
 spec/features/atom/users_spec.rb              |  6 +-
 spec/features/profile_spec.rb                 | 14 ---
 .../concerns/token_authenticatable_spec.rb    |  2 +-
 spec/models/user_spec.rb                      |  9 --
 spec/requests/api/doorkeeper_access_spec.rb   |  2 +-
 spec/requests/api/helpers_spec.rb             | 53 +-----------
 spec/requests/api/users_spec.rb               | 26 ------
 spec/support/api_helpers.rb                   | 28 +++---
 15 files changed, 75 insertions(+), 200 deletions(-)

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 391a0519195..3be7aee69bc 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -11,7 +11,7 @@ class ApplicationController < ActionController::Base
   include EnforcesTwoFactorAuthentication
   include WithPerformanceBar
 
-  before_action :authenticate_user_from_private_token!
+  before_action :authenticate_user_from_personal_access_token!
   before_action :authenticate_user_from_rss_token!
   before_action :authenticate_user!
   before_action :validate_user_service_ticket!
@@ -100,13 +100,12 @@ class ApplicationController < ActionController::Base
     return try(:authenticated_user)
   end
 
-  # This filter handles both private tokens and personal access tokens
-  def authenticate_user_from_private_token!
+  def authenticate_user_from_personal_access_token!
     token = params[:private_token].presence || request.headers['PRIVATE-TOKEN'].presence
 
     return unless token.present?
 
-    user = User.find_by_authentication_token(token) || User.find_by_personal_access_token(token)
+    user = User.find_by_personal_access_token(token)
 
     sessionless_sign_in(user)
   end
diff --git a/doc/topics/autodevops/index.md b/doc/topics/autodevops/index.md
index 5561784ed0b..042cde3f01e 100644
--- a/doc/topics/autodevops/index.md
+++ b/doc/topics/autodevops/index.md
@@ -517,7 +517,7 @@ Feature.get(:auto_devops_banner_disabled).enable
 Or through the HTTP API with an admin access token:
 
 ```sh
-curl --data "value=true" --header "PRIVATE-TOKEN: private_token" https://gitlab.example.com/api/v4/features/auto_devops_banner_disabled
+curl --data "value=true" --header "PRIVATE-TOKEN: personal_access_token" https://gitlab.example.com/api/v4/features/auto_devops_banner_disabled
 ```
 
 [ce-37115]: https://gitlab.com/gitlab-org/gitlab-ce/issues/37115
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 87b9db66efd..0ff376bbab6 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -44,7 +44,7 @@ module API
     module HelperMethods
       def find_current_user
         user =
-          find_user_from_private_token ||
+          find_user_from_personal_access_token ||
           find_user_from_oauth_token ||
           find_user_from_warden
 
@@ -61,13 +61,14 @@ module API
 
       private
 
-      def find_user_from_private_token
+      def find_user_from_personal_access_token
         token_string = private_token.to_s
         return nil unless token_string.present?
 
-        user =
-          find_user_by_authentication_token(token_string) ||
-          find_user_by_personal_access_token(token_string)
+        access_token = PersonalAccessToken.find_by_token(token_string)
+        raise UnauthorizedError unless access_token
+
+        user = find_user_by_access_token(access_token)
 
         raise UnauthorizedError unless user
 
@@ -99,17 +100,6 @@ module API
         find_user_by_access_token(access_token)
       end
 
-      def find_user_by_authentication_token(token_string)
-        User.find_by_authentication_token(token_string)
-      end
-
-      def find_user_by_personal_access_token(token_string)
-        access_token = PersonalAccessToken.find_by_token(token_string)
-        return unless access_token
-
-        find_user_by_access_token(access_token)
-      end
-
       # Check the Rails session for valid authentication details
       def find_user_from_warden
         warden.try(:authenticate) if verified_request?
diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
index 6802b839eaa..b73ca0c2346 100644
--- a/spec/controllers/application_controller_spec.rb
+++ b/spec/controllers/application_controller_spec.rb
@@ -50,70 +50,36 @@ describe ApplicationController do
     end
   end
 
-  describe "#authenticate_user_from_token!" do
-    describe "authenticating a user from a private token" do
-      controller(described_class) do
-        def index
-          render text: "authenticated"
-        end
-      end
-
-      context "when the 'private_token' param is populated with the private token" do
-        it "logs the user in" do
-          get :index, private_token: user.private_token
-          expect(response).to have_gitlab_http_status(200)
-          expect(response.body).to eq("authenticated")
-        end
-      end
-
-      context "when the 'PRIVATE-TOKEN' header is populated with the private token" do
-        it "logs the user in" do
-          @request.headers['PRIVATE-TOKEN'] = user.private_token
-          get :index
-          expect(response).to have_gitlab_http_status(200)
-          expect(response.body).to eq("authenticated")
-        end
-      end
-
-      it "doesn't log the user in otherwise" do
-        @request.headers['PRIVATE-TOKEN'] = "token"
-        get :index, private_token: "token", authenticity_token: "token"
-        expect(response.status).not_to eq(200)
-        expect(response.body).not_to eq("authenticated")
+  describe "#authenticate_user_from_personal_access_token!" do
+    controller(described_class) do
+      def index
+        render text: 'authenticated'
       end
     end
 
-    describe "authenticating a user from a personal access token" do
-      controller(described_class) do
-        def index
-          render text: 'authenticated'
-        end
-      end
-
-      let(:personal_access_token) { create(:personal_access_token, user: user) }
+    let(:personal_access_token) { create(:personal_access_token, user: user) }
 
-      context "when the 'personal_access_token' param is populated with the personal access token" do
-        it "logs the user in" do
-          get :index, private_token: personal_access_token.token
-          expect(response).to have_gitlab_http_status(200)
-          expect(response.body).to eq('authenticated')
-        end
+    context "when the 'personal_access_token' param is populated with the personal access token" do
+      it "logs the user in" do
+        get :index, private_token: personal_access_token.token
+        expect(response).to have_gitlab_http_status(200)
+        expect(response.body).to eq('authenticated')
       end
+    end
 
-      context "when the 'PERSONAL_ACCESS_TOKEN' header is populated with the personal access token" do
-        it "logs the user in" do
-          @request.headers["PRIVATE-TOKEN"] = personal_access_token.token
-          get :index
-          expect(response).to have_gitlab_http_status(200)
-          expect(response.body).to eq('authenticated')
-        end
+    context "when the 'PERSONAL_ACCESS_TOKEN' header is populated with the personal access token" do
+      it "logs the user in" do
+        @request.headers["PRIVATE-TOKEN"] = personal_access_token.token
+        get :index
+        expect(response).to have_gitlab_http_status(200)
+        expect(response.body).to eq('authenticated')
       end
+    end
 
-      it "doesn't log the user in otherwise" do
-        get :index, private_token: "token"
-        expect(response.status).not_to eq(200)
-        expect(response.body).not_to eq('authenticated')
-      end
+    it "doesn't log the user in otherwise" do
+      get :index, private_token: "token"
+      expect(response.status).not_to eq(200)
+      expect(response.body).not_to eq('authenticated')
     end
   end
 
@@ -152,11 +118,15 @@ describe ApplicationController do
       end
     end
 
+    before do
+      sign_in user
+    end
+
     context 'when format is handled' do
       let(:requested_format) { :json }
 
       it 'returns 200 response' do
-        get :index, private_token: user.private_token, format: requested_format
+        get :index, format: requested_format
 
         expect(response).to have_gitlab_http_status 200
       end
@@ -164,7 +134,7 @@ describe ApplicationController do
 
     context 'when format is not handled' do
       it 'returns 404 response' do
-        get :index, private_token: user.private_token
+        get :index
 
         expect(response).to have_gitlab_http_status 404
       end
diff --git a/spec/features/atom/dashboard_issues_spec.rb b/spec/features/atom/dashboard_issues_spec.rb
index 5aae2dbaf91..89c9d377003 100644
--- a/spec/features/atom/dashboard_issues_spec.rb
+++ b/spec/features/atom/dashboard_issues_spec.rb
@@ -13,8 +13,10 @@ describe "Dashboard Issues Feed"  do
     end
 
     describe "atom feed" do
-      it "renders atom feed via private token" do
-        visit issues_dashboard_path(:atom, private_token: user.private_token)
+      it "renders atom feed via personal access token" do
+        personal_access_token = create(:personal_access_token, user: user)
+
+        visit issues_dashboard_path(:atom, private_token: personal_access_token.token)
 
         expect(response_headers['Content-Type']).to have_content('application/atom+xml')
         expect(body).to have_selector('title', text: "#{user.name} issues")
diff --git a/spec/features/atom/dashboard_spec.rb b/spec/features/atom/dashboard_spec.rb
index 321c8a2a670..2c0c331b6db 100644
--- a/spec/features/atom/dashboard_spec.rb
+++ b/spec/features/atom/dashboard_spec.rb
@@ -4,9 +4,11 @@ describe "Dashboard Feed"  do
   describe "GET /" do
     let!(:user) { create(:user, name: "Jonh") }
 
-    context "projects atom feed via private token" do
+    context "projects atom feed via personal access token" do
       it "renders projects atom feed" do
-        visit dashboard_projects_path(:atom, private_token: user.private_token)
+        personal_access_token = create(:personal_access_token, user: user)
+
+        visit dashboard_projects_path(:atom, private_token: personal_access_token.token)
         expect(body).to have_selector('feed title')
       end
     end
diff --git a/spec/features/atom/issues_spec.rb b/spec/features/atom/issues_spec.rb
index 3eeb4d35131..4102ac0588a 100644
--- a/spec/features/atom/issues_spec.rb
+++ b/spec/features/atom/issues_spec.rb
@@ -28,10 +28,12 @@ describe 'Issues Feed'  do
       end
     end
 
-    context 'when authenticated via private token' do
+    context 'when authenticated via personal access token' do
       it 'renders atom feed' do
+        personal_access_token = create(:personal_access_token, user: user)
+
         visit project_issues_path(project, :atom,
-                                            private_token: user.private_token)
+                                            private_token: personal_access_token.token)
 
         expect(response_headers['Content-Type'])
           .to have_content('application/atom+xml')
diff --git a/spec/features/atom/users_spec.rb b/spec/features/atom/users_spec.rb
index 9ce687afb31..2b934d81674 100644
--- a/spec/features/atom/users_spec.rb
+++ b/spec/features/atom/users_spec.rb
@@ -4,9 +4,11 @@ describe "User Feed"  do
   describe "GET /" do
     let!(:user) { create(:user) }
 
-    context 'user atom feed via private token' do
+    context 'user atom feed via personal access token' do
       it "renders user atom feed" do
-        visit user_path(user, :atom, private_token: user.private_token)
+        personal_access_token = create(:personal_access_token, user: user)
+
+        visit user_path(user, :atom, private_token: personal_access_token.token)
         expect(body).to have_selector('feed title')
       end
     end
diff --git a/spec/features/profile_spec.rb b/spec/features/profile_spec.rb
index 1cddd35fd8a..e05853110fe 100644
--- a/spec/features/profile_spec.rb
+++ b/spec/features/profile_spec.rb
@@ -56,20 +56,6 @@ describe 'Profile account page' do
     end
   end
 
-  describe 'when I reset private token' do
-    before do
-      visit profile_account_path
-    end
-
-    it 'resets private token' do
-      previous_token = find("#private-token").value
-
-      click_link('Reset private token')
-
-      expect(find('#private-token').value).not_to eq(previous_token)
-    end
-  end
-
   describe 'when I reset RSS token' do
     before do
       visit profile_account_path
diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb
index 882afeccfc6..dfb83578fce 100644
--- a/spec/models/concerns/token_authenticatable_spec.rb
+++ b/spec/models/concerns/token_authenticatable_spec.rb
@@ -12,7 +12,7 @@ shared_examples 'TokenAuthenticatable' do
 end
 
 describe User, 'TokenAuthenticatable' do
-  let(:token_field) { :authentication_token }
+  let(:token_field) { :rss_token }
   it_behaves_like 'TokenAuthenticatable'
 
   describe 'ensures authentication token' do
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 1c3c9068f12..fb03e320734 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -346,7 +346,6 @@ describe User do
   describe "Respond to" do
     it { is_expected.to respond_to(:admin?) }
     it { is_expected.to respond_to(:name) }
-    it { is_expected.to respond_to(:private_token) }
     it { is_expected.to respond_to(:external?) }
   end
 
@@ -526,14 +525,6 @@ describe User do
     end
   end
 
-  describe 'authentication token' do
-    it "has authentication token" do
-      user = create(:user)
-
-      expect(user.authentication_token).not_to be_blank
-    end
-  end
-
   describe 'ensure incoming email token' do
     it 'has incoming email token' do
       user = create(:user)
diff --git a/spec/requests/api/doorkeeper_access_spec.rb b/spec/requests/api/doorkeeper_access_spec.rb
index de7ce848a31..174593593f8 100644
--- a/spec/requests/api/doorkeeper_access_spec.rb
+++ b/spec/requests/api/doorkeeper_access_spec.rb
@@ -25,7 +25,7 @@ describe 'doorkeeper access' do
     end
   end
 
-  describe "authorization by private token" do
+  describe "authorization by OAuth token" do
     it "returns authentication success" do
       get api("/user", user)
       expect(response).to have_gitlab_http_status(200)
diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index 9f3b5a809d7..0ab9f94376c 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -28,17 +28,17 @@ describe API::Helpers do
     allow_any_instance_of(self.class).to receive(:options).and_return({})
   end
 
-  def set_env(user_or_token, identifier)
+  def set_env(token, identifier)
     clear_env
     clear_param
-    env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
+    env[API::APIGuard::PRIVATE_TOKEN_HEADER] = token
     env[API::Helpers::SUDO_HEADER] = identifier.to_s
   end
 
-  def set_param(user_or_token, identifier)
+  def set_param(token, identifier)
     clear_env
     clear_param
-    params[API::APIGuard::PRIVATE_TOKEN_PARAM] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
+    params[API::APIGuard::PRIVATE_TOKEN_PARAM] = token
     params[API::Helpers::SUDO_PARAM] = identifier.to_s
   end
 
@@ -160,41 +160,6 @@ describe API::Helpers do
       end
     end
 
-    describe "when authenticating using a user's private token" do
-      it "returns a 401 response for an invalid token" do
-        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token'
-        allow_any_instance_of(self.class).to receive(:doorkeeper_guard) { false }
-
-        expect { current_user }.to raise_error /401/
-      end
-
-      it "returns a 401 response for a user without access" do
-        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token
-        allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
-
-        expect { current_user }.to raise_error /401/
-      end
-
-      it 'returns a 401 response for a user who is blocked' do
-        user.block!
-        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token
-
-        expect { current_user }.to raise_error /401/
-      end
-
-      it "leaves user as is when sudo not specified" do
-        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token
-
-        expect(current_user).to eq(user)
-
-        clear_env
-
-        params[API::APIGuard::PRIVATE_TOKEN_PARAM] = user.private_token
-
-        expect(current_user).to eq(user)
-      end
-    end
-
     describe "when authenticating using a user's personal access tokens" do
       let(:personal_access_token) { create(:personal_access_token, user: user) }
 
@@ -445,16 +410,6 @@ describe API::Helpers do
           expect { sudo? }.to raise_error '403 - {"message"=>"403 Forbidden  - Private token must be specified in order to use sudo"}'
         end
       end
-
-      context 'private access token is used' do
-        before do
-          set_env(admin.private_token, user.id)
-        end
-
-        it 'returns true' do
-          expect(sudo?).to be_truthy
-        end
-      end
     end
   end
 
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 4737f034f21..fc1d055afe2 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -1097,14 +1097,6 @@ describe API::Users do
         end
       end
 
-      context 'with private token' do
-        it 'returns 403 without private token when sudo defined' do
-          get api("/user?private_token=#{user.private_token}&sudo=123")
-
-          expect(response).to have_gitlab_http_status(403)
-        end
-      end
-
       it 'returns current user without private token when sudo not defined' do
         get api("/user", user)
 
@@ -1139,24 +1131,6 @@ describe API::Users do
           expect(json_response['id']).to eq(admin.id)
         end
       end
-
-      context 'with private token' do
-        it 'returns sudoed user with private token when sudo defined' do
-          get api("/user?private_token=#{admin.private_token}&sudo=#{user.id}")
-
-          expect(response).to have_gitlab_http_status(200)
-          expect(response).to match_response_schema('public_api/v4/user/login')
-          expect(json_response['id']).to eq(user.id)
-        end
-
-        it 'returns initial current user without private token but with is_admin when sudo not defined' do
-          get api("/user?private_token=#{admin.private_token}")
-
-          expect(response).to have_gitlab_http_status(200)
-          expect(response).to match_response_schema('public_api/v4/user/admin')
-          expect(json_response['id']).to eq(admin.id)
-        end
-      end
     end
 
     context 'with unauthenticated user' do
diff --git a/spec/support/api_helpers.rb b/spec/support/api_helpers.rb
index 01aca74274c..ac0c7a9b493 100644
--- a/spec/support/api_helpers.rb
+++ b/spec/support/api_helpers.rb
@@ -18,21 +18,23 @@ module ApiHelpers
   #
   # Returns the relative path to the requested API resource
   def api(path, user = nil, version: API::API.version, personal_access_token: nil, oauth_access_token: nil)
-    "/api/#{version}#{path}" +
+    full_path = "/api/#{version}#{path}"
 
-      # Normalize query string
-      (path.index('?') ? '' : '?') +
+    if oauth_access_token
+      query_string = "access_token=#{oauth_access_token.token}"
+    elsif personal_access_token
+      query_string = "private_token=#{personal_access_token.token}"
+    elsif user
+      personal_access_token = create(:personal_access_token, user: user)
+      query_string = "private_token=#{personal_access_token.token}"
+    end
 
-      if personal_access_token.present?
-        "&private_token=#{personal_access_token.token}"
-      elsif oauth_access_token.present?
-        "&access_token=#{oauth_access_token.token}"
-      # Append private_token if given a User object
-      elsif user.respond_to?(:private_token)
-        "&private_token=#{user.private_token}"
-      else
-        ''
-      end
+    if query_string
+      full_path << (path.index('?') ? '&' : '?')
+      full_path << query_string
+    end
+
+    full_path
   end
 
   # Temporary helper method for simplifying V3 exclusive API specs
-- 
GitLab