DAST rescan #236

vishalhcl-5960 · 2024-11-26T13:22:03Z

Implementation of rescan functionality for the dynamic scans.
Also, There is change in the implementation of rescanning in the Jenkins code, earlier we had the rescan parameter in the scanner class which is common class across 3 scan type classes. With this structure, I am facing issues in handling the UI part that while re-configuring a job, "include-SCA" checkbox is not getting disabled even the rescan checkbox is checked. As this is a important use-case to be handled i have to insert the rescan parameter in the respective classes itself.

mattmurp · 2024-12-03T15:31:04Z

src/main/java/com/hcl/appscan/jenkins/plugin/builders/AppScanBuildStep.java

@@ -338,14 +338,20 @@ private void validateGeneralSettings(boolean isAppScan360, Map<String, String> p
    }

    private void scanIdValidation(Map<String, String> properties, IProgress progress) throws JSONException, IOException {
-        IScanServiceProvider scanServiceProvider = new CloudScanServiceProvider(progress, m_authProvider);
-        JSONObject scanDetails = scanServiceProvider.getScanDetails(properties.get(CoreConstants.SCAN_ID));
+        JSONObject scanDetails = ServiceUtil.scanSpecificDetails(properties.get(CoreConstants.SCANNER_TYPE), properties.get(CoreConstants.SCAN_ID), m_authProvider);


The scanIdValidation() method does not belong in the AppScanBuildStep class. This should be a part of the Scanner.validateSettings() and any scanner specific checks (e.g. the one specific to SAST) should be in that scanner's validateSettings() method.
Also, the name of the method should be an action, like validateScanId().

3 out of 4 validations are of general form means that is applicable for all the scan types. We can have 1 remaining validation of SAST scan in the respective class but for that we have to again make the API call in that staticAnalyzer class to fetch the scan details.
Sure, I will rename the method to "validateScanId()".

There is currently an abstract method on the Scanner class named validateSettings(). Each of the individual scanners override that method to perform their own validation. There is also a validateGeneralSettings() method in the AppScanBuildStep, as well as this scanIdValidation() method. Why not put all of the settings validation into a single place in the scanners? The Scanner base class would perform the general settings validation and each individual scanner could call super.validateSettings() and then perform it's own scanner-specific validation. As it is now, the validation is happening in multiple places.

Okay Matt, Moved the general validations from the AppScanBuildStep to the Scanner base class & will call that new method from the respective scanner class.

mattmurp · 2024-12-03T15:33:51Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/Scanner.java

@@ -23,20 +23,11 @@ public abstract class Scanner extends AbstractDescribableImpl<Scanner> implement

 	private String m_target;
 	private boolean m_hasOptions;
-	private boolean m_rescan;


Why is the rescan logic pulled out of the base class and added to each individual subclass?

As mentioned in the description of the PR, Earlier we had the rescan parameter in the scanner class which is common class across 3 scan type classes. With that structure, I am facing issues in handling the UI part that while re-configuring a job, "include-SCA" checkbox is not getting disabled even the rescan checkbox is checked. As this is a important use-case to be handled because of that i have to insert the rescan parameter in the respective classes.

Do you know the technical reason why it wasn't working with the rescan logic in the base class?

If I keep the rescan in the base scanner class then I have to assign the same name of the rescan optional block across all the config files. We can't differentiate the rescan options if we use the same name of optional block as the Event listener reads the elements by "name" or "id".

I'm not sure I'm following the problem, but if we need a special value in the DASTScanner, why not keep the rescan option in the base Scanner class and add a new member variable only in the DASTScanner that gets it's value from the base class?

Matt, We want to have special value in the config file of dynamic scan. If we put the rescan in the base class then we have to use same value in all the respective config files.

mattmurp · 2024-12-03T15:38:36Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/DynamicAnalyzer.java

 			return FormValidation.validateRequired(target);
 		}

