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

Data Enrichment and Threat Intelligence and Authentication templates #4462

Merged
merged 19 commits into from
Oct 27, 2019

Conversation

jochman
Copy link
Contributor

@jochman jochman commented Sep 23, 2019

Status

Ready

Description

New Data Enrichment and Threat Intelligence and Authentication templates.

Does it break backward compatibility?

  • No

Must have

  • Tests
  • Documentation (with link to it)
  • Code Review

Technical writer review

Mention and link to the files that require a technical writer review.

Authentication

DETI

@jochman jochman self-assigned this Sep 23, 2019
@jochman jochman requested a review from Itay4 September 23, 2019 09:57
@jochman
Copy link
Contributor Author

jochman commented Sep 23, 2019

@kirbles19 need to review .yml files and python files.

return_error(err_msg, error=e)


if __name__ == '__builtin__': # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

whats the pragma comment here for ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for coverage that I run on the files.
I'm trying to add it to the circle also.


def main(): # pragma: no cover
params = demisto.params()
base_url = params.get('url', '').rstrip('/') + '/api/v1'
Copy link
Contributor

Choose a reason for hiding this comment

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

use f-strings here

return {
'user': credentials.get('username'),
'name': credentials.get('name'),
'password': credentials.get('password'),
Copy link
Contributor

Choose a reason for hiding this comment

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

remove trailing comma

return {
'Username': credentials.get('username'),
'Name': credentials.get('name'),
'IsLocked': credentials.get('isLocked'),
Copy link
Contributor

Choose a reason for hiding this comment

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

remove trailing comma

Copy link
Contributor

Choose a reason for hiding this comment

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

return_error(err_msg, error=e)


if __name__ == '__builtin__': # pragma: no cover
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 __name__ == '__builtin__': # pragma: no cover
if __name__ == 'builtins': # pragma: no cover

name: url
required: true
type: 0
- defaultvalue: 'true'
Copy link
Contributor

Choose a reason for hiding this comment

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

remove default value

name: insecure
required: false
type: 8
- display: Use system proxy
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
- display: Use system proxy
- display: Use system proxy settings

required: true
type: 0
- defaultvalue: 'true'
display: Trust any certificate (insecure)
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
display: Trust any certificate (insecure)
display: Trust any certificate (not secure)

@demisto demisto deleted a comment from jochman Sep 25, 2019
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.

looks good, see my comments

from typing import Dict, Tuple, Optional, List, Union
import urllib3

# Disable insecure warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be in CommonServerPython (next to the requests import.



class Client(BaseClient):
def __init__(self, base_url, threshold: int = 70, *args, **kwargs):
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
def __init__(self, base_url, threshold: int = 70, *args, **kwargs):
def __init__(self, base_url, threshold: int = DEFAULT_THRESHOLD, *args, **kwargs):

"""
if isinstance(results, list):
return [build_entry_context(entry, indicator_type) for entry in results] # pragma: no cover
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

add blank line after condition block

raw_response = client.get_domain(url)
results = raw_response.get('result')
if results:
result = results[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

why only the first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because the api will return one result per call with 1 domain.

Jochman and others added 3 commits October 27, 2019 09:57
@kirbles19
Copy link
Contributor

@jochman Done reviewing both YMLs.

@jochman jochman merged commit fd42cf8 into master Oct 27, 2019
@jochman jochman deleted the authentication-deti-templates branch October 27, 2019 09:28
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.

4 participants