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

Cookies are handled for cybereason asynchronous changes #1301

Conversation

thangaraj-ramesh
Copy link
Contributor

Cookies handled for session login and logout asynchronous calls. Response status code corrected.

Arthur Muradyan and others added 3 commits January 24, 2023 16:17
…securityalliance#1038)

* Async calls to transmission (opencybersecurityalliance#655)

* Translation calls will receive proper json types instead of string
Cookies handled for session login and logout asynchronous calls.
Response status code corrected.
@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Base: 84.92% // Head: 84.85% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (1016df0) compared to base (b72bfba).
Patch coverage: 33.33% of modified lines in pull request are covered.

Additional details and impacted files
@@               Coverage Diff                @@
##           async-round2    #1301      +/-   ##
================================================
- Coverage         84.92%   84.85%   -0.07%     
================================================
  Files               575      575              
  Lines             42551    42586      +35     
================================================
  Hits              36137    36137              
- Misses             6414     6449      +35     
Impacted Files Coverage Δ
..._modules/alertflex/stix_transmission/api_client.py 30.43% <0.00%> (-2.90%) ⬇️
...es/alertflex/stix_transmission/delete_connector.py 30.00% <0.00%> (-1.58%) ⬇️
...s/alertflex/stix_transmission/results_connector.py 28.00% <0.00%> (-1.17%) ⬇️
...s/proofpoint/stix_transmission/delete_connector.py 45.00% <0.00%> (ø)
...ules/alertflex/stix_transmission/ping_connector.py 23.33% <7.14%> (-8.25%) ⬇️
...tils/stix_transmission/utils/RestApiClientAsync.py 47.05% <36.84%> (-2.95%) ⬇️
...modules/cybereason/stix_transmission/api_client.py 57.89% <50.00%> (+0.31%) ⬆️
...odules/elastic_ecs/stix_transmission/api_client.py 51.66% <50.00%> (-0.12%) ⬇️
...ter_modules/bigfix/stix_transmission/api_client.py 47.82% <100.00%> (+2.37%) ⬆️
..._modules/cybereason/stix_transmission/connector.py 84.76% <100.00%> (-7.62%) ⬇️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -0,0 +1 @@
aiohttp==3.8.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could go in the general requirements file since it's possibly not connector-specific. Also I noticed that aiohttp-retry is imported, is that something you could use instead? https://github.com/opencybersecurityalliance/stix-shifter/blob/5.0.1/stix_shifter/requirements.txt#L4

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to update aiohttp-retry to latest version 2.8.3
https://pypi.org/project/aiohttp-retry/

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 aiohttp==3.8.3 in any requirements file, it is downloaded as a dependancy from aiohttp-retry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

requirements.txt file is removed

@delliott90 delliott90 requested a review from baulus January 31, 2023 12:52
@delliott90
Copy link
Collaborator

The async-round2 branch has already been merged so you can retarget this PR to release/5.0.x

@baulus baulus changed the base branch from async-round2 to release/5.0.x January 31, 2023 13:23
@baulus baulus changed the base branch from release/5.0.x to async-round2 January 31, 2023 13:24
response_wrapper = await self.client.call_api(self.LOGIN_ENDPOINT, 'POST', headers=headers, data=self.auth)

return response_wrapper.response.cookies
async with aiohttp.ClientSession(headers=headers) as session:
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it still possible to use the self.client which is initiated from general RestApiClientAsync?
I am going to try one thing and if successful will suggest further here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baulus ,
We have tried using "self.client," but we couldn't get cookies.
Let us know any possible way to get cookies using "self.client".

Copy link
Contributor

Choose a reason for hiding this comment

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

I have added support in RestApiClientAsync to get and set cookies
You will get it once you change the PR target to release/5.0.x and bring your branch up-to-date with release/5.0.x
for getting a cookie you need to use:

url = self.host+self.LOGIN_ENDPOINT
response = await self.client.call_api(url, 'POST', headers=headers, data=self.auth)
cookie = response.get_cookies(url)

for setting cookies just add cookies=cookies in self.client.call_api arguments.
cookies var is dictionary, just like header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a new PR for this changes and target to release/5.0.x branch.
#1313

@@ -42,7 +42,7 @@ async def create_results_connection(self, query, offset, length):
query = json.dumps(query)
response_wrapper = await self.api_client.get_search_results(query)
if response_wrapper.response.history: # If the authentication is invalid, the history
if response_wrapper.response.history[0].status_code == 302: # will be returned with 302 status code.
if response_wrapper.response.history[0].status == 302: # will be returned with 302 status code.
Copy link
Contributor

Choose a reason for hiding this comment

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

response_wrapper does not have response.history arguments anymore
you can take a look at any other connector (example)
if this section is not covered with unit test please consider covering them

Copy link
Contributor Author

@thangaraj-ramesh thangaraj-ramesh Feb 2, 2023

Choose a reason for hiding this comment

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

response_wrapper contains response.history value, we tested it.
While doing valid authentication it contains empty list.
Invalid authentication it will contains the status value.

This field is required for identify the invalid authentication.

@@ -116,10 +116,10 @@ async def ping_connection(self):
try:
response = await self.api_client.ping_box()
if response.response.history: # If the authentication is invalid, the history
if response.response.history[0].status_code == 302: # will be returned with 302 status code.
if response.response.history[0].status == 302: # will be returned with 302 status code.
Copy link
Contributor

Choose a reason for hiding this comment

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

same as the previous comment

# Conflicts:
#	stix_shifter_modules/alertflex/stix_transmission/ping_connector.py
#	stix_shifter_modules/cybereason/stix_transmission/api_client.py
#	stix_shifter_modules/cybereason/stix_transmission/connector.py
#	stix_shifter_modules/proofpoint/stix_transmission/api_client.py
#	stix_shifter_utils/stix_transmission/utils/RestApiClientAsync.py
@thangaraj-ramesh thangaraj-ramesh changed the base branch from async-round2 to release/5.0.x February 2, 2023 09:08
@thangaraj-ramesh thangaraj-ramesh changed the base branch from release/5.0.x to async-round2 February 2, 2023 09:10
@thangaraj-ramesh
Copy link
Contributor Author

Created a new PR #1313 for this and will close this PR.
Please continue your review in new PR.

@thangaraj-ramesh
Copy link
Contributor Author

New PR created for this change #1313

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants