change to acl_read module for supporting dirsync module

Kamen Mazdrashki kamenim at
Sat Mar 12 06:37:11 MST 2011

Hi Nadya,

The way a read it, it seems to me acl_read module changes its behavior
in case we are processing DIRSYNC control.
'dirsync' module needs just one attribute not to be processed, but now we
have a 'dirsync' flag and changed behavior during Access Check.
And this is because 'dirsync' module requires replPropertyMetaData
attribute not to be filtered.
So, my question is: what will happen if we need another attribute in a module
above acl_read not to be filtered? Going in this direction, we should have
another branch (and perhaps another flag in acl_read module context).
And this is a dependency in my opinion as acl_read module changes
it behavior because of a module above it.

Btw, I think there is a bug in this patch:;a=commitdiff;h=a53c047602aa60dca71ac1c42e5b5263b046d107
and it is that if replPropertyMetaData is not the last attribute in
then we are going to skip the check on the next element.

And this is another aspect of my concern - implementing special cases
leads to hard to catch bugs.

I certainly have nothing against this patch to enter the master at
this point as I don't see a quick way to overcome it.

My appeal here is for all of us to thing about a generic
mechanism to handle such cases rather than implementing
special cases.


On Fri, Mar 11, 2011 at 10:04, Nadezhda Ivanova <nivanova at> wrote:
> Hi Kamen,
> The way I understood it, the acl_read needs to do a bit of extra filtering
> if the dirsync control with a certain flag is present in the request. It is
> just an extra action depending on the result of access checks and whether we
> have dirsync control specified, not a dependency. To avoid this, we will
> have to repeat a lot of the code from acl_read in dirsync module, or have
> acl_read signal dirsync that attributes are inaccessible to this user, which
> I think is more of a dependency that just checking for the same control.
> Regards,
> Nadya
> On Fri, Mar 11, 2011 at 12:55 AM, Kamen Mazdrashki <kamenim at>
> wrote:
>> Hi Matthieu,
>> Sorry I am entering this discussion so late, but I just want to share with
>> you my concerns about this patch.
>> I haven't looked in depth your 'dyrsinc' patches, so excuse my ignorance
>> if I am missing something :)
>> And thanks a lot for taking the Dyrsinc task!
>> (it was about time for us to support it)
>> My main concern is that this patch makes a subtle link between acl_read
>> module and dyr_sync module. I don't feel like it is the responsibility of
>> acl_read module to "know" we are processing another control or something.
>> As far as I see it, we should make our best to implement clean interfaces
>> between module. I mean, that it is very complex to support a filter system
>> like that in LDB when there are inter-dependencies between modules.
>> Can we avoid this somehow?
>> One way to avoid this is for us to mark certain attributes as INTERNAL
>> or SERVICE or something else (I think we have such a mechanism
>> introduced by abartlet and tridge with attribute flags).
>> What I am thinking is, that it will be easier to teach modules like
>> acl_read
>> not to touch attributes with given flags, rather that teaching them how to
>> work in different situations.
>> --
>> CU,
>> Kamen
>> On Wed, Mar 9, 2011 at 00:34, Matthieu Patou <mat at> wrote:
>> > Hello Nadya,
>> >
>> > Can you have a look at this:
>> >
>> >
>> >;a=blobdiff;f=source4/dsdb/samdb/ldb_modules/acl_read.c;h=e7c54c970bd0ff9aaa8f1e5e85e7754bb845a7c6;hp=cde6d11c75bd07d4f6dd032e9f01d2b8e78eb2a9;hb=a53c047602aa60dca71ac1c42e5b5263b046d107;hpb=370d40b848b18bdceeda06f156662860a525615d
>> >
>> > And tell me if you are OK, basically it's about not to return
>> > when a searched attribute is not accessible but instead to remove the
>> > replPropertyMetaData attribute to give the signal to dirsync that the
>> > user
>> > didn't have an access on this object and so an empty DN with just the
>> > objectGUID should be returned.
>> >
>> > Matthieu.
>> >
>> > --
>> > Matthieu Patou
>> > Samba Team
>> > Private repo;a=summary
>> >
>> >
>> >

More information about the samba-technical mailing list