Commit d75ec6cd authored by Robert Speicher's avatar Robert Speicher

Merge branch 'fix-missing-saml-error-handling' into 'master'

Add proper nil and error handling to SAML login process

While writing the feature that would allow certain Omniauth providers to be marked as external I noticed that there is a scenario where the `gl_user` method can return `nil` and if this is not properly checked, it will lead to exceptions that will cause 500 errors. It is quite easy to land in this scenario, so I added `nil` checks.

I also noticed that the `saml` method in the `omniauth_callbacks_controller.rb` file lacked a `rescue` for `Gitlab::OAuth::SignupDisabledError`, which can happen if the default configuration from `1_settings.rb` is applied. So I also added this check.

See merge request !3609
parents d65d5c2d 05a611a0
...@@ -60,6 +60,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -60,6 +60,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
continue_login_process continue_login_process
end end
rescue Gitlab::OAuth::SignupDisabledError
handle_signup_error
end end
def omniauth_error def omniauth_error
...@@ -92,16 +94,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -92,16 +94,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
continue_login_process continue_login_process
end end
rescue Gitlab::OAuth::SignupDisabledError rescue Gitlab::OAuth::SignupDisabledError
label = Gitlab::OAuth::Provider.label_for(oauth['provider']) handle_signup_error
message = "Signing in using your #{label} account without a pre-existing GitLab account is not allowed."
if current_application_settings.signup_enabled?
message << " Create a GitLab account first, and then connect it to your #{label} account."
end
flash[:notice] = message
redirect_to new_user_session_path
end end
def handle_service_ticket provider, ticket def handle_service_ticket provider, ticket
...@@ -122,6 +115,19 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -122,6 +115,19 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
end end
end end
def handle_signup_error
label = Gitlab::OAuth::Provider.label_for(oauth['provider'])
message = "Signing in using your #{label} account without a pre-existing GitLab account is not allowed."
if current_application_settings.signup_enabled?
message << " Create a GitLab account first, and then connect it to your #{label} account."
end
flash[:notice] = message
redirect_to new_user_session_path
end
def oauth def oauth
@oauth ||= request.env['omniauth.auth'] @oauth ||= request.env['omniauth.auth']
end end
......
...@@ -26,7 +26,7 @@ module Gitlab ...@@ -26,7 +26,7 @@ module Gitlab
@user ||= build_new_user @user ||= build_new_user
end end
if external_users_enabled? if external_users_enabled? && @user
# Check if there is overlap between the user's groups and the external groups # Check if there is overlap between the user's groups and the external groups
# setting then set user as external or internal. # setting then set user as external or internal.
if (auth_hash.groups & Gitlab::Saml::Config.external_groups).empty? if (auth_hash.groups & Gitlab::Saml::Config.external_groups).empty?
...@@ -48,6 +48,7 @@ module Gitlab ...@@ -48,6 +48,7 @@ module Gitlab
end end
def changed? def changed?
return true unless gl_user
gl_user.changed? || gl_user.identities.any?(&:changed?) gl_user.changed? || gl_user.identities.any?(&:changed?)
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment