Audit module (Change Tracking) #390

litvinovg · 2023-05-19T10:32:38Z

VIVO GitHub issue 1
VIVO GitHub issue 2

What does this pull request do?

The pull request adds a module that enables tracking of changes being made in the triple store by users and non-person entitites.
Changes are recorded in a triple store, with the users ID (URI), the time, and the changes that have been made.
A user interface (/audit) is also provided that, when logged in, displays the changes.

What's new?

Adds a new AuditModule application module
TDBAuditModule can be configured / enabled via the applicationSetup.n3
RDFService change listener capture any changes made to the content and configuration stores, and record those changes in a dedicated audit store.
Tests included

How should this be tested?

Enable AuditModule in applicationSetup.n3 (uncomment :hasAuditModule and :tdbAuditModule section)
Login as admin or root user, open /audit page,
View changes presented on the page.
Try different filters
Run tests
Try using n3 generators to add/delete data in profiles

Supersedes

#369
#81

Interested parties

@chenejac @brianjlowe

chenejac

@litvinovg thanks for this PR. It looks nice. However, I have posted three comments. There is also an issue with running tests on Windows. It is throwing an exception with permissions for deletion temporary folders. I have posted exception in a comment.

api/src/test/java/edu/cornell/mannlib/vitro/webapp/audit/storage/AuditDAOTDBTest.java

api/src/main/java/edu/cornell/mannlib/vitro/webapp/rdfservice/impl/ChangeSetImpl.java

api/src/test/java/edu/cornell/mannlib/vitro/webapp/audit/AuditDaoTDBTest.java

brianjlowe · 2023-08-24T10:01:00Z

Commenting at Georgy's request: Georgy and I discussed the possibility of refactoring the audit model to use a supplied RDFService instead of having its own triple store implementations. This would allow us to avoid the Windows temp file problems by having the tests use an in-memory triple store and would also allow for users to configure remote/cloud triple stores for their audit data if desired.

chenejac

@litvinovg well done.

milospp

Reviewed and tested the changes on MacOS M1. Everything appears to be functioning as expected. Great job on the functionality implementation!

The only suggestion I have pertains to the UI design. Additionally, it's important to avoid using StringBuilder or string concatenation for building SPARQL queries due to their susceptibility to injection vulnerabilities.

milospp · 2023-12-14T07:24:04Z

webapp/src/main/webapp/css/audit/audit.css

+
+section#audit table.history td{
+    padding: 2px 5px 2px 5px;
+}


Triplets in a table column cause text overflow on the right side of the screen. This could be solved by enabling text to wrap or by adding horizontal scroll inside the table.

Suggested change

}

}

section#audit table.history pre{

text-wrap: balance;

line-height: normal;

}

I added additional css rules to improve the view.
Didn't use text-wrap: balance as it is not widely supported yet.

milospp · 2023-12-14T08:13:47Z

api/src/main/java/edu/cornell/mannlib/vitro/webapp/audit/storage/AuditDAOJena.java

