ACL support for omindex

[egarette]

I've done an ACL support patch.

This patch add complet ACL's and posix's right support. All user's and group's informations are store (all posix right too).

ACL support is only supported if compiled with libacl header. Otherwise ACL informations are ignore.

If rights are change, omindex did not reindex files (so new posix/ACL rights are not modifying). So, I've change mtime to ctime check in omindex.

This patch is generated from master's branch.

[Olly Betts]

Thanks for the patch. Some thoughts:

The switch in get_acls() is missing break; at the end of the cases. If this is deliberate, it really deserves a comment to make it clear this isn't an oversight.

You seem to have removed the code which suppresses adding I<user> and I@<group> terms when the file is world-readable (and so has an I* term). What's the reasoning behind that?

I don't understand the logic for adding O and G prefixed terms from ACLs. These are meant to indicate which user and group own the file, so you can search for "all files owned by X".

I don't understand how your patch handles an ACL saying who can't read a file. You need to add V prefixed terms for those.

The change from mtime to ctime will mean that the "last modified" time reported in the Omega UI will now in general not actually be the last time the contents of the file were changed.

I'm also slightly concerned that the mtime -> ctime change will result in reindexing files in many more cases - e.g. if I tar up a file tree and Xapian database and untar it on another machine (as a non-privileged user), the mtimes are preserved but the ctimes change. So this change would mean that omindex would have to reindex every document in this case (and without root access, I don't think one can avoid that).

I think we probably need to store the ctime separately (so lastmod still works as before) and make whether ctime or mtime is used for reindexing an option, or else find a better way to know when ACLs have changed - perhaps only checking the ACL for changes if the ctime has changed but the mtime hasn't.

It would also be good to have some some tests for this, which create some files and set up ACLs on them, index them, and check the results of filtering match what they should.

I have a patch waiting to be merged which handles ACLs on SMB shares - I'll dig that out and compare it. In that patch, I just ignored the issue of the ACLs changing after a file had been indexed (at worst it means a user can see document they would once have been able to read in the search results, and any snippets shown will come from the version they were able to read).

[egarette]

Replying to olly:

Thanks for the patch. Some thoughts:

Let met reply in separate comments.

The switch in get_acls() is missing break; at the end of the cases. If this is deliberate, it really deserves a comment to make it clear this isn't an oversight.

Oh! Sorry for this mistake, it's unintentional oversight during the patch creation process.

[egarette]

Replying to olly:

You seem to have removed the code which suppresses adding I<user> and I@<group> terms when the file is world-readable (and so has an I* term). What's the reasoning behind that?

I don't understand the logic for adding O and G prefixed terms from ACLs. These are meant to indicate which user and group own the file, so you can search for "all files owned by X".

I don't understand how your patch handles an ACL saying who can't read a file. You need to add V prefixed terms for those.

I've remove this code because it's same problem as group's right describe here : ​http://lists.xapian.org/pipermail/xapian-discuss/2013-October/009024.html. The problem arrive when you have an ACL like this:

$ getfacl file1.txt
# file: file1.txt
# owner: root
# group: root
user::rw-
user:user1:---
user:user2:r--
group::r--
mask::r--
other::r--

$ delve -r 1 ../db/
Term List for record #1: D20140226 Etxt Ffile1 Groot I#root I* I@root I@user2 M201402 Oroot Ouser1 Ouser2 P/ Ttext/plain U/file1.txt Y2014 ZFfile1 Zappl Zeat Zi Zlike Zto apples eat i like to

$ getfacl file2.txt
# file: file2.txt
# owner: root
# group: root
user::rw-
group::r--
group:user1:---
group:user2:r--
mask::r--
other::r--

$ delve -r 4 ../db/
Term List for record #4: D20140226 Etxt Ffile2 Groot Guser1 Guser2 I#root I#user2 I* I@root M201402 Oroot P/ Ttext/plain U/file2.txt Y2014 ZFfile2 Zeat Zhoney Zi Zlike Zto eat honey i like to

$ getfacl file3.txt
# file: file3.txt
# owner: root
# group: root
user::rw-
group::r--
group:user1:r--
mask::r--
other::r--

$ delve -r 3 ../db/
Term List for record #3: D20140226 Etxt Ffile3 Groot Guser1 I#root I#user1 I* I@root M201402 Oroot P/ Ttext/plain U/file3.txt Y2014 ZFfile3 Zchees Zeat Zi Zlike Zto cheese eat i like to

$ getfacl file4.txt
# file: file4.txt
# owner: root
# group: root
user::rw-
group::r--
other::r--

$ delve -r 2 ../db/
Term List for record #2: D20140226 Etxt Ffile4 Groot I#root I* I@root M201402 Oroot P/ Ttext/plain U/file4.txt Y2014 ZFfile4 Zeat Zi Zlike Zmushroom Zto eat i like mushrooms to

$ getfacl file5.txt
# file: file5.txt
# owner: root
# group: root
user::rw-
group::r--
other::---

$ delve -r 5 ../db/
Term List for record #5: D20140226 Etxt Ffile5 Groot I#root I@root M201402 Oroot P/ Ttext/plain U/file5.txt Y2014 ZFfile5 Zeat Zi Zlike Zmushroom Zto eat i like mushrooms to

