From fe5f75930e781ef854b458fafa307ebb90a8ed2e Mon Sep 17 00:00:00 2001 From: Cindy Pallares Date: Wed, 28 Nov 2018 19:06:02 +0000 Subject: Merge branch 'security-fix-pat-web-access' into 'master' [master] Resolve "Personal access token with only `read_user` scope can be used to authenticate any web request" See merge request gitlab/gitlabhq!2583 --- app/controllers/application_controller.rb | 23 +--- .../concerns/sessionless_authentication.rb | 28 ++++ app/controllers/dashboard/projects_controller.rb | 1 + app/controllers/dashboard_controller.rb | 3 + app/controllers/graphql_controller.rb | 1 + app/controllers/groups_controller.rb | 3 + app/controllers/projects/commits_controller.rb | 1 + app/controllers/projects/issues_controller.rb | 17 +-- app/controllers/projects/tags_controller.rb | 2 + app/controllers/projects_controller.rb | 2 + app/controllers/users_controller.rb | 1 + .../unreleased/security-fix-pat-web-access.yml | 5 + config/initializers/rack_attack_global.rb | 10 +- lib/gitlab/auth/request_authenticator.rb | 14 +- lib/gitlab/auth/user_auth_finders.rb | 39 +++++- spec/controllers/application_controller_spec.rb | 151 --------------------- .../dashboard/projects_controller_spec.rb | 5 + spec/controllers/dashboard_controller_spec.rb | 31 +++-- spec/controllers/graphql_controller_spec.rb | 47 ++++++- spec/controllers/groups_controller_spec.rb | 20 +++ .../projects/commits_controller_spec.rb | 138 +++++++++++-------- .../controllers/projects/issues_controller_spec.rb | 36 +++++ spec/controllers/projects/tags_controller_spec.rb | 22 +++ spec/controllers/projects_controller_spec.rb | 22 +++ spec/controllers/users_controller_spec.rb | 8 ++ spec/lib/gitlab/auth/request_authenticator_spec.rb | 18 +-- spec/lib/gitlab/auth/user_auth_finders_spec.rb | 79 +++++++++-- .../sessionless_auth_controller_shared_examples.rb | 92 +++++++++++++ 28 files changed, 538 insertions(+), 281 deletions(-) create mode 100644 app/controllers/concerns/sessionless_authentication.rb create mode 100644 changelogs/unreleased/security-fix-pat-web-access.yml create mode 100644 spec/controllers/dashboard/projects_controller_spec.rb create mode 100644 spec/support/controllers/sessionless_auth_controller_shared_examples.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 9b40ffb26a2..dbb22127e82 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -12,11 +12,11 @@ class ApplicationController < ActionController::Base include WorkhorseHelper include EnforcesTwoFactorAuthentication include WithPerformanceBar + include SessionlessAuthentication # this can be removed after switching to rails 5 # https://gitlab.com/gitlab-org/gitlab-ce/issues/51908 include InvalidUTF8ErrorHandler unless Gitlab.rails5? - before_action :authenticate_sessionless_user! before_action :authenticate_user! before_action :enforce_terms!, if: :should_enforce_terms? before_action :validate_user_service_ticket! @@ -153,13 +153,6 @@ class ApplicationController < ActionController::Base end end - # This filter handles personal access tokens, and atom requests with rss tokens - def authenticate_sessionless_user! - user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user - - sessionless_sign_in(user) if user - end - def log_exception(exception) Raven.capture_exception(exception) if sentry_enabled? @@ -426,25 +419,11 @@ class ApplicationController < ActionController::Base Gitlab::I18n.with_user_locale(current_user, &block) end - def sessionless_sign_in(user) - if user && can?(user, :log_in) - # Notice we are passing store false, so the user is not - # actually stored in the session and a token is needed - # for every request. If you want the token to work as a - # sign in token, you can simply remove store: false. - sign_in(user, store: false, message: :sessionless_sign_in) - end - end - def set_page_title_header # Per https://tools.ietf.org/html/rfc5987, headers need to be ISO-8859-1, not UTF-8 response.headers['Page-Title'] = URI.escape(page_title('GitLab')) end - def sessionless_user? - current_user && !session.keys.include?('warden.user.user.key') - end - def peek_request? request.path.start_with?('/-/peek') end diff --git a/app/controllers/concerns/sessionless_authentication.rb b/app/controllers/concerns/sessionless_authentication.rb new file mode 100644 index 00000000000..590eefc6dab --- /dev/null +++ b/app/controllers/concerns/sessionless_authentication.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +# == SessionlessAuthentication +# +# Controller concern to handle PAT and RSS token authentication methods +# +module SessionlessAuthentication + # This filter handles personal access tokens, and atom requests with rss tokens + def authenticate_sessionless_user!(request_format) + user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user(request_format) + + sessionless_sign_in(user) if user + end + + def sessionless_user? + current_user && !session.keys.include?('warden.user.user.key') + end + + def sessionless_sign_in(user) + if user && can?(user, :log_in) + # Notice we are passing store false, so the user is not + # actually stored in the session and a token is needed + # for every request. If you want the token to work as a + # sign in token, you can simply remove store: false. + sign_in(user, store: false, message: :sessionless_sign_in) + end + end +end diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index e9686ed8d06..57e612d89d3 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -4,6 +4,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController include ParamsBackwardCompatibility include RendersMemberAccess + prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) } before_action :set_non_archived_param before_action :default_sorting skip_cross_project_access_check :index, :starred diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 4ce9be44403..be2d9512c01 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -4,6 +4,9 @@ class DashboardController < Dashboard::ApplicationController include IssuesAction include MergeRequestsAction + prepend_before_action(only: [:issues]) { authenticate_sessionless_user!(:rss) } + prepend_before_action(only: [:issues_calendar]) { authenticate_sessionless_user!(:ics) } + before_action :event_filter, only: :activity before_action :projects, only: [:issues, :merge_requests] before_action :set_show_full_reference, only: [:issues, :merge_requests] diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb index a1ec144410b..6ea4758ec32 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -3,6 +3,7 @@ class GraphqlController < ApplicationController # Unauthenticated users have access to the API for public data skip_before_action :authenticate_user! + prepend_before_action(only: [:execute]) { authenticate_sessionless_user!(:api) } before_action :check_graphql_feature_flag! diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 062c8c4e9e1..c5d8ac2ed77 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -9,6 +9,9 @@ class GroupsController < Groups::ApplicationController respond_to :html + prepend_before_action(only: [:show, :issues]) { authenticate_sessionless_user!(:rss) } + prepend_before_action(only: [:issues_calendar]) { authenticate_sessionless_user!(:ics) } + before_action :authenticate_user!, only: [:new, :create] before_action :group, except: [:index, :new, :create] diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index 84a2a461da7..8ba18aacc58 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -6,6 +6,7 @@ class Projects::CommitsController < Projects::ApplicationController include ExtractsPath include RendersCommits + prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) } before_action :whitelist_query_limiting, except: :commits_root before_action :require_non_empty_project before_action :assign_ref_vars, except: :commits_root diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index d6d7110355b..c6ab6b4642e 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -9,10 +9,6 @@ class Projects::IssuesController < Projects::ApplicationController include IssuesCalendar include SpammableActions - def self.authenticate_user_only_actions - %i[new] - end - def self.issue_except_actions %i[index calendar new create bulk_update] end @@ -21,7 +17,10 @@ class Projects::IssuesController < Projects::ApplicationController %i[index calendar] end - prepend_before_action :authenticate_user!, only: authenticate_user_only_actions + prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) } + prepend_before_action(only: [:calendar]) { authenticate_sessionless_user!(:ics) } + prepend_before_action :authenticate_new_issue!, only: [:new] + prepend_before_action :store_uri, only: [:new, :show] before_action :whitelist_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update] before_action :check_issues_available! @@ -232,16 +231,18 @@ class Projects::IssuesController < Projects::ApplicationController ] + [{ label_ids: [], assignee_ids: [] }] end - def authenticate_user! + def authenticate_new_issue! return if current_user notice = "Please sign in to create the new issue." + redirect_to new_user_session_path, notice: notice + end + + def store_uri if request.get? && !request.xhr? store_location_for :user, request.fullpath end - - redirect_to new_user_session_path, notice: notice end def serializer diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb index c8442ff3592..2b28670a49b 100644 --- a/app/controllers/projects/tags_controller.rb +++ b/app/controllers/projects/tags_controller.rb @@ -3,6 +3,8 @@ class Projects::TagsController < Projects::ApplicationController include SortingHelper + prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) } + # Authorize before_action :require_non_empty_project before_action :authorize_download_code! diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 7f4a9f5151b..8bf93bfd68d 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -7,6 +7,8 @@ class ProjectsController < Projects::ApplicationController include PreviewMarkdown include SendFileUpload + prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) } + before_action :whitelist_query_limiting, only: [:create] before_action :authenticate_user!, except: [:index, :show, :activity, :refs] before_action :redirect_git_extension, only: [:show] diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 5b70c69d7f4..8b040dc080e 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -14,6 +14,7 @@ class UsersController < ApplicationController calendar_activities: true skip_before_action :authenticate_user! + prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) } before_action :user, except: [:exists] before_action :authorize_read_user_profile!, only: [:calendar, :calendar_activities, :groups, :projects, :contributed_projects, :snippets] diff --git a/changelogs/unreleased/security-fix-pat-web-access.yml b/changelogs/unreleased/security-fix-pat-web-access.yml new file mode 100644 index 00000000000..62ffb908fe5 --- /dev/null +++ b/changelogs/unreleased/security-fix-pat-web-access.yml @@ -0,0 +1,5 @@ +--- +title: Restrict Personal Access Tokens to API scope on web requests +merge_request: +author: +type: security diff --git a/config/initializers/rack_attack_global.rb b/config/initializers/rack_attack_global.rb index 45963831c41..86cb930eca9 100644 --- a/config/initializers/rack_attack_global.rb +++ b/config/initializers/rack_attack_global.rb @@ -33,22 +33,22 @@ class Rack::Attack throttle('throttle_authenticated_api', Gitlab::Throttle.authenticated_api_options) do |req| Gitlab::Throttle.settings.throttle_authenticated_api_enabled && req.api_request? && - req.authenticated_user_id + req.authenticated_user_id([:api]) end throttle('throttle_authenticated_web', Gitlab::Throttle.authenticated_web_options) do |req| Gitlab::Throttle.settings.throttle_authenticated_web_enabled && req.web_request? && - req.authenticated_user_id + req.authenticated_user_id([:api, :rss, :ics]) end class Request def unauthenticated? - !authenticated_user_id + !authenticated_user_id([:api, :rss, :ics]) end - def authenticated_user_id - Gitlab::Auth::RequestAuthenticator.new(self).user&.id + def authenticated_user_id(request_formats) + Gitlab::Auth::RequestAuthenticator.new(self).user(request_formats)&.id end def api_request? diff --git a/lib/gitlab/auth/request_authenticator.rb b/lib/gitlab/auth/request_authenticator.rb index cb9f2582936..176766d1a8b 100644 --- a/lib/gitlab/auth/request_authenticator.rb +++ b/lib/gitlab/auth/request_authenticator.rb @@ -13,12 +13,18 @@ module Gitlab @request = request end - def user - find_sessionless_user || find_user_from_warden + def user(request_formats) + request_formats.each do |format| + user = find_sessionless_user(format) + + return user if user + end + + find_user_from_warden end - def find_sessionless_user - find_user_from_access_token || find_user_from_feed_token + def find_sessionless_user(request_format) + find_user_from_web_access_token(request_format) || find_user_from_feed_token(request_format) rescue Gitlab::Auth::AuthenticationError nil end diff --git a/lib/gitlab/auth/user_auth_finders.rb b/lib/gitlab/auth/user_auth_finders.rb index c304adc64db..adba9084845 100644 --- a/lib/gitlab/auth/user_auth_finders.rb +++ b/lib/gitlab/auth/user_auth_finders.rb @@ -27,8 +27,8 @@ module Gitlab current_request.env['warden']&.authenticate if verified_request? end - def find_user_from_feed_token - return unless rss_request? || ics_request? + def find_user_from_feed_token(request_format) + return unless valid_rss_format?(request_format) # NOTE: feed_token was renamed from rss_token but both needs to be supported because # users might have already added the feed to their RSS reader before the rename @@ -38,6 +38,17 @@ module Gitlab User.find_by_feed_token(token) || raise(UnauthorizedError) end + # We only allow Private Access Tokens with `api` scope to be used by web + # requests on RSS feeds or ICS files for backwards compatibility. + # It is also used by GraphQL/API requests. + def find_user_from_web_access_token(request_format) + return unless access_token && valid_web_access_format?(request_format) + + validate_access_token!(scopes: [:api]) + + access_token.user || raise(UnauthorizedError) + end + def find_user_from_access_token return unless access_token @@ -109,6 +120,26 @@ module Gitlab @current_request ||= ensure_action_dispatch_request(request) end + def valid_web_access_format?(request_format) + case request_format + when :rss + rss_request? + when :ics + ics_request? + when :api + api_request? + end + end + + def valid_rss_format?(request_format) + case request_format + when :rss + rss_request? + when :ics + ics_request? + end + end + def rss_request? current_request.path.ends_with?('.atom') || current_request.format.atom? end @@ -116,6 +147,10 @@ module Gitlab def ics_request? current_request.path.ends_with?('.ics') || current_request.format.ics? end + + def api_request? + current_request.path.starts_with?("/api/") + end end end end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index efc3ce74627..1b585bcd4c6 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -107,59 +107,6 @@ describe ApplicationController do end end - describe "#authenticate_user_from_personal_access_token!" do - before do - stub_authentication_activity_metrics(debug: false) - end - - controller(described_class) do - def index - render text: 'authenticated' - end - end - - 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 - expect(authentication_metrics) - .to increment(:user_authenticated_counter) - .and increment(:user_session_override_counter) - .and increment(:user_sessionless_authentication_counter) - - 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 - expect(authentication_metrics) - .to increment(:user_authenticated_counter) - .and increment(:user_session_override_counter) - .and increment(:user_sessionless_authentication_counter) - - @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 - expect(authentication_metrics) - .to increment(:user_unauthenticated_counter) - - get :index, private_token: "token" - - expect(response.status).not_to eq(200) - expect(response.body).not_to eq('authenticated') - end - end - describe 'session expiration' do controller(described_class) do # The anonymous controller will report 401 and fail to run any actions. @@ -224,74 +171,6 @@ describe ApplicationController do end end - describe '#authenticate_sessionless_user!' do - before do - stub_authentication_activity_metrics(debug: false) - end - - describe 'authenticating a user from a feed token' do - controller(described_class) do - def index - render text: 'authenticated' - end - end - - context "when the 'feed_token' param is populated with the feed token" do - context 'when the request format is atom' do - it "logs the user in" do - expect(authentication_metrics) - .to increment(:user_authenticated_counter) - .and increment(:user_session_override_counter) - .and increment(:user_sessionless_authentication_counter) - - get :index, feed_token: user.feed_token, format: :atom - - expect(response).to have_gitlab_http_status 200 - expect(response.body).to eq 'authenticated' - end - end - - context 'when the request format is ics' do - it "logs the user in" do - expect(authentication_metrics) - .to increment(:user_authenticated_counter) - .and increment(:user_session_override_counter) - .and increment(:user_sessionless_authentication_counter) - - get :index, feed_token: user.feed_token, format: :ics - - expect(response).to have_gitlab_http_status 200 - expect(response.body).to eq 'authenticated' - end - end - - context 'when the request format is neither atom nor ics' do - it "doesn't log the user in" do - expect(authentication_metrics) - .to increment(:user_unauthenticated_counter) - - get :index, feed_token: user.feed_token - - expect(response.status).not_to have_gitlab_http_status 200 - expect(response.body).not_to eq 'authenticated' - end - end - end - - context "when the 'feed_token' param is populated with an invalid feed token" do - it "doesn't log the user" do - expect(authentication_metrics) - .to increment(:user_unauthenticated_counter) - - get :index, feed_token: 'token', format: :atom - - expect(response.status).not_to eq 200 - expect(response.body).not_to eq 'authenticated' - end - end - end - end - describe '#route_not_found' do it 'renders 404 if authenticated' do allow(controller).to receive(:current_user).and_return(user) @@ -557,36 +436,6 @@ describe ApplicationController do expect(response).to have_gitlab_http_status(200) end - - context 'for sessionless users' do - render_views - - before do - sign_out user - end - - it 'renders a 403 when the sessionless user did not accept the terms' do - get :index, feed_token: user.feed_token, format: :atom - - expect(response).to have_gitlab_http_status(403) - end - - it 'renders the error message when the format was html' do - get :index, - private_token: create(:personal_access_token, user: user).token, - format: :html - - expect(response.body).to have_content /accept the terms of service/i - end - - it 'renders a 200 when the sessionless user accepted the terms' do - accept_terms(user) - - get :index, feed_token: user.feed_token, format: :atom - - expect(response).to have_gitlab_http_status(200) - end - end end end diff --git a/spec/controllers/dashboard/projects_controller_spec.rb b/spec/controllers/dashboard/projects_controller_spec.rb new file mode 100644 index 00000000000..2975205e09c --- /dev/null +++ b/spec/controllers/dashboard/projects_controller_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe Dashboard::ProjectsController do + it_behaves_like 'authenticates sessionless user', :index, :atom +end diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb index 187542ba30c..c857a78d5e8 100644 --- a/spec/controllers/dashboard_controller_spec.rb +++ b/spec/controllers/dashboard_controller_spec.rb @@ -1,21 +1,26 @@ require 'spec_helper' describe DashboardController do - let(:user) { create(:user) } - let(:project) { create(:project) } + context 'signed in' do + let(:user) { create(:user) } + let(:project) { create(:project) } - before do - project.add_maintainer(user) - sign_in(user) - end + before do + project.add_maintainer(user) + sign_in(user) + end - describe 'GET issues' do - it_behaves_like 'issuables list meta-data', :issue, :issues - it_behaves_like 'issuables requiring filter', :issues - end + describe 'GET issues' do + it_behaves_like 'issuables list meta-data', :issue, :issues + it_behaves_like 'issuables requiring filter', :issues + end - describe 'GET merge requests' do - it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests - it_behaves_like 'issuables requiring filter', :merge_requests + describe 'GET merge requests' do + it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests + it_behaves_like 'issuables requiring filter', :merge_requests + end end + + it_behaves_like 'authenticates sessionless user', :issues, :atom, author_id: User.first + it_behaves_like 'authenticates sessionless user', :issues_calendar, :ics end diff --git a/spec/controllers/graphql_controller_spec.rb b/spec/controllers/graphql_controller_spec.rb index 1449036e148..949ad532365 100644 --- a/spec/controllers/graphql_controller_spec.rb +++ b/spec/controllers/graphql_controller_spec.rb @@ -52,15 +52,58 @@ describe GraphqlController do end end + context 'token authentication' do + before do + stub_authentication_activity_metrics(debug: false) + end + + let(:user) { create(:user, username: 'Simon') } + 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 + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + + run_test_query!(private_token: personal_access_token.token) + + expect(response).to have_gitlab_http_status(200) + expect(query_response).to eq('echo' => '"Simon" says: test success') + end + end + + context 'when the personal access token has no api scope' do + it 'does not log the user in' do + personal_access_token.update(scopes: [:read_user]) + + run_test_query!(private_token: personal_access_token.token) + + expect(response).to have_gitlab_http_status(200) + + expect(query_response).to eq('echo' => 'nil says: test success') + end + end + + context 'without token' do + it 'shows public data' do + run_test_query! + + expect(query_response).to eq('echo' => 'nil says: test success') + end + end + end + # Chosen to exercise all the moving parts in GraphqlController#execute - def run_test_query!(variables: { 'text' => 'test success' }) + def run_test_query!(variables: { 'text' => 'test success' }, private_token: nil) query = <<~QUERY query Echo($text: String) { echo(text: $text) } QUERY - post :execute, query: query, operationName: 'Echo', variables: variables + post :execute, query: query, operationName: 'Echo', variables: variables, private_token: private_token end def query_response diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 4de61b65f71..f6c85102830 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -606,4 +606,24 @@ describe GroupsController do end end end + + context 'token authentication' do + it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do + before do + default_params.merge!(id: group) + end + end + + it_behaves_like 'authenticates sessionless user', :issues, :atom, public: true do + before do + default_params.merge!(id: group, author_id: user.id) + end + end + + it_behaves_like 'authenticates sessionless user', :issues_calendar, :ics, public: true do + before do + default_params.merge!(id: group) + end + end + end end diff --git a/spec/controllers/projects/commits_controller_spec.rb b/spec/controllers/projects/commits_controller_spec.rb index a43bdd3ea80..5c72dab698c 100644 --- a/spec/controllers/projects/commits_controller_spec.rb +++ b/spec/controllers/projects/commits_controller_spec.rb @@ -5,87 +5,115 @@ describe Projects::CommitsController do let(:user) { create(:user) } before do - sign_in(user) project.add_maintainer(user) end - describe "GET commits_root" do - context "no ref is provided" do - it 'should redirect to the default branch of the project' do - get(:commits_root, - namespace_id: project.namespace, - project_id: project) + context 'signed in' do + before do + sign_in(user) + end + + describe "GET commits_root" do + context "no ref is provided" do + it 'should redirect to the default branch of the project' do + get(:commits_root, + namespace_id: project.namespace, + project_id: project) - expect(response).to redirect_to project_commits_path(project) + expect(response).to redirect_to project_commits_path(project) + end end end - end - describe "GET show" do - render_views + describe "GET show" do + render_views - context 'with file path' do - before do - get(:show, - namespace_id: project.namespace, - project_id: project, - id: id) - end + context 'with file path' do + before do + get(:show, + namespace_id: project.namespace, + project_id: project, + id: id) + end - context "valid branch, valid file" do - let(:id) { 'master/README.md' } + context "valid branch, valid file" do + let(:id) { 'master/README.md' } - it { is_expected.to respond_with(:success) } - end + it { is_expected.to respond_with(:success) } + end - context "valid branch, invalid file" do - let(:id) { 'master/invalid-path.rb' } + context "valid branch, invalid file" do + let(:id) { 'master/invalid-path.rb' } - it { is_expected.to respond_with(:not_found) } - end + it { is_expected.to respond_with(:not_found) } + end - context "invalid branch, valid file" do - let(:id) { 'invalid-branch/README.md' } + context "invalid branch, valid file" do + let(:id) { 'invalid-branch/README.md' } - it { is_expected.to respond_with(:not_found) } + it { is_expected.to respond_with(:not_found) } + end end - end - context "when the ref name ends in .atom" do - context "when the ref does not exist with the suffix" do - before do - get(:show, - namespace_id: project.namespace, - project_id: project, - id: "master.atom") + context "when the ref name ends in .atom" do + context "when the ref does not exist with the suffix" do + before do + get(:show, + namespace_id: project.namespace, + project_id: project, + id: "master.atom") + end + + it "renders as atom" do + expect(response).to be_success + expect(response.content_type).to eq('application/atom+xml') + end + + it 'renders summary with type=html' do + expect(response.body).to include('') + end end - it "renders as atom" do - expect(response).to be_success - expect(response.content_type).to eq('application/atom+xml') - end + context "when the ref exists with the suffix" do + before do + commit = project.repository.commit('master') - it 'renders summary with type=html' do - expect(response.body).to include('') + allow_any_instance_of(Repository).to receive(:commit).and_call_original + allow_any_instance_of(Repository).to receive(:commit).with('master.atom').and_return(commit) + + get(:show, + namespace_id: project.namespace, + project_id: project, + id: "master.atom") + end + + it "renders as HTML" do + expect(response).to be_success + expect(response.content_type).to eq('text/html') + end end end + end + end - context "when the ref exists with the suffix" do + context 'token authentication' do + context 'public project' do + it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do before do - commit = project.repository.commit('master') + public_project = create(:project, :repository, :public) - allow_any_instance_of(Repository).to receive(:commit).and_call_original - allow_any_instance_of(Repository).to receive(:commit).with('master.atom').and_return(commit) - - get(:show, - namespace_id: project.namespace, - project_id: project, - id: "master.atom") + default_params.merge!(namespace_id: public_project.namespace, project_id: public_project, id: "master.atom") end + end + end + + context 'private project' do + it_behaves_like 'authenticates sessionless user', :show, :atom, public: false do + before do + private_project = create(:project, :repository, :private) + private_project.add_maintainer(user) - it "renders as HTML" do - expect(response).to be_success - expect(response.content_type).to eq('text/html') + default_params.merge!(namespace_id: private_project.namespace, project_id: private_project, id: "master.atom") end end end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 80138183c07..02930edbf72 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1068,4 +1068,40 @@ describe Projects::IssuesController do end end end + + context 'private project with token authentication' do + let(:private_project) { create(:project, :private) } + + it_behaves_like 'authenticates sessionless user', :index, :atom do + before do + default_params.merge!(project_id: private_project, namespace_id: private_project.namespace) + + private_project.add_maintainer(user) + end + end + + it_behaves_like 'authenticates sessionless user', :calendar, :ics do + before do + default_params.merge!(project_id: private_project, namespace_id: private_project.namespace) + + private_project.add_maintainer(user) + end + end + end + + context 'public project with token authentication' do + let(:public_project) { create(:project, :public) } + + it_behaves_like 'authenticates sessionless user', :index, :atom, public: true do + before do + default_params.merge!(project_id: public_project, namespace_id: public_project.namespace) + end + end + + it_behaves_like 'authenticates sessionless user', :calendar, :ics, public: true do + before do + default_params.merge!(project_id: public_project, namespace_id: public_project.namespace) + end + end + end end diff --git a/spec/controllers/projects/tags_controller_spec.rb b/spec/controllers/projects/tags_controller_spec.rb index c48f41ca12e..6fbf75d0259 100644 --- a/spec/controllers/projects/tags_controller_spec.rb +++ b/spec/controllers/projects/tags_controller_spec.rb @@ -35,4 +35,26 @@ describe Projects::TagsController do it { is_expected.to respond_with(:not_found) } end end + + context 'private project with token authentication' do + let(:private_project) { create(:project, :repository, :private) } + + it_behaves_like 'authenticates sessionless user', :index, :atom do + before do + default_params.merge!(project_id: private_project, namespace_id: private_project.namespace) + + private_project.add_maintainer(user) + end + end + end + + context 'public project with token authentication' do + let(:public_project) { create(:project, :repository, :public) } + + it_behaves_like 'authenticates sessionless user', :index, :atom, public: true do + before do + default_params.merge!(project_id: public_project, namespace_id: public_project.namespace) + end + end + end end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 3bc9cbe64c5..7849bec4762 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -882,6 +882,28 @@ describe ProjectsController do end end + context 'private project with token authentication' do + let(:private_project) { create(:project, :private) } + + it_behaves_like 'authenticates sessionless user', :show, :atom do + before do + default_params.merge!(id: private_project, namespace_id: private_project.namespace) + + private_project.add_maintainer(user) + end + end + end + + context 'public project with token authentication' do + let(:public_project) { create(:project, :public) } + + it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do + before do + default_params.merge!(id: public_project, namespace_id: public_project.namespace) + end + end + end + def project_moved_message(redirect_route, project) "Project '#{redirect_route.path}' was moved to '#{project.full_path}'. Please update any links and bookmarks that may still have the old path." end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 071f96a729e..fe438e71e9e 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -395,6 +395,14 @@ describe UsersController do end end + context 'token authentication' do + it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do + before do + default_params.merge!(username: user.username) + end + end + end + def user_moved_message(redirect_route, user) "User '#{redirect_route.path}' was moved to '#{user.full_path}'. Please update any links and bookmarks that may still have the old path." end diff --git a/spec/lib/gitlab/auth/request_authenticator_spec.rb b/spec/lib/gitlab/auth/request_authenticator_spec.rb index 242ab4a91dd..3d979132880 100644 --- a/spec/lib/gitlab/auth/request_authenticator_spec.rb +++ b/spec/lib/gitlab/auth/request_authenticator_spec.rb @@ -19,17 +19,17 @@ describe Gitlab::Auth::RequestAuthenticator do allow_any_instance_of(described_class).to receive(:find_sessionless_user).and_return(sessionless_user) allow_any_instance_of(described_class).to receive(:find_user_from_warden).and_return(session_user) - expect(subject.user).to eq sessionless_user + expect(subject.user([:api])).to eq sessionless_user end it 'returns session user if no sessionless user found' do allow_any_instance_of(described_class).to receive(:find_user_from_warden).and_return(session_user) - expect(subject.user).to eq session_user + expect(subject.user([:api])).to eq session_user end it 'returns nil if no user found' do - expect(subject.user).to be_blank + expect(subject.user([:api])).to be_blank end it 'bubbles up exceptions' do @@ -42,26 +42,26 @@ describe Gitlab::Auth::RequestAuthenticator do let!(:feed_token_user) { build(:user) } it 'returns access_token user first' do - allow_any_instance_of(described_class).to receive(:find_user_from_access_token).and_return(access_token_user) + allow_any_instance_of(described_class).to receive(:find_user_from_web_access_token).and_return(access_token_user) allow_any_instance_of(described_class).to receive(:find_user_from_feed_token).and_return(feed_token_user) - expect(subject.find_sessionless_user).to eq access_token_user + expect(subject.find_sessionless_user([:api])).to eq access_token_user end it 'returns feed_token user if no access_token user found' do allow_any_instance_of(described_class).to receive(:find_user_from_feed_token).and_return(feed_token_user) - expect(subject.find_sessionless_user).to eq feed_token_user + expect(subject.find_sessionless_user([:api])).to eq feed_token_user end it 'returns nil if no user found' do - expect(subject.find_sessionless_user).to be_blank + expect(subject.find_sessionless_user([:api])).to be_blank end it 'rescue Gitlab::Auth::AuthenticationError exceptions' do - allow_any_instance_of(described_class).to receive(:find_user_from_access_token).and_raise(Gitlab::Auth::UnauthorizedError) + allow_any_instance_of(described_class).to receive(:find_user_from_web_access_token).and_raise(Gitlab::Auth::UnauthorizedError) - expect(subject.find_sessionless_user).to be_blank + expect(subject.find_sessionless_user([:api])).to be_blank end end end diff --git a/spec/lib/gitlab/auth/user_auth_finders_spec.rb b/spec/lib/gitlab/auth/user_auth_finders_spec.rb index 454ad1589b9..5d3fbba264f 100644 --- a/spec/lib/gitlab/auth/user_auth_finders_spec.rb +++ b/spec/lib/gitlab/auth/user_auth_finders_spec.rb @@ -9,7 +9,7 @@ describe Gitlab::Auth::UserAuthFinders do 'rack.input' => '' } end - let(:request) { Rack::Request.new(env)} + let(:request) { Rack::Request.new(env) } def set_param(key, value) request.update_param(key, value) @@ -49,6 +49,7 @@ describe Gitlab::Auth::UserAuthFinders do describe '#find_user_from_feed_token' do context 'when the request format is atom' do before do + env['SCRIPT_NAME'] = 'url.atom' env['HTTP_ACCEPT'] = 'application/atom+xml' end @@ -56,17 +57,17 @@ describe Gitlab::Auth::UserAuthFinders do it 'returns user if valid feed_token' do set_param(:feed_token, user.feed_token) - expect(find_user_from_feed_token).to eq user + expect(find_user_from_feed_token(:rss)).to eq user end it 'returns nil if feed_token is blank' do - expect(find_user_from_feed_token).to be_nil + expect(find_user_from_feed_token(:rss)).to be_nil end it 'returns exception if invalid feed_token' do set_param(:feed_token, 'invalid_token') - expect { find_user_from_feed_token }.to raise_error(Gitlab::Auth::UnauthorizedError) + expect { find_user_from_feed_token(:rss) }.to raise_error(Gitlab::Auth::UnauthorizedError) end end @@ -74,34 +75,38 @@ describe Gitlab::Auth::UserAuthFinders do it 'returns user if valid rssd_token' do set_param(:rss_token, user.feed_token) - expect(find_user_from_feed_token).to eq user + expect(find_user_from_feed_token(:rss)).to eq user end it 'returns nil if rss_token is blank' do - expect(find_user_from_feed_token).to be_nil + expect(find_user_from_feed_token(:rss)).to be_nil end it 'returns exception if invalid rss_token' do set_param(:rss_token, 'invalid_token') - expect { find_user_from_feed_token }.to raise_error(Gitlab::Auth::UnauthorizedError) + expect { find_user_from_feed_token(:rss) }.to raise_error(Gitlab::Auth::UnauthorizedError) end end end context 'when the request format is not atom' do it 'returns nil' do + env['SCRIPT_NAME'] = 'json' + set_param(:feed_token, user.feed_token) - expect(find_user_from_feed_token).to be_nil + expect(find_user_from_feed_token(:rss)).to be_nil end end context 'when the request format is empty' do it 'the method call does not modify the original value' do + env['SCRIPT_NAME'] = 'url.atom' + env.delete('action_dispatch.request.formats') - find_user_from_feed_token + find_user_from_feed_token(:rss) expect(env['action_dispatch.request.formats']).to be_nil end @@ -111,8 +116,12 @@ describe Gitlab::Auth::UserAuthFinders do describe '#find_user_from_access_token' do let(:personal_access_token) { create(:personal_access_token, user: user) } + before do + env['SCRIPT_NAME'] = 'url.atom' + end + it 'returns nil if no access_token present' do - expect(find_personal_access_token).to be_nil + expect(find_user_from_access_token).to be_nil end context 'when validate_access_token! returns valid' do @@ -131,9 +140,59 @@ describe Gitlab::Auth::UserAuthFinders do end end + describe '#find_user_from_web_access_token' do + let(:personal_access_token) { create(:personal_access_token, user: user) } + + before do + env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token + end + + it 'returns exception if token has no user' do + allow_any_instance_of(PersonalAccessToken).to receive(:user).and_return(nil) + + expect { find_user_from_access_token }.to raise_error(Gitlab::Auth::UnauthorizedError) + end + + context 'no feed or API requests' do + it 'returns nil if the request is not RSS' do + expect(find_user_from_web_access_token(:rss)).to be_nil + end + + it 'returns nil if the request is not ICS' do + expect(find_user_from_web_access_token(:ics)).to be_nil + end + + it 'returns nil if the request is not API' do + expect(find_user_from_web_access_token(:api)).to be_nil + end + end + + it 'returns the user for RSS requests' do + env['SCRIPT_NAME'] = 'url.atom' + + expect(find_user_from_web_access_token(:rss)).to eq(user) + end + + it 'returns the user for ICS requests' do + env['SCRIPT_NAME'] = 'url.ics' + + expect(find_user_from_web_access_token(:ics)).to eq(user) + end + + it 'returns the user for API requests' do + env['SCRIPT_NAME'] = '/api/endpoint' + + expect(find_user_from_web_access_token(:api)).to eq(user) + end + end + describe '#find_personal_access_token' do let(:personal_access_token) { create(:personal_access_token, user: user) } + before do + env['SCRIPT_NAME'] = 'url.atom' + end + context 'passed as header' do it 'returns token if valid personal_access_token' do env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token diff --git a/spec/support/controllers/sessionless_auth_controller_shared_examples.rb b/spec/support/controllers/sessionless_auth_controller_shared_examples.rb new file mode 100644 index 00000000000..7e4958f177a --- /dev/null +++ b/spec/support/controllers/sessionless_auth_controller_shared_examples.rb @@ -0,0 +1,92 @@ +shared_examples 'authenticates sessionless user' do |path, format, params| + params ||= {} + + before do + stub_authentication_activity_metrics(debug: false) + end + + let(:user) { create(:user) } + let(:personal_access_token) { create(:personal_access_token, user: user) } + let(:default_params) { { format: format }.merge(params.except(:public) || {}) } + + context "when the 'personal_access_token' param is populated with the personal access token" do + it 'logs the user in' do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + + get path, default_params.merge(private_token: personal_access_token.token) + + expect(response).to have_gitlab_http_status(200) + expect(controller.current_user).to eq(user) + end + + it 'does not log the user in if page is public', if: params[:public] do + get path, default_params + + expect(response).to have_gitlab_http_status(200) + expect(controller.current_user).to be_nil + end + end + + context 'when the personal access token has no api scope', unless: params[:public] do + it 'does not log the user in' do + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + + personal_access_token.update(scopes: [:read_user]) + + get path, default_params.merge(private_token: personal_access_token.token) + + expect(response).not_to have_gitlab_http_status(200) + end + end + + context "when the 'PERSONAL_ACCESS_TOKEN' header is populated with the personal access token" do + it 'logs the user in' do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + + @request.headers['PRIVATE-TOKEN'] = personal_access_token.token + get path, default_params + + expect(response).to have_gitlab_http_status(200) + end + end + + context "when the 'feed_token' param is populated with the feed token", if: format == :rss do + it "logs the user in" do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + + get path, default_params.merge(feed_token: user.feed_token) + + expect(response).to have_gitlab_http_status 200 + end + end + + context "when the 'feed_token' param is populated with an invalid feed token", if: format == :rss, unless: params[:public] do + it "logs the user" do + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + + get path, default_params.merge(feed_token: 'token') + + expect(response.status).not_to eq 200 + end + end + + it "doesn't log the user in otherwise", unless: params[:public] do + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + + get path, default_params.merge(private_token: 'token') + + expect(response.status).not_to eq(200) + end +end -- cgit v1.2.1