Project

General

Profile

Actions

Feature #33345

closed

Include an authentication method name in LDAP connection error messages

Added by Yuichi HARADA about 4 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Accounts / authentication
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:

Description

When LDAP cannot be connected for some reason, the following message is displayed.

The error message is displayed as it is, but I don't know if it is a message from LDAP. You can understand that it is an LDAP message by adding a fixed message to the beginning as shown below.


Files

before.png (29.6 KB) before.png Yuichi HARADA, 2020-04-23 07:12
after.png (34.1 KB) after.png Yuichi HARADA, 2020-04-23 07:13
improvement-ldap-error-message.patch (2.68 KB) improvement-ldap-error-message.patch Yuichi HARADA, 2020-04-23 07:19
33345-v2.patch (2.23 KB) 33345-v2.patch Go MAEDA, 2021-10-15 02:58
Actions #1

Updated by Go MAEDA about 4 years ago

  • Target version set to Candidate for next major release
Actions #2

Updated by Go MAEDA almost 4 years ago

  • Category set to Accounts / authentication
Actions #3

Updated by Go MAEDA almost 3 years ago

I think that just simply adding the prefix "LDAP" to an error message is better.

LDAP: Connection refused - connect(2) for 192.0.2.1:389

The reasons are as follows:

  • Adding the prefix "LDAP" is enough to understand that there is a problem with communication with the LDAP server
  • We don't have to add a new string to the locales
  • Redmine may support new auth sources other than LDAP in the future. The following way of using auth_method_name method can be applied for new auth sources
diff --git a/app/models/auth_source_ldap.rb b/app/models/auth_source_ldap.rb
index 8ed7ce27f..846c7d2f3 100644
--- a/app/models/auth_source_ldap.rb
+++ b/app/models/auth_source_ldap.rb
@@ -63,7 +63,7 @@ class AuthSourceLdap < AuthSource
       end
     end
   rescue *NETWORK_EXCEPTIONS => e
-    raise AuthSourceException.new(e.message)
+    raise AuthSourceException.new("#{auth_method_name}: #{e.message}")
   end

   # Test the connection to the LDAP
@@ -77,7 +77,7 @@ class AuthSourceLdap < AuthSource
       end
     end
   rescue *NETWORK_EXCEPTIONS => e
-    raise AuthSourceException.new(e.message)
+    raise AuthSourceException.new("#{auth_method_name}: #{e.message}")
   end

   def auth_method_name
@@ -107,7 +107,7 @@ class AuthSourceLdap < AuthSource
     end
     results
   rescue *NETWORK_EXCEPTIONS => e
-    raise AuthSourceException.new(e.message)
+    raise AuthSourceException.new("#{auth_method_name}: #{e.message}")
   end

   def ldap_mode
Actions #4

Updated by Yuichi HARADA almost 3 years ago

Go MAEDA wrote:

I think that just simply adding the prefix "LDAP" to an error message is better.

[...]

The reasons are as follows:

  • Adding the prefix "LDAP" is enough to understand that there is a problem with communication with the LDAP server
  • We don't have to add a new string to the locales
  • Redmine may support new auth sources other than LDAP in the future. The following way of using auth_method_name method can be applied for new auth sources

[...]

I agree. I think a patch will be simple.

Actions #5

Updated by Go MAEDA almost 3 years ago

Updated the patch using the code posted in #33345#note-3.

Actions #6

Updated by Go MAEDA over 2 years ago

  • Target version changed from Candidate for next major release to 5.0.0

Setting the target version to 5.0.0.

Actions #7

Updated by Go MAEDA over 2 years ago

  • Subject changed from Improvement of LDAP connection error message to Include an authentication method name in LDAP connection error messages
  • Status changed from New to Closed
  • Assignee set to Go MAEDA

Committed the patch. Thank you.

Actions #8

Updated by Go MAEDA over 2 years ago

  • Tracker changed from Patch to Feature
Actions

Also available in: Atom PDF