Support JFrog Apps Config file #405

yahavi · 2023-09-12T13:26:37Z

All tests passed. If this feature is not already covered by the tests, I added new tests.
I used npm run format for formatting the code before submitting the pull request.

Support the new .jfrog/jfrog-apps-config.yml file to allow configuring JFrog Advanced Security scanners. https://github.com/jfrog/jfrog-apps-config
Rename all EOS -> Sast
Add unit tests to Sast
Upgrade analyzer manager 1.3.2.2005632 -> 1.3.2.2019257

Or-Geva

@yahavi, excellent work!. You have significantly improved the clarity and structure of the code 🚀.

Or-Geva · 2023-10-10T05:27:01Z

src/main/scanLogic/scanManager.ts

@@ -128,23 +122,16 @@ export class ScanManager implements ExtensionComponent {
        return !ScanManager.lastOutdatedCheck || Date.now() - ScanManager.lastOutdatedCheck > ScanManager.RESOURCE_CHECK_UPDATE_INTERVAL_MILLISECS;
    }

-    private getResources(supportedScans: SupportedScans): Resource[] {
+    private getResources(supportedScans: EntitledScans): Resource[] {


'Resource' is a vague name that refers to any artifact that is downloaded from Artifactory. The 'Resource' in 'scanManager' talks about a specific resource which is the JasRunners. I suggest we use the most descriptive name we can and replace 'Resource' with 'JasRunners'

Or-Geva · 2023-10-10T05:58:05Z

src/main/scanLogic/scanManager.ts

    }

    /**
     * Updates all the resources that are outdated.
     * @param supportedScans - the supported scan to get the needed resources. if default, should call getSupportedScans before calling this method.
     * @returns true if all the outdated resources updated successfully, false otherwise
     */
-    public async updateResources(supportedScans: SupportedScans = this._supportedScans): Promise<boolean> {
+    public async updateResources(supportedScans: EntitledScans = this._entitledScans): Promise<boolean> {


I have a couple of quistions regarding the param:

Given that its name suggests that ScanManager is responsible for managing all scans, why is it necessary to pass in the supported scan as a parameter? As a manager, one would expect it to handle all aspects of the scan without revealing internal implementation details to other functions. A manager should inherently know what is supported or not.

The only reference I could find, calling this func is at 'IssuesTreeDataProvider'. Why data providers should care about scan manager updates? again, no one can tell a manager what to do regarding internal details that are being used for scans operations.

Or-Geva · 2023-10-10T06:08:18Z

src/main/scanLogic/scanManager.ts

        let resources: Resource[] = [];
        if (supportedScans.applicability || supportedScans.iac || supportedScans.secrets) {
-            resources.push(BinaryRunner.getAnalyzerManagerResource(this._logManager));
+            resources.push(JasRunner.getAnalyzerManagerResource(this._logManager));
        } else {
            this.logManager.logMessage('You are not entitled to run Advanced Security scans', 'DEBUG');


Could we identify if we are not entitled before calling getResources at getSupportedScans?

src/main/scanLogic/scanRunners/analyzerModels.ts

Or-Geva · 2023-10-10T07:17:17Z

src/main/scanLogic/scanManager.ts

    }

    /**
     * Get all the entitlement status for each type of scan the manager offers
     */
-    public async getSupportedScans(): Promise<SupportedScans> {
-        let supportedScans: SupportedScans = {} as SupportedScans;
+    public async getSupportedScans(): Promise<EntitledScans> {


I suggest that we change it to be private, and each support check will be response to update it own property
For example:

public async isApplicabilitySupported(): Promise<boolean> { return await ConnectionUtils.testXrayEntitlementForFeature(this._connectionManager.createJfrogClient(), EntitlementScanFeature.Applicability); }

--->

public async setApplicabilityEntitled(): Promise<void> { ConnectionUtils.testXrayEntitlementForFeature(this._connectionManager.createJfrogClient(), EntitlementScanFeature.Applicability) .then(res => (this.entitledScans.applicability = res)) .catch(err => ScanUtils.onScanError(err, this._logManager, true)) }

I think the suggestion is not related to Apps Config and not to the changes I did in this PR.
This PR has already become too big.

src/main/scanLogic/scanRunners/jasRunner.ts

src/main/utils/jfrogAppsConfig/jfrogAppsConfig.ts

src/test/tests/mavenUpdate.test.ts

Or-Geva · 2023-10-10T11:20:24Z

src/test/tests/appsConfig.test.ts

+describe('JFrog Apps Config Tests', () => {
+    const jfrogAppsConfigDir: string = path.join(__dirname, '..', 'resources', 'jfrogAppsConfig');
+
+    it('Load full config', () => {


We can break the test into two separate tests, each with its own particular assertion. for example, a check sast scanner can be encapsulated in its own it().

It will be clearer what fails in the config and it will enable us to scale if we would like to add additional specific module tests

The bond between the tests is too strong to split them into subtests - it reads one file and parses it. This is the result of the parsing. If we add more tests, it will require creating another input yaml and by that a new it.

I do agree that the test method was too long and therefore I shorted the test.

Maybe we can move the file read logic into the before section, each it will check a specific configuration section. Adding a new section to the config file in the future will make us add a single it test case.

yahavi had a problem deploying to frogbot September 12, 2023 13:26 — with GitHub Actions Failure

yahavi added the safe to test Approve running integration tests on a pull request label Sep 12, 2023