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

Improved the handling of cookies. #172

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ojima-ryou
Copy link
Contributor

Hello. I've improved rtcclient's use of cookies. Before the improvement, communication with the RTC server was performed while the unnecessary character string was set in the cookie of the HTTP-Header. I think this implementation is more secure. I hope you like it.

Copy link
Owner

@dixudx dixudx left a comment

Choose a reason for hiding this comment

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

Nice cleanup. Good work!

@@ -39,7 +39,6 @@ def wrapper(*args, **kwargs):
except RTCException:
raise RTCException("Relogin Failed: "
"Invalid username or password")
kwargs["headers"]["Cookie"] = rtc_obj.headers["Cookie"]
Copy link
Owner

Choose a reason for hiding this comment

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

Here should be kwargs["cookies"] = rtc_obj.cookies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. I've modified the source code.

@@ -107,23 +114,25 @@ def _get_headers(self):
raise exception.RTCException("Authentication Failed: "
"Invalid username or password")

_cookies = None
Copy link
Owner

Choose a reason for hiding this comment

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

L97 _headers = {"Content-Type": self.CONTENT_XML} is useless for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. I've modified the source code.

@ojima-ryou
Copy link
Contributor Author

Please wait to merge. I may have found a bug in this code.

@ojima-ryou
Copy link
Contributor Author

Sorry. There was a problem with how to set the cookie when not redirecting, so I fixed it.

@dixudx
Copy link
Owner

dixudx commented Mar 28, 2023

@ojima-ryou Please feel free to ping when this PR is ready. Thanks.

cc @gpongelli for the cookie handling part, this could be useful for backward compatibility.

@gpongelli
Copy link
Collaborator

cc @gpongelli for the cookie handling part, this could be useful for backward compatibility.

Sure. Before the final review, I suggest to rebase this PR .

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.

3 participants