Support for rfc3062 Password Modify, closes #163

[mynameisrufus]

This implements the password modify extended request
http://tools.ietf.org/html/rfc3062

Change existing password:

dn = 'uid=modify-password-user1,ou=People,dc=rubyldap,dc=com'

auth = {
  method: :simple,
  username: dn,
  password: 'passworD1'
}

ldap.password_modify(
  dn: dn,
  auth: auth,
  old_password: 'passworD1',
  new_password: 'passworD2'
)

Or get the LDAP server to generate a password for you:

dn = 'uid=modify-password-user1,ou=People,dc=rubyldap,dc=com'

auth = {
  method: :simple,
  username: dn,
  password: 'passworD1'
}

ldap.password_modify(
  dn: dn,
  auth: auth,
  old_password: 'passworD1'
)

ldap.get_operation_result.extended_response[0][0] #=> 'VtcgGf/G'

[jch]

@mynameisrufus thanks for creating this PR. One of us will take a look when we free up a bit, possibly tomorrow.

[mtodd]

👎 as much as I like pry, I don't think this is a necessary development dependency.

[mtodd]

Thanks for submitting this!

My only gripe would be the get_gen_passwd_result API (as mentioned above). My reasoning is that our API should not be hardcoded for each extended operation possible but expose that data generally so new extensions don't require adding new methods to Net::LDAP et al. Does that make sense?

Caveat lector: I'm not intimately familiar with extended operations and how other APIs implement this. From taking a quick look at the Perl Net::LDAP library, its API is different enough such that returning a message object that responds to gen_password doesn't produce the same code smell. I'm in favor of emulating this API but that would require drastic changes to our own.

[mynameisrufus]

Thanks for the great feedback, agree on all points.

[jch]

👍 to this abstraction.

Couple of nitpicks:

• What's your reasoning for adding the ! in assert_response!?
• Minitest assertions typically have the expected value as the first argument, and the subject under test as the 2nd. It'd be good to flip the order of your argument list to match unless there's a good reason otherwise.
• Rather than raise an exception, you can use flunk with a descriptive message. Something like:

unless pdu && pdu.app_tag == tag
  flunk "Expected #{tag.inspect}, but got #{(pdu && pdu.app_tag).inspect}"
end

[mtodd]

@jch this is a runtime assertion, not in test. We were already enforcing this assertion, but it was repeated everywhere, hence the abstraction.

[jch]

Aaah, sorry. My brain automatically assumed it was a test helper. I guess the bang ! is because it raises an exception.

[mynameisrufus]

Rebased with feedback changes made, I have left the extended response a little raw as I think migrating to a better approach would require drastic changes like you said. Supporting auto generated password is a bit of a cherry on top anyway, I actually don't need it but figured implementing the entire operation as per the spec would be a win.

ldap.get_operation_result.extended_response[0][0] #=> 'VtcgGf/G'

If I get time I will look around at other libs an see what a good way of doing this would be, maybe open a separate PR....

[mynameisrufus]

Any chance of getting this merged?

[mtodd]

@mynameisrufus I'm really only blocked on researching the 139 primitive value, and haven't had the time to do that. If you'd like to dig in and get to the bottom of it, I'd really appreciate it. Otherwise hopefully the end of the week I'll have some time to check it out.

[mynameisrufus]

I have moved the the value from primitive into APPLICATION see http://tools.ietf.org/html/rfc4511#section-4.10 for example that has [APPLICATION 14], this seems incorrect as http://tools.ietf.org/html/rfc3062#section-2 does not have [APPLICATION 43] it's just a SEQUENCE. 0b01000000, # 64 + 43 gives us 139 the number that we had in primitive, this is only for the genPasswd response value if returned.

I could be wrong but other libs I have been reading do not really concern themselves with these types, they work more along the line of (python3-ldap):

o = ModifyPassword.new(connection, options)
o.result #=> '2ex7c3'

Here it just assumes it's getting back a sequence?

[mynameisrufus]

I have captured a response from wireshark:

decode = ->(str) { str.split(':').map(&:hex).map(&:chr).join }
data = '30:1a:02:01:02:78:15:0a:01:00:04:00:04:00:8b:0c:30:0a:80:08:66:47:6d:63:75:62:34:71'

str = decode.call(data)

str #=> "0\x1A\x02\x01\x02x\x15\n\x01\x00\x04\x00\x04\x00\x8B\f0\n\x80\bfGmcub4q"

bytes = []

io = StringIO.new(str)

while byte = io.getbyte
  bytes << byte
end

bytes #=> [48, 26, 2, 1, 2, 120, 21, 10, 1, 0, 4, 0, 4, 0, 139, 12, 48, 10, 128, 8, 102, 71, 109, 99, 117, 98, 52, 113]

139.chr #=> "\x8B"

[mynameisrufus]

@mtodd looks like unicode 139, 008B is ‹ partial line forward http://www.fileformat.info/info/unicode/char/008b/index.htm

https://github.com/osstech-jp/openldap/blob/master/libraries/liblunicode/UnicodeData.txt#L140

The response should look like this:

0����x�
���‹0
€�fGmcub4q

I'm having trouble actually printing that out properly in irb 😞

[mynameisrufus]

@mtodd I have moved the value into universal, context specific constructed will not work because it starts at 160 and it's not a primitive.

To be honest I don't know why this stuff is written the way it is when all that is resulted is an array with 256 length with values at index with the types, why not just one simple flat dict.

[mtodd]

@mynameisrufus I agree 100% that the parsing behavior needs to be refactored.

I'm uncomfortable accepting this as is without a better understanding of the specific value we're adding here. This change doesn't align with my understanding of the ExtendedResponse mechanics. I don't want to accept something that makes it work but isn't clear why because it will likely have unintended/unaccounted for consequences.

[mynameisrufus]

What do you mean by 'specific value'?, to me the specific value is in the title of the PR, support for modify password http://tools.ietf.org/html/rfc3062.

What is your understanding of the mechanics of ExtendedResponse, if you could share with me your understanding I would be happy to make changes to this PR.

I have invested considerable time in understanding this codebase and have addressed every point raised by yourself and @jch. I have sniffed packets, groked Open LDAP source and the source of many other libs in order to comprehensively document and explain each change I have made, I am committed to delivering this feature and like I said, I am happy to address any concerns you have or explain the workings of LDAP or this library. I fully understand that you have inherited this codebase so I understand the fear you may have in accepting changes to this lib.

[mynameisrufus]

@jch @satoryu rebased 👍

[jch]

@mynameisrufus thanks for following through on this PR. I think you did a great job addressing all of our concerns. There's more cleanup we could do to the general API, but that's out of scope for your changes, and I think this feature has merit on it's own. I just released 0.13.0, so I will wait a bit in case there are other PR's to merge before cutting a new release.

Thanks!

[mynameisrufus]

@jch happy new year and thank you.