From 1436598e49792b78f5f753477a9d8c097d666b99 Mon Sep 17 00:00:00 2001 From: Francisco Lopez Date: Thu, 16 Nov 2017 17:03:19 +0100 Subject: [PATCH] Moved Exceptions to Gitlab::Auth --- .../omniauth_callbacks_controller.rb | 6 ++- lib/api/api_guard.rb | 20 +++++----- lib/api/helpers.rb | 2 +- lib/gitlab/auth/request_authenticator.rb | 2 +- lib/gitlab/auth/user_auth_finders.rb | 37 ++++++++++--------- .../gitlab/auth/request_authenticator_spec.rb | 4 +- .../lib/gitlab/auth/user_auth_finders_spec.rb | 20 +++++----- spec/requests/api/helpers_spec.rb | 8 ++-- 8 files changed, 51 insertions(+), 48 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 9612b8d8514..56baa19f864 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -54,7 +54,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController if current_user log_audit_event(current_user, with: :saml) # Update SAML identity if data has changed. - identity = current_user.identities.find_by(extern_uid: oauth['uid'], provider: :saml) + identity = current_user.identities.with_extern_uid(:saml, oauth['uid']).take if identity.nil? current_user.identities.create(extern_uid: oauth['uid'], provider: :saml) redirect_to profile_account_path, notice: 'Authentication method updated' @@ -98,7 +98,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController def handle_omniauth if current_user # Add new authentication method - current_user.identities.find_or_create_by(extern_uid: oauth['uid'], provider: oauth['provider']) + current_user.identities + .with_extern_uid(oauth['provider'], oauth['uid']) + .first_or_create(extern_uid: oauth['uid']) log_audit_event(current_user, with: oauth['provider']) redirect_to profile_account_path, notice: 'Authentication method updated' else diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index a07015406b1..1953a613f1d 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -93,11 +93,11 @@ module API private def install_error_responders(base) - error_classes = [Gitlab::Auth::UserAuthFinders::MissingTokenError, - Gitlab::Auth::UserAuthFinders::TokenNotFoundError, - Gitlab::Auth::UserAuthFinders::ExpiredError, - Gitlab::Auth::UserAuthFinders::RevokedError, - Gitlab::Auth::UserAuthFinders::InsufficientScopeError] + error_classes = [Gitlab::Auth::MissingTokenError, + Gitlab::Auth::TokenNotFoundError, + Gitlab::Auth::ExpiredError, + Gitlab::Auth::RevokedError, + Gitlab::Auth::InsufficientScopeError] base.__send__(:rescue_from, *error_classes, oauth2_bearer_token_error_handler) # rubocop:disable GitlabSecurity/PublicSend end @@ -106,25 +106,25 @@ module API proc do |e| response = case e - when Gitlab::Auth::UserAuthFinders::MissingTokenError + when Gitlab::Auth::MissingTokenError Rack::OAuth2::Server::Resource::Bearer::Unauthorized.new - when Gitlab::Auth::UserAuthFinders::TokenNotFoundError + when Gitlab::Auth::TokenNotFoundError Rack::OAuth2::Server::Resource::Bearer::Unauthorized.new( :invalid_token, "Bad Access Token.") - when Gitlab::Auth::UserAuthFinders::ExpiredError + when Gitlab::Auth::ExpiredError Rack::OAuth2::Server::Resource::Bearer::Unauthorized.new( :invalid_token, "Token is expired. You can either do re-authorization or token refresh.") - when Gitlab::Auth::UserAuthFinders::RevokedError + when Gitlab::Auth::RevokedError Rack::OAuth2::Server::Resource::Bearer::Unauthorized.new( :invalid_token, "Token was revoked. You have to re-authorize from the user.") - when Gitlab::Auth::UserAuthFinders::InsufficientScopeError + when Gitlab::Auth::InsufficientScopeError # FIXME: ForbiddenError (inherited from Bearer::Forbidden of Rack::Oauth2) # does not include WWW-Authenticate header, which breaks the standard. Rack::OAuth2::Server::Resource::Bearer::Forbidden.new( diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 09e9753b010..b26c61ab8da 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -398,7 +398,7 @@ module API begin @initial_current_user = Gitlab::Auth::UniqueIpsLimiter.limit_user! { find_current_user! } - rescue Gitlab::Auth::UserAuthFinders::UnauthorizedError + rescue Gitlab::Auth::UnauthorizedError unauthorized! end end diff --git a/lib/gitlab/auth/request_authenticator.rb b/lib/gitlab/auth/request_authenticator.rb index 8316d0f40d5..4322fb83cdf 100644 --- a/lib/gitlab/auth/request_authenticator.rb +++ b/lib/gitlab/auth/request_authenticator.rb @@ -17,7 +17,7 @@ module Gitlab def find_sessionless_user find_user_from_access_token || find_user_from_rss_token - rescue API::APIGuard::AuthenticationException + rescue Gitlab::Auth::AuthenticationException nil end end diff --git a/lib/gitlab/auth/user_auth_finders.rb b/lib/gitlab/auth/user_auth_finders.rb index 6ee957a0cd6..104fff3b56c 100644 --- a/lib/gitlab/auth/user_auth_finders.rb +++ b/lib/gitlab/auth/user_auth_finders.rb @@ -1,27 +1,28 @@ module Gitlab module Auth + + # + # Exceptions + # + + AuthenticationException = Class.new(StandardError) + MissingTokenError = Class.new(AuthenticationException) + TokenNotFoundError = Class.new(AuthenticationException) + ExpiredError = Class.new(AuthenticationException) + RevokedError = Class.new(AuthenticationException) + UnauthorizedError = Class.new(AuthenticationException) + + class InsufficientScopeError < AuthenticationException + attr_reader :scopes + def initialize(scopes) + @scopes = scopes.map { |s| s.try(:name) || s } + end + end + module UserAuthFinders PRIVATE_TOKEN_HEADER = 'HTTP_PRIVATE_TOKEN'.freeze PRIVATE_TOKEN_PARAM = :private_token - # - # Exceptions - # - - AuthenticationException = Class.new(StandardError) - MissingTokenError = Class.new(AuthenticationException) - TokenNotFoundError = Class.new(AuthenticationException) - ExpiredError = Class.new(AuthenticationException) - RevokedError = Class.new(AuthenticationException) - UnauthorizedError = Class.new(AuthenticationException) - - class InsufficientScopeError < AuthenticationException - attr_reader :scopes - def initialize(scopes) - @scopes = scopes.map { |s| s.try(:name) || s } - end - end - # Check the Rails session for valid authentication details def find_user_from_warden current_request.env['warden']&.authenticate if verified_request? diff --git a/spec/lib/gitlab/auth/request_authenticator_spec.rb b/spec/lib/gitlab/auth/request_authenticator_spec.rb index 6b8a8759314..b7348f5cd78 100644 --- a/spec/lib/gitlab/auth/request_authenticator_spec.rb +++ b/spec/lib/gitlab/auth/request_authenticator_spec.rb @@ -33,7 +33,7 @@ describe Gitlab::Auth::RequestAuthenticator do end it 'bubbles up exceptions' do - allow_any_instance_of(described_class).to receive(:find_user_from_warden).and_raise(Gitlab::Auth::UserAuthFinders::UnauthorizedError) + allow_any_instance_of(described_class).to receive(:find_user_from_warden).and_raise(Gitlab::Auth::UnauthorizedError) end end @@ -59,7 +59,7 @@ describe Gitlab::Auth::RequestAuthenticator do end it 'rescue API::APIGuard::AuthenticationException exceptions' do - allow_any_instance_of(described_class).to receive(:find_user_from_access_token).and_raise(Gitlab::Auth::UserAuthFinders::UnauthorizedError) + allow_any_instance_of(described_class).to receive(:find_user_from_access_token).and_raise(Gitlab::Auth::UnauthorizedError) expect(subject.find_sessionless_user).to be_blank end diff --git a/spec/lib/gitlab/auth/user_auth_finders_spec.rb b/spec/lib/gitlab/auth/user_auth_finders_spec.rb index 522e82c5912..4637816570c 100644 --- a/spec/lib/gitlab/auth/user_auth_finders_spec.rb +++ b/spec/lib/gitlab/auth/user_auth_finders_spec.rb @@ -65,7 +65,7 @@ describe Gitlab::Auth::UserAuthFinders do it 'returns exception if invalid rss_token' do set_param(:rss_token, 'invalid_token') - expect { find_user_from_rss_token }.to raise_error(Gitlab::Auth::UserAuthFinders::UnauthorizedError) + expect { find_user_from_rss_token }.to raise_error(Gitlab::Auth::UnauthorizedError) end end @@ -96,7 +96,7 @@ describe Gitlab::Auth::UserAuthFinders do env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token allow_any_instance_of(PersonalAccessToken).to receive(:user).and_return(nil) - expect { find_user_from_access_token }.to raise_error(Gitlab::Auth::UserAuthFinders::UnauthorizedError) + expect { find_user_from_access_token }.to raise_error(Gitlab::Auth::UnauthorizedError) end end end @@ -127,7 +127,7 @@ describe Gitlab::Auth::UserAuthFinders do it 'returns exception if invalid personal_access_token' do env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = 'invalid_token' - expect { find_personal_access_token }.to raise_error(Gitlab::Auth::UserAuthFinders::UnauthorizedError) + expect { find_personal_access_token }.to raise_error(Gitlab::Auth::UnauthorizedError) end end @@ -158,7 +158,7 @@ describe Gitlab::Auth::UserAuthFinders do it 'returns exception if invalid oauth_access_token' do env['HTTP_AUTHORIZATION'] = "Bearer invalid_token" - expect { find_oauth_access_token }.to raise_error(Gitlab::Auth::UserAuthFinders::UnauthorizedError) + expect { find_oauth_access_token }.to raise_error(Gitlab::Auth::UnauthorizedError) end end @@ -174,20 +174,20 @@ describe Gitlab::Auth::UserAuthFinders do allow_any_instance_of(described_class).to receive(:access_token).and_return(personal_access_token) end - it 'returns Gitlab::Auth::UserAuthFinders::ExpiredError if token expired' do + it 'returns Gitlab::Auth::ExpiredError if token expired' do personal_access_token.expires_at = 1.day.ago - expect { validate_access_token! }.to raise_error(Gitlab::Auth::UserAuthFinders::ExpiredError) + expect { validate_access_token! }.to raise_error(Gitlab::Auth::ExpiredError) end - it 'returns Gitlab::Auth::UserAuthFinders::RevokedError if token revoked' do + it 'returns Gitlab::Auth::RevokedError if token revoked' do personal_access_token.revoke! - expect { validate_access_token! }.to raise_error(Gitlab::Auth::UserAuthFinders::RevokedError) + expect { validate_access_token! }.to raise_error(Gitlab::Auth::RevokedError) end - it 'returns Gitlab::Auth::UserAuthFinders::InsufficientScopeError if invalid token scope' do - expect { validate_access_token!(scopes: [:sudo]) }.to raise_error(Gitlab::Auth::UserAuthFinders::InsufficientScopeError) + it 'returns Gitlab::Auth::InsufficientScopeError if invalid token scope' do + expect { validate_access_token!(scopes: [:sudo]) }.to raise_error(Gitlab::Auth::InsufficientScopeError) end end end diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb index 919df575c6e..0462f494e15 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -166,21 +166,21 @@ describe API::Helpers do personal_access_token = create(:personal_access_token, user: user, scopes: ['read_user']) env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token - expect { current_user }.to raise_error Gitlab::Auth::UserAuthFinders::InsufficientScopeError + expect { current_user }.to raise_error Gitlab::Auth::InsufficientScopeError end it 'does not allow revoked tokens' do personal_access_token.revoke! env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token - expect { current_user }.to raise_error Gitlab::Auth::UserAuthFinders::RevokedError + expect { current_user }.to raise_error Gitlab::Auth::RevokedError end it 'does not allow expired tokens' do personal_access_token.update_attributes!(expires_at: 1.day.ago) env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token - expect { current_user }.to raise_error Gitlab::Auth::UserAuthFinders::ExpiredError + expect { current_user }.to raise_error Gitlab::Auth::ExpiredError end end end @@ -392,7 +392,7 @@ describe API::Helpers do end it 'raises an error' do - expect { current_user }.to raise_error Gitlab::Auth::UserAuthFinders::InsufficientScopeError + expect { current_user }.to raise_error Gitlab::Auth::InsufficientScopeError end end end