change to acl_read module for supporting dirsync module

[Kamen Mazdrashki]

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:
http://git.samba.org/?p=mat/samba.git;a=commitdiff;h=a53c047602aa60dca71ac1c42e5b5263b046d107
and it is that if replPropertyMetaData is not the last attribute in
msg->elements,
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.

-- 
CU,
Kamen


On Fri, Mar 11, 2011 at 10:04, Nadezhda Ivanova <nivanova at samba.org> 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 samba.org>
> 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 samba.org> wrote:
>> > Hello Nadya,
>> >
>> > Can you have a look at this:
>> >
>> >
>> > http://git.samba.org/?p=mat/samba.git;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
>> > LDB_SUCCESS
>> > 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        http://samba.org
>> > Private repo      http://git.samba.org/?p=mat/samba.git;a=summary
>> >
>> >
>> >
>
>