Assuming we have two users:

$ id user1
uid=1001(user1) gid=1001(user1) groupes=1001(user1),1000(user)
$ id user2
uid=1002(user2) gid=1002(user2) groupes=1002(user2),1000(user)

user1 can only read file3.txt and file4.txt user2 can read file1.txt, file2.txt, file3.txt and file4.txt

To get files with write restrinction:

for user1: eat AND (write:@user1 OR ( ( write:#user OR write:#user1 ) NOT user:user1 ) OR ( write:* NOT user:user1 NOT group:user NOT group:user1) )

Parsed query is: Xapian::Query((Zeat:(pos=1) AND (0 * I@user1 OR ((0 * I#user OR 0 * I#user1) AND_NOT 0 * Ouser1) OR (((0 * I* AND_NOT 0 * Ouser1) AND_NOT 0 * Guser) AND_NOT 0 * Guser1))))

2 results found:
1: 100% docid=4 [url=/file4.txt
sample=I like to eat mushrooms 
type=text/plain
modtime=1393396086
size=24]

2: 100% docid=6 [url=/file3.txt
sample=I like to eat cheese 
type=text/plain
modtime=1393395683
size=21]

for user2: eat AND (write:@user2 OR ( ( write:#user OR write:#user2 ) NOT user:user2 ) OR ( write:* NOT user:user2 NOT group:user NOT group:user2) )

Parsed query is: Xapian::Query((Zeat:(pos=1) AND (0 * I@user2 OR ((0 * I#user OR 0 * I#user2) AND_NOT 0 * Ouser2) OR (((0 * I* AND_NOT 0 * Ouser2) AND_NOT 0 * Guser) AND_NOT 0 * Guser2))))

4 results found:
1: 100% docid=4 [url=/file4.txt
sample=I like to eat mushrooms 
type=text/plain
modtime=1393396086
size=24]

2: 100% docid=2 [url=/file1.txt
sample=I like to eat apples 
type=text/plain
modtime=1393388770
size=21]

3: 100% docid=6 [url=/file3.txt
sample=I like to eat cheese 
type=text/plain
modtime=1393395683
size=21]

4: 100% docid=8 [url=/file2.txt
sample=I like to eat honey 
type=text/plain
modtime=1393391391
size=20]

[egarette]

Replying to olly:

The change from mtime to ctime will mean that the "last modified" time reported in the Omega UI will now in general not actually be the last time the contents of the file were changed.

I'm also slightly concerned that the mtime -> ctime change will result in reindexing files in many more cases - e.g. if I tar up a file tree and Xapian database and untar it on another machine (as a non-privileged user), the mtimes are preserved but the ctimes change. So this change would mean that omindex would have to reindex every document in this case (and without root access, I don't think one can avoid that).

I think we probably need to store the ctime separately (so lastmod still works as before) and make whether ctime or mtime is used for reindexing an option, or else find a better way to know when ACLs have changed - perhaps only checking the ACL for changes if the ctime has changed but the mtime hasn't.

Ok, I'll remove this part of my patch and open a new ticket. It's more complicated than I had anticipated.

[egarette]

New version of my patch

[egarette]

Replying to olly:

It would also be good to have some some tests for this, which create some files and set up ACLs on them, index them, and check the results of filtering match what they should.

I did not even look at your test infrastructure but I would suggest a patch soon

[Olly Betts]

Marking to consider for 1.3.3.

[Olly Betts]

Need to get #639 sorted first, so bumping the milestone on this.

[Olly Betts]

(In #639) The thing left to do here is the optimisation for the case of "ctime changed, mtime unchanged". I don't think it makes much sense to block 1.4.0 by that - it's not a correctness issue. This also no longer blocks 632.

[Olly Betts]

It would also be good to have some some tests for this, which create some files and set up ACLs on them, index them, and check the results of filtering match what they should.

I did not even look at your test infrastructure but I would suggest a patch soon

Sadly we don't have any indexing tests of omindex currently. In git master we have the relatively new omegatest which runs some tests on the omega CGI, but currently all of these run with empty database (the current tests are mostly about how the CGI parameters are turned into a Xapian::Query object). So the test infrastructure for the indexing part of this doesn't currently exist, but it would be good to have such infrastructure so adding tests was possible. And omegatest needs a small tweak to allow using use a built database instead of an empty one.

I've been rather busy I'm afraid and haven't got to digging out my SMB ACL patch to compare yet.

It's also occurred to me that this would be an optional feature (if you're not interested in checking ACLs, there's no point wasting effort indexing them or filtering by them), so rather than blocking 1.4.0 on merging this I think it makes more sense to target this to be merged in an early 1.4.x release.

Also, have you already agreed to the patch licensing requirements:

​https://trac.xapian.org/browser/trunk/xapian-core/HACKING#L1364

[Olly Betts]

Sadly we don't have any indexing tests of omindex currently.

We've since gained omindextest which tests omindex's indexing.

Also, have you already agreed to the patch licensing requirements:

​> ​https://trac.xapian.org/browser/trunk/xapian-core/HACKING#L1364

No response to this and unfortunately we can't merge without it.

Adjusting milestone.