+            queryString.append("   ?dataset a <").append(AuditVocabulary.TYPE_CHANGESET).append("> . ");
+            queryString.append("   ?dataset <").append(AuditVocabulary.PROP_DATE).append("> ?date . ");
+            if (!StringUtils.isBlank(userUri)) {
+                queryString.append("   ?dataset <").append(AuditVocabulary.PROP_USER).append("> <").append(userUri)


This part of the code is vulnerable to SparQL Injection.
For example I could send a filter param for userUri like this
http://vivoweb.org/audit/resource/unknown> %7d LIMIT 999999 #

It might be possible to request any data in db through this. But since this is admin only feature I guess it's not a problem at this moment

Thanks for catching that. I think I made it safer now. Please check again.

Injection resolved. Great job!

milospp · 2023-12-14T09:53:04Z

I noticed that everytime admin makes a change his uri is unknown.

http://vivoweb.org/audit/resource/unknown

Does anyone have simmilar problem?

…ditDatTDB

…st errors on Windows and the directory will be deleted at some point anyway.

fixes

litvinovg · 2023-12-14T18:20:04Z

Does anyone have simmilar problem?

Yes. It can happen as some code still uses raw statement modifications.
For example code that logs authentication count in http://vitro.mannlib.cornell.edu/default/vitro-kb-userAccounts.
At the same time most changes related to http://vitro.mannlib.cornell.edu/default/vitro-kb-2 has correct editor uri for me.
Please, try again. I rebased PR branches on top of main.

milospp · 2023-12-20T10:11:09Z

Does anyone have simmilar problem?

Yes. It can happen as some code still uses raw statement modifications. For example code that logs authentication count in http://vitro.mannlib.cornell.edu/default/vitro-kb-userAccounts. At the same time most changes related to http://vitro.mannlib.cornell.edu/default/vitro-kb-2 has correct editor uri for me. Please, try again. I rebased PR branches on top of main.

I tested again now, and it works for graph: http://vitro.mannlib.cornell.edu/default/vitro-kb-2
Thank you!

wwelling · 2023-12-20T16:21:51Z

api/src/main/java/edu/cornell/mannlib/vitro/webapp/audit/AuditResults.java

+ * Results object for retrieving entries from the audit store
+ */
+public class AuditResults {
+    private long total;


Please make these final as they are set in the constructor.

wwelling · 2023-12-20T16:22:20Z

api/src/main/java/edu/cornell/mannlib/vitro/webapp/audit/AuditResults.java

+    private long limit;
+
+    private List<AuditChangeSet> datasets;
+


Can you provide a private default constructor?

Why do we need a private default constructor here?

Good practice. But I have tested with https://www.jdoodle.com/online-java-compiler/ and confirmed that the default constructor is not able to be called outside of reflection or classpath overlay when a constructor is declared.

wwelling · 2023-12-20T16:22:42Z

api/src/main/java/edu/cornell/mannlib/vitro/webapp/audit/AuditResults.java

+    private List<AuditChangeSet> datasets;
+
+    public AuditResults(long total, long offset, long limit, List<AuditChangeSet> datasets) {
+        this.total = total;


Call default constructor before setting class members.

wwelling · 2023-12-20T16:36:28Z

api/src/main/java/edu/cornell/mannlib/vitro/webapp/audit/controller/AuditController.java

+    private static final Log log = LogFactory.getLog(AuditController.class);
+
+    // Template for the UI
+    private static final String TEMPLATE_DEFAULT = "auditHistory.ftl";


Are these constants or their matching literal in use anywhere else? I may be ideal for package level constant files to avoid building up the static memory across the application.

No, TEMPLATE_DEFAULT used only in this class.

wwelling · 2023-12-20T16:36:53Z

api/src/main/java/edu/cornell/mannlib/vitro/webapp/audit/controller/AuditController.java

+        }
+        // Get the offset parameter (or default if unset)
+        int offset = getOffset(vreq);
+        body.put("offset", offset);


Why use the string literal when the constant is above?

wwelling · 2023-12-20T16:37:30Z

api/src/main/java/edu/cornell/mannlib/vitro/webapp/audit/controller/AuditController.java

+            offset = Integer.parseInt(vreq.getParameter(PARAM_OFFSET));
+        } catch (Throwable e) {
+            log.debug(e, e);
+            offset = 0;


This is a redundant reassignment of offset variable as if the exception was thrown it would not of reassigned in the try block.

wwelling · 2023-12-20T16:38:16Z

api/src/main/java/edu/cornell/mannlib/vitro/webapp/audit/controller/AuditController.java

+        try {
+            if (!StringUtils.isBlank(end)) {
+                Date endDate = sdf.parse(end);
+                return endDate;


Why not just return the response to the parse method call instead of temporarily allocating memory for garbage cleanup?

wwelling · 2023-12-21T15:17:19Z

api/src/main/java/edu/cornell/mannlib/vitro/webapp/audit/AbstractListStatementsMethod.java

+        // We expect two arguments
+        // 1 - an AuditChangeSet
+        // 2 - a graph URI
+        if (arguments.size() == 2) {


This appears to require a null check.

A Freemarker SimpleMethodModel
https://github.com/litvinovg/Vitro/blob/c0b48511e2b7b4446b90a4096ed19de3bae6d7bd/api/src/main/java/freemarker/ext/dump/BaseDumpDirective.java#L433C25-L433C43 calling with null. May be a red herring.

However, the API does not indicate the argument to be non-null.

https://freemarker.apache.org/docs/api/freemarker/ext/beans/SimpleMethodModel.html#exec-java.util.List-

chenejac

@litvinovg well done.

This was referenced May 19, 2023

Audit module (Change Tracking) vivo-project/VIVO#3879

Merged

Changeset user info #369

Closed

litvinovg requested a review from chenejac May 19, 2023 11:20

chenejac requested changes Aug 17, 2023

View reviewed changes

chenejac requested a review from brianjlowe August 17, 2023 14:54

litvinovg requested a review from chenejac August 24, 2023 08:30

chenejac linked an issue Aug 31, 2023 that may be closed by this pull request

VIVO-788: Developer panel - log RDF changesets. vivo-project/VIVO#2475

Closed

chenejac requested a review from milospp September 7, 2023 08:45

chenejac requested review from wwelling and removed request for brianjlowe November 16, 2023 16:01

chenejac previously approved these changes Dec 5, 2023

View reviewed changes

milospp approved these changes Dec 14, 2023

View reviewed changes

litvinovg added 16 commits December 14, 2023 18:02

merged changeset_user_info

9794e55

changed default graph uri

edf94db

audit code

6d7fe36

Modified audit controller

d8de8ae

Audit controller added user information

d7c0dc5

Add listener to RDFService configuration models

edeef81

added some filters for audit history

d44b1f3

More filtering options for audit controller

b93db9e

added user id in audit history table

591db33

Allow only admins to access audit page

5aa66e5

fix for prev commit

b92aa85

Set default end date to be the next day to avoid empty results

6bc75f7

Removed not implemented AuditDaoFS implementation, added tests for Au…

b8b280b

…ditDatTDB

added example configuration

388ae28

Fixed some typos

29d319a

Don't show empty changes on audit history page

3446c27

litvinovg added 6 commits December 14, 2023 18:02

fix: release dataset before close

b238eae

remove empty test file

4292ffb

chore: fixed formatting

290373d

Don't delete TDB dataset from temporary directory as it results in te…

6db6aa7

…st errors on Windows and the directory will be deleted at some point anyway.

refactored and improved sparql queries

6060ba8

fixes

style fixes

e102dc1

litvinovg dismissed chenejac’s stale review via e102dc1 December 14, 2023 18:09

litvinovg force-pushed the audit_tool branch from a4c3976 to e102dc1 Compare December 14, 2023 18:09

litvinovg requested a review from milospp December 14, 2023 18:20

milospp approved these changes Dec 20, 2023

View reviewed changes

wwelling reviewed Dec 20, 2023

View reviewed changes

litvinovg added 2 commits December 21, 2023 11:09

fixes

1eb38b5

one more fix

2d7621e

wwelling reviewed Dec 21, 2023

View reviewed changes

wwelling approved these changes Dec 21, 2023

View reviewed changes

chenejac approved these changes Dec 22, 2023

View reviewed changes

chenejac merged commit 440d4e8 into vivo-project:main Dec 22, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit module (Change Tracking) #390

Audit module (Change Tracking) #390

litvinovg commented May 19, 2023 •

edited

Loading

chenejac left a comment

brianjlowe commented Aug 24, 2023

chenejac left a comment

milospp left a comment

milospp Dec 14, 2023

milospp Dec 14, 2023

litvinovg Dec 14, 2023

milospp Dec 14, 2023

litvinovg Dec 14, 2023

milospp Dec 19, 2023

milospp commented Dec 14, 2023 •

edited

Loading

litvinovg commented Dec 14, 2023 •

edited

Loading

milospp commented Dec 20, 2023

wwelling Dec 20, 2023

litvinovg Dec 21, 2023

wwelling Dec 20, 2023

litvinovg Dec 21, 2023

wwelling Dec 21, 2023

wwelling Dec 20, 2023

wwelling Dec 20, 2023

litvinovg Dec 21, 2023 •

edited

Loading

wwelling Dec 20, 2023

litvinovg Dec 21, 2023

wwelling Dec 20, 2023

litvinovg Dec 21, 2023

wwelling Dec 20, 2023

litvinovg Dec 21, 2023

wwelling Dec 21, 2023

chenejac left a comment

-}
+}
+section#audit table.history pre{
+    text-wrap: balance;
+    line-height: normal;
+}

Audit module (Change Tracking) #390

Audit module (Change Tracking) #390

Conversation

litvinovg commented May 19, 2023 • edited Loading

What does this pull request do?

What's new?

How should this be tested?

Supersedes

Interested parties

chenejac left a comment

Choose a reason for hiding this comment

brianjlowe commented Aug 24, 2023

chenejac left a comment

Choose a reason for hiding this comment

milospp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milospp commented Dec 14, 2023 • edited Loading

litvinovg commented Dec 14, 2023 • edited Loading

milospp commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litvinovg Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenejac left a comment

Choose a reason for hiding this comment

litvinovg commented May 19, 2023 •

edited

Loading

milospp commented Dec 14, 2023 •

edited

Loading

litvinovg commented Dec 14, 2023 •

edited

Loading

litvinovg Dec 21, 2023 •

edited

Loading