Directory Studio
  1. Directory Studio
  2. DIRSTUDIO-744

The strategy used when deleting the last attribute value causes issues in the case when ACLs/ACIs hide and forbid access to other values

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.5.3
    • Fix Version/s: 2.0.0
    • Component/s: studio-ldapbrowser
    • Labels:
      None

      Description

      This issue has been reported to me by Daniel Pluta, who I met at LDAPCon 2011.

      Here's a copy of the bug he described me and which i've been able to reproduce with OpenLDAP.

      %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
      ADS causes a problem when I want to delete a value from a multi-value attribute (here e.g. member) in case the following ACLs are active:

      access to dn.base="ou=groups,dc=foo,dc=bar" attrs=children
      by users read
      by * none

      access to dn.onelevel="ou=groups,dc=foo,dc=bar" attrs=entry,cn,description
      by users read
      by * none break

      access to dn.onelevel="ou=groups,dc=foo,dc=bar" attrs=entry,member
      by dnattr=member selfwrite
      by * none

      Based on these ACL each user that is a member of a group entry seems to
      be just the only member of these group (from the user's point of view,
      in case the user accesses the group's member attribute by read). When
      using Apache Directoy Studio to delete this only/single/last group
      member ("right click --> delete value") this results in a "to all value" operation, instead of a "to value memberDN" operation.

      => acl_mask: access to entry "cn=test,groups,dc=foo,dc=bar", attr
      "member" requested
      => acl_mask: to all values by "cn=user,ou=users,dc=foo,dc=bar", (=0)

      It seems to me that this "to all values ..." appears to be a bug in ADS, where the client (ADS) tries to be more clever than needed.
      %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

        Activity

        Hide
        Pierre-Arnaud Marcelot added a comment -

        I was pretty sure there was a valid reason for this code to specifically request the deletion of the whole attribute.

        Thanks for these insights.

        Show
        Pierre-Arnaud Marcelot added a comment - I was pretty sure there was a valid reason for this code to specifically request the deletion of the whole attribute. Thanks for these insights.
        Hide
        Stefan Seelmann added a comment -

        I think the specific case for delting the whole attribute was to support deletion of attributes that lack a equality matching rule. An example is facsimileTelphoneNumber, when you try to delete the specific value most servers complain that no equality matching rules exists.

        There were multiple issues regarding this and the result was that we now check if a matching rule exists, additionally we allow the user to override some behaviour in the edit options tab of the connection properties. But that's not enough.

        I think the CompoundModification class is not the right place to fix the problem as it only changes the internal model. The right place should be in module ldapbrowser.core, class org.apache.directory.studio.ldapbrowser.core.utils.Utils, method computeDiff(IEntry, IEntry). It computes the difference in LDIF format that is used to create the LDAP request.

        I'll take a look at this code at the weekend. I'm shocked that I wrote that much code without test cases, I'm really ashamed.

        Show
        Stefan Seelmann added a comment - I think the specific case for delting the whole attribute was to support deletion of attributes that lack a equality matching rule. An example is facsimileTelphoneNumber, when you try to delete the specific value most servers complain that no equality matching rules exists. There were multiple issues regarding this and the result was that we now check if a matching rule exists, additionally we allow the user to override some behaviour in the edit options tab of the connection properties. But that's not enough. I think the CompoundModification class is not the right place to fix the problem as it only changes the internal model. The right place should be in module ldapbrowser.core, class org.apache.directory.studio.ldapbrowser.core.utils.Utils, method computeDiff(IEntry, IEntry). It computes the difference in LDIF format that is used to create the LDAP request. I'll take a look at this code at the weekend. I'm shocked that I wrote that much code without test cases, I'm really ashamed.
        Hide
        Pierre-Arnaud Marcelot added a comment -

        As we've noticed with Daniel, it seems in that particular case that Studio tries to be more clever than it should be and removes the whole attribute based on the fact that on its side, the deleted value was the last remaining one and therefore the full attribute itself has to be removed.

        I debugged this in the code and the part of code deleting the whole attribute is located in the 'org.apache.directory.studio.ldapbrowser.core.utils.CompoundModification' class.

        Between lines 151 to 154, the following code is responsible for the whole attribute removal:
        151: if ( attribute.getValueSize() == 0 )
        152:

        { 153: attribute.getEntry().deleteAttribute( attribute ); 154: }

        I've commented it and it seems to fix the issue without creating any particular side effect I could notice.

        @Stefan: Do you remember any specific case where this code might be needed or do you think we can simply get rid of it?

        Show
        Pierre-Arnaud Marcelot added a comment - As we've noticed with Daniel, it seems in that particular case that Studio tries to be more clever than it should be and removes the whole attribute based on the fact that on its side, the deleted value was the last remaining one and therefore the full attribute itself has to be removed. I debugged this in the code and the part of code deleting the whole attribute is located in the 'org.apache.directory.studio.ldapbrowser.core.utils.CompoundModification' class. Between lines 151 to 154, the following code is responsible for the whole attribute removal: 151: if ( attribute.getValueSize() == 0 ) 152: { 153: attribute.getEntry().deleteAttribute( attribute ); 154: } I've commented it and it seems to fix the issue without creating any particular side effect I could notice. @Stefan: Do you remember any specific case where this code might be needed or do you think we can simply get rid of it?

          People

          • Assignee:
            Pierre-Arnaud Marcelot
            Reporter:
            Pierre-Arnaud Marcelot
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development