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

GraphAPI - SSL Certificate Verification #412

Closed
wants to merge 2 commits into from

Conversation

RikhavM
Copy link

@RikhavM RikhavM commented Apr 9, 2018

At present, the GraphAPI does not contain verify and cert parameters which exist in requests.request. These parameters help in dealing with SSL certificate verification. In our project, we needed to bypass this verification but by default requests.request was verifying the SSL certificates and creating an issue for us. We simply included the said parameters in the GraphAPI as verify_ssl and verify_cert and used it subsequently in the call to requests.request

If some further changes are to be made, let us know - we could modify accordingly too

@RikhavM
Copy link
Author

RikhavM commented Apr 9, 2018

Not sure why the automated tests are failing. It seems there is some issue with the Facebook backend as indicated in the test exceptions:
facebook.GraphAPIError: (#200) Access to this data is temporarily disabled for non-active apps or apps that have not recently accessed this data due to changes we are making to the Facebook Platform. https://developers.facebook.com/status/issues/205942813488872/

Copy link
Member

@martey martey left a comment

Choose a reason for hiding this comment

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

This pull request is missing documentation.

In our project, we needed to bypass this verification but by default requests.request was verifying the SSL certificates and creating an issue for us.

I would also like to see additional explanation of why this would be generally useful to people using this project.

@RikhavM
Copy link
Author

RikhavM commented Apr 9, 2018

We have added documentation detailing usage of added parameters along with relevant links.

This is useful because by default SSL verification is enabled and at present, there is no option available in GraphAPI to ignore the SSL verification. By introducing the said parameters, we get a handle to underlying Requests.request parameter verify which allows users to choose if they want the API to verify SSL or ignore it.
Reference: http://www.python-requests.org/en/latest/user/advanced/#ssl-cert-verification

@martey
Copy link
Member

martey commented Apr 9, 2018

This library doesn't expose every single parameter that the underlying requests library does.

I still don't understand why these two options are generally useful - it would help if you could give a use case or an example in which using them would be required.

@martey
Copy link
Member

martey commented Apr 19, 2018

Closing because my questions on general usefulness have not been answered.

Unless you can provide a rationale for why users of this SDK would need to disable SSL verification on a regular basis, it doesn't make sense to accept this pull request.

@martey martey closed this Apr 19, 2018
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.

2 participants