Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have i been Pwned fix #4185

Merged
merged 36 commits into from
Sep 26, 2019
Merged

Have i been Pwned fix #4185

merged 36 commits into from
Sep 26, 2019

Conversation

teizenman
Copy link
Contributor

@teizenman teizenman commented Aug 22, 2019

Status

Ready

Related Issues

fixes: https://github.com/demisto/etc/issues/18102
fixes: https://github.com/demisto/etc/issues/18781

Description

Removed 'Mandatory' tag from commands' arguments

@teizenman teizenman requested a review from ronykoz August 22, 2019 12:02
@teizenman teizenman self-assigned this Aug 22, 2019
@ronykoz ronykoz changed the title Removed 'Mandatory' tag from commands' arguments Have i been Pwned fix Aug 25, 2019
Integrations/Pwned-V2/CHANGELOG.md Outdated Show resolved Hide resolved
Integrations/Pwned-V2/CHANGELOG.md Outdated Show resolved Hide resolved
Integrations/Pwned-V2/Pwned-V2.py Show resolved Hide resolved
Integrations/Pwned-V2/Pwned-V2.py Outdated Show resolved Hide resolved
Integrations/Pwned-V2/Pwned-V2.py Outdated Show resolved Hide resolved
Integrations/Pwned-V2/Pwned-V2.py Outdated Show resolved Hide resolved
@ronykoz
Copy link
Contributor

ronykoz commented Aug 25, 2019

@kirbles19 please review the CHANGELOG.md file please

@kirbles19
Copy link
Contributor

@teizenman @ronykoz Done. Please check for technical accuracy.

Copy link
Contributor

@glicht glicht left a comment

Choose a reason for hiding this comment

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

We should have an integration configuration of a max time for retries. Also retry should try in a loop up to max time.

@teizenman teizenman requested a review from yaakovi September 24, 2019 14:39
Copy link
Contributor

@yaakovi yaakovi left a comment

Choose a reason for hiding this comment

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

why aren't you handling the retries inside the http function?

@@ -33,6 +35,8 @@
EMAIL_TRUNCATE_VERIFIED_SUFFIX = '?truncateResponse=false&includeUnverified=true'
DOMAIN_TRUNCATE_VERIFIED_SUFFIX = '&truncateResponse=false&includeUnverified=true'

retries_end_time = datetime.min
Copy link
Contributor

Choose a reason for hiding this comment

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

global should be upper case

@@ -48,7 +52,13 @@ def http_request(method, url_suffix, params=None, data=None):

if res.status_code == 404:
return None
elif not res.status_code == 200:
elif res.status_code == 429:
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for elif after return

Integrations/Pwned-V2/Pwned-V2.py Show resolved Hide resolved
records_found = False

md = '### Have I Been Pwned query for ' + query_type.lower() + ': *' + query_arg + '*\n'

if hibp_res and len(hibp_res) > 0:
if api_res and len(api_res) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

both expressions check the same thing

Suggested change
if api_res and len(api_res) > 0:
if api_res:

verified_breach = 'Verified' if breach['IsVerified'] else 'Unverified'
md += '#### ' + breach['Title'] + ' (' + breach['Domain'] + '): ' + str(breach['PwnCount']) + \
' records breached [' + verified_breach + ' breach]\n'
md += 'Date: **' + breach['BreachDate'] + '**\n\n'
md += html_description_to_human_readable(breach['Description']) + '\n'
md += 'Data breached: **' + ','.join(breach['DataClasses']) + '**\n'

if hibp_paste_res and len(hibp_paste_res) > 0:
if api_paste_res and len(api_paste_res) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@@ -187,6 +197,29 @@ def domain_to_entry_context(domain, hibp_res):
return comp_domain


def rate_limit_retry(amount_of_seconds, request_type):
if datetime.now() > retries_end_time:
return_error('Max retry time has exceeded')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return_error('Max retry time has exceeded')
return_error('Max retry time has exceeded.')

Integrations/Pwned-V2/Pwned-V2.py Outdated Show resolved Hide resolved

def retry_needed(api_res, api_paste_res=None):
global retries_end_time
if retries_end_time == datetime.min and not MAX_RETRY_ALLOWED == -1:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if retries_end_time == datetime.min and not MAX_RETRY_ALLOWED == -1:
if retries_end_time == datetime.min and MAX_RETRY_ALLOWED != -1:

Integrations/Pwned-V2/Pwned-V2.yml Show resolved Hide resolved
Integrations/Pwned-V2/Pwned-V2.py Show resolved Hide resolved
@teizenman
Copy link
Contributor Author

@kirbles19 I removed the docs-approved label since I added something new that needs to be reviewed.
file #1: https://github.com/demisto/content/blob/Pwned-V2/Integrations/Pwned-V2/CHANGELOG.md
file #2: https://github.com/demisto/content/blob/Pwned-V2/Integrations/Pwned-V2/Pwned-V2.yml lines 10-14, only added the max_retry_time parameter

@kirbles19
Copy link
Contributor

@teizenman Done.

@teizenman teizenman merged commit 6ebad6e into master Sep 26, 2019
@teizenman teizenman deleted the Pwned-V2 branch September 26, 2019 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants