When I try to add a new server with the Service Account Bind option selected, I get a form validation error that the bind password is required even though I've provided one. (See attached screen shot.)
I am looking at the code now to see if I can identify the problem.
If I add the server using Anonymous Bind, then come back later and switch to Service Account Bind, it appears to work, although the password field is still visible, so it's hard to tell if the password has been set or not. (According to the database, it has.) The old LDAP Integration module hid the password field when one was set. For now, I am considering this to be a secondary portion of this issue that we may prefer to address separately.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 1119774-04.patch | 2.54 KB | micahw156 |
| #2 | 1119774-01.patch | 1.86 KB | micahw156 |
| ldap-server-bindpw-error.gif | 6.57 KB | micahw156 |
Comments
Comment #1
johnbarclay commentedThanks. Yes there is an additional usability issue. The clear stored password confuses some and there should be an indicator that there is or is not a stored password in the db.
Comment #2
micahw156I believe the attached patch fixes all three problems mentioned in this issue.
Form validation was failing because it was testing $this->bindpw instead of $this->bindpw_new, which is the value returned by populateFromDrupalForm().
I changed an existing test in drupalForm() to make either bindpw or clear_bindpw field hidden depending on whether or not the bindpw value was already populated. This might not be the cleanest way to handle this, but it eliminates conflicting fields that will confuse site admins without breaking any of the existing form processing.
Comment #3
micahw156Sorry, submitted that patch too soon. Validation breaks when you try to edit an existing server. Let me track that one down, too.
Comment #4
micahw156Ok, the validation part of the patch now checks to see if either $this->bindpw or $this->bindpw_new are set. That handles cases where $this has an existing password already.
I also discovered that the clear_bindpw checkbox didn't actually do anything, so I fixed that, too.
Comment #5
weboide commentedI had the same issue and the above patch solved it. Thanks micahw156.
Comment #6
johnbarclay commentedI did this a little differently. Can you give feedback. I'm pushing my commit out once I get through the issue queue.
Comment #7
micahw156I was able to add a new server without any problem after commit 64e2ff0.
However, the changes to field hiding are not there, and the checkbox to clear existing password does nothing. I will work on an updated patch for this after I complete my current round of testing.
Comment #8
johnbarclay commentedthanks. I approached this a little diferently than your patch. I added a message "You currently have a password stored in the database. Leave password field emtpy to leave password unchanged. Enter a new password to replace the current password. Check the checkbox below to simply remove it from the database. " when a password is in db. I meant to highlight it with a green checkbox to the left when the user has a password in the db to further highlight that they already have a pwd in the db. And some sort of red x when they have no password AND have service account bind selected. I'll add this at some point.
So I didn't use your patch. But a patch to fix the checkbox will be helpful.
Comment #9
johnbarclay commented