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

Possibility to turn off ssl certificate validation and proxy connection #105

Merged
merged 7 commits into from May 4, 2016
Merged

Conversation

tosher
Copy link
Contributor

@tosher tosher commented Dec 22, 2015

  • Possibility to turn off ssl certificate validation (for self-signed certs on corp. wikies)
  • Proxy connection support.

@tosher tosher changed the title Possibility to turn off ssl certificate validation Possibility to turn off ssl certificate validation and proxy connection Jan 10, 2016
@danmichaelo
Copy link
Member

Sorry for the delay on this! The argument name do_ssl_cert_verify is quite long. What do you think about renaming it to just verify to follow requests? Another idea would be to just pass **kwargs to requests.

@scholer
Copy link

scholer commented Feb 8, 2016

I'd be very happy if **kwargs is passed on to requests.
In my research lab we are maintaining a custom version of @tosher 's Mediawiker plugin for Sublime, which again has a custom (old) version of mwclient, all so that we can inject custom headers (with a stored SessionID data) before making the initial connection. Yes, there are other ways to do this, using do_init=False, and then updating headers/etc after init, but this would just require more customizations to Mediawiker instead. If we can simply configure the requests Session with kwargs on init, that would make life easier. And thanks again, Dan, for making this library! :)

@tosher
Copy link
Contributor Author

tosher commented Feb 8, 2016

It will be requests specified kwargs?

class Site(object):
    def __init__(self, ..... **kwargs):
        self.kwargs = kwargs
        # ...
    # in raw_call
        stream = self.connection.post(fullurl, data=data, files=files, headers=headers, **self.kwargs)

@tosher tosher mentioned this pull request Feb 9, 2016
@danmichaelo
Copy link
Member

👍 @tosher . But please name it something like self.connection_kwargs = kwargs.

@tosher
Copy link
Contributor Author

tosher commented Feb 9, 2016

done

@eighthave
Copy link

This kind of change should really be accompanied by tests. This is messing with the core security of this library, and getting it wrong could mean the certificate verification gets disabled for people who need it.

@tosher
Copy link
Contributor Author

tosher commented Feb 16, 2016

  • It's not default option for mwclient
  • It's base functionality of requests lib

@tosher
Copy link
Contributor Author

tosher commented Feb 23, 2016

Maybe, variant like this will be better?

Special parameter for requests library, not global kwargs..

@danmichaelo
Copy link
Member

Maybe, variant like this will be better?

Special parameter for requests library, not global kwargs..

Agree, that's a better solution since we will never get a conflict with internal arguments. Would you mind updating the pull request? Then I can merge it right away. Sorry I forgot about this.

@tosher
Copy link
Contributor Author

tosher commented Apr 28, 2016

Ok, I will..

@tosher
Copy link
Contributor Author

tosher commented Apr 28, 2016

Travis is broken?

@danmichaelo
Copy link
Member

Travis is broken?

Related to one of the deps, we can ignore that.

Being a little picky, I'm not sure if we need to introduce kwargs here.. I'd suggest just adding requests=None as a param to the method, and then do:

self.requests = requests or {}

(since adding requests={} as a param is not a good idea)

Also, could you squash all the commits into one commit? Just do a git push --force afterwards

@tosher
Copy link
Contributor Author

tosher commented Apr 29, 2016

  • 'Code quality decreased by -0.63%'
    • Too many local variables

:) No problem, i'll remove kwargs.

all the commits into one commit

ok

@waldyrious
Copy link
Member

waldyrious commented Apr 29, 2016

@danmichaelo for future reference, PRs can now be squashed at merge time by the maintainer, so there's no need for the awkward push --force dance anymore :)

@danmichaelo
Copy link
Member

danmichaelo commented Apr 29, 2016

@danmichaelo for future reference, PRs can now be squashed at merge time by the maintainer, so there's no need for the awkward push --force dance anymore :)

Oooh, that's useful

@tosher
Copy link
Contributor Author

tosher commented May 4, 2016

Done.

no need for the awkward push --force dance anymore :)

P.S. I'm skipped the dance :)

@danmichaelo danmichaelo merged commit e1cc288 into mwclient:master May 4, 2016
@danmichaelo
Copy link
Member

Thanks, no dance needed :)

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.

None yet

5 participants