change to acl_read module for supporting dirsync module

[Nadezhda Ivanova]

Kamen,
I see your point - this is a common issue and acl_read has the same problem
- how exactly do we handle interaction with modules that by definition have
to remove content from the search result.
About this particular patch:
Matthieu, I read MS-ADTS 3.1.1.3.4.1, but I did not see where it says that
if any attribute is inaccessible, then we only return objectGUID. What it
says is:
If this flag is specified, the client can only view
objects<http://msdn.microsoft.com/en-us/library/b645c125-a7da-4097-84a1-2fa7cea07714%28v=PROT.10%29#object>and
attributes<http://msdn.microsoft.com/en-us/library/b645c125-a7da-4097-84a1-2fa7cea07714%28v=PROT.10%29#attribute>that
are otherwise accessible to the client, except that changes made to
the
isDeleted<http://msdn.microsoft.com/en-us/library/cc220036%28v=PROT.10%29.aspx>and
isRecycled<http://msdn.microsoft.com/en-us/library/dd357315%28v=PROT.10%29.aspx>attributes
are returned.

So the problem comes when an user does not have the rights to see deleted
objects? Do you have a test that investigates what would happen in this
case?

Regards,
Nadya
On Sat, Mar 12, 2011 at 3:37 PM, Kamen Mazdrashki <kamenim at samba.org> wrote:

> 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
> >> >
> >> >
> >> >
> >
> >
>