#2954 Make --*attr more robust
Closed: wontfix 5 years ago Opened 11 years ago by jcholast.

The --*attr handling code makes assumptions about attribute values that are not always true:

--addattr assumes that the attribute is multi-valued:

$ ipa user-add test --first Test --last Test --addattr sn=Test
ipa: ERROR: an internal error has occurred

--delattr assumes that the attribute is unicode:

$ ipa user-mod test --delattr ipasshpubkey=AAAA<snip>
ipa: ERROR: ipasshpubkey does not contain 'AAAA<snip>'

Fix --*attr handling so all of the above work correctly.


When trying to rebase the DN work I ran into significant issues with this patch, I don't think it should have been accepted. I spent almost a full day trying to make the patch work but gave up. In the dn branch I've reverted the patch so we can move forward, below are my commit comments from the commit which reverts it. We need to take a deeper look at how setattr, addattr and delattr are handled, they are a very peculiar exception to our public API whereby we expose internal LDAP that we otherwise have tried very hard to hide. Because these are LDAP and NOT command parameters we should be using code that knows how to read and convert LDAP attribute/values (we have code to do this but it probably needs refactoring). At a minimum the code that reads the setattr, addattr, delattr MUST know the syntax of the attribute so it can perform proper type conversion and we have to finally decide how binary data is passed in commands (I think we're still a bit inconsistent). For example do we have a rule which says when binary data is passed to a command parameters it must be base64 encoded? If so then at the command parameter level for setattr, addattr, delattr we need ask the question "is the attribute binary?", if so then base64 decode it. The ldap2 code knows which attributes are binary and has table driven logic to perform type conversion based on the syntax, but currently it never expects the input will be base64,. We need an internal API that knows how to perform attribute/value conversion not in the context of LDAP results, but in the context of command parameters.

There were multiple problems with this patch:

* test_attr.py cannot assume the tests/test_xmlrpc/service.crt file exists
  it needs to test for its existence and skip if it's not there.

* test_attr.py is using the wrong type of data for certs
  - cert LDAP attribute values are DER encoded binary data

  - the cert data should have been loaded like this:
    servercert_der = x509.load_certificate_from_file('tests/test_xmlrpc/service.crt').der_data

  - Commands which accept cert parameters expect base64 encoded data, thus
    usercertificate=base64.b64encode(servercert_der)

  - But results are binary, thus the expected value needs to be:
    usercertificate=servercert_der

  - setattr, addattr, delattr operate on LDAP values, *NOT* command
    parameters In the case of certs the expected type is binary
    because that's the LDAP type for that attribute. It's impossible
    to pass binary data to setattr, addattr, delattr because we
    require those parameters to be unicode strings but you can't have
    a unicode string that looks like:

    delattr=u'usercertificate=%s' % servercert_der,

    The %s will attempt encoding, using %r produces a string
    representaton which isn't correct either. If you try to
    concatenate like this:

    u'usercertificate=' + servercert_der

    you'll get decode errrors because it attempt to decode the binary
    data to produce unicode, and if you try to use str like this:

    'usercertificate=' + servercert_der

    The param type will reject it as non-Unicode.

    Hence with our current mechanisms you can't pass binary data to
    setattr, addattr, delattr. We need a much more comprehensive
    mechanism for type conversion. In the case of setattr, addattr,
    delattr since those are LDAP attributes with known syntax we need
    to interrogate the schema. The ldap code already has mechaism to
    convert values based on attribute syntax, we should be using
    those functions.

* The _convert_entry() function was introducing base64 encoded values
  where they didn't belong playing havoc with our data model.

* The _convert_entry() function changed scalar values into lists. Thus
  command results prior to the patch which were a scalar now became a
  list with one scalar value, once again changing the data model.

Changing 3.2 priority

Note: interactive_prompt_callback (for example in dns-zone-add) should work sanely with --*attr, too.

3.4 development was shifted by one month, moving tickets to reflect reality better.

Adjusting time plan - 3.4 development was postponed as we focused on 3.3.x testing and stabilization.

Moving unfinished November tickets to January.

This refactoring was not finished in 4.0 time frame, moving to later release.

The FreeIPA 4.2 was already shaped (see [[milestone:FreeIPA 4.2]] milestone), this does not fit. Pushing out.

If anyone is willing to help and contribute to this one, please let us know!

Metadata Update from @jcholast:
- Issue assigned to jcholast
- Issue set to the milestone: FreeIPA 4.5 backlog

7 years ago

Thank you taking time to submit this request for FreeIPA. Unfortunately this bug was not given priority and the team lacks the capacity to work on it at this time.

Given that we are unable to fulfil this request I am closing the issue as wontfix. To request re-consideration of this decision please reopen this issue and provide additional technical details about its importance to you.

Metadata Update from @rcritten:
- Issue close_status updated to: wontfix
- Issue status updated to: Closed (was: Open)

5 years ago

Login to comment on this ticket.

Metadata