Adds audit trail comments for all fields of new analysis #3551

sebD · 2024-03-14T01:56:10Z

Description

Adds audit comments for all properties of a newly created analysis entry.

Addressed Issue

Additional Details

This fix proposition changes the current behavior a bit when audits are performed manually via the UI. At the moment, setting the Analysis state only adds a comment about this particular field even though justification and vendor response are also set with default values. The proposed fix would add comments for those too.

This behavior seem coherent with what I understood from the Vex import feature (cf CycloneDXVexImporter.

This is a quick fix solution, a better approach would be to create a dedicated AnalysisService that would be called from the CycloneDXVexImporter and the AnalysisResource. Currently these two classes have too many responsabilities. Moreover, they share the analysis creation responsability and do it in a different manner.

I'll be happy to discuss this refactor further if deemed interesting.

Checklist

I have read and understand the contributing guidelines
This PR fixes a defect, and I have provided tests to verify that the fix is effective
This PR implements an enhancement, and I have provided tests to verify that it works as intended
This PR introduces changes to the database model, and I have added corresponding update logic
This PR introduces new or alters existing behavior, and I have updated the documentation accordingly

codacy-production · 2024-03-18T01:30:45Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `757a966`¹	✅ 94.34% (target: 70.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`757a966`)	Report Missing	Report Missing	Report Missing
Head commit (`5dffd9f`)	21728	16113	74.16%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3551)	212	200	94.34%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

nscuro

Thanks for the PR @sebD! A few suggestions.

a better approach would be to create a dedicated AnalysisService that would be called from the CycloneDXVexImporter and the AnalysisResource. Currently these two classes have too many responsabilities. Moreover, they share the analysis creation responsability and do it in a different manner.

Indeed it would be great to have this logic centralized. Would you like to take a shot at that in this PR?

nscuro · 2024-03-19T17:47:32Z

src/main/java/org/dependencytrack/util/AnalysisCommentUtil.java

