From c144db2935f0f71c7f282a3015d126526bc16b57 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Tue, 6 Sep 2016 16:32:39 -0500 Subject: [PATCH] Better authentication handling, syntax fixes and better actor handling for LFS Tokens --- .../projects/git_http_client_controller.rb | 27 +++++++------- app/helpers/lfs_helper.rb | 2 +- lib/api/internal.rb | 9 +---- lib/gitlab/auth.rb | 35 +++++++++---------- lib/gitlab/lfs_token.rb | 21 +++++++++-- spec/requests/api/internal_spec.rb | 2 +- 6 files changed, 51 insertions(+), 45 deletions(-) diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb index 4dff1ce6568..b4ec5b3fae1 100644 --- a/app/controllers/projects/git_http_client_controller.rb +++ b/app/controllers/projects/git_http_client_controller.rb @@ -4,8 +4,6 @@ class Projects::GitHttpClientController < Projects::ApplicationController include ActionController::HttpAuthentication::Basic include KerberosSpnegoHelper - class MissingPersonalTokenError < StandardError; end - attr_reader :user # Git clients will not know what authenticity token to send along @@ -40,10 +38,8 @@ class Projects::GitHttpClientController < Projects::ApplicationController send_challenges render plain: "HTTP Basic: Access denied\n", status: 401 - - rescue MissingPersonalTokenError + rescue Gitlab::Auth::MissingPersonalTokenError render_missing_personal_token - return end def basic_auth_provided? @@ -117,17 +113,20 @@ class Projects::GitHttpClientController < Projects::ApplicationController def handle_authentication(login, password) auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip) - if auth_result.type == :ci && download_request? - @ci = true - elsif auth_result.type == :oauth && !download_request? - # Not allowed - elsif auth_result.type == :missing_personal_token - raise MissingPersonalTokenError - elsif auth_result.type == :lfs_deploy_token && download_request? - @lfs_deploy_key = true + case auth_result.type + when :ci + @ci = true if download_request? + when :oauth + @user = auth_result.user if download_request? + when :lfs_deploy_token + if download_request? + @lfs_deploy_key = true + @user = auth_result.user + end + when :lfs_token, :personal_token, :gitlab_or_ldap @user = auth_result.user else - @user = auth_result.user + # Not allowed end end diff --git a/app/helpers/lfs_helper.rb b/app/helpers/lfs_helper.rb index 031e7e72909..de7c9f253b2 100644 --- a/app/helpers/lfs_helper.rb +++ b/app/helpers/lfs_helper.rb @@ -27,7 +27,7 @@ module LfsHelper return true if project.public? || ci? || lfs_deploy_key? - (user && user.can?(:download_code, project)) + user && user.can?(:download_code, project) end def lfs_upload_access? diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 1f189d81d16..f8211bdd8af 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -78,14 +78,7 @@ module API status 200 key = Key.find(params[:key_id]) - user = key.user - - token_handler = - if user - Gitlab::LfsToken.new(user) - else - Gitlab::LfsToken.new(key) - end + token_handler = Gitlab::LfsToken.new(key) { username: token_handler.actor_name, diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 02b33c8c683..14e29124aac 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -2,21 +2,13 @@ module Gitlab module Auth Result = Struct.new(:user, :type) + class MissingPersonalTokenError < StandardError; end + class << self def find_for_git_client(login, password, project:, ip:) raise "Must provide an IP for rate limiting" if ip.nil? - result = Result.new - - if valid_ci_request?(login, password, project) - result.type = :ci - else - result = populate_result(login, password) - end - - success = result.user.present? || [:ci, :missing_personal_token].include?(result.type) - rate_limit!(ip, success: success, login: login) - result + populate_result(login, password, project, ip) end def find_with_user_password(login, password) @@ -75,21 +67,26 @@ module Gitlab end end - def populate_result(login, password) - result = + def populate_result(login, password, project, ip) + result = Result.new(nil, :ci) if valid_ci_request?(login, password, project) + + result ||= user_with_password_for_git(login, password) || oauth_access_token_check(login, password) || lfs_token_check(login, password) || personal_access_token_check(login, password) - if result + if result && result.type != :ci result.type = nil unless result.user if result.user && result.type == :gitlab_or_ldap && result.user.two_factor_enabled? - result.type = :missing_personal_token + raise Gitlab::Auth::MissingPersonalTokenError end end + success = result ? result.user.present? || [:ci].include?(result.type) : false + rate_limit!(ip, success: success, login: login) + result || Result.new end @@ -118,15 +115,17 @@ module Gitlab def lfs_token_check(login, password) actor = - if login.start_with?('lfs-deploy-key') - DeployKey.find(login.sub('lfs-deploy-key-', '')) + if login =~ /\Alfs-deploy-key-\d+\Z/ + /\d+\Z/.match(login) do |id| + DeployKey.find(id[0]) + end else User.by_login(login) end token_handler = Gitlab::LfsToken.new(actor) - Result.new(actor, token_handler.type) if actor && token_handler.value == password + Result.new(actor, token_handler.type) if actor && Devise.secure_compare(token_handler.value, password) end end end diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb index 8f49deb4d03..d7db8017475 100644 --- a/lib/gitlab/lfs_token.rb +++ b/lib/gitlab/lfs_token.rb @@ -2,15 +2,18 @@ module Gitlab class LfsToken attr_accessor :actor + TOKEN_LENGTH = 50 + EXPIRY_TIME = 1800 + def initialize(actor) - @actor = actor + set_actor(actor) end def generate - token = Devise.friendly_token(50) + token = Devise.friendly_token(TOKEN_LENGTH) Gitlab::Redis.with do |redis| - redis.set(redis_key, token, ex: 600) + redis.set(redis_key, token, ex: EXPIRY_TIME) end token @@ -35,5 +38,17 @@ module Gitlab def redis_key "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}" if actor end + + def set_actor(actor) + @actor = + case actor + when DeployKey, User + actor + when Key + actor.user + else + # + end + end end end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index ff697286927..1ee390e0a19 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -111,7 +111,7 @@ describe API::API, api: true do expect(response).to have_http_status(200) expect(json_response['username']).to eq(user.username) - expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).value) + expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).value) expect(json_response['repository_http_path']).to eq(project.http_url_to_repo) end