Skip to content
This repository was archived by the owner on May 30, 2020. It is now read-only.

Multi apikey#11

Open
rbicelli wants to merge 7 commits intoercpe:masterfrom
rbicelli:multi-apikey
Open

Multi apikey#11
rbicelli wants to merge 7 commits intoercpe:masterfrom
rbicelli:multi-apikey

Conversation

@rbicelli
Copy link
Copy Markdown

@rbicelli rbicelli commented Nov 4, 2019

Hello!
I added a feature in which you can use multiple API keys, with temporary disabling it in memcached in case limit is reached.

Comment thread amavisvt/client.py
Comment on lines +609 to +610
if self.config.pretend or self.config.do_not_report:
logger.info("NOT reporting resource to virustotal")
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added new parameter do-not-report in config if it helps to save some calls to VT

@ercpe
Copy link
Copy Markdown
Owner

ercpe commented Nov 7, 2019

First of all: thank you for your contribution.

I'm somewhat reluctant to merge this PR as it sounds to me that this is a violation of the Virustotal ToS although i couldn't find anything that explicitly forbids multiple accounts (and thus, multiple API keys).
I would probably be fine with it if the README and the example config explicitly states that the user is responsible for complying with the ToS.

Anyway: before merging, please:

  • Make sure your branch is based on the current master as you PR adds unrelated stuff
  • Make sure that all existing tests pass
  • Add new tests for your new feature

I'll leave some comments in diff too.

Comment thread amavisvt/client.py
for api_key in self.config.apikeys:
use_api_key = self.memcached.get(api_key)
if not use_api_key:
response = requests.post(self.config.api_url, {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reformat these arguments to be more compliant with the python style guide.

Comment thread amavisvt/client.py
for api_key in self.config.apikeys:
use_api_key = self.memcached.get(api_key)
if not use_api_key:
response = requests.post(self.config.report_url, data={
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reformat these arguments to be more compliant with the python style guide.

Comment thread amavisvt/client.py
]))

def api_call_check(self, checksums):
for api_key in self.config.apikeys:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split up the loop over self.config.apikeys and the actual API call and invert the expression in line 495.

Comment thread amavisvt/client.py
logger.info("No more API Keys available")
return False

def api_call_report(self, files):
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically the same as api_call_check (except for the url and the arguments), isn't it?
Please refactor it to not duplicate everything.

Comment thread amavisvt/client.py
'User-Agent': 'amavisvt/%s (+https://ercpe.de/projects/amavisvt)' % VERSION
})
response.raise_for_status()
if response.status_code == 204:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is safe to hard code 204 here. I would expect that every api call counts against the API limit, no matter what the response code is.

Comment thread amavisvt/client.py
response = requests.post(self.config.api_url, {
#Call VT API for checksums
response = self.api_call_check(send_checksums)
"""response = requests.post(self.config.api_url, {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this block of code a multi-line string?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm very new to python. I thought it was fine for commenting.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way you're effectively creating a string which gets thrown away right away. The proper way to comment out code is with the hash sign:

# i'm a comment

However, if the code isn't used anymore, remove it.

Comment thread amavisvt/client.py
'file': (resource.filename, open(resource.path, 'rb')),
}
response = requests.post(self.config.report_url, data={
"""response = requests.post(self.config.report_url, data={
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants