Consensys / armlet

a MythX API client wrapper
MIT License
17 stars 7 forks source link

add login function in Client for external #64

Closed tagomaru closed 5 years ago

tagomaru commented 5 years ago

I propose to add login function in Client class for external module.

background

truffle-security uses this package and calls analyzeWithStatus.

https://github.com/ConsenSys/truffle-security/blob/master/helpers.js#L252

some of analyzeWithStatus are executed simultaneously. (concurrency) This causes sending of unnecesally login requests to Mythirl API. The reason is the below part.

https://github.com/ConsenSys/armlet/blob/master/index.js#L136-L140

    if (!this.accessToken) {
      const tokens = await login.do(this.ethAddress, this.userId, this.password, this.apiUrl)
      this.accessToken = tokens.access
      this.refreshToken = tokens.refresh
    }

The first line can be true for each call from truffle-security since truffle-security calls analyzeWithStatus simultaneously. The evidence is the below.

1. exeucte truffle-security

capture_002_15022019_212738

2. Many login requests were sent from truffle-security

capture_003_15022019_212809

countermeasure

This PR is for the countermeasure at armlet. I implemented login part as a new function for external module such as truffle-security. By this, truffle-security can call login request independent from analysis.

For truffle-security, I will send a new PR to truffle-security repository after confirming that this PR is mergerd and the version of armlet is incremented. We have to increment the version of armlet in package.json of truffle-security before updating truffle-security to avoid an error at truffle-security side.

My idea of modification of truffle-security is adding step 'await client.login();' to call login before calling analyzeWithStatus like below.

https://github.com/ConsenSys/truffle-security/blob/master/helpers.js#L414-L416

    await client.login();
    const { objects, errors } = await doAnalysis(client, config, jsonFiles, contractNames, limit);
    const notFoundContracts = getNotFoundContracts(objects, contractNames);
    doReport(config, objects, errors, notFoundContracts);

Could you give me any comments? Thx!

tagomaru commented 5 years ago

np! I look forward to the new version API !

I just moved the steps for login within analysis function out to the new function, so I think analysis function behavior never changed.

As I said, this change is for bug fix of truffle-security relying on armlet which is the problem to issue unnecessary multiple login caused by concurrency. This change makes the bug fix of truffle-security easier.

We need two releases.

  1. Add create public login function to armlet. (This PR)
  2. Add a step to call the above login function to truffle-security before calling analyzeWithStatus function. (PR for this should be sent after updating armlet )

Thx!

tagomaru commented 5 years ago

@rocky will send the new PR for this after #79 is handled to avoid conflict. Thx!

tagomaru commented 5 years ago

refer to #80