Added ACL feature #2190
Added ACL feature #2190
Conversation
|
@AlexBlueSteele Do you want to continue on this PR? If so, could you make sure that all the tests are passing locally? And then I can start reviewing the code ;) |
I will work on this. Thanks! |
|
|
I'm wondering if we need the acl_support setting? I believe that the other one is enough. WDYT? |
I think it's nice to have because some people might not care about ACLs when crawling on windows. It adds slight overhead in storage and performance. |
|
@dadoonet I updated the demo data above to reflect the ACL collection for the folder index as well. The final part that I think needs to be changed its the verbosity of the debug statements. I don't know how verbose you want it. Thoughts? |
dadoonet
left a comment
dadoonet
left a comment
There was a problem hiding this comment.
That's a great start and thank you so much for working on this.
Bonus point for writing some documentation ;)
Could you fix the issues I mentioned before?
Also, next steps would be IMO to add if possible an Integration Tests about this. May be in FsCrawlerTestAttributesIT?
Would that be possible for you?
Also did you run the code locally and does it do what you are expecting?
I'm still wondering if we really need another acl_support attribute or consider that we want want to collect all the available metadata on files and folders whenever we are asking for attributes...
| import java.time.temporal.ChronoUnit; | ||
| import java.util.ArrayList; | ||
| import java.util.Collection; | ||
| import java.util.List; |
| if (acls == null || acls.isEmpty()) { | ||
| this.acls = Collections.emptyList(); | ||
| } else { | ||
| this.acls = Collections.unmodifiableList(new ArrayList<>(acls)); |
There was a problem hiding this comment.
Can't we write this?
this.acls = Collections.unmodifiableList(acls);Or may be better:
Replace the whole:
if (acls == null || acls.isEmpty()) {
this.acls = Collections.emptyList();
} else {
this.acls = Collections.unmodifiableList(new ArrayList<>(acls));
}With:
this.acls = acls;| } | ||
| if (fsSettings.getFs().isAclSupport()) { | ||
| List<FileAcl> fileAcls = fileAbstractModel.getAcls(); | ||
| if (!fileAcls.isEmpty()) { |
There was a problem hiding this comment.
Why do we need to check this? Can't we just return whatever the fileAbstractModel.getAcls(); sent?
| path = new Path(); | ||
| file = new File(); | ||
| file.setContentType(CONTENT_TYPE); | ||
| attributes = null; |
| file.setLastModified(localDateTimeToDate(modification)); | ||
| file.setCreated(localDateTimeToDate(creation)); | ||
| file.setLastAccessed(localDateTimeToDate(lastAccess)); | ||
| attributes = null; |
| System.out.println("[ACL DEBUG] Entry -> principal=" + principal + ", type=" + type + ", permissions=" + permissions + ", flags=" + flags); | ||
| } | ||
|
|
||
| return Collections.unmodifiableList(result); |
| result.add(new FileAcl(principal, type, permissions, flags)); | ||
| System.out.println("[ACL DEBUG] Entry -> principal=" + principal + ", type=" + type + ", permissions=" + permissions + ", flags=" + flags); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add something like:
logger.debug("ACL entries found for [{}]: {}", file, result);|
|
||
| return Collections.unmodifiableList(result); | ||
| } catch (Exception e) { | ||
| System.out.println("[ACL DEBUG] Failed to resolve ACLs for " + file.getAbsolutePath() + ": " + e.getMessage()); |
| } catch (Exception e) { | ||
| System.out.println("[ACL DEBUG] Failed to resolve ACLs for " + file.getAbsolutePath() + ": " + e.getMessage()); | ||
| logger.warn("Failed to determine 'acl' of {}: {}", file, e.getMessage()); | ||
| return Collections.emptyList(); |
There was a problem hiding this comment.
Also add logger.debug("Failed to determine acl for [{}]", file, e);
|
|
||
| @Test | ||
| public void aclEntries() { | ||
| assertThat(FsCrawlerUtil.getFileAcls(file)).isNotNull(); |
There was a problem hiding this comment.
May be it's not going to work on Linux?
I did not test your code yet ;)
But ideally, if we are running on Windows (see lines 56-58), we can check if we have some acl for the file and then test that the output of the getFileAcls is what we expect?
I will work on this as soon as I get the chance! Thanks for spending the time to CR!! |
2 participants
Ticket: #2186
Test run output:
File index
Folder index
Note
Adds
fs.acl_supportto collect and index file/folder ACLs, updates mappings/templates, wiring, docs, and tests.FileAclandFsCrawlerUtil#getFileAcls(...); adds ACL extraction for local files.beans.Attributeswithaclandbeans.Folderwithattributes; propagate attributes/ACLs inFsParserAbstract.FileAbstractModelcarriesacls;FileAbstractorFilefills ACLs (FTP/SSH return empty lists).fs.acl_support(+ defaults, parsing, validation warning ifattributes_supportis false).fscrawler_mapping_attributeswithattributes.acl.{principal,type,permissions,flags}.acl_supportin admin guides (index.rst,local-fs.rst,rest.rst) with examples.FsCrawlerUtilTest,JsonUtilTest, settings loader/parser tests) and update sample configs.Written by Cursor Bugbot for commit b61843c. This will update automatically on new commits. Configure here.