+		public FormValidation doCheckScanId(@QueryParameter String scanId, @RelativePath("..") @QueryParameter String application, @RelativePath("..") @QueryParameter String credentials, @AncestorInPath ItemGroup<?> context) throws JSONException {


Much of the same logic is repeated numerous times in each of the scanners doCheckScanId() method. Common logic should be placed in the base class and only scanner specific logic should be in the scanner classes.

As this method for doing the UI validations, we can't have the same validations in the abstract scanner class.

Why is that the case? We could add a protected static method to the Scanner base class that performed all of the common checks. That method would then be called in each of the subclasses doCheckScanId() method, along with any scanner specific checks that are needed for that technology.

As the "DescriptorImpl" class of each scan-type extends the "ScanDescriptor" class. I will add a new method in common class which will handle all the common validations for scanId.
protected FormValidation scanIdValidation(JSONObject scanDetails, String application)

mattmurp · 2024-12-03T15:38:49Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/SoftwareCompositionAnalyzer.java

@@ -74,10 +101,22 @@ public String getDisplayName() {
            return "Software Composition Analysis (SCA)";
        }

-        public FormValidation doCheckScanId(@QueryParameter String scanId, @RelativePath("..") @QueryParameter String application, @RelativePath("..") @QueryParameter String credentials, @AncestorInPath ItemGroup<?> context) {
+        public FormValidation doCheckScanId(@QueryParameter String scanId, @RelativePath("..") @QueryParameter String application, @RelativePath("..") @QueryParameter String credentials, @AncestorInPath ItemGroup<?> context) throws JSONException {


Much of the same logic is repeated numerous times in each of the scanners doCheckScanId() method. Common logic should be placed in the base class and only scanner specific logic should be in the scanner classes.

As this method for doing the UI validations, we can't have the same validations in the abstract scanner class.
Even for "target" field, though that is present in the scanner class but we are doing the FormValidation for that field in the respective classes.

Same comment as above.

As the "DescriptorImpl" class of each scan-type extends the "ScanDescriptor" class. I will add a new method in common class which will handle all the common validations for scanId.
protected FormValidation scanIdValidation(JSONObject scanDetails, String application)

mattmurp · 2024-12-03T15:38:59Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/StaticAnalyzer.java

@@ -255,27 +285,44 @@ public FormValidation doCheckIncludeSCAGenerateIRX(@QueryParameter String includ
            return FormValidation.ok();
        }

-        public FormValidation doCheckScanId(@QueryParameter String scanId, @RelativePath("..") @QueryParameter String application, @RelativePath("..") @QueryParameter String credentials, @AncestorInPath ItemGroup<?> context) {
+        public FormValidation doCheckScanId(@QueryParameter String scanId, @RelativePath("..") @QueryParameter String application, @RelativePath("..") @QueryParameter String credentials, @AncestorInPath ItemGroup<?> context) throws JSONException {


Much of the same logic is repeated numerous times in each of the scanners doCheckScanId() method. Common logic should be placed in the base class and only scanner specific logic should be in the scanner classes.

As this method for doing the UI validations, we can't have the same validations in the abstract scanner class.
Even for "target" field, though that is present in the scanner class but we are doing the FormValidation for that field in the respective classes.

Same as above.

As the "DescriptorImpl" class of each scan-type extends the "ScanDescriptor" class. I will add a new method in common class which will handle all the common validations for scanId.
protected FormValidation scanIdValidation(JSONObject scanDetails, String application)

mattmurp · 2024-12-11T21:54:10Z

src/main/webapp/help/rescan.html

@@ -1,4 +1,4 @@
 <div>
    Select this option to rescan the same application, updating and overwriting the previous scan results with the latest findings.<br/>
-    <b>Note:</b> If you are looking to use the Auto Close feature, ensure it has been enabled by your AppScan on Cloud/AppScan 360° organization administrator. Learn more about <a href="https://help.hcl-software.com/appscan/ASoC/appseccloud_scanning_rescan_cm.html" target="_blank">rescanning</a>.
+    <b>Note:</b> SCA scans and Auto Close feature are not supported in AppScan 360°. If you are looking to use the Auto Close feature, ensure it has been enabled by your AppScan on Cloud/AppScan 360° organization administrator. Learn more about <a href="https://help.hcl-software.com/appscan/ASoC/appseccloud_scanning_rescan_cm.html" target="_blank">rescanning</a>.


Since auto close is not supported on AS360, we should should "AppScan 360" from the sentence "...ensure it has been enabled by your AppScan on Cloud/AppScan 360° organization administrator."

Okay, I will remove the "AppScan 360" from the sentence.

mattmurp · 2024-12-11T21:56:01Z

src/main/resources/com/hcl/appscan/jenkins/plugin/Messages.properties

+error.empty.execution.id = Execution ID value is empty. Verify and try again.
+error.target.wrong.input = Wrong user input. Please specify a directory to scan.
+error.scan.id.validation.rescan.allowed = Rescan not permitted: Demo scans or organization restrictions detected.
+error.scan.id.validation.status = The selected scan is in the {0}. Rescans can only be performed when the scan is Ready, Paused, or Failed. Please wait or update the scan state.


Did you mean to say "The selected scan is in the {0} state."?

Yes, I will add "state".

mattmurp · 2024-12-11T22:02:40Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/Scanner.java


 	public abstract Map<String, String> getProperties(VariableResolver<String> resolver) throws AbortException;

-	public abstract void validateSettings(JenkinsAuthenticationProvider authProvider, Map<String, String> properties, IProgress progress) throws AbortException;
+	public abstract void validateSettings(JenkinsAuthenticationProvider authProvider, Map<String, String> properties, IProgress progress, boolean isAppScan360) throws IOException;


Rather than having a validateSettings() that each scanner needs to implement as well as a validateGeneralSettings() method that each scanner needs to call, I suggest making 1 public method called validateSettings() that takes care of verifying all of the settings that apply to every scanner. There can also be an abstract method called something like validateScannerSettings() that is called at the end of validateSettings() in the Scanner class. That way, each subclass simply implements the abstract validateScannerSettings() method and there's no need to worry about base classes calling the method from the super class, etc.

Made the changes as per suggestion.

mattmurp · 2024-12-11T22:04:00Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/Scanner.java

+        }
+    }
+
+    protected void scanIdValidation(JSONObject scanDetails, Map<String, String> properties) throws JSONException, IOException {


Similar to my other comment, we shouldn't force the subclasses to know that they need to call scanIdValidation(). This should be a private method that's called in the Scanner.validateSettings() method. Also, the name should be an action, like validateScanId().

Okay, Now, calling the validateScanId() method call from the generic validateSettings() method.

mattmurp · 2024-12-11T22:06:16Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/Scanner.java

@@ -23,20 +23,11 @@ public abstract class Scanner extends AbstractDescribableImpl<Scanner> implement

 	private String m_target;
 	private boolean m_hasOptions;
-	private boolean m_rescan;


I'm not sure I'm following the problem, but if we need a special value in the DASTScanner, why not keep the rescan option in the base Scanner class and add a new member variable only in the DASTScanner that gets it's value from the base class?

…ugin into DAST-rescan

vishalhcl-5960 added 9 commits October 16, 2024 16:05

dast-rescan

93187be

Update DynamicAnalyzer.java

a6e7a87

Update DynamicAnalyzer.java

5ae512d

URL validation check for A360

aa98bd6

updated scanId validation method

a7ea86f

UI scanId validation

4a405fa

Code enhancement for better UI

4ce66b7

indentation checks & bug fixes

54e4c81

updated the scanId validation method

372de05

vishalhcl-5960 requested a review from mattmurp November 26, 2024 13:22

vishalhcl-5960 added 3 commits November 29, 2024 11:38

A360 version check for URL validation

b3797ba

UI URL validation for A360 connection

408208a

A360 version check for URL validation

16b7a85

mattmurp requested changes Dec 3, 2024

View reviewed changes

vishalhcl-5960 added 4 commits December 4, 2024 19:39

As per PR comments

b686bdb

Addressing PR comments

193d928

Moved the common validation method to the base scanner class

66a62fb

UI validation for base scan field

ffb79e6

mattmurp reviewed Dec 11, 2024

View reviewed changes

vishalhcl-5960 and others added 8 commits December 12, 2024 23:32

As per comments

63b9f39

indentation checks

9d05f21

ASA-10068

d75f3cc

ASA-10067

c8bf28e

Merge branch 'DAST-rescan' of https://github.com/jenkinsci/appscan-pl…

2a1c29f

…ugin into DAST-rescan

Update DynamicAnalyzer.java

c9b1522

Updated the warning message statement

cb9ddc5

ASA-9275

c4e1cbd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAST rescan #236

DAST rescan #236

vishalhcl-5960 commented Nov 26, 2024

mattmurp Dec 3, 2024

vishalhcl-5960 Dec 4, 2024 •

edited

Loading

mattmurp Dec 4, 2024

vishalhcl-5960 Dec 8, 2024

mattmurp Dec 3, 2024

vishalhcl-5960 Dec 4, 2024

mattmurp Dec 4, 2024

vishalhcl-5960 Dec 8, 2024

mattmurp Dec 11, 2024

vishalhcl-5960 Dec 12, 2024

mattmurp Dec 3, 2024

vishalhcl-5960 Dec 4, 2024

mattmurp Dec 4, 2024

vishalhcl-5960 Dec 5, 2024

mattmurp Dec 3, 2024

vishalhcl-5960 Dec 4, 2024

mattmurp Dec 4, 2024

vishalhcl-5960 Dec 5, 2024

mattmurp Dec 3, 2024

vishalhcl-5960 Dec 4, 2024

mattmurp Dec 4, 2024

vishalhcl-5960 Dec 5, 2024

mattmurp Dec 11, 2024

vishalhcl-5960 Dec 12, 2024

mattmurp Dec 11, 2024

vishalhcl-5960 Dec 12, 2024

mattmurp Dec 11, 2024

vishalhcl-5960 Dec 12, 2024

mattmurp Dec 11, 2024

vishalhcl-5960 Dec 12, 2024

mattmurp Dec 11, 2024

DAST rescan #236

Are you sure you want to change the base?

DAST rescan #236

Conversation

vishalhcl-5960 commented Nov 26, 2024

Choose a reason for hiding this comment

vishalhcl-5960 Dec 4, 2024 • 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

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

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

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

vishalhcl-5960 Dec 4, 2024 •

edited

Loading