Improve handling of misconfigured LDAP accounts.

Gitlab requires an email address for all user accounts as this is the
default account id and is used for sending notifications. LDAP accounts
may be missing email fields so handle this by showing a sensible error
message before redirecting to the login screen again.

Resolves github issue #899

Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>

2 files changed, 15 insertions, 1 deletions

diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index 629b6819fb1..fb759c371c4 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/app/controllers/omniauth_callbacks_controller.rb
@@ -1,4 +1,17 @@
 class OmniauthCallbacksController < Devise::OmniauthCallbacksController
+
+  # Extend the standard message generation to accept our custom exception
+  def failure_message
+    exception = env["omniauth.error"]
+    if exception.class == OmniAuth::Error
+      error = exception.message
+    else
+      error   = exception.error_reason if exception.respond_to?(:error_reason)
+      error ||= exception.error        if exception.respond_to?(:error)
+      error ||= env["omniauth.error.type"].to_s
+    end
+    error.to_s.humanize if error
+  end
  
   def ldap
     # We only find ourselves here if the authentication to LDAP was successful.
diff --git a/app/models/user.rb b/app/models/user.rb
index b87e149ca0f..a3e08fa7d0b 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -80,7 +80,8 @@ class User < ActiveRecord::Base
 
   def self.find_for_ldap_auth(omniauth_info)
     name = omniauth_info.name.force_encoding("utf-8")
-    email = omniauth_info.email.downcase
+    email = omniauth_info.email.downcase unless omniauth_info.email.nil?
+    raise OmniAuth::Error, "LDAP accounts must provide an email address" if email.nil?
 
     if @user = User.find_by_email(email)
       @user