+
+    public static void makeFirstStateComment(final QueryManager qm, final Analysis analysis, final String commenter) {
+        if (analysis.getAnalysisState() != null) {
+            addAnalysisStateComment(qm, analysis, null, analysis.getAnalysisState(), commenter);


We should use NOT_SET here instead of null. null should not be exposed to users.

Suggested change

addAnalysisStateComment(qm, analysis, null, analysis.getAnalysisState(), commenter);

addAnalysisStateComment(qm, analysis, AnalysisState.NOT_SET, analysis.getAnalysisState(), commenter);

nscuro · 2024-03-19T17:48:19Z

src/test/java/org/dependencytrack/resources/v1/AnalysisResourceTest.java

+        Condition<String> elfBearer = new Condition<String>() {
+            public boolean matches(String value) {
+                return value.contains("toto");
+            }
+        };


This appears to be unused.

Oops. Good catch

nscuro · 2024-03-19T17:48:52Z

src/test/java/org/dependencytrack/resources/v1/AnalysisResourceTest.java

        assertThat(responseJson.getJsonArray("analysisComments").getJsonObject(0))
-                .hasFieldOrPropertyWithValue("comment", Json.createValue("Analysis: NOT_SET → NOT_AFFECTED"))
+                .hasFieldOrPropertyWithValue("comment", Json.createValue("Analysis: null → NOT_AFFECTED"))


This is a regression and would be resolved by passing NOT_SET instead of null when creating the comment.

nscuro · 2024-03-19T17:50:01Z

src/main/java/org/dependencytrack/util/AnalysisCommentUtil.java

+
+    public static void makeFirstAnalysisResponseComment(QueryManager qm, Analysis analysis, String commenter) {
+        if (analysis.getAnalysisResponse() != null) {
+            addAnalysisResponseComment(qm, analysis, null, analysis.getAnalysisResponse(), commenter);


Suggested change

addAnalysisResponseComment(qm, analysis, null, analysis.getAnalysisResponse(), commenter);

addAnalysisResponseComment(qm, analysis, AnalysisResponse.NOT_SET, analysis.getAnalysisResponse(), commenter);

nscuro · 2024-03-19T17:50:45Z

src/main/java/org/dependencytrack/util/AnalysisCommentUtil.java

+
+    public static void makeFirstJustificationComment(QueryManager qm, Analysis analysis, String commenter) {
+        if (analysis.getAnalysisJustification() != null) {
+            addAnalysisJustificationComment(qm, analysis, null, analysis.getAnalysisJustification(), commenter);


Suggested change

addAnalysisJustificationComment(qm, analysis, null, analysis.getAnalysisJustification(), commenter);

addAnalysisJustificationComment(qm, analysis, AnalysisJustification.NOT_SET, analysis.getAnalysisJustification(), commenter);

sebD · 2024-03-19T22:55:02Z

I'll have a look.

sebD · 2024-03-22T03:10:13Z

@nscuro I am currently working on fixing some tests. Do not hesitate to give me any remarks on the proposed solution. I have taken some initiatives that differ a bit from the rest of the app. As I'm a new contributor, I remain open to make any change in the design.

sebD · 2024-04-07T22:09:00Z

@nscuro PR ready for second review round

nscuro · 2024-04-10T11:26:31Z

Thanks @sebD I'll have a look over the coming days. Trying to wrap up remaining work for v4.11 at the moment.

sebD · 2024-04-10T11:31:45Z

No problem. Take your time.

…nalysis added either via UI or directly via REST call. Does add more comments when using the UI but reflects state of the vulnerability audit better. Fixes: DependencyTrack#3470 Signed-off-by: Sebastien Delcoigne <sebastien.delcoigne@gmail.com>

…d service Adds analysis comments for justification, state and details for new analysis added either via UI or directly via REST call. Fixes DependencyTrack#3470 Signed-off-by: Sebastien Delcoigne <sebastien.delcoigne@gmail.com>

Signed-off-by: Sebastien Delcoigne <sebastien.delcoigne@gmail.com>

… AnalysisService to avoid "Object with id 'x' is managed by a different persistence manager" error types Signed-off-by: Sebastien Delcoigne <sebastien.delcoigne@gmail.com>

codacy-production · 2024-04-12T01:11:58Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.13% (target: -1.00%)	✅ 94.34% (target: 70.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`3e8f93f`)	21657	16127	74.47%
Head commit (`62bf367`)	21762 (+105)	16233 (+106)	74.59% (+0.13%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3551)	212	200	94.34%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

sebD marked this pull request as ready for review March 18, 2024 02:58

nscuro requested changes Mar 19, 2024

View reviewed changes

sebD force-pushed the 3470-analysis-details-missing branch from 1030f44 to 315ad3e Compare March 20, 2024 22:23

sebD marked this pull request as draft March 20, 2024 22:57

sebD force-pushed the 3470-analysis-details-missing branch 2 times, most recently from 067338c to 5dffd9f Compare April 7, 2024 10:55

sebD added 4 commits April 12, 2024 10:41

DependencyTrack#3470: fixes failing tests

1bf9874

Signed-off-by: Sebastien Delcoigne <sebastien.delcoigne@gmail.com>

Fixes DependencyTrack#3470: adds possibility to pass query manager to…

62bf367

… AnalysisService to avoid "Object with id 'x' is managed by a different persistence manager" error types Signed-off-by: Sebastien Delcoigne <sebastien.delcoigne@gmail.com>

sebD force-pushed the 3470-analysis-details-missing branch from 5dffd9f to 62bf367 Compare April 12, 2024 00:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds audit trail comments for all fields of new analysis #3551

Adds audit trail comments for all fields of new analysis #3551

sebD commented Mar 14, 2024

codacy-production bot commented Mar 18, 2024 •

edited

nscuro left a comment

nscuro Mar 19, 2024

nscuro Mar 19, 2024

sebD Mar 19, 2024

nscuro Mar 19, 2024

nscuro Mar 19, 2024

nscuro Mar 19, 2024

sebD commented Mar 19, 2024

sebD commented Mar 22, 2024

sebD commented Apr 7, 2024

nscuro commented Apr 10, 2024

sebD commented Apr 10, 2024

codacy-production bot commented Apr 12, 2024

	addAnalysisStateComment(qm, analysis, null, analysis.getAnalysisState(), commenter);
	addAnalysisStateComment(qm, analysis, AnalysisState.NOT_SET, analysis.getAnalysisState(), commenter);

	addAnalysisResponseComment(qm, analysis, null, analysis.getAnalysisResponse(), commenter);
	addAnalysisResponseComment(qm, analysis, AnalysisResponse.NOT_SET, analysis.getAnalysisResponse(), commenter);

	addAnalysisJustificationComment(qm, analysis, null, analysis.getAnalysisJustification(), commenter);
	addAnalysisJustificationComment(qm, analysis, AnalysisJustification.NOT_SET, analysis.getAnalysisJustification(), commenter);

Adds audit trail comments for all fields of new analysis #3551

Are you sure you want to change the base?

Adds audit trail comments for all fields of new analysis #3551

Conversation

sebD commented Mar 14, 2024

Description

Addressed Issue

Additional Details

Checklist

codacy-production bot commented Mar 18, 2024 • edited

Coverage summary from Codacy

Footnotes

nscuro 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

sebD commented Mar 19, 2024

sebD commented Mar 22, 2024

sebD commented Apr 7, 2024

nscuro commented Apr 10, 2024

sebD commented Apr 10, 2024

codacy-production bot commented Apr 12, 2024

Coverage summary from Codacy

codacy-production bot commented Mar 18, 2024 